[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v4 8/8] ioreq-server: bring the PCI hotplug controller implementation into Xen
> -----Original Message----- > From: Ian Campbell > Sent: 07 April 2014 17:14 > To: Paul Durrant > Cc: xen-devel@xxxxxxxxxxxxx; Ian Jackson; Stefano Stabellini; Jan Beulich > Subject: Re: [PATCH v4 8/8] ioreq-server: bring the PCI hotplug controller > implementation into Xen > > On Wed, 2014-04-02 at 16:11 +0100, Paul Durrant wrote: > > Because we may now have more than one emulator, the implementation > of the > > PCI hotplug controller needs to be done by Xen. Happily the code is very > > short and simple and it also removes the need for a different ACPI DSDT > > when using different variants of QEMU. > > > > As a precaution, we obscure the IO ranges used by QEMU traditional's gpe > > and hotplug controller implementations to avoid the possibility of it > > raising an SCI which will never be cleared. > > > > VMs started on an older host and then migrated in will not use the in-Xen > > controller as the AML may still point at QEMU traditional's hotplug > > controller implementation. This means hotplug ops will fail with > EOPNOTSUPP > > and it is up to the caller to decide whether this is a problem or not. > > libxl will ignore EOPNOTSUPP as it is always hotplugging via QEMU so it does > > not matter whether it is Xen or QEMU providing the implementation. > > I don't follow the second half of this paragraph. > > If it is always hotplugging via qemu where does it see EOPNOTSUPP from? > Sorry, I should have said hotplug ops via libxc. > Also, if you are obscuring those regions now how does it continue to > work? > We only obscure the old regions if we create the in-xen hotplug controller. This doesn't happen for migrated-in guests. > > Signed-off-by: Paul Durrant <paul.durrant@xxxxxxxxxx> > > Cc: Ian Jackson <ian.jackson@xxxxxxxxxxxxx> > > Cc: Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx> > > Cc: Ian Campbell <ian.campbell@xxxxxxxxxx> > > Cc: Jan Beulich <jbeulich@xxxxxxxx> > > --- > > tools/firmware/hvmloader/acpi/mk_dsdt.c | 191 +++++-------------------- > > tools/libxc/xc_domain.c | 24 ++++ > > tools/libxc/xenctrl.h | 9 ++ > > tools/libxl/libxl_pci.c | 15 ++ > > xen/arch/x86/hvm/Makefile | 1 + > > xen/arch/x86/hvm/hotplug.c | 231 > +++++++++++++++++++++++++++++++ > > xen/arch/x86/hvm/hvm.c | 40 ++++++ > > xen/include/asm-x86/hvm/domain.h | 11 ++ > > xen/include/asm-x86/hvm/io.h | 8 +- > > xen/include/public/hvm/hvm_op.h | 9 ++ > > xen/include/public/hvm/ioreq.h | 4 + > > 11 files changed, 387 insertions(+), 156 deletions(-) > > create mode 100644 xen/arch/x86/hvm/hotplug.c > > > > @@ -116,14 +88,7 @@ int main(int argc, char **argv) > > break; > > } > > case 'q': > > - if (strcmp(optarg, "qemu-xen") == 0) { > > - dm_version = QEMU_XEN; > > - } else if (strcmp(optarg, "qemu-xen-traditional") == 0) { > > - dm_version = QEMU_XEN_TRADITIONAL; > > - } else { > > - fprintf(stderr, "Unknown device model version `%s'.\n", > > optarg); > > - return -1; > > - } > > + /* qemu version - no longer used */ > > No need to keep this sort of legacy stuff in tools used solely by the > build system. All uses of this option should be removed and it should be > an error to use it. > Ok. > > diff --git a/tools/libxc/xc_domain.c b/tools/libxc/xc_domain.c > > index 6eacce6..0f3c56b 100644 > > --- a/tools/libxc/xc_domain.c > > +++ b/tools/libxc/xc_domain.c > > @@ -1485,6 +1485,30 @@ int > xc_hvm_set_ioreq_server_state(xc_interface *xch, > > return rc; > > } > > > > +int xc_hvm_pci_hotplug(xc_interface *xch, > > + domid_t domid, > > + uint32_t slot, > > + int enable) > > +{ > > + DECLARE_HYPERCALL; > > + DECLARE_HYPERCALL_BUFFER(xen_hvm_pci_hotplug_t, arg); > > + int rc; > > + > > + arg = xc_hypercall_buffer_alloc(xch, arg, sizeof(*arg)); > > + if ( arg == NULL ) > > + return -1; > > + > > + hypercall.op = __HYPERVISOR_hvm_op; > > + hypercall.arg[0] = HVMOP_pci_hotplug; > > + hypercall.arg[1] = HYPERCALL_BUFFER_AS_ARG(arg); > > + arg->domid = domid; > > + arg->slot = slot; > > + arg->enable = enable; > > + rc = do_xen_hypercall(xch, &hypercall); > > + xc_hypercall_buffer_free(xch, arg); > > + return rc; > > Newlines for clarity please. Ok. > > > diff --git a/tools/libxc/xenctrl.h b/tools/libxc/xenctrl.h > > index cc0dab9..1eee77b 100644 > > --- a/tools/libxc/xenctrl.h > > +++ b/tools/libxc/xenctrl.h > > @@ -1849,6 +1849,15 @@ int > xc_hvm_set_ioreq_server_state(xc_interface *xch, > > ioservid_t id, > > int enabled); > > > > +/* > > + * Hotplug controller API > > + */ > > + > > According to the commit message the error codes from this function have > some pretty interesting semantics which should be documented IMHO. > Either here or in xen/include/public. > Well, the fact that the hotplug controller is not necessarily created (and thus functions may return with errno==EOPNOTSUPP) is worth mentioning, so I'll add a comment here. > > +int xc_hvm_pci_hotplug(xc_interface *xch, > > + domid_t domid, > > + uint32_t slot, > > + int enable); > > + > > /* HVM guest pass-through */ > > int xc_assign_device(xc_interface *xch, > > uint32_t domid, > > diff --git a/tools/libxl/libxl_pci.c b/tools/libxl/libxl_pci.c > > index 44d0453..968cd5a 100644 > > --- a/tools/libxl/libxl_pci.c > > +++ b/tools/libxl/libxl_pci.c > > @@ -867,6 +867,13 @@ static int do_pci_add(libxl__gc *gc, uint32_t > domid, libxl_device_pci *pcidev, i > > } > > if ( rc ) > > return ERROR_FAIL; > > + > > + rc = xc_hvm_pci_hotplug(ctx->xch, domid, pcidev->dev, 1); > > CTX->xch is somewhat preferred for new code. > Ok. > > + if (rc < 0 && errno != EOPNOTSUPP) { > > + LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "Error: > xc_hvm_pci_hotplug enable failed"); > > You can use the LOGE macro to shorten this line. > Ok. > Hrm, so I didn't see anything on the restore side which handles the > registration or not of the thing which would lead to EOPNOTSUPP vs > success on a guest started on an older Xen. How does all that actually > hang together? > If the guest was started on an older xen then HVM_PARAM_NR_IOREQ_SERVER_PAGES would not have been set, so when xc_domain_restore runs it would find no corresponding save record. Thus that param would not be set on restore and hence the controller would not be created. I could add another hvm op to make hotplug controller creation explicit if you like, but it seemed like rather a lot of extra code. Paul > Ian. _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx http://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |