Re: [Qemu-devel] [for-2.10 PATCH 3/3] spapr: error out if PHB fails to setup PCI DRCs

[Greg Kurz]

On Tue, 8 Aug 2017 16:16:36 +1000
David Gibson <address@hidden> wrote:

> On Mon, Aug 07, 2017 at 07:25:03PM +0200, Greg Kurz wrote:
> > It is currently possible to start QEMU with two PHBs without using the
> > index property:
> > 
> > -device spapr-pci-host-bridge,id=pci1,\
> >                               buid=0x800000020000001,\
> >                               liobn=0x80000100,\
> >                               liobn64=0x80000101,\
> >                               mem_win_addr=0x200100000000,\
> >                               mem64_win_addr=0x220000000000,\
> >                               io_win_addr=0x200000010000 \
> > -device spapr-pci-host-bridge,id=pci2,\
> >                               buid=0x800000020000002,\
> >                               liobn=0x80000200,\
> >                               liobn64=0x80000201,\
> >                               mem_win_addr=0x200180000000,\
> >                               mem64_win_addr=0x230000000000,\
> >                               io_win_addr=0x200000020000 \
> > 
> > Each PHB has its index property equal to -1. As a consequence, each PHB
> > will want to create PCI DRCs with the same ids:
> > 
> >     /* allocate connectors for child PCI devices */
> >     if (sphb->dr_enabled) {
> >         for (i = 0; i < PCI_SLOT_MAX * 8; i++) {
> >             spapr_dr_connector_new(OBJECT(phb), TYPE_SPAPR_DRC_PCI,
> >                                    (sphb->index << 16) | i);
> >         }
> >     }
> > 
> > When DRC objects are added to the composition tree, an alias using the
> > DRC id is created in the "/dr-connector" path. But DRC ids are supposed
> > to be globally unique and the alias creation fails, leaving the DRC
> > object unrealized, which isn't expected by the rest of the DR logic.
> > 
> > This has the effect of silently turning-off PCI hotplug support (ie, PCI
> > hotplug no longer works on any PHB and no error message is printed).
> > 
> > This bug always existed and would even cause a non-fatal error to be
> > reported on the console (until recent commit bf26ae32a92a). This patch
> > causes the error message to be printed again and QEMU to exit.
> > 
> > Signed-off-by: Greg Kurz <address@hidden>  
> 
> Given that the bug isn't a regression, I'm a bit disinclined to merge

FWIW, 2.9 would at least print an error message, but 2.10 doesn't because
of commit bf26ae32a92a.

> patches 2&3 this late in 2.10.
> 
> It just seems bogus to have all this code to (supposedly) allow
> bridges without an index, but then have it error out if there's more
> than one of them.
> 

I agree this is weird.

> I think skip straight to the real fix of making index madatory in 2.11.
> 

Ok.

> > ---
> >  hw/ppc/spapr_pci.c |    9 ++++++++-
> >  1 file changed, 8 insertions(+), 1 deletion(-)
> > 
> > diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> > index 4b7882e3613d..4a1e6c5f697c 100644
> > --- a/hw/ppc/spapr_pci.c
> > +++ b/hw/ppc/spapr_pci.c
> > @@ -1731,9 +1731,16 @@ static void spapr_phb_realize(DeviceState *dev, 
> > Error **errp)
> >  
> >      /* allocate connectors for child PCI devices */
> >      if (sphb->dr_enabled) {
> > +        Error *local_err = NULL;
> > +
> >          for (i = 0; i < PCI_SLOT_MAX * 8; i++) {
> >              spapr_dr_connector_new(OBJECT(phb), TYPE_SPAPR_DRC_PCI,
> > -                                   (sphb->index << 16) | i, NULL);
> > +                                   (sphb->index << 16) | i, &local_err);
> > +            if (local_err) {
> > +                error_propagate(errp, local_err);
> > +                error_prepend(errp, "Failed to create DRC: ");
> > +                return;
> > +            }
> >          }
> >      }
> >  
> >   
>

pgpfY0sorGgs4.pgp
Description: OpenPGP digital signature