[Qemu-devel] [PATCH] hw/arm/virt: Fix address in PCIe device tree node's unit name

[Qemu-devel] [PATCH] hw/arm/virt: Fix address in PCIe device tree node's unit name

[Alexander Gordeev]

PCIe device tree unit name is pcie@10000000 - which denotes
IO space base address. However, the corresponding node's
"reg" property points to PCI configuration space base address
0x3f000000.

Set the unit name to pcie@3f000000 which is not only correct,
but also conforms to Open Firmware (IEEE 1275).

CC: Peter Maydell <peter.maydell@linaro.org>
CC: Andrew Jones <drjones@redhat.com>

Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
---
 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

Re: [Qemu-devel] [PATCH] hw/arm/virt: Fix address in PCIe device tree node's unit name

[Peter Maydell]

On 21 October 2015 at 21:43, Alexander Gordeev <agordeev@redhat.com> wrote:
> PCIe device tree unit name is pcie@10000000 - which denotes
> IO space base address. However, the corresponding node's
> "reg" property points to PCI configuration space base address
> 0x3f000000.
>
> Set the unit name to pcie@3f000000 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

[Alexander Gordeev]

On Wed, Oct 21, 2015 at 10:26:27PM +0100, Peter Maydell wrote:
> On 21 October 2015 at 21:43, Alexander Gordeev <agordeev@redhat.com> wrote:
> > PCIe device tree unit name is pcie@10000000 - which denotes
> > IO space base address. However, the corresponding node's
> > "reg" property points to PCI configuration space base address
> > 0x3f000000.
> >
> > Set the unit name to pcie@3f000000 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
agordeev@redhat.com

Re: [Qemu-devel] [PATCH] hw/arm/virt: Fix address in PCIe device tree node's unit name

[Peter Maydell]

On 21 October 2015 at 23:01, Alexander Gordeev <agordeev@redhat.com> wrote:
> On Wed, Oct 21, 2015 at 10:26:27PM +0100, Peter Maydell wrote:
>> On 21 October 2015 at 21:43, Alexander Gordeev <agordeev@redhat.com> wrote:
>> > PCIe device tree unit name is pcie@10000000 - which denotes
>> > IO space base address. However, the corresponding node's
>> > "reg" property points to PCI configuration space base address
>> > 0x3f000000.
>> >
>> > Set the unit name to pcie@3f000000 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

[Alexander Gordeev]

On Fri, Oct 23, 2015 at 03:18:34PM +0100, Peter Maydell wrote:
> On 21 October 2015 at 23:01, Alexander Gordeev <agordeev@redhat.com> wrote:
> > On Wed, Oct 21, 2015 at 10:26:27PM +0100, Peter Maydell wrote:
> >> On 21 October 2015 at 21:43, Alexander Gordeev <agordeev@redhat.com> wrote:
> >> > PCIe device tree unit name is pcie@10000000 - which denotes
> >> > IO space base address. However, the corresponding node's
> >> > "reg" property points to PCI configuration space base address
> >> > 0x3f000000.
> >> >
> >> > Set the unit name to pcie@3f000000 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
agordeev@redhat.com

Re: [Qemu-devel] [PATCH] hw/arm/virt: Fix address in PCIe device tree node's unit name

[Peter Maydell]

On 23 October 2015 at 16:26, Alexander Gordeev <agordeev@redhat.com> wrote:
> On Fri, Oct 23, 2015 at 03:18:34PM +0100, Peter Maydell wrote:
>> On 21 October 2015 at 23:01, Alexander Gordeev <agordeev@redhat.com> wrote:
>> > On Wed, Oct 21, 2015 at 10:26:27PM +0100, Peter Maydell wrote:
>> >> On 21 October 2015 at 21:43, Alexander Gordeev <agordeev@redhat.com> wrote:
>> >> > PCIe device tree unit name is pcie@10000000 - which denotes
>> >> > IO space base address. However, the corresponding node's
>> >> > "reg" property points to PCI configuration space base address
>> >> > 0x3f000000.
>> >> >
>> >> > Set the unit name to pcie@3f000000 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