[Qemu-devel] [PATCH] PPC: fix PCI configuration space MemoryRegions for grackle/uninorth

[Qemu-devel] [PATCH] PPC: fix PCI configuration space MemoryRegions for grackle/uninorth

[Mark Cave-Ayland]

OpenBIOS prior to SVN r1225 had a horrible bug when accessing PCI
configuration space for PPC Mac architectures - instead of writing the PCI
configuration data value to the data register address, it would instead write
it to the data register address plus the PCI configuration address.

For this reason, the MemoryRegions for the PCI data register for
grackle/uninorth were extremely large in order to accomodate the entire PCI
configuration space being accessed during OpenBIOS PCI bus enumeration. Now
that the OpenBIOS images have been updated, reduce the MemoryRegion sizes down
to a single 32-bit register as intended.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
CC: Hervé Poussineau <hpoussin@reactos.org>
CC: Andreas Färber <afaerber@suse.de>
CC: Alexander Graf <agraf@suse.de>
---
 hw/pci-host/grackle.c  |    4 ++--
 hw/pci-host/uninorth.c |   16 ++++++++--------
 2 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/hw/pci-host/grackle.c b/hw/pci-host/grackle.c
index 4991ec4..d70c519 100644
--- a/hw/pci-host/grackle.c
+++ b/hw/pci-host/grackle.c
@@ -105,9 +105,9 @@ static int pci_grackle_init_device(SysBusDevice *dev)
     phb = PCI_HOST_BRIDGE(dev);
 
     memory_region_init_io(&phb->conf_mem, OBJECT(dev), &pci_host_conf_le_ops,
-                          dev, "pci-conf-idx", 0x1000);
+                          dev, "pci-conf-idx", 4);
     memory_region_init_io(&phb->data_mem, OBJECT(dev), &pci_host_data_le_ops,
-                          dev, "pci-data-idx", 0x1000);
+                          dev, "pci-data-idx", 4);
     sysbus_init_mmio(dev, &phb->conf_mem);
     sysbus_init_mmio(dev, &phb->data_mem);
 
diff --git a/hw/pci-host/uninorth.c b/hw/pci-host/uninorth.c
index 91530cd..ad92c35 100644
--- a/hw/pci-host/uninorth.c
+++ b/hw/pci-host/uninorth.c
@@ -153,9 +153,9 @@ static int pci_unin_main_init_device(SysBusDevice *dev)
     h = PCI_HOST_BRIDGE(dev);
 
     memory_region_init_io(&h->conf_mem, OBJECT(h), &pci_host_conf_le_ops,
-                          dev, "pci-conf-idx", 0x1000);
+                          dev, "pci-conf-idx", 4);
     memory_region_init_io(&h->data_mem, OBJECT(h), &unin_data_ops, dev,
-                          "pci-conf-data", 0x1000);
+                          "pci-conf-data", 4);
     sysbus_init_mmio(dev, &h->conf_mem);
     sysbus_init_mmio(dev, &h->data_mem);
 
@@ -171,9 +171,9 @@ static int pci_u3_agp_init_device(SysBusDevice *dev)
     h = PCI_HOST_BRIDGE(dev);
 
     memory_region_init_io(&h->conf_mem, OBJECT(h), &pci_host_conf_le_ops,
-                          dev, "pci-conf-idx", 0x1000);
+                          dev, "pci-conf-idx", 4);
     memory_region_init_io(&h->data_mem, OBJECT(h), &unin_data_ops, dev,
-                          "pci-conf-data", 0x1000);
+                          "pci-conf-data", 4);
     sysbus_init_mmio(dev, &h->conf_mem);
     sysbus_init_mmio(dev, &h->data_mem);
 
@@ -188,9 +188,9 @@ static int pci_unin_agp_init_device(SysBusDevice *dev)
     h = PCI_HOST_BRIDGE(dev);
 
     memory_region_init_io(&h->conf_mem, OBJECT(h), &pci_host_conf_le_ops,
-                          dev, "pci-conf-idx", 0x1000);
+                          dev, "pci-conf-idx", 4);
     memory_region_init_io(&h->data_mem, OBJECT(h), &pci_host_data_le_ops,
-                          dev, "pci-conf-data", 0x1000);
+                          dev, "pci-conf-data", 4);
     sysbus_init_mmio(dev, &h->conf_mem);
     sysbus_init_mmio(dev, &h->data_mem);
     return 0;
@@ -204,9 +204,9 @@ static int pci_unin_internal_init_device(SysBusDevice *dev)
     h = PCI_HOST_BRIDGE(dev);
 
     memory_region_init_io(&h->conf_mem, OBJECT(h), &pci_host_conf_le_ops,
-                          dev, "pci-conf-idx", 0x1000);
+                          dev, "pci-conf-idx", 4);
     memory_region_init_io(&h->data_mem, OBJECT(h), &pci_host_data_le_ops,
-                          dev, "pci-conf-data", 0x1000);
+                          dev, "pci-conf-data", 4);
     sysbus_init_mmio(dev, &h->conf_mem);
     sysbus_init_mmio(dev, &h->data_mem);
     return 0;
-- 
1.7.10.4

Re: [Qemu-devel] [PATCH] PPC: fix PCI configuration space MemoryRegions for grackle/uninorth

[Hervé Poussineau]

Mark Cave-Ayland a écrit :
> OpenBIOS prior to SVN r1225 had a horrible bug when accessing PCI
> configuration space for PPC Mac architectures - instead of writing the PCI
> configuration data value to the data register address, it would instead write
> it to the data register address plus the PCI configuration address.
> 
> For this reason, the MemoryRegions for the PCI data register for
> grackle/uninorth were extremely large in order to accomodate the entire PCI
> configuration space being accessed during OpenBIOS PCI bus enumeration. Now
> that the OpenBIOS images have been updated, reduce the MemoryRegion sizes down
> to a single 32-bit register as intended.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> CC: Hervé Poussineau <hpoussin@reactos.org>
> CC: Andreas Färber <afaerber@suse.de>
> CC: Alexander Graf <agraf@suse.de>

Tested-by: Hervé Poussineau <hpoussin@reactos.org>

Re: [Qemu-devel] [PATCH] PPC: fix PCI configuration space MemoryRegions for grackle/uninorth

[Alexander Graf]

On 11.10.2013, at 12:53, Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> wrote:

> OpenBIOS prior to SVN r1225 had a horrible bug when accessing PCI
> configuration space for PPC Mac architectures - instead of writing the PCI
> configuration data value to the data register address, it would instead write
> it to the data register address plus the PCI configuration address.
> 
> For this reason, the MemoryRegions for the PCI data register for
> grackle/uninorth were extremely large in order to accomodate the entire PCI
> configuration space being accessed during OpenBIOS PCI bus enumeration. Now
> that the OpenBIOS images have been updated, reduce the MemoryRegion sizes down
> to a single 32-bit register as intended.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> CC: Hervé Poussineau <hpoussin@reactos.org>
> CC: Andreas Färber <afaerber@suse.de>
> CC: Alexander Graf <agraf@suse.de>

With this patch applied, mac99 emulation seems to break:

  http://award.ath.cx/results/288-alex/x86/kvm.qemu-git-tcg.ppc-debian.mac99-G4.etch.e1000.reboot/debug/serial-vm1.log


Alex

Re: [Qemu-devel] [PATCH] PPC: fix PCI configuration space MemoryRegions for grackle/uninorth

[Mark Cave-Ayland]

[-- Attachment #1: Type: text/plain, Size: 2473 bytes --]

On 08/11/13 03:20, Alexander Graf wrote:

> On 11.10.2013, at 12:53, Mark Cave-Ayland<mark.cave-ayland@ilande.co.uk>  wrote:
>
>> OpenBIOS prior to SVN r1225 had a horrible bug when accessing PCI
>> configuration space for PPC Mac architectures - instead of writing the PCI
>> configuration data value to the data register address, it would instead write
>> it to the data register address plus the PCI configuration address.
>>
>> For this reason, the MemoryRegions for the PCI data register for
>> grackle/uninorth were extremely large in order to accomodate the entire PCI
>> configuration space being accessed during OpenBIOS PCI bus enumeration. Now
>> that the OpenBIOS images have been updated, reduce the MemoryRegion sizes down
>> to a single 32-bit register as intended.
>>
>> Signed-off-by: Mark Cave-Ayland<mark.cave-ayland@ilande.co.uk>
>> CC: Hervé Poussineau<hpoussin@reactos.org>
>> CC: Andreas Färber<afaerber@suse.de>
>> CC: Alexander Graf<agraf@suse.de>
>
> With this patch applied, mac99 emulation seems to break:
>
>    http://award.ath.cx/results/288-alex/x86/kvm.qemu-git-tcg.ppc-debian.mac99-G4.etch.e1000.reboot/debug/serial-vm1.log
>
>
> Alex

Hi Alex,

Thanks for the heads-up - with the information above I was able to 
reproduce this fairly easily. I had look at some of the uninorth 
drivers, and while it's not particularly apparent from Linux that the 
PCI configuration data is accessed via MMIO rather than ioport access, 
FreeBSD seems to suggest that this is the case: 
http://code.google.com/p/freebsd-head/source/browse/sys/powerpc/powermac/uninorth.c?spec=svnc6989e24706228678e454517dad4ad465a36e556&r=c6989e24706228678e454517dad4ad465a36e556#274.

The key is that the QEMU uninorth host bridge contains a hack to allow 
PCI configuration mechanism #1 as used by OpenBIOS to work at all (see 
unin_get_config_reg() in hw/pci-host/uninorth.c) which is why I didn't 
notice it in my OpenBIOS boot tests; and in fact, the name of the 
uninorth PCI configuration data MemoryRegions have a "-data" rather than 
a "-idx" suffix is also a big clue.

Hence please find a revised version of the patch which is unaltered for 
grackle, and only changes the MemoryRegion size for the PCI 
configuration address register for uninorth so that the PCI 
configuration data space is still accessible using MMIO. This resolves 
the issue for me, so if you're satisifed that it works for you then I'll 
post a revised version to the list.


ATB,

Mark.

[-- Attachment #2: qemu-ppc-pci-fix.patch --]
[-- Type: text/x-diff, Size: 2727 bytes --]

diff --git a/hw/pci-host/grackle.c b/hw/pci-host/grackle.c
index 4991ec4..d70c519 100644
--- a/hw/pci-host/grackle.c
+++ b/hw/pci-host/grackle.c
@@ -105,9 +105,9 @@ static int pci_grackle_init_device(SysBusDevice *dev)
     phb = PCI_HOST_BRIDGE(dev);
 
     memory_region_init_io(&phb->conf_mem, OBJECT(dev), &pci_host_conf_le_ops,
-                          dev, "pci-conf-idx", 0x1000);
+                          dev, "pci-conf-idx", 4);
     memory_region_init_io(&phb->data_mem, OBJECT(dev), &pci_host_data_le_ops,
-                          dev, "pci-data-idx", 0x1000);
+                          dev, "pci-data-idx", 4);
     sysbus_init_mmio(dev, &phb->conf_mem);
     sysbus_init_mmio(dev, &phb->data_mem);
 
diff --git a/hw/pci-host/uninorth.c b/hw/pci-host/uninorth.c
index 91530cd..2238646 100644
--- a/hw/pci-host/uninorth.c
+++ b/hw/pci-host/uninorth.c
@@ -153,7 +153,7 @@ static int pci_unin_main_init_device(SysBusDevice *dev)
     h = PCI_HOST_BRIDGE(dev);
 
     memory_region_init_io(&h->conf_mem, OBJECT(h), &pci_host_conf_le_ops,
-                          dev, "pci-conf-idx", 0x1000);
+                          dev, "pci-conf-idx", 4);
     memory_region_init_io(&h->data_mem, OBJECT(h), &unin_data_ops, dev,
                           "pci-conf-data", 0x1000);
     sysbus_init_mmio(dev, &h->conf_mem);
@@ -171,7 +171,7 @@ static int pci_u3_agp_init_device(SysBusDevice *dev)
     h = PCI_HOST_BRIDGE(dev);
 
     memory_region_init_io(&h->conf_mem, OBJECT(h), &pci_host_conf_le_ops,
-                          dev, "pci-conf-idx", 0x1000);
+                          dev, "pci-conf-idx", 4);
     memory_region_init_io(&h->data_mem, OBJECT(h), &unin_data_ops, dev,
                           "pci-conf-data", 0x1000);
     sysbus_init_mmio(dev, &h->conf_mem);
@@ -188,7 +188,7 @@ static int pci_unin_agp_init_device(SysBusDevice *dev)
     h = PCI_HOST_BRIDGE(dev);
 
     memory_region_init_io(&h->conf_mem, OBJECT(h), &pci_host_conf_le_ops,
-                          dev, "pci-conf-idx", 0x1000);
+                          dev, "pci-conf-idx", 4);
     memory_region_init_io(&h->data_mem, OBJECT(h), &pci_host_data_le_ops,
                           dev, "pci-conf-data", 0x1000);
     sysbus_init_mmio(dev, &h->conf_mem);
@@ -204,7 +204,7 @@ static int pci_unin_internal_init_device(SysBusDevice *dev)
     h = PCI_HOST_BRIDGE(dev);
 
     memory_region_init_io(&h->conf_mem, OBJECT(h), &pci_host_conf_le_ops,
-                          dev, "pci-conf-idx", 0x1000);
+                          dev, "pci-conf-idx", 4);
     memory_region_init_io(&h->data_mem, OBJECT(h), &pci_host_data_le_ops,
                           dev, "pci-conf-data", 0x1000);
     sysbus_init_mmio(dev, &h->conf_mem);

Re: [Qemu-devel] [PATCH] PPC: fix PCI configuration space MemoryRegions for grackle/uninorth

[Alexander Graf]

On 08.11.2013, at 23:18, Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> wrote:

> On 08/11/13 03:20, Alexander Graf wrote:
> 
>> On 11.10.2013, at 12:53, Mark Cave-Ayland<mark.cave-ayland@ilande.co.uk>  wrote:
>> 
>>> OpenBIOS prior to SVN r1225 had a horrible bug when accessing PCI
>>> configuration space for PPC Mac architectures - instead of writing the PCI
>>> configuration data value to the data register address, it would instead write
>>> it to the data register address plus the PCI configuration address.
>>> 
>>> For this reason, the MemoryRegions for the PCI data register for
>>> grackle/uninorth were extremely large in order to accomodate the entire PCI
>>> configuration space being accessed during OpenBIOS PCI bus enumeration. Now
>>> that the OpenBIOS images have been updated, reduce the MemoryRegion sizes down
>>> to a single 32-bit register as intended.
>>> 
>>> Signed-off-by: Mark Cave-Ayland<mark.cave-ayland@ilande.co.uk>
>>> CC: Hervé Poussineau<hpoussin@reactos.org>
>>> CC: Andreas Färber<afaerber@suse.de>
>>> CC: Alexander Graf<agraf@suse.de>
>> 
>> With this patch applied, mac99 emulation seems to break:
>> 
>>   http://award.ath.cx/results/288-alex/x86/kvm.qemu-git-tcg.ppc-debian.mac99-G4.etch.e1000.reboot/debug/serial-vm1.log
>> 
>> 
>> Alex
> 
> Hi Alex,
> 
> Thanks for the heads-up - with the information above I was able to reproduce this fairly easily. I had look at some of the uninorth drivers, and while it's not particularly apparent from Linux that the PCI configuration data is accessed via MMIO rather than ioport access, FreeBSD seems to suggest that this is the case: http://code.google.com/p/freebsd-head/source/browse/sys/powerpc/powermac/uninorth.c?spec=svnc6989e24706228678e454517dad4ad465a36e556&r=c6989e24706228678e454517dad4ad465a36e556#274.
> 
> The key is that the QEMU uninorth host bridge contains a hack to allow PCI configuration mechanism #1 as used by OpenBIOS to work at all (see unin_get_config_reg() in hw/pci-host/uninorth.c) which is why I didn't notice it in my OpenBIOS boot tests; and in fact, the name of the uninorth PCI configuration data MemoryRegions have a "-data" rather than a "-idx" suffix is also a big clue.
> 
> Hence please find a revised version of the patch which is unaltered for grackle, and only changes the MemoryRegion size for the PCI configuration address register for uninorth so that the PCI configuration data space is still accessible using MMIO. This resolves the issue for me, so if you're satisifed that it works for you then I'll post a revised version to the list.

Hrm. Are you 100% sure this correct? This UniNorth is a real headache. The closest thing to a spec for it is the U4 spec which is generations ahead:

  http://www.datasheetarchive.com/dl/Datasheets-SW3/DSASW0048084.pdf

On that at page 109 you can see that you do indeed have a range of registers and a few fancy modes that can even be used to directly access config space registers without the usual index/data cycle.

Ben, do you have any more insight into how the original Uninorth worked?


Alex

Re: [Qemu-devel] [PATCH] PPC: fix PCI configuration space MemoryRegions for grackle/uninorth

[Benjamin Herrenschmidt]

On Wed, 2013-12-18 at 13:34 +0100, Alexander Graf wrote:
> Hrm. Are you 100% sure this correct? This UniNorth is a real headache.
> The closest thing to a spec for it is the U4 spec which is generations
> ahead:
> 
>   http://www.datasheetarchive.com/dl/Datasheets-SW3/DSASW0048084.pdf
> 
> On that at page 109 you can see that you do indeed have a range of
> registers and a few fancy modes that can even be used to directly
> access config space registers without the usual index/data cycle.
> 
> Ben, do you have any more insight into how the original Uninorth
> worked?

Just index/data. Look at Linux :-)

Cheers,
Ben.

Re: [Qemu-devel] [PATCH] PPC: fix PCI configuration space MemoryRegions for grackle/uninorth

[Alexander Graf]

On 18.12.2013, at 22:04, Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:

> On Wed, 2013-12-18 at 13:34 +0100, Alexander Graf wrote:
>> Hrm. Are you 100% sure this correct? This UniNorth is a real headache.
>> The closest thing to a spec for it is the U4 spec which is generations
>> ahead:
>> 
>>  http://www.datasheetarchive.com/dl/Datasheets-SW3/DSASW0048084.pdf
>> 
>> On that at page 109 you can see that you do indeed have a range of
>> registers and a few fancy modes that can even be used to directly
>> access config space registers without the usual index/data cycle.
>> 
>> Ben, do you have any more insight into how the original Uninorth
>> worked?
> 
> Just index/data. Look at Linux :-)

Then I don't understand why we break when we limit the data region to 4 bytes.


Alex

Re: [Qemu-devel] [PATCH] PPC: fix PCI configuration space MemoryRegions for grackle/uninorth

[Benjamin Herrenschmidt]

On Wed, 2013-12-18 at 22:24 +0100, Alexander Graf wrote:
> Then I don't understand why we break when we limit the data region to
> 4 bytes.

This is old uninorth, not U3 HT right ? The latter is memory mapped.

Ben.

Re: [Qemu-devel] [PATCH] PPC: fix PCI configuration space MemoryRegions for grackle/uninorth

[Alexander Graf]

On 18.12.2013, at 23:04, Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:

> On Wed, 2013-12-18 at 22:24 +0100, Alexander Graf wrote:
>> Then I don't understand why we break when we limit the data region to
>> 4 bytes.
> 
> This is old uninorth, not U3 HT right ? The latter is memory mapped.

Depends, we use the same code to cover both. With 32bit guests we expose an old UniNorth. With 64bit guests we have to expose a U3 as the guest doesn't know what old UniNorth is anymore.

So yeah, maybe that's biting us.


Alex

Re: [Qemu-devel] [Qemu-ppc] [PATCH] PPC: fix PCI configuration space MemoryRegions for grackle/uninorth

[Alexander Graf]

On 18.12.2013, at 23:07, Alexander Graf <agraf@suse.de> wrote:

> 
> On 18.12.2013, at 23:04, Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:
> 
>> On Wed, 2013-12-18 at 22:24 +0100, Alexander Graf wrote:
>>> Then I don't understand why we break when we limit the data region to
>>> 4 bytes.
>> 
>> This is old uninorth, not U3 HT right ? The latter is memory mapped.
> 
> Depends, we use the same code to cover both. With 32bit guests we expose an old UniNorth. With 64bit guests we have to expose a U3 as the guest doesn't know what old UniNorth is anymore.
> 
> So yeah, maybe that's biting us.

But then again the breaking case was with a G4 which was 32bit so it should have emulated an old UniNorth. Hrm.


Alex

Re: [Qemu-devel] [PATCH] PPC: fix PCI configuration space MemoryRegions for grackle/uninorth

[Benjamin Herrenschmidt]

On Wed, 2013-12-18 at 23:07 +0100, Alexander Graf wrote:
> On 18.12.2013, at 23:04, Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:
> 
> > On Wed, 2013-12-18 at 22:24 +0100, Alexander Graf wrote:
> >> Then I don't understand why we break when we limit the data region to
> >> 4 bytes.
> > 
> > This is old uninorth, not U3 HT right ? The latter is memory mapped.
> 
> Depends, we use the same code to cover both. With 32bit guests we expose an old UniNorth.
> With 64bit guests we have to expose a U3 as the guest doesn't know what old UniNorth is anymore.
> 
> So yeah, maybe that's biting us.

Well, it's different.

Old uninorth uses some form of indirect address/data registers, as does
U3 AGP... but U3 HT uses memory mapped. So U3 has a bit of both :)

I think U4 PCIe is yet another beast as well.

Cheers,
Ben.