Re: [Qemu-devel] [PATCH] hw/arm/virt: Fix address in PCIe device tree node's unit name
On 23 October 2015 at 16:26, Alexander Gordeev wrote: > On Fri, Oct 23, 2015 at 03:18:34PM +0100, Peter Maydell wrote: >> On 21 October 2015 at 23:01, Alexander Gordeev wrote: >> > On Wed, Oct 21, 2015 at 10:26:27PM +0100, Peter Maydell wrote: >> >> On 21 October 2015 at 21:43, Alexander Gordeev >> >> wrote: >> >> > PCIe device tree unit name is pcie@1000 - which denotes >> >> > IO space base address. However, the corresponding node's >> >> > "reg" property points to PCI configuration space base address >> >> > 0x3f00. >> >> > >> >> > Set the unit name to pcie@3f00 which is not only correct, >> >> > but also conforms to Open Firmware (IEEE 1275). >> >> >> >> Nothing should actually care about the address in the >> >> nodename, though, right -- it's just for human readability >> >> and debugging (and guests will be looking at the regs >> >> etc properties of the node to figure out where it is)? >> >> Or have I misunderstood this and there's an actual visible >> >> consequence to this bug? >> > >> > I do not think there are actual consequences out there. >> > It is just misleading and does not honour the standard. >> >> Do you have a more precise reference than just "IEEE 1275" ? >> I found the bit that says node names should be driver-name@unit-address, >> and unit address is the "text representation of the physical address >> of the device", but it seems to me that our current choice of >> "the lowest physical address where you can find any part of this >> device" is closer to that than deciding that we should use the >> address of the config space window instead. > > No, I do not have it other than indirectly (though I probably could locate > it). > I relied on Linux Documentation/devicetree/bindings/pci/host-generic-pci.txt > instead: > > - reg: The Configuration Space base address and size, as accessed >from the parent bus. [...] Yes, we do this correctly. This has nothing to do with the nodename, though, does it? > IMHO it perfectly make sense - either io or memory mapped regions are rather > used to map attached devices, while the unit name for a host bridge is implied > to "address" the bridge itself, isn't it? The config space is also just talking to attached devices. The 'generic' host bridge itself has no registers at all. Well, the only purpose as far as I can tell for the addr part of the node name is: * to make sure it's unique * as a reasonably easy way to remember which one it is if there are several of a device in the system And using the lowest address in the map that the PCI device uses seems as good a way as any other. So unless you have a really strong definition from the standard for why this should be the config space in particular I don't really think there's any need to change it. thanks -- PMM
Re: [Qemu-devel] [PATCH] hw/arm/virt: Fix address in PCIe device tree node's unit name
On Fri, Oct 23, 2015 at 03:18:34PM +0100, Peter Maydell wrote: > On 21 October 2015 at 23:01, Alexander Gordeev wrote: > > On Wed, Oct 21, 2015 at 10:26:27PM +0100, Peter Maydell wrote: > >> On 21 October 2015 at 21:43, Alexander Gordeev wrote: > >> > PCIe device tree unit name is pcie@1000 - which denotes > >> > IO space base address. However, the corresponding node's > >> > "reg" property points to PCI configuration space base address > >> > 0x3f00. > >> > > >> > Set the unit name to pcie@3f00 which is not only correct, > >> > but also conforms to Open Firmware (IEEE 1275). > >> > >> Nothing should actually care about the address in the > >> nodename, though, right -- it's just for human readability > >> and debugging (and guests will be looking at the regs > >> etc properties of the node to figure out where it is)? > >> Or have I misunderstood this and there's an actual visible > >> consequence to this bug? > > > > I do not think there are actual consequences out there. > > It is just misleading and does not honour the standard. > > Do you have a more precise reference than just "IEEE 1275" ? > I found the bit that says node names should be driver-name@unit-address, > and unit address is the "text representation of the physical address > of the device", but it seems to me that our current choice of > "the lowest physical address where you can find any part of this > device" is closer to that than deciding that we should use the > address of the config space window instead. No, I do not have it other than indirectly (though I probably could locate it). I relied on Linux Documentation/devicetree/bindings/pci/host-generic-pci.txt instead: - reg: The Configuration Space base address and size, as accessed from the parent bus. [...] IMHO it perfectly make sense - either io or memory mapped regions are rather used to map attached devices, while the unit name for a host bridge is implied to "address" the bridge itself, isn't it? Thanks! > thanks > -- PMM -- Regards, Alexander Gordeev agord...@redhat.com
Re: [Qemu-devel] [PATCH] hw/arm/virt: Fix address in PCIe device tree node's unit name
On 21 October 2015 at 23:01, Alexander Gordeev wrote: > On Wed, Oct 21, 2015 at 10:26:27PM +0100, Peter Maydell wrote: >> On 21 October 2015 at 21:43, Alexander Gordeev wrote: >> > PCIe device tree unit name is pcie@1000 - which denotes >> > IO space base address. However, the corresponding node's >> > "reg" property points to PCI configuration space base address >> > 0x3f00. >> > >> > Set the unit name to pcie@3f00 which is not only correct, >> > but also conforms to Open Firmware (IEEE 1275). >> >> Nothing should actually care about the address in the >> nodename, though, right -- it's just for human readability >> and debugging (and guests will be looking at the regs >> etc properties of the node to figure out where it is)? >> Or have I misunderstood this and there's an actual visible >> consequence to this bug? > > I do not think there are actual consequences out there. > It is just misleading and does not honour the standard. Do you have a more precise reference than just "IEEE 1275" ? I found the bit that says node names should be driver-name@unit-address, and unit address is the "text representation of the physical address of the device", but it seems to me that our current choice of "the lowest physical address where you can find any part of this device" is closer to that than deciding that we should use the address of the config space window instead. thanks -- PMM
Re: [Qemu-devel] [PATCH] hw/arm/virt: Fix address in PCIe device tree node's unit name
On 21 October 2015 at 21:43, Alexander Gordeev wrote: > PCIe device tree unit name is pcie@1000 - which denotes > IO space base address. However, the corresponding node's > "reg" property points to PCI configuration space base address > 0x3f00. > > Set the unit name to pcie@3f00 which is not only correct, > but also conforms to Open Firmware (IEEE 1275). Nothing should actually care about the address in the nodename, though, right -- it's just for human readability and debugging (and guests will be looking at the regs etc properties of the node to figure out where it is)? Or have I misunderstood this and there's an actual visible consequence to this bug? thanks -- PMM
Re: [Qemu-devel] [PATCH] hw/arm/virt: Fix address in PCIe device tree node's unit name
On Wed, Oct 21, 2015 at 10:26:27PM +0100, Peter Maydell wrote: > On 21 October 2015 at 21:43, Alexander Gordeev wrote: > > PCIe device tree unit name is pcie@1000 - which denotes > > IO space base address. However, the corresponding node's > > "reg" property points to PCI configuration space base address > > 0x3f00. > > > > Set the unit name to pcie@3f00 which is not only correct, > > but also conforms to Open Firmware (IEEE 1275). > > Nothing should actually care about the address in the > nodename, though, right -- it's just for human readability > and debugging (and guests will be looking at the regs > etc properties of the node to figure out where it is)? > Or have I misunderstood this and there's an actual visible > consequence to this bug? I do not think there are actual consequences out there. It is just misleading and does not honour the standard. > thanks > -- PMM -- Regards, Alexander Gordeev agord...@redhat.com
[Qemu-devel] [PATCH] hw/arm/virt: Fix address in PCIe device tree node's unit name
PCIe device tree unit name is pcie@1000 - which denotes
IO space base address. However, the corresponding node's
"reg" property points to PCI configuration space base address
0x3f00.
Set the unit name to pcie@3f00 which is not only correct,
but also conforms to Open Firmware (IEEE 1275).
CC: Peter Maydell
CC: Andrew Jones
Signed-off-by: Alexander Gordeev
---
hw/arm/virt.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 5d38c47..567be08 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -740,7 +740,6 @@ static void create_pcie(const VirtBoardInfo *vbi, qemu_irq
*pic,
hwaddr size_pio = vbi->memmap[VIRT_PCIE_PIO].size;
hwaddr base_ecam = vbi->memmap[VIRT_PCIE_ECAM].base;
hwaddr size_ecam = vbi->memmap[VIRT_PCIE_ECAM].size;
-hwaddr base = base_mmio;
int nr_pcie_buses = size_ecam / PCIE_MMCFG_SIZE_MIN;
int irq = vbi->irqmap[VIRT_PCIE];
MemoryRegion *mmio_alias;
@@ -789,7 +788,7 @@ static void create_pcie(const VirtBoardInfo *vbi, qemu_irq
*pic,
sysbus_connect_irq(SYS_BUS_DEVICE(dev), i, pic[irq + i]);
}
-nodename = g_strdup_printf("/pcie@%" PRIx64, base);
+nodename = g_strdup_printf("/pcie@%" PRIx64, base_ecam);
qemu_fdt_add_subnode(vbi->fdt, nodename);
qemu_fdt_setprop_string(vbi->fdt, nodename,
"compatible", "pci-host-ecam-generic");
--
1.8.3.1
