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

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Tue, 2014-05-06 at 14:02 +0100, Paul Durrant wrote:
> > -----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.

OK

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

Thanks.

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

When libxl does the qmp call to tell qemu about a new device what stops
qemu from generating hotplug events (SCI or whatever the interrupt
mechanism is) even if Xen is emulating the HP controller? Because Qemu
will still have the controller emulation, it's just "shadowed", right?

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

Even if Xen is implementing the PCIHP qemu is still involved in plugging
the device, since it has to know about it though, so in some sense
hotplugging is always (partially) via qemu (which is why I'm a bit
confused, but also why the lack of a Qemu side patch surprises me)

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

Phew, that would have been tricky to deal with!

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

I think this is worth noting in the commit message, if not splitting
into a separate patch

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

It's the problems we can't imagine that I'm worried about. This stuff is
certainly subtle in places WRT migration.

Ian.


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