Re: [Qemu-devel] [PATCH v3 09/10] raven: fix PCI bus accesses with size > 1

Re: [Qemu-devel] [PATCH v3 09/10] raven: fix PCI bus accesses with size > 1

[Artyom Tarasenko]

On Tue, Nov 5, 2013 at 12:09 AM, Hervé Poussineau <hpoussin@reactos.org> wrote:
> Signed-off-by: Hervé Poussineau <hpoussin@reactos.org>

Without this patch PReP is broken really bad. Was going to submit the
same fix, and then found that the bug was already fixed 4 months ago.

Hope it helps getting it closer to master:

Tested-by: Artyom Tarasenko <atar4qemu@gmail.com>

> ---
>  hw/pci-host/prep.c |    8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/hw/pci-host/prep.c b/hw/pci-host/prep.c
> index c11679a..4eabe31 100644
> --- a/hw/pci-host/prep.c
> +++ b/hw/pci-host/prep.c
> @@ -222,12 +222,12 @@ static void raven_pcihost_realizefn(DeviceState *d, Error **errp)
>
>      pci_bus_irqs(&s->pci_bus, prep_set_irq, prep_map_irq, s->irq, PCI_NUM_PINS);
>
> -    memory_region_init_io(&h->conf_mem, OBJECT(h), &pci_host_conf_be_ops, s,
> -                          "pci-conf-idx", 1);
> +    memory_region_init_io(&h->conf_mem, OBJECT(h), &pci_host_conf_le_ops, s,
> +                          "pci-conf-idx", 4);
>      memory_region_add_subregion(&s->pci_io, 0xcf8, &h->conf_mem);
>
> -    memory_region_init_io(&h->data_mem, OBJECT(h), &pci_host_data_be_ops, s,
> -                          "pci-conf-data", 1);
> +    memory_region_init_io(&h->data_mem, OBJECT(h), &pci_host_data_le_ops, s,
> +                          "pci-conf-data", 4);
>      memory_region_add_subregion(&s->pci_io, 0xcfc, &h->data_mem);
>
>      memory_region_init_io(&h->mmcfg, OBJECT(s), &PPC_PCIIO_ops, s, "pciio", 0x00400000);
> --
> 1.7.10.4
>
>



-- 
Regards,
Artyom Tarasenko

linux/sparc and solaris/sparc under qemu blog:
http://tyom.blogspot.com/search/label/qemu

Re: [Qemu-devel] [Qemu-ppc] [PATCH v3 09/10] raven: fix PCI bus accesses with size > 1

[Mark Cave-Ayland]

On 10/02/14 22:46, Artyom Tarasenko wrote:

> On Tue, Nov 5, 2013 at 12:09 AM, Hervé Poussineau<hpoussin@reactos.org>  wrote:
>> Signed-off-by: Hervé Poussineau<hpoussin@reactos.org>
>
> Without this patch PReP is broken really bad. Was going to submit the
> same fix, and then found that the bug was already fixed 4 months ago.
>
> Hope it helps getting it closer to master:
>
> Tested-by: Artyom Tarasenko<atar4qemu@gmail.com>

What happened to the rest of the PReP patchset in the end? Is it still 
stuck waiting on someone to rebuild OHW?


ATB,

Mark.

Re: [Qemu-devel] [Qemu-ppc] [PATCH v3 09/10] raven: fix PCI bus accesses with size > 1

[Andreas Färber]

Am 10.02.2014 23:55, schrieb Mark Cave-Ayland:
> On 10/02/14 22:46, Artyom Tarasenko wrote:
> 
>> On Tue, Nov 5, 2013 at 12:09 AM, Hervé
>> Poussineau<hpoussin@reactos.org>  wrote:
>>> Signed-off-by: Hervé Poussineau<hpoussin@reactos.org>
>>
>> Without this patch PReP is broken really bad. Was going to submit the
>> same fix, and then found that the bug was already fixed 4 months ago.
>>
>> Hope it helps getting it closer to master:
>>
>> Tested-by: Artyom Tarasenko<atar4qemu@gmail.com>
> 
> What happened to the rest of the PReP patchset in the end? Is it still
> stuck waiting on someone to rebuild OHW?

