| Message ID | 150212670348.12227.5534815630591997333.stgit@bahia |
|---|---|
| State | New |
| Headers | show |
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 <groug@kaod.org> Given that the bug isn't a regression, I'm a bit disinclined to merge 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 think skip straight to the real fix of making index madatory in 2.11. > --- > 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; > + } > } > } > >
On Tue, 8 Aug 2017 16:16:36 +1000 David Gibson <david@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 <groug@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; > > + } > > } > > } > > > > >
On Tue, Aug 08, 2017 at 11:18:35AM +0200, Greg Kurz wrote: > On Tue, 8 Aug 2017 16:16:36 +1000 > David Gibson <david@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 <groug@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. Ah, yeah that is a bit nasty. Still, I'm not comfortable with the complexity of patch required to fix it this late in 2.10. > > > 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; > > > + } > > > } > > > } > > > > > > > > >
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; + } } }
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 <groug@kaod.org> --- hw/ppc/spapr_pci.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)