| Message ID | 20151130104331.11064.57278.stgit@bahia.huguette.org |
|---|---|
| State | New |
| Headers | show |
On 30/11/15 11:45, Greg Kurz wrote: > Since commit 1d2d974244c6 "spapr_pci: enumerate and add PCI device tree", QEMU > populates the PCI device tree in the opposite order compared to SLOF. > > Before 1d2d974244c6: > > Populating /pci@800000020000000 > 00 0000 (D) : 1af4 1000 virtio [ net ] > 00 0800 (D) : 1af4 1001 virtio [ block ] > 00 1000 (D) : 1af4 1009 virtio [ network ] > Populating /pci@800000020000000/unknown-legacy-device@2 > > > 7e5294b8 : /pci@800000020000000 > 7e52b998 : |-- ethernet@0 > 7e52c0c8 : |-- scsi@1 > 7e52c7e8 : +-- unknown-legacy-device@2 ok > > Since 1d2d974244c6: > > Populating /pci@800000020000000 > 00 1000 (D) : 1af4 1009 virtio [ network ] > Populating /pci@800000020000000/unknown-legacy-device@2 > 00 0800 (D) : 1af4 1001 virtio [ block ] > 00 0000 (D) : 1af4 1000 virtio [ net ] > > > 7e5e8118 : /pci@800000020000000 > 7e5ea6a0 : |-- unknown-legacy-device@2 > 7e5eadb8 : |-- scsi@1 > 7e5eb4d8 : +-- ethernet@0 ok > > This behaviour change is not actually a bug since no assumptions should be > made on DT ordering. But it has no real justification either, other than > being the consequence of the way fdt_add_subnode() inserts new elements > to the front of the FDT rather than adding them to the tail. > > This patch reverts to the historical SLOF ordering by walking PCI devices in > reverse order. I've applied your patch here locally, and indeed, the device tree looks nicer to me, too, when the nodes are listed in ascending order. Tested-by: Thomas Huth <thuth@redhat.com>
On Tue, 1 Dec 2015 22:48:38 +0100 Thomas Huth <thuth@redhat.com> wrote: > On 30/11/15 11:45, Greg Kurz wrote: > > Since commit 1d2d974244c6 "spapr_pci: enumerate and add PCI device tree", QEMU > > populates the PCI device tree in the opposite order compared to SLOF. > > > > Before 1d2d974244c6: > > > > Populating /pci@800000020000000 > > 00 0000 (D) : 1af4 1000 virtio [ net ] > > 00 0800 (D) : 1af4 1001 virtio [ block ] > > 00 1000 (D) : 1af4 1009 virtio [ network ] > > Populating /pci@800000020000000/unknown-legacy-device@2 > > > > > > 7e5294b8 : /pci@800000020000000 > > 7e52b998 : |-- ethernet@0 > > 7e52c0c8 : |-- scsi@1 > > 7e52c7e8 : +-- unknown-legacy-device@2 ok > > > > Since 1d2d974244c6: > > > > Populating /pci@800000020000000 > > 00 1000 (D) : 1af4 1009 virtio [ network ] > > Populating /pci@800000020000000/unknown-legacy-device@2 > > 00 0800 (D) : 1af4 1001 virtio [ block ] > > 00 0000 (D) : 1af4 1000 virtio [ net ] > > > > > > 7e5e8118 : /pci@800000020000000 > > 7e5ea6a0 : |-- unknown-legacy-device@2 > > 7e5eadb8 : |-- scsi@1 > > 7e5eb4d8 : +-- ethernet@0 ok > > > > This behaviour change is not actually a bug since no assumptions should be > > made on DT ordering. But it has no real justification either, other than > > being the consequence of the way fdt_add_subnode() inserts new elements > > to the front of the FDT rather than adding them to the tail. > > > > This patch reverts to the historical SLOF ordering by walking PCI devices in > > reverse order. > > I've applied your patch here locally, and indeed, the device tree looks > nicer to me, too, when the nodes are listed in ascending order. > > Tested-by: Thomas Huth <thuth@redhat.com> > > Thanks for testing ! Cheers. -- Greg
On Thu, 3 Dec 2015 15:53:17 +0100 Greg Kurz <gkurz@linux.vnet.ibm.com> wrote: > On Tue, 1 Dec 2015 22:48:38 +0100 > Thomas Huth <thuth@redhat.com> wrote: > > > On 30/11/15 11:45, Greg Kurz wrote: > > > Since commit 1d2d974244c6 "spapr_pci: enumerate and add PCI device tree", QEMU > > > populates the PCI device tree in the opposite order compared to SLOF. > > > > > > Before 1d2d974244c6: > > > > > > Populating /pci@800000020000000 > > > 00 0000 (D) : 1af4 1000 virtio [ net ] > > > 00 0800 (D) : 1af4 1001 virtio [ block ] > > > 00 1000 (D) : 1af4 1009 virtio [ network ] > > > Populating /pci@800000020000000/unknown-legacy-device@2 > > > > > > > > > 7e5294b8 : /pci@800000020000000 > > > 7e52b998 : |-- ethernet@0 > > > 7e52c0c8 : |-- scsi@1 > > > 7e52c7e8 : +-- unknown-legacy-device@2 ok > > > > > > Since 1d2d974244c6: > > > > > > Populating /pci@800000020000000 > > > 00 1000 (D) : 1af4 1009 virtio [ network ] > > > Populating /pci@800000020000000/unknown-legacy-device@2 > > > 00 0800 (D) : 1af4 1001 virtio [ block ] > > > 00 0000 (D) : 1af4 1000 virtio [ net ] > > > > > > > > > 7e5e8118 : /pci@800000020000000 > > > 7e5ea6a0 : |-- unknown-legacy-device@2 > > > 7e5eadb8 : |-- scsi@1 > > > 7e5eb4d8 : +-- ethernet@0 ok > > > > > > This behaviour change is not actually a bug since no assumptions should be > > > made on DT ordering. But it has no real justification either, other than > > > being the consequence of the way fdt_add_subnode() inserts new elements > > > to the front of the FDT rather than adding them to the tail. > > > > > > This patch reverts to the historical SLOF ordering by walking PCI devices in > > > reverse order. > > > > I've applied your patch here locally, and indeed, the device tree looks > > nicer to me, too, when the nodes are listed in ascending order. > > > > Tested-by: Thomas Huth <thuth@redhat.com> > > > > > Ping ? > Thanks for testing ! > > Cheers. > > -- > Greg > >
On Thu, Dec 17, 2015 at 09:43:29AM +0100, Greg Kurz wrote: > On Thu, 3 Dec 2015 15:53:17 +0100 > Greg Kurz <gkurz@linux.vnet.ibm.com> wrote: > > > On Tue, 1 Dec 2015 22:48:38 +0100 > > Thomas Huth <thuth@redhat.com> wrote: > > > > > On 30/11/15 11:45, Greg Kurz wrote: > > > > Since commit 1d2d974244c6 "spapr_pci: enumerate and add PCI device tree", QEMU > > > > populates the PCI device tree in the opposite order compared to SLOF. > > > > > > > > Before 1d2d974244c6: > > > > > > > > Populating /pci@800000020000000 > > > > 00 0000 (D) : 1af4 1000 virtio [ net ] > > > > 00 0800 (D) : 1af4 1001 virtio [ block ] > > > > 00 1000 (D) : 1af4 1009 virtio [ network ] > > > > Populating /pci@800000020000000/unknown-legacy-device@2 > > > > > > > > > > > > 7e5294b8 : /pci@800000020000000 > > > > 7e52b998 : |-- ethernet@0 > > > > 7e52c0c8 : |-- scsi@1 > > > > 7e52c7e8 : +-- unknown-legacy-device@2 ok > > > > > > > > Since 1d2d974244c6: > > > > > > > > Populating /pci@800000020000000 > > > > 00 1000 (D) : 1af4 1009 virtio [ network ] > > > > Populating /pci@800000020000000/unknown-legacy-device@2 > > > > 00 0800 (D) : 1af4 1001 virtio [ block ] > > > > 00 0000 (D) : 1af4 1000 virtio [ net ] > > > > > > > > > > > > 7e5e8118 : /pci@800000020000000 > > > > 7e5ea6a0 : |-- unknown-legacy-device@2 > > > > 7e5eadb8 : |-- scsi@1 > > > > 7e5eb4d8 : +-- ethernet@0 ok > > > > > > > > This behaviour change is not actually a bug since no assumptions should be > > > > made on DT ordering. But it has no real justification either, other than > > > > being the consequence of the way fdt_add_subnode() inserts new elements > > > > to the front of the FDT rather than adding them to the tail. > > > > > > > > This patch reverts to the historical SLOF ordering by walking PCI devices in > > > > reverse order. > > > > > > I've applied your patch here locally, and indeed, the device tree looks > > > nicer to me, too, when the nodes are listed in ascending order. > > > > > > Tested-by: Thomas Huth <thuth@redhat.com> > > > > > > > > > > Ping ? Sorry I didn't reply. I'm still dubious about this. It seems like a fair bit of effort to restore a behaviour that the client isn't supposed to be relying on anyway. Plus, the version with the changed order is already released, so applying this will mean a second behaviour change.
On Mon, 21 Dec 2015 12:56:24 +1100 David Gibson <david@gibson.dropbear.id.au> wrote: > On Thu, Dec 17, 2015 at 09:43:29AM +0100, Greg Kurz wrote: > > On Thu, 3 Dec 2015 15:53:17 +0100 > > Greg Kurz <gkurz@linux.vnet.ibm.com> wrote: > > > > > On Tue, 1 Dec 2015 22:48:38 +0100 > > > Thomas Huth <thuth@redhat.com> wrote: > > > > > > > On 30/11/15 11:45, Greg Kurz wrote: > > > > > Since commit 1d2d974244c6 "spapr_pci: enumerate and add PCI device tree", QEMU > > > > > populates the PCI device tree in the opposite order compared to SLOF. > > > > > > > > > > Before 1d2d974244c6: > > > > > > > > > > Populating /pci@800000020000000 > > > > > 00 0000 (D) : 1af4 1000 virtio [ net ] > > > > > 00 0800 (D) : 1af4 1001 virtio [ block ] > > > > > 00 1000 (D) : 1af4 1009 virtio [ network ] > > > > > Populating /pci@800000020000000/unknown-legacy-device@2 > > > > > > > > > > > > > > > 7e5294b8 : /pci@800000020000000 > > > > > 7e52b998 : |-- ethernet@0 > > > > > 7e52c0c8 : |-- scsi@1 > > > > > 7e52c7e8 : +-- unknown-legacy-device@2 ok > > > > > > > > > > Since 1d2d974244c6: > > > > > > > > > > Populating /pci@800000020000000 > > > > > 00 1000 (D) : 1af4 1009 virtio [ network ] > > > > > Populating /pci@800000020000000/unknown-legacy-device@2 > > > > > 00 0800 (D) : 1af4 1001 virtio [ block ] > > > > > 00 0000 (D) : 1af4 1000 virtio [ net ] > > > > > > > > > > > > > > > 7e5e8118 : /pci@800000020000000 > > > > > 7e5ea6a0 : |-- unknown-legacy-device@2 > > > > > 7e5eadb8 : |-- scsi@1 > > > > > 7e5eb4d8 : +-- ethernet@0 ok > > > > > > > > > > This behaviour change is not actually a bug since no assumptions should be > > > > > made on DT ordering. But it has no real justification either, other than > > > > > being the consequence of the way fdt_add_subnode() inserts new elements > > > > > to the front of the FDT rather than adding them to the tail. > > > > > > > > > > This patch reverts to the historical SLOF ordering by walking PCI devices in > > > > > reverse order. > > > > > > > > I've applied your patch here locally, and indeed, the device tree looks > > > > nicer to me, too, when the nodes are listed in ascending order. > > > > > > > > Tested-by: Thomas Huth <thuth@redhat.com> > > > > > > > > > > > > > > > Ping ? > > Sorry I didn't reply. > > I'm still dubious about this. It seems like a fair bit of effort to > restore a behaviour that the client isn't supposed to be relying on > anyway. > > Plus, the version with the changed order is already released, so > applying this will mean a second behaviour change. > And since nobody apart from Thomas expressed interest, I guess it is not something people dearly want. Just forget about this patch. :) Cheers. -- Greg
David Gibson <david@gibson.dropbear.id.au> writes: > On Thu, Dec 17, 2015 at 09:43:29AM +0100, Greg Kurz wrote: >> On Thu, 3 Dec 2015 15:53:17 +0100 >> Greg Kurz <gkurz@linux.vnet.ibm.com> wrote: >> >> > On Tue, 1 Dec 2015 22:48:38 +0100 >> > Thomas Huth <thuth@redhat.com> wrote: >> > >> > > On 30/11/15 11:45, Greg Kurz wrote: >> > > > Since commit 1d2d974244c6 "spapr_pci: enumerate and add PCI device tree", QEMU >> > > > populates the PCI device tree in the opposite order compared to SLOF. >> > > > >> > > > Before 1d2d974244c6: >> > > > >> > > > Populating /pci@800000020000000 >> > > > 00 0000 (D) : 1af4 1000 virtio [ net ] >> > > > 00 0800 (D) : 1af4 1001 virtio [ block ] >> > > > 00 1000 (D) : 1af4 1009 virtio [ network ] >> > > > Populating /pci@800000020000000/unknown-legacy-device@2 >> > > > >> > > > >> > > > 7e5294b8 : /pci@800000020000000 >> > > > 7e52b998 : |-- ethernet@0 >> > > > 7e52c0c8 : |-- scsi@1 >> > > > 7e52c7e8 : +-- unknown-legacy-device@2 ok >> > > > >> > > > Since 1d2d974244c6: >> > > > >> > > > Populating /pci@800000020000000 >> > > > 00 1000 (D) : 1af4 1009 virtio [ network ] >> > > > Populating /pci@800000020000000/unknown-legacy-device@2 >> > > > 00 0800 (D) : 1af4 1001 virtio [ block ] >> > > > 00 0000 (D) : 1af4 1000 virtio [ net ] >> > > > >> > > > >> > > > 7e5e8118 : /pci@800000020000000 >> > > > 7e5ea6a0 : |-- unknown-legacy-device@2 >> > > > 7e5eadb8 : |-- scsi@1 >> > > > 7e5eb4d8 : +-- ethernet@0 ok >> > > > >> > > > This behaviour change is not actually a bug since no assumptions should be >> > > > made on DT ordering. But it has no real justification either, other than >> > > > being the consequence of the way fdt_add_subnode() inserts new elements >> > > > to the front of the FDT rather than adding them to the tail. >> > > > >> > > > This patch reverts to the historical SLOF ordering by walking PCI devices in >> > > > reverse order. >> > > >> > > I've applied your patch here locally, and indeed, the device tree looks >> > > nicer to me, too, when the nodes are listed in ascending order. >> > > >> > > Tested-by: Thomas Huth <thuth@redhat.com> >> > > >> > > >> > >> >> Ping ? > > Sorry I didn't reply. > > I'm still dubious about this. It seems like a fair bit of effort to > restore a behaviour that the client isn't supposed to be relying on > anyway. > > Plus, the version with the changed order is already released, so > applying this will mean a second behaviour change. The behaviour change was not intentional by me, so I would vote for restoring the old order. Reviewed-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com> Regards Nikunj
diff --git a/hw/pci/pci.c b/hw/pci/pci.c index 168b9cc56b69..3e95745900fc 100644 --- a/hw/pci/pci.c +++ b/hw/pci/pci.c @@ -1420,6 +1420,34 @@ static const pci_class_desc pci_class_descriptions[] = { 0, NULL} }; +static void pci_for_each_device_under_bus_reverse(PCIBus *bus, + void (*fn)(PCIBus *b, + PCIDevice *d, + void *opaque), + void *opaque) +{ + PCIDevice *d; + int devfn; + + for (devfn = 0; devfn < ARRAY_SIZE(bus->devices); devfn++) { + d = bus->devices[ARRAY_SIZE(bus->devices) - 1 - devfn]; + if (d) { + fn(bus, d, opaque); + } + } +} + +void pci_for_each_device_reverse(PCIBus *bus, int bus_num, + void (*fn)(PCIBus *b, PCIDevice *d, void *opaque), + void *opaque) +{ + bus = pci_find_bus_nr(bus, bus_num); + + if (bus) { + pci_for_each_device_under_bus_reverse(bus, fn, opaque); + } +} + static void pci_for_each_device_under_bus(PCIBus *bus, void (*fn)(PCIBus *b, PCIDevice *d, void *opaque), diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c index 55fa8db9e290..b3c9294af74d 100644 --- a/hw/ppc/spapr_pci.c +++ b/hw/ppc/spapr_pci.c @@ -1612,9 +1612,9 @@ static void spapr_populate_pci_devices_dt(PCIBus *bus, PCIDevice *pdev, s_fdt.fdt = p->fdt; s_fdt.node_off = offset; s_fdt.sphb = p->sphb; - pci_for_each_device(sec_bus, pci_bus_num(sec_bus), - spapr_populate_pci_devices_dt, - &s_fdt); + pci_for_each_device_reverse(sec_bus, pci_bus_num(sec_bus), + spapr_populate_pci_devices_dt, + &s_fdt); } static void spapr_phb_pci_enumerate_bridge(PCIBus *bus, PCIDevice *pdev, @@ -1755,9 +1755,9 @@ int spapr_populate_pci_dt(sPAPRPHBState *phb, s_fdt.fdt = fdt; s_fdt.node_off = bus_off; s_fdt.sphb = phb; - pci_for_each_device(bus, pci_bus_num(bus), - spapr_populate_pci_devices_dt, - &s_fdt); + pci_for_each_device_reverse(bus, pci_bus_num(bus), + spapr_populate_pci_devices_dt, + &s_fdt); ret = spapr_drc_populate_dt(fdt, bus_off, OBJECT(phb), SPAPR_DR_CONNECTOR_TYPE_PCI); diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h index 379b6e1a4561..465483b2ee79 100644 --- a/include/hw/pci/pci.h +++ b/include/hw/pci/pci.h @@ -393,6 +393,10 @@ int pci_bus_numa_node(PCIBus *bus); void pci_for_each_device(PCIBus *bus, int bus_num, void (*fn)(PCIBus *bus, PCIDevice *d, void *opaque), void *opaque); +void pci_for_each_device_reverse(PCIBus *bus, int bus_num, + void (*fn)(PCIBus *bus, PCIDevice *d, + void *opaque), + void *opaque); void pci_for_each_bus_depth_first(PCIBus *bus, void *(*begin)(PCIBus *bus, void *parent_state), void (*end)(PCIBus *bus, void *state),
Since commit 1d2d974244c6 "spapr_pci: enumerate and add PCI device tree", QEMU populates the PCI device tree in the opposite order compared to SLOF. Before 1d2d974244c6: Populating /pci@800000020000000 00 0000 (D) : 1af4 1000 virtio [ net ] 00 0800 (D) : 1af4 1001 virtio [ block ] 00 1000 (D) : 1af4 1009 virtio [ network ] Populating /pci@800000020000000/unknown-legacy-device@2 7e5294b8 : /pci@800000020000000 7e52b998 : |-- ethernet@0 7e52c0c8 : |-- scsi@1 7e52c7e8 : +-- unknown-legacy-device@2 ok Since 1d2d974244c6: Populating /pci@800000020000000 00 1000 (D) : 1af4 1009 virtio [ network ] Populating /pci@800000020000000/unknown-legacy-device@2 00 0800 (D) : 1af4 1001 virtio [ block ] 00 0000 (D) : 1af4 1000 virtio [ net ] 7e5e8118 : /pci@800000020000000 7e5ea6a0 : |-- unknown-legacy-device@2 7e5eadb8 : |-- scsi@1 7e5eb4d8 : +-- ethernet@0 ok This behaviour change is not actually a bug since no assumptions should be made on DT ordering. But it has no real justification either, other than being the consequence of the way fdt_add_subnode() inserts new elements to the front of the FDT rather than adding them to the tail. This patch reverts to the historical SLOF ordering by walking PCI devices in reverse order. Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com> --- Repost with Cc: to qemu-devel and qemu-ppc :) Michael, David, I hope it is okay that this is a single patch to be applied by any of you. Otherwise I can post distinct patches for PCI and sPAPR. hw/pci/pci.c | 28 ++++++++++++++++++++++++++++ hw/ppc/spapr_pci.c | 12 ++++++------ include/hw/pci/pci.h | 4 ++++ 3 files changed, 38 insertions(+), 6 deletions(-)