On Tue, 8 Aug 2017 16:16:36 +1000 David Gibson <da...@gibson.dropbear.id.au> 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 <gr...@kaod.org> > > 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; > > + } > > } > > } > > > > >
pgpUII_gdAgV4.pgp
Description: OpenPGP digital signature