|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v5 9/9] ioreq-server: bring the PCI hotplug controller implementation into Xen
> -----Original Message-----
> From: Ian Campbell
> Sent: 06 May 2014 12:25
> To: Paul Durrant
> Cc: xen-devel@xxxxxxxxxxxxx; Ian Jackson; Stefano Stabellini; Jan Beulich
> Subject: Re: [PATCH v5 9/9] ioreq-server: bring the PCI hotplug controller
> implementation into Xen
>
> On Thu, 2014-05-01 at 13:08 +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.
>
> Does that imply that this series must come sooner in the series?
>
Well, it needs to be done before anyone expects to be able to hotplug a device
implemented by a secondary emulator but someone getting hold of a build of Xen
which is part way through this series is hopefully unlikely. I think it's ok to
leave it here.
> > 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.
>
> "... will not ... may ...". I think perhaps one of those should be the
> other?
Ok - if they were started on an old Xen *and* using qemu trad then the AML
*will* point to qemu trad's implementation.
>
> > This means xc_hvm_pci_hotplug() 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.
>
> Just to clarify: there is no qemu patch involved here because this
> change will mean that PCI HP controller accesses will never be passed to
> qemu?
>
If Xen is not implementing the PCIHP then EOPNOTSUPP will be returned. Libxl
should not care because, if that is the case, QEMU will be implementing the
PCIHP and libxl only ever deals with devices being emulated by QEMU (because
that was the only possible emulator until now).
> What does "hotplugging via qemu" mean here if qemu isn't patched to call
> this new call?
>
It means QEMU is implementing the hotplug device. So, if Xen is implementing
the PCIHP libxl will call the new function and it will succeed. If QEMU is
implementing the PCIHP then libxl will call the new function, it will fail, but
QEMU will implicitly do the hotplug. Either way, the guest sees a hotplug event.
> What is the condition which causes the EOPNOTSUPP? I think it is when
> HVM_PARAM_IOREQ_SERVER_PFN has not been set. Is that correct? Can
> you
> write it down here please.
>
Ok.
> Is there any save/restore state associated with the hotplug controller?
> If yes then how is that handled when migrating a qemu-xen (new qemu)
> guest from a system which uses PCIHP in qemu to one which does it in
> Xen?
>
No, there is no state. I don't believe there ever was.
>
> >
> > 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/Makefile | 2 +-
> > tools/firmware/hvmloader/acpi/mk_dsdt.c | 193 +++++---------------------
> > tools/libxc/xc_domain.c | 27 ++++
> > tools/libxc/xc_domain_restore.c | 1 +
> > tools/libxc/xc_hvm_build_x86.c | 1 +
> > tools/libxc/xenctrl.h | 24 ++++
> > tools/libxl/libxl_pci.c | 14 ++
> > xen/arch/x86/hvm/Makefile | 1 +
> > xen/arch/x86/hvm/hotplug.c | 224
> +++++++++++++++++++++++++++++++
> > 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 | 12 ++
> > xen/include/public/hvm/ioreq.h | 4 +
> > 14 files changed, 402 insertions(+), 160 deletions(-)
> > create mode 100644 xen/arch/x86/hvm/hotplug.c
> >
> > diff --git a/tools/firmware/hvmloader/acpi/mk_dsdt.c
> b/tools/firmware/hvmloader/acpi/mk_dsdt.c
> > index a4b693b..1de88ac 100644
> > --- a/tools/firmware/hvmloader/acpi/mk_dsdt.c
> > +++ b/tools/firmware/hvmloader/acpi/mk_dsdt.c
> > @@ -222,11 +183,8 @@ int main(int argc, char **argv)
> >
> > /* Define GPE control method. */
> > push_block("Scope", "\\_GPE");
> > - if (dm_version == QEMU_XEN_TRADITIONAL) {
> > - push_block("Method", "_L02");
> > - } else {
> > - push_block("Method", "_E02");
> > - }
> > + push_block("Method", "_L02");
>
> Aren't you leaving the wrong case behind here?
>
No. AFAICT the pcihp code in upstream QEMU actually implemented level triggered
semantics anyway - the AML was just wrong.
> > /*
> > - * Reserve the IO port ranges [0x10c0, 0x1101] and [0xb044, 0xb047].
> > - * Or else, for a hotplugged-in device, the port IO BAR assigned
> > - * by guest OS may conflict with the ranges here.
> > + * Reserve the IO port ranges used by PCI hotplug controller or else,
> > + * for a hotplugged-in device, the port IO BAR assigned by guest OS may
> > + * conflict with the ranges here.
>
> AIUI you are also reserving the ranges used by qemu-trad to avoid
> accidental conflicts?
>
Yes, that was added later. I'll make sure that's correct.
> > @@ -322,64 +274,21 @@ int main(int argc, char **argv)
> > dev, intx, ((dev*4+dev/8+intx)&31)+16);
> > printf("})\n");
> >
> > - /*
> > - * Each PCI hotplug slot needs at least two methods to handle
> > - * the ACPI event:
> > - * _EJ0: eject a device
> > - * _STA: return a device's status, e.g. enabled or removed
> > - *
> > - * Eject button would generate a general-purpose event, then the
> > - * control method for this event uses Notify() to inform OSPM which
> > - * action happened and on which device.
> > - *
> > - * Pls. refer "6.3 Device Insertion, Removal, and Status Objects"
> > - * in ACPI spec 3.0b for details.
> > - *
> > - * QEMU provides a simple hotplug controller with some I/O to handle
> > - * the hotplug action and status, which is beyond the ACPI scope.
>
> Is this comment not still relevant with s/QEMU/Xen/?
>
Yes, actually it is. I'll leave it in.
> > [...]
> > diff --git a/tools/libxc/xc_domain_restore.c
> b/tools/libxc/xc_domain_restore.c
> > index af2bf3a..9b49509 100644
> > --- a/tools/libxc/xc_domain_restore.c
> > +++ b/tools/libxc/xc_domain_restore.c
> > @@ -1784,6 +1784,7 @@ int xc_domain_restore(xc_interface *xch, int
> io_fd, uint32_t dom,
> > */
> > if (pagebuf.nr_ioreq_server_pages != 0) {
> > if (pagebuf.ioreq_server_pfn != 0) {
> > + /* Setting this parameter also enables the hotplug controller
> > */
>
> "Xen's hotplug controller" ? (maybe I only think that because I'm aware
> of the qemu alternative).
>
Yes, I think QEMU is probably out of scope at this point in the code.
> > +/**
> > + * This function either enables or disables a hotplug PCI slot
> > + *
> > + * @parm xch a handle to an open hypervisor interface.
> > + * @parm domid the domain id to be serviced
> > + * @parm slot the slot number
> > + * @parm enable enable/disable the slot
> > + * @return 0 on success, -1 on failure.
> > + *
> > + * VMs started on an old version of Xen may not have a hotplug
> > + * controller, in which case this function will fail with errno
> > + * set to EOPNOTSUPP. Such a failure can be safely ignored if
> > + * the device in question is being emulated by QEMU since it
> > + * will providing the hotplug controller implementation.
>
> Is this more strictly required to be the QEMU providing the fallback
> ioreq, as opposed to some potentially disaggregated QEMU functionality
> in a secondary thing?
>
EOPNOTSUPP can only ever happen in the case where there is no secondary
emulator, so the fallback is necessarily QEMU.
> > diff --git a/tools/libxl/libxl_pci.c b/tools/libxl/libxl_pci.c
> > index 44d0453..55cb8a2 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);
> > + if (rc < 0 && errno != EOPNOTSUPP) {
> > + LOGE(ERROR, "Error: xc_hvm_pci_hotplug enable failed");
> > + return ERROR_FAIL;
> > + }
>
> I initially thought you needed to also reset rc to indicate success in
> the errno==EOPNOTSUPP, but actually the error handling in this function
> is just confusing...
>
> But, have you tried hotpluging into a migrated guest?
>
Not yet, but I can't see why that would be a problem over hotplugging at all,
and I've certainly tested that (although not via libxl).
> This might be a location worth reiterating (or at least referring to)
> the comment about EONOTSUPP and where the HP controller lives
>
> What would it take for the toolstack to know if it needed to do this
> call or not? Reading the IOREQ PFNs HVM param perhaps? I wonder if that
> is worth it.
>
That seem like unnecessary baggage given that EOPNOTSUPP is there purely to
cover the case where Xen's PCIHP does not exist. A comment should be enough
IMO. I'll add one.
> > +
> > break;
> > case LIBXL_DOMAIN_TYPE_PV:
> > {
> > @@ -1188,6 +1195,13 @@ static int do_pci_remove(libxl__gc *gc, uint32_t
> domid,
> > NULL, NULL, NULL) < 0)
> > goto out_fail;
> >
> > + rc = xc_hvm_pci_hotplug(CTX->xch, domid, pcidev->dev, 0);
> > + if (rc < 0 && errno != EOPNOTSUPP) {
> > + LOGE(ERROR, "Error: xc_hvm_pci_hotplug disable failed");
>
> Similar comments to above.
>
Ok.
Paul
> > + rc = ERROR_FAIL;
> > + goto out_fail;
> > + }
> > +
> > switch (libxl__device_model_version_running(gc, domid)) {
> > case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL:
> > rc = qemu_pci_remove_xenstore(gc, domid, pcidev, force);
>
> Ian.
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |