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

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Ian Campbell
> Sent: 06 May 2014 14:24
> 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 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?
>

The controller emulation is there but since the ports are handled by Xen the 
I/O to enable the hotplug events will never get through. So, yes QEMU will go 
through the motions of hotplugging but it will never raise the SCI.
 
> > > 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)
> 

Yes, but because QEMU's PCIHP implementation never has any of its enabled bits 
set it remains silent.

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

No kidding.

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

I'll add a comment to the commit message.

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

I'll give it a go on my dev box. I'm also trying to get this series backported 
onto Xen 4.4 so I can throw it at XenRT.

  Paul

> Ian.

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