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

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

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