|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v5 6/9] ioreq-server: add support for multiple servers
On Thu, 2014-05-01 at 13:08 +0100, Paul Durrant wrote:
> NOTE: To prevent emulators running in non-privileged guests from
> potentially allocating very large amounts of xen heap, the core
> rangeset code has been modified to introduce a hard limit of 256
> ranges per set.
OOI how much RAM does that correspond to?
(Arguably this and the asprintf change should/could have been separate)
I've only really looks at tools/ and xen/include/public here.
> diff --git a/tools/libxc/xc_domain.c b/tools/libxc/xc_domain.c
> index 369c3f3..b3ed029 100644
> --- a/tools/libxc/xc_domain.c
> +++ b/tools/libxc/xc_domain.c
> @@ -1284,6 +1284,225 @@ int xc_get_hvm_param(xc_interface *handle, domid_t
> dom, int param, unsigned long
> return rc;
> }
>
> +int xc_hvm_create_ioreq_server(xc_interface *xch,
> + domid_t domid,
> + ioservid_t *id)
> +{
[...]
> +}
> +
> +int xc_hvm_get_ioreq_server_info(xc_interface *xch,
> + domid_t domid,
> + ioservid_t id,
> + xen_pfn_t *ioreq_pfn,
> + xen_pfn_t *bufioreq_pfn,
> + evtchn_port_t *bufioreq_port)
> +{
[...]
> +}
> +
> +int xc_hvm_map_io_range_to_ioreq_server(xc_interface *xch, domid_t domid,
> + ioservid_t id, int is_mmio,
> + uint64_t start, uint64_t end)
> +{
[...]
> +}
> +
> +int xc_hvm_unmap_io_range_from_ioreq_server(xc_interface *xch, domid_t domid,
> + ioservid_t id, int is_mmio,
> + uint64_t start, uint64_t end)
> +{
[...]
> +}
Those all look like reasonable layers over an underlying hypercall, so
if the hypervisor guys are happy with the hypervisor interface then I'm
happy with this.
> +
> +int xc_hvm_map_pcidev_to_ioreq_server(xc_interface *xch, domid_t domid,
> + ioservid_t id, uint16_t segment,
> + uint8_t bus, uint8_t device,
> + uint8_t function)
> +{
> + DECLARE_HYPERCALL;
> + DECLARE_HYPERCALL_BUFFER(xen_hvm_io_range_t, arg);
> + int rc;
> +
> + if (device > 0x1f || function > 0x7) {
> + errno = EINVAL;
> + return -1;
I suppose without this HVMOP_PCI_SBDF will produce nonsense, which the
hypervisor may or may not reject. Hopefully we aren't relying on this
check here for any security properties.
> + }
> +
> + arg = xc_hypercall_buffer_alloc(xch, arg, sizeof(*arg));
> + if ( arg == NULL )
> + return -1;
> +
> + hypercall.op = __HYPERVISOR_hvm_op;
> + hypercall.arg[0] = HVMOP_map_io_range_to_ioreq_server;
> + hypercall.arg[1] = HYPERCALL_BUFFER_AS_ARG(arg);
> +
> + arg->domid = domid;
> + arg->id = id;
> + arg->type = HVMOP_IO_RANGE_PCI;
> + arg->start = arg->end = HVMOP_PCI_SBDF((uint64_t)segment,
Since you have HVMOP_IO_RANGE_PCI do you not want to expose that via
this interface?
> diff --git a/tools/libxc/xc_domain_restore.c b/tools/libxc/xc_domain_restore.c
> index bcb0ae0..af2bf3a 100644
> --- a/tools/libxc/xc_domain_restore.c
> +++ b/tools/libxc/xc_domain_restore.c
> @@ -1748,6 +1770,29 @@ int xc_domain_restore(xc_interface *xch, int io_fd,
> uint32_t dom,
> if (pagebuf.viridian != 0)
> xc_set_hvm_param(xch, dom, HVM_PARAM_VIRIDIAN, 1);
>
> + /*
> + * If we are migrating in from a host that does not support
> + * secondary emulators then nr_ioreq_server_pages will be 0, since
> + * there will be no XC_SAVE_ID_HVM_NR_IOREQ_SERVER_PAGES chunk in
> + * the image.
> + * If we are migrating from a host that does support secondary
> + * emulators then the XC_SAVE_ID_HVM_NR_IOREQ_SERVER_PAGES chunk
> + * will exist and is guaranteed to have a non-zero value. The
> + * existence of that chunk also implies the existence of the
> + * XC_SAVE_ID_HVM_IOREQ_SERVER_PFN chunk, which is also guaranteed
> + * to have a non-zero value.
Please can you also note this both or neither behaviour in
xg_save_restore.h
> + */
> + if (pagebuf.nr_ioreq_server_pages != 0) {
> + if (pagebuf.ioreq_server_pfn != 0) {
> + xc_set_hvm_param(xch, dom, HVM_PARAM_NR_IOREQ_SERVER_PAGES,
> + pagebuf.nr_ioreq_server_pages);
> + xc_set_hvm_param(xch, dom, HVM_PARAM_IOREQ_SERVER_PFN,
> + pagebuf.ioreq_server_pfn);
> + } else {
> + ERROR("ioreq_server_pfn is invalid");
Or ioreq_server_pages was. Perhaps say they are inconsistent? Perhaps
log their values?
> + }
> + }
else if (..server_pfn != 0)
Also an error I think
(there might be better ways to structure things to catch both error
cases...)
> +
> if (pagebuf.acpi_ioport_location == 1) {
> DBGPRINTF("Use new firmware ioport from the checkpoint\n");
> xc_set_hvm_param(xch, dom, HVM_PARAM_ACPI_IOPORTS_LOCATION, 1);
> diff --git a/tools/libxc/xc_domain_save.c b/tools/libxc/xc_domain_save.c
> index 71f9b59..acf3685 100644
> --- a/tools/libxc/xc_domain_save.c
> +++ b/tools/libxc/xc_domain_save.c
> @@ -1737,6 +1737,30 @@ int xc_domain_save(xc_interface *xch, int io_fd,
> uint32_t dom, uint32_t max_iter
> PERROR("Error when writing the viridian flag");
> goto out;
> }
> +
> + chunk.id = XC_SAVE_ID_HVM_IOREQ_SERVER_PFN;
> + chunk.data = 0;
> + xc_get_hvm_param(xch, dom, HVM_PARAM_IOREQ_SERVER_PFN,
> + (unsigned long *)&chunk.data);
> +
> + if ( (chunk.data != 0) &&
> + wrexact(io_fd, &chunk, sizeof(chunk)) )
> + {
> + PERROR("Error when writing the ioreq server gmfn base");
> + goto out;
> + }
> +
> + chunk.id = XC_SAVE_ID_HVM_NR_IOREQ_SERVER_PAGES;
> + chunk.data = 0;
> + xc_get_hvm_param(xch, dom, HVM_PARAM_NR_IOREQ_SERVER_PAGES,
> + (unsigned long *)&chunk.data);
> +
> + if ( (chunk.data != 0) &&
> + wrexact(io_fd, &chunk, sizeof(chunk)) )
> + {
> + PERROR("Error when writing the ioreq server gmfn count");
> + goto out;
> + }
Probably arranging to assert that both of these are either zero or
non-zero is too much faff.
> @@ -502,6 +505,31 @@ static int setup_guest(xc_interface *xch,
> special_pfn(SPECIALPAGE_SHARING));
>
> /*
> + * Allocate and clear additional ioreq server pages. The default
> + * server will use the IOREQ and BUFIOREQ special pages above.
> + */
> + for ( i = 0; i < NR_IOREQ_SERVER_PAGES; i++ )
> + {
> + xen_pfn_t pfn = ioreq_server_pfn(i);
> +
> + rc = xc_domain_populate_physmap_exact(xch, dom, 1, 0, 0, &pfn);
> + if ( rc != 0 )
> + {
> + PERROR("Could not allocate %d'th ioreq server page.", i);
This will say things like "1'th". "Could not allocate ioreq server page
%d" avoids that.
You could do this allocation all in one go if pfn was an array of
[NR_IOREQ_SERVER_PAGES], you can still use order-0 allocations.
Ian.
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |