Re: [Qemu-devel] [for-2.11 PATCH 26/26] spapr: add hotplug hooks for PHB hotplug

[David Gibson]

On Tue, Aug 01, 2017 at 05:30:46PM +0200, Greg Kurz wrote:
> On Fri, 28 Jul 2017 14:24:03 +1000
> David Gibson <address@hidden> wrote:
> 
> > On Thu, Jul 27, 2017 at 07:09:55PM +0200, Greg Kurz wrote:
> > > On Thu, 27 Jul 2017 14:41:31 +1000
> > > Alexey Kardashevskiy <address@hidden> wrote:
> > >   
> > > > On 26/07/17 18:40, Greg Kurz wrote:  
> > > > > Hotplugging PHBs is a machine-level operation, but PHBs reside on the
> > > > > main system bus, so we register spapr machine as the handler for the
> > > > > main system bus.
> > > > > 
> > > > > Signed-off-by: Michael Roth <address@hidden>
> > > > > Signed-off-by: Greg Kurz <address@hidden>
> > > > > ---
> > > > > - rebased against ppc-for-2.10
> > > > > - converted to unplug_request
> > > > > - handle drc_id at pre-plug
> > > > > - reset hotplugged PHB at plug
> > > > > - compatibility with older machine types
> > > > > ---
> > > > >  hw/ppc/spapr.c              |  114 
> > > > > +++++++++++++++++++++++++++++++++++++++++++
> > > > >  hw/ppc/spapr_drc.c          |    1 
> > > > >  hw/ppc/spapr_pci.c          |    2 -
> > > > >  include/hw/pci-host/spapr.h |    2 +
> > > > >  include/hw/ppc/spapr.h      |    1 
> > > > >  5 files changed, 118 insertions(+), 2 deletions(-)
> > > > > 
> > > > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > > > > index 90485054c2e7..589f76ef9fb8 100644
> > > > > --- a/hw/ppc/spapr.c
> > > > > +++ b/hw/ppc/spapr.c
> > > > > @@ -2540,6 +2540,10 @@ static void ppc_spapr_init(MachineState 
> > > > > *machine)
> > > > >      register_savevm_live(NULL, "spapr/htab", -1, 1,
> > > > >                           &savevm_htab_handlers, spapr);
> > > > >  
> > > > > +    if (spapr->dr_phb_enabled) {
> > > > > +        qbus_set_hotplug_handler(sysbus_get_default(), 
> > > > > OBJECT(machine), NULL);
> > > > > +    }
> > > > > +
> > > > >      qemu_register_boot_set(spapr_boot_set, spapr);
> > > > >  
> > > > >      if (kvm_enabled()) {
> > > > > @@ -3238,6 +3242,103 @@ out:
> > > > >      error_propagate(errp, local_err);
> > > > >  }
> > > > >  
> > > > > +static void spapr_phb_pre_plug(HotplugHandler *hotplug_dev, 
> > > > > DeviceState *dev,
> > > > > +                               Error **errp)
> > > > > +{
> > > > > +    sPAPRPHBState *sphb = SPAPR_PCI_HOST_BRIDGE(dev);
> > > > > +
> > > > > +    if (sphb->drc_id == (uint32_t)-1) {
> > > > > +        sphb->drc_id = sphb->index;
> > > > > +    }
> > > > > +
> > > > > +    if (sphb->drc_id >= SPAPR_DRC_MAX_PHB) {
> > > > > +        error_setg(errp, "PHB id %d out of range", sphb->drc_id);
> > > > > +    }    
> > > > 
> > > > 
> > > > sphb->index in considered 16bits in the existing code (even though it is
> > > > defined as 32bit) and SPAPR_DRC_MAX_PHB is just 256. I'd suggest using 
> > > > the
> > > > same limit for both, either 256 or 65536 is fine for me.
> > > > 
> > > > It is actually a bit weird - it is possible to completely configure few
> > > > PHBs in the command line so they will have index==-1 but PCI hotplug 
> > > > code -
> > > > spapr_phb_get_pci_func_drc() and spapr_phb_realize() - does not check 
> > > > for
> > > > this and just does (sphb->index << 16).  
> > > 
> > > You're right and this looks like a bug... I'll try to come up with a fix.
> > >   
> > > > May be just ditch drc_id, enforce index not to be -1 and use it as 
> > > > drc_id?
> > > >   
> > > 
> > > This was how Mike did it in the original patchset but David suggested
> > > to introduce drc_id (to preserve existing setups I guess):
> > > 
> > > http://patchwork.ozlabs.org/patch/466262/  
> > 
> > Huh.  So I did.  But.. sorry, I've changed my mind.
> > 
> > The fact that needing a DRC forces us to have a reasonable small id
> > for each PHB seems like a good excuse to make index mandatory - I'm
> > not convinced anyone was actually creating PHBs without index, and
> > this does allow us to simplify a bunch of things.
> > 
> > I'd like to see that done as a preliminary cleanup patch, though.
> > 
> 
> Just to be sure. I could verify that the weirdness reported by Alexey
> causes QEMU to misbehave. Only the first "index-less" PHB has realized
> DRCs:
> 
> => subsequent "index-less" PHBs silently ignore hotplugging of PCI devices
> 
> => QEMU won't even start with coldplugged devices in these "index-less"
>    PHBs
> 
> This preliminary cleanup for hotpluggable PHBs is hence also a bug fix
> for current PHBs.

Ok.

> Do we want to fix this long-standing bug in 2.10 ?

No, not worth pushing in this late.

> Do we want to preserve the current buggy behavior for older machine
> types ?

No, I don't think so.  I think the reasonable course here is to push
the new behaviour out.  Only if someone complains that they were
actually relying on the old behaviour in the wild do we try to
preserve it for the older machine types.

-- 
David Gibson                    | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
                                | _way_ _around_!
http://www.ozlabs.org/~dgibson

signature.asc
Description: PGP signature