Unfortunately yes. Earlier this week I made some progress towards a
buildable and commitable patchset for OHW - once agreed on, we'll need
to go through the same process as you do for OpenBIOS for a submodule
before we can continue with the remainder of the patchset that relies on it.

http://repo.or.cz/w/openhackware.git does not yet link for me.

If there's any part of the Raven patchset or other PReP patches that
don't rely on changes to our unholy OHW blob, do let me know.

Regards,
Andreas

Re: [Qemu-devel] [PATCH v3 09/10] raven: fix PCI bus accesses with size > 1

[Artyom Tarasenko]

Hi Andreas, Hervé,

this patch seems still be missing in master. Is it causing any problems?

Artyom

On Mon, Feb 10, 2014 at 11:46 PM, Artyom Tarasenko <atar4qemu@gmail.com> wrote:
> On Tue, Nov 5, 2013 at 12:09 AM, Hervé Poussineau <hpoussin@reactos.org> wrote:
>> Signed-off-by: Hervé Poussineau <hpoussin@reactos.org>
>
> Without this patch PReP is broken really bad. Was going to submit the
> same fix, and then found that the bug was already fixed 4 months ago.
>
> Hope it helps getting it closer to master:
>
> Tested-by: Artyom Tarasenko <atar4qemu@gmail.com>
>
>> ---
>>  hw/pci-host/prep.c |    8 ++++----
>>  1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/hw/pci-host/prep.c b/hw/pci-host/prep.c
>> index c11679a..4eabe31 100644
>> --- a/hw/pci-host/prep.c
>> +++ b/hw/pci-host/prep.c
>> @@ -222,12 +222,12 @@ static void raven_pcihost_realizefn(DeviceState *d, Error **errp)
>>
>>      pci_bus_irqs(&s->pci_bus, prep_set_irq, prep_map_irq, s->irq, PCI_NUM_PINS);
>>
>> -    memory_region_init_io(&h->conf_mem, OBJECT(h), &pci_host_conf_be_ops, s,
>> -                          "pci-conf-idx", 1);
>> +    memory_region_init_io(&h->conf_mem, OBJECT(h), &pci_host_conf_le_ops, s,
>> +                          "pci-conf-idx", 4);
>>      memory_region_add_subregion(&s->pci_io, 0xcf8, &h->conf_mem);
>>
>> -    memory_region_init_io(&h->data_mem, OBJECT(h), &pci_host_data_be_ops, s,
>> -                          "pci-conf-data", 1);
>> +    memory_region_init_io(&h->data_mem, OBJECT(h), &pci_host_data_le_ops, s,
>> +                          "pci-conf-data", 4);
>>      memory_region_add_subregion(&s->pci_io, 0xcfc, &h->data_mem);
>>
>>      memory_region_init_io(&h->mmcfg, OBJECT(s), &PPC_PCIIO_ops, s, "pciio", 0x00400000);

Re: [Qemu-devel] [PATCH v3 09/10] raven: fix PCI bus accesses with size > 1

[Andreas Färber]

Hi Artyom,

Am 16.03.2014 23:27, schrieb Artyom Tarasenko:
> Hi Andreas, Hervé,
> 
> this patch seems still be missing in master. Is it causing any problems?

It does not apply without the preceding patches. Here's my cherry-pick
result:

diff --cc hw/pci-host/prep.c
index 94fdffa,fed6c26..0000000
--- a/hw/pci-host/prep.c
+++ b/hw/pci-host/prep.c
@@@ -133,17 -219,17 +133,27 @@@ static void raven_pcihost_realizefn(Dev
          sysbus_init_irq(dev, &s->irq[i]);
      }

 -    qdev_init_gpio_in(d, raven_change_gpio, 1);
 -
      pci_bus_irqs(&s->pci_bus, prep_set_irq, prep_map_irq, s->irq,
PCI_NUM_PINS
);

++<<<<<<< HEAD
 +    memory_region_init_io(&h->conf_mem, OBJECT(h),
&pci_host_conf_be_ops, s,
 +                          "pci-conf-idx", 1);
 +    sysbus_add_io(dev, 0xcf8, &h->conf_mem);
 +    sysbus_init_ioports(&h->busdev, 0xcf8, 1);
 +
 +    memory_region_init_io(&h->data_mem, OBJECT(h),
&pci_host_data_be_ops, s,
 +                          "pci-conf-data", 1);
 +    sysbus_add_io(dev, 0xcfc, &h->data_mem);
 +    sysbus_init_ioports(&h->busdev, 0xcfc, 1);
++=======
+     memory_region_init_io(&h->conf_mem, OBJECT(h),
&pci_host_conf_le_ops, s,
+                           "pci-conf-idx", 4);
+     memory_region_add_subregion(&s->pci_io, 0xcf8, &h->conf_mem);
+
+     memory_region_init_io(&h->data_mem, OBJECT(h),
&pci_host_data_le_ops, s,
+                           "pci-conf-data", 4);
+     memory_region_add_subregion(&s->pci_io, 0xcfc, &h->data_mem);
++>>>>>>> 67472dc... raven: fix PCI bus accesses with size > 1

      memory_region_init_io(&h->mmcfg, OBJECT(s), &PPC_PCIIO_ops, s,
"pciio", 0x00400000);
      memory_region_add_subregion(address_space_mem, 0x80800000, &h->mmcfg);

I.e. we might change 1 -> 4 in the SysBus API, but would that work given
that endianness is being changed alongside?

If either of you could submit a version limited to bug fixes or explain
why the whole refactoring is needed as bug fix and provide a bisectable
version, I can certainly apply it for -rc1 if my test cases continue
working.

BTW another unresolved issue that's been discussed is whether we should
change the default CPU for -M prep. I've been open to doing so for 2.0
but would like some pointer that such a machine did exist rather than
just happens to work better with OpenBIOS.

Regards,
Andreas


> On Mon, Feb 10, 2014 at 11:46 PM, Artyom Tarasenko <atar4qemu@gmail.com> wrote:
>> On Tue, Nov 5, 2013 at 12:09 AM, Hervé Poussineau <hpoussin@reactos.org> wrote:
>>> Signed-off-by: Hervé Poussineau <hpoussin@reactos.org>
>>
>> Without this patch PReP is broken really bad. Was going to submit the
>> same fix, and then found that the bug was already fixed 4 months ago.
>>
>> Hope it helps getting it closer to master:
>>
>> Tested-by: Artyom Tarasenko <atar4qemu@gmail.com>
>>
>>> ---
>>>  hw/pci-host/prep.c |    8 ++++----
>>>  1 file changed, 4 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/hw/pci-host/prep.c b/hw/pci-host/prep.c
>>> index c11679a..4eabe31 100644
>>> --- a/hw/pci-host/prep.c
>>> +++ b/hw/pci-host/prep.c
>>> @@ -222,12 +222,12 @@ static void raven_pcihost_realizefn(DeviceState *d, Error **errp)
>>>
>>>      pci_bus_irqs(&s->pci_bus, prep_set_irq, prep_map_irq, s->irq, PCI_NUM_PINS);
>>>
>>> -    memory_region_init_io(&h->conf_mem, OBJECT(h), &pci_host_conf_be_ops, s,
>>> -                          "pci-conf-idx", 1);
>>> +    memory_region_init_io(&h->conf_mem, OBJECT(h), &pci_host_conf_le_ops, s,
>>> +                          "pci-conf-idx", 4);
>>>      memory_region_add_subregion(&s->pci_io, 0xcf8, &h->conf_mem);
>>>
>>> -    memory_region_init_io(&h->data_mem, OBJECT(h), &pci_host_data_be_ops, s,
>>> -                          "pci-conf-data", 1);
>>> +    memory_region_init_io(&h->data_mem, OBJECT(h), &pci_host_data_le_ops, s,
>>> +                          "pci-conf-data", 4);
>>>      memory_region_add_subregion(&s->pci_io, 0xcfc, &h->data_mem);
>>>
>>>      memory_region_init_io(&h->mmcfg, OBJECT(s), &PPC_PCIIO_ops, s, "pciio", 0x00400000);

Re: [Qemu-devel] [PATCH v3 09/10] raven: fix PCI bus accesses with size > 1

[Artyom Tarasenko]

Hi Andreas,

On Mon, Mar 17, 2014 at 8:59 PM, Andreas Färber <andreas.faerber@web.de> wrote:
>> this patch seems still be missing in master. Is it causing any problems?
>
> It does not apply without the preceding patches. Here's my cherry-pick
> result:
>[...]
> I.e. we might change 1 -> 4 in the SysBus API, but would that work given
> that endianness is being changed alongside?

Yes, and that's the point of this patch. PCI configuration space is
little-endian. With 1 byte access size, no byte swapping happens, so
the bug is hidden.
But on 32- and 16- bit accesses the bytes are swapped.

> If either of you could submit a version limited to bug fixes or explain
> why the whole refactoring is needed as bug fix and provide a bisectable
> version, I can certainly apply it for -rc1 if my test cases continue
> working.

No refactoring is necessary: only be->le and 1->4, and this is a pure
bugfix, which has no side effects because OHW seems to use 1 byte
accesses only.

> BTW another unresolved issue that's been discussed is whether we should
> change the default CPU for -M prep. I've been open to doing so for 2.0
> but would like some pointer that such a machine did exist

That's fair. I don't have any preference here though, as long as the
necessary cpu can be selected via the command line.

> rather than just happens to work better with OpenBIOS.

Oh, there is a compatible version of OpenBIOS available?! Are the
binaries shared somewhere?
BTW is there any Linux distribution newer than Debian Woody available for PReP?

Artyom

>> On Mon, Feb 10, 2014 at 11:46 PM, Artyom Tarasenko <atar4qemu@gmail.com> wrote:
>>> On Tue, Nov 5, 2013 at 12:09 AM, Hervé Poussineau <hpoussin@reactos.org> wrote:
>>>> Signed-off-by: Hervé Poussineau <hpoussin@reactos.org>
>>>
>>> Without this patch PReP is broken really bad. Was going to submit the
>>> same fix, and then found that the bug was already fixed 4 months ago.
>>>
>>> Hope it helps getting it closer to master:
>>>
>>> Tested-by: Artyom Tarasenko <atar4qemu@gmail.com>
>>>
>>>> ---
>>>>  hw/pci-host/prep.c |    8 ++++----
>>>>  1 file changed, 4 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/hw/pci-host/prep.c b/hw/pci-host/prep.c
>>>> index c11679a..4eabe31 100644
>>>> --- a/hw/pci-host/prep.c
>>>> +++ b/hw/pci-host/prep.c
>>>> @@ -222,12 +222,12 @@ static void raven_pcihost_realizefn(DeviceState *d, Error **errp)
>>>>
>>>>      pci_bus_irqs(&s->pci_bus, prep_set_irq, prep_map_irq, s->irq, PCI_NUM_PINS);
>>>>
>>>> -    memory_region_init_io(&h->conf_mem, OBJECT(h), &pci_host_conf_be_ops, s,
>>>> -                          "pci-conf-idx", 1);
>>>> +    memory_region_init_io(&h->conf_mem, OBJECT(h), &pci_host_conf_le_ops, s,
>>>> +                          "pci-conf-idx", 4);
>>>>      memory_region_add_subregion(&s->pci_io, 0xcf8, &h->conf_mem);
>>>>
>>>> -    memory_region_init_io(&h->data_mem, OBJECT(h), &pci_host_data_be_ops, s,
>>>> -                          "pci-conf-data", 1);
>>>> +    memory_region_init_io(&h->data_mem, OBJECT(h), &pci_host_data_le_ops, s,
>>>> +                          "pci-conf-data", 4);
>>>>      memory_region_add_subregion(&s->pci_io, 0xcfc, &h->data_mem);
>>>>
>>>>      memory_region_init_io(&h->mmcfg, OBJECT(s), &PPC_PCIIO_ops, s, "pciio", 0x00400000);
>



-- 
Regards,
Artyom Tarasenko

linux/sparc and solaris/sparc under qemu blog:
http://tyom.blogspot.com/search/label/qemu

Re: [Qemu-devel] [PATCH v3 09/10] raven: fix PCI bus accesses with size > 1

[Hervé Poussineau]

Hi,
Le lun. 17 mars 2014 22:55:37 CET, Artyom Tarasenko a écrit :
[...]
>> BTW another unresolved issue that's been discussed is whether we should
>> change the default CPU for -M prep. I've been open to doing so for 2.0
>> but would like some pointer that such a machine did exist
>
> That's fair. I don't have any preference here though, as long as the
> necessary cpu can be selected via the command line.
>
>> rather than just happens to work better with OpenBIOS.
>
> Oh, there is a compatible version of OpenBIOS available?! Are the
> binaries shared somewhere?

You can check http://repo.or.cz/w/qemu/hpoussin.git , branch raven.
It contains the patchset I just sent, and a few more patches to switch 
to OpenBIOS on PReP.
The required binary is already committed in QEMU (pc-bios/openbios-ppc)

> BTW is there any Linux distribution newer than Debian Woody available for PReP?

My PReP machine is running Debian Woody with a custom 2.4.22 kernel. 
Enough for most things :)

Hervé

Re: [Qemu-devel] [PATCH v3 09/10] raven: fix PCI bus accesses with size > 1

[Mark Cave-Ayland]

On 17/03/14 21:55, Artyom Tarasenko wrote:

> Hi Andreas,
>
> On Mon, Mar 17, 2014 at 8:59 PM, Andreas Färber<andreas.faerber@web.de>  wrote:
>>> this patch seems still be missing in master. Is it causing any problems?
>>
>> It does not apply without the preceding patches. Here's my cherry-pick
>> result:
>> [...]
>> I.e. we might change 1 ->  4 in the SysBus API, but would that work given
>> that endianness is being changed alongside?
>
> Yes, and that's the point of this patch. PCI configuration space is
> little-endian. With 1 byte access size, no byte swapping happens, so
> the bug is hidden.
> But on 32- and 16- bit accesses the bytes are swapped.
>
>> If either of you could submit a version limited to bug fixes or explain
>> why the whole refactoring is needed as bug fix and provide a bisectable
>> version, I can certainly apply it for -rc1 if my test cases continue
>> working.
>
> No refactoring is necessary: only be->le and 1->4, and this is a pure
> bugfix, which has no side effects because OHW seems to use 1 byte
> accesses only.

Yes, this is my understanding of the patch. However I also see that 
Hervé has just posted a revised raven patchset. If this patchset passes 
testing, I'd be inclined to apply it for 2.0 mainly because Hervé has 
done a great deal of testing on real OSs during development over the 
last 4 months and I'd bet that this fixes many more bugs than it would 
likely introduce.

Andreas, what's your test harness for PReP look like? Can you point us 
towards specific ISOs so that we can try these patches out?

>> BTW another unresolved issue that's been discussed is whether we should
>> change the default CPU for -M prep. I've been open to doing so for 2.0
>> but would like some pointer that such a machine did exist
>
> That's fair. I don't have any preference here though, as long as the
> necessary cpu can be selected via the command line.

I think I'd prefer to stick with the 600 series as I know there has been 
some talk of people wanting to run BeOS under QEMU, and given that OHW 
runs fine under an emulated 600 series processor then we should try and 
keep compatibility with that.

Given where we are time-wise, my preference would be to do the switch to 
OpenBIOS during the 2.1 cycle.

>> rather than just happens to work better with OpenBIOS.
>
> Oh, there is a compatible version of OpenBIOS available?! Are the
> binaries shared somewhere?

Yes - it's called git master ;) Make sure you apply Hervé's patcheset 
just posted to the list, plus the last 3 [RFC] OpenBIOS patches at 
http://repo.or.cz/w/qemu/hpoussin.git/shortlog/refs/heads/raven.

With those in place you should be able to launch something like this:

./qemu-system-ppc -M prep -cpu 750 -bios openbios-ppc

As I mentioned above, the main problem with switching is that OpenBIOS 
doesn't seem to (yet) work with the PReP machine's default 604 processor.


ATB,

Mark.

Re: [Qemu-devel] [PATCH v3 09/10] raven: fix PCI bus accesses with size > 1

[Andreas Färber]

Am 17.03.2014 23:25, schrieb Hervé Poussineau:
> Hi,
> Le lun. 17 mars 2014 22:55:37 CET, Artyom Tarasenko a écrit :
> [...]
>>> BTW another unresolved issue that's been discussed is whether we should
>>> change the default CPU for -M prep. I've been open to doing so for 2.0
>>> but would like some pointer that such a machine did exist
>>
>> That's fair. I don't have any preference here though, as long as the
>> necessary cpu can be selected via the command line.
>>
>>> rather than just happens to work better with OpenBIOS.
>>
>> Oh, there is a compatible version of OpenBIOS available?! Are the
>> binaries shared somewhere?
> 
> You can check http://repo.or.cz/w/qemu/hpoussin.git , branch raven.
> It contains the patchset I just sent, and a few more patches to switch
> to OpenBIOS on PReP.
> The required binary is already committed in QEMU (pc-bios/openbios-ppc)

Do you or Mark want to mention that in the 2.0 release notes somehow?

http://wiki.qemu.org/ChangeLog/2.0#Power

On the other hand, do your patches differ from my two? If so, how? I
think I still have an ugly #if 0 somewhere...

Regards,
Andreas

Re: [Qemu-devel] [PATCH v3 09/10] raven: fix PCI bus accesses with size > 1

[Andreas Färber]

Am 17.03.2014 22:55, schrieb Artyom Tarasenko:
> Hi Andreas,
> 
> On Mon, Mar 17, 2014 at 8:59 PM, Andreas Färber <andreas.faerber@web.de> wrote:
>>> this patch seems still be missing in master. Is it causing any problems?
>>
>> It does not apply without the preceding patches. Here's my cherry-pick
>> result:
>> [...]
>> I.e. we might change 1 -> 4 in the SysBus API, but would that work given
>> that endianness is being changed alongside?
> 
> Yes, and that's the point of this patch. PCI configuration space is
> little-endian. With 1 byte access size, no byte swapping happens, so
> the bug is hidden.
> But on 32- and 16- bit accesses the bytes are swapped.

I think you misunderstood my question: I wanted to know whether
switching from SysBus API to pure MemoryRegion API was part of the fix
or an independent cleanup. But now that I'm going through the respun
series it doesn't really matter any more...

Andreas

[Qemu-devel] [PATCH v3 09/10] raven: fix PCI bus accesses with size > 1

[Hervé Poussineau]

Signed-off-by: Hervé Poussineau <hpoussin@reactos.org>
---
 hw/pci-host/prep.c |    8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/hw/pci-host/prep.c b/hw/pci-host/prep.c
index c11679a..4eabe31 100644
--- a/hw/pci-host/prep.c
+++ b/hw/pci-host/prep.c
@@ -222,12 +222,12 @@ static void raven_pcihost_realizefn(DeviceState *d, Error **errp)
 
     pci_bus_irqs(&s->pci_bus, prep_set_irq, prep_map_irq, s->irq, PCI_NUM_PINS);
 
-    memory_region_init_io(&h->conf_mem, OBJECT(h), &pci_host_conf_be_ops, s,
-                          "pci-conf-idx", 1);
+    memory_region_init_io(&h->conf_mem, OBJECT(h), &pci_host_conf_le_ops, s,
+                          "pci-conf-idx", 4);
     memory_region_add_subregion(&s->pci_io, 0xcf8, &h->conf_mem);
 
-    memory_region_init_io(&h->data_mem, OBJECT(h), &pci_host_data_be_ops, s,
-                          "pci-conf-data", 1);
+    memory_region_init_io(&h->data_mem, OBJECT(h), &pci_host_data_le_ops, s,
+                          "pci-conf-data", 4);
     memory_region_add_subregion(&s->pci_io, 0xcfc, &h->data_mem);
 
     memory_region_init_io(&h->mmcfg, OBJECT(s), &PPC_PCIIO_ops, s, "pciio", 0x00400000);
-- 
1.7.10.4