Re: [PATCH] spapr_pci: Robustify support of PCI bridges

[Greg Kurz <groug@kaod.org>]

[-- Attachment #1: Type: text/plain, Size: 6909 bytes --]

On Thu, 16 Jul 2020 14:45:40 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Thu, Jul 09, 2020 at 07:12:47PM +0200, Greg Kurz wrote:
> > Some recent error handling cleanups unveiled issues with our support of
> > PCI bridges:
> > 
> > 1) QEMU aborts when using non-standard PCI bridge types,
> >    unveiled by commit 7ef1553dac "spapr_pci: Drop some dead error handling"
> > 
> > $ qemu-system-ppc64 -M pseries -device pcie-pci-bridge
> > Unexpected error in object_property_find() at qom/object.c:1240:
> > qemu-system-ppc64: -device pcie-pci-bridge: Property '.chassis_nr' not found
> > Aborted (core dumped)
> 
> Oops, I thought we had a check that we actually had a "pci-bridge"
> device before continuing with the hotplug, but I guess not.
> 

Ah... are you suggesting we should explicitly check the actual type
of the bridge rather than looking for the "chassis_nr" property ?

> > This happens because we assume all PCI bridge types to have a "chassis_nr"
> > property. This property only exists with the standard PCI bridge type
> > "pci-bridge" actually. We could possibly revert 7ef1553dac but it seems
> > much simpler to check the presence of "chassis_nr" earlier.
> 
> Hrm, right, 7ef1553dac was not really correct since add_drcs() really
> can fail.
> 

Yes.

> > 2) QEMU abort if same "chassis_nr" value is used several times,
> >    unveiled by commit d2623129a7de "qom: Drop parameter @errp of
> >    object_property_add() & friends"
> > 
> > $ qemu-system-ppc64 -M pseries -device pci-bridge,chassis_nr=1 \
> >                         -device pci-bridge,chassis_nr=1
> > Unexpected error in object_property_try_add() at qom/object.c:1167:
> > qemu-system-ppc64: -device pci-bridge,chassis_nr=1: attempt to add duplicate property '40000100' to object (type 'container')
> > Aborted (core dumped)
> > 
> > This happens because we assume that "chassis_nr" values are unique, but
> > nobody enforces that and we end up generating duplicate DRC ids. The PCI
> > code doesn't really care for duplicate "chassis_nr" properties since it
> > is only used to initialize the "Chassis Number Register" of the bridge,
> > with no functional impact on QEMU. So, even if passing the same value
> > several times might look weird, it never broke anything before, so
> > I guess we don't necessarily want to enforce strict checking in the PCI
> > code now.
> 
> Yeah, I guess.  I'm pretty sure that the chassis number of bridges is
> supposed to be system-unique (well, unique within the PCI domain at
> least, I guess) as part of the hardware spec.  So specifying multiple
> chassis ids the same is a user error, but we need a better failure
> mode.
> 

According to section 3.2.6.4 of "PCI-to-PCI Bridge Architecture
Specification", the chassis number is exposed to the OS as a 
non-volatile r/w register. It seems to be expected that chassis
numbers might collide, in which case the system software can
overwrite the register with a new number. So I'm not sure that
specifying the same number multiple times is an actual user error.

> > Workaround both issues in the PAPR code: check that the bridge has a
> > unique and non null "chassis_nr" when plugging it into its parent bus.
> >
> > Fixes: 05929a6c5dfe ("spapr: Don't use bus number for building DRC ids")
> 
> Arguably, it's really fixing 7ef1553dac.
> 

True for issue 1 but not for issue 2, which is the result of
05929a6c5dfe (switch to "chassis_nr" introduces a condition
to end up with duplicate DRC ids) and d2623129a7de (assert
when trying to add a duplicated DRC).

I'm starting to think I should maybe split this in
two patches. One for each issue.

> > Reported-by: Thomas Huth <thuth@redhat.com>
> > Signed-off-by: Greg Kurz <groug@kaod.org>
> 
> I had a few misgivings about the details of this, but I think I've
> convinced myself they're fine.  There's a couple of things I'd like to
> polish, but I'll do that as a follow up.
> 

Some fixes for d2623129a7de just got merged. Let me have a look
again.

> > ---
> >  hw/ppc/spapr_pci.c |   57 ++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 57 insertions(+)
> > 
> > diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> > index 329002ac040e..09d52ef7954d 100644
> > --- a/hw/ppc/spapr_pci.c
> > +++ b/hw/ppc/spapr_pci.c
> > @@ -1480,6 +1480,57 @@ static void spapr_pci_bridge_plug(SpaprPhbState *phb,
> >      add_drcs(phb, bus);
> >  }
> >  
> > +/* Returns non-zero if the value of "chassis_nr" is already in use */
> > +static int check_chassis_nr(Object *obj, void *opaque)
> > +{
> > +    int new_chassis_nr =
> > +        object_property_get_uint(opaque, "chassis_nr", &error_abort);
> > +    int chassis_nr =
> > +        object_property_get_uint(obj, "chassis_nr", NULL);
> > +
> > +    if (!object_dynamic_cast(obj, TYPE_PCI_BRIDGE)) {
> > +        return 0;
> > +    }
> > +
> > +    /* Skip unsupported bridge types */
> > +    if (!chassis_nr) {
> > +        return 0;
> > +    }
> > +
> > +    /* Skip self */
> > +    if (obj == opaque) {
> > +        return 0;
> > +    }
> > +
> > +    return chassis_nr == new_chassis_nr;
> > +}
> > +
> > +static bool bridge_has_valid_chassis_nr(Object *bridge, Error **errp)
> > +{
> > +    int chassis_nr =
> > +        object_property_get_uint(bridge, "chassis_nr", NULL);
> > +
> > +    /*
> > +     * slotid_cap_init() already ensures that "chassis_nr" isn't null for
> > +     * standard PCI bridges, so this really tells if "chassis_nr" is present
> > +     * or not.
> > +     */
> > +    if (!chassis_nr) {
> > +        error_setg(errp, "PCI Bridge lacks a \"chassis_nr\" property");
> > +        error_append_hint(errp, "Try -device pci-bridge instead.\n");
> > +        return false;
> > +    }
> > +
> > +    /* We want unique values for "chassis_nr" */
> > +    if (object_child_foreach_recursive(object_get_root(), check_chassis_nr,
> > +                                       bridge)) {
> > +        error_setg(errp, "Bridge chassis %d already in use", chassis_nr);
> > +        return false;
> > +    }
> > +
> > +    return true;
> > +}
> > +
> >  static void spapr_pci_plug(HotplugHandler *plug_handler,
> >                             DeviceState *plugged_dev, Error **errp)
> >  {
> > @@ -1491,6 +1542,12 @@ static void spapr_pci_plug(HotplugHandler *plug_handler,
> >      PCIBus *bus = PCI_BUS(qdev_get_parent_bus(DEVICE(pdev)));
> >      uint32_t slotnr = PCI_SLOT(pdev->devfn);
> >  
> > +    if (pc->is_bridge) {
> > +        if (!bridge_has_valid_chassis_nr(OBJECT(plugged_dev), errp)) {
> > +            return;
> > +        }
> > +    }
> > +
> >      /* if DR is disabled we don't need to do anything in the case of
> >       * hotplug or coldplug callbacks
> >       */
> > 
> > 
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]