|
[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
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?
> 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?
> 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?
What does "hotplugging via qemu" mean here if qemu isn't patched to call
this new call?
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.
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?
>
> 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?
> /*
> - * 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?
> @@ -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/?
> [...]
> 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).
> +/**
> + * 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?
> 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?
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.
> +
> 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.
> + 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 |