Re: [Xen-devel] [PATCH v4 8/8] ioreq-server: bring the PCI hotplug controller implementation into Xen

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

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?

Also, if you are obscuring those regions now how does it continue to
work?

> 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.

> 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.

> 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.

> +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.

> +        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. 

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?

Ian.


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel