[PATCH v8 0/5] Mac Old World ROM experiment (ppc/mac_* clean ups and loading binary ROM)

BALATON Zoltan via posted 5 patches 4 days ago
Test checkpatch passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/cover.1602805637.git.balaton@eik.bme.hu
Maintainers: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>, David Gibson <david@gibson.dropbear.id.au>
hw/ppc/mac.h          |  2 --
hw/ppc/mac_newworld.c | 22 ++++++++------
hw/ppc/mac_oldworld.c | 67 ++++++++++++++++++++++++-------------------
3 files changed, 52 insertions(+), 39 deletions(-)

[PATCH v8 0/5] Mac Old World ROM experiment (ppc/mac_* clean ups and loading binary ROM)

Posted by BALATON Zoltan via 4 days ago
This is the cut down version of the earlier series omitting unfinished
patches that I plan to rework later and rebased to Mark's qemu-macppc
branch. Compared to v7 the only change is the cast to (target_ulong)
from (uint32_t) as requested by Mark in patch 1.

Regards,
BALATON Zoltan

BALATON Zoltan (5):
  mac_oldworld: Allow loading binary ROM image
  mac_newworld: Allow loading binary ROM image
  mac_oldworld: Drop a variable, use get_system_memory() directly
  mac_oldworld: Drop some variables
  mac_oldworld: Change PCI address of macio to match real hardware

 hw/ppc/mac.h          |  2 --
 hw/ppc/mac_newworld.c | 22 ++++++++------
 hw/ppc/mac_oldworld.c | 67 ++++++++++++++++++++++++-------------------
 3 files changed, 52 insertions(+), 39 deletions(-)

-- 
2.21.3


Re: [PATCH v8 0/5] Mac Old World ROM experiment (ppc/mac_* clean ups and loading binary ROM)

Posted by Mark Cave-Ayland 3 days ago
On 16/10/2020 00:47, BALATON Zoltan via wrote:

> This is the cut down version of the earlier series omitting unfinished
> patches that I plan to rework later and rebased to Mark's qemu-macppc
> branch. Compared to v7 the only change is the cast to (target_ulong)
> from (uint32_t) as requested by Mark in patch 1.

FWIW the reason for suggesting the cast to target_ulong is so that the same code 
works for both qemu-system-ppc and qemu-system-ppc64. For qemu-system-ppc that should 
correctly drop the sign extension from 32-bit, whilst still allowing someone to load 
a 64-bit ELF into qemu-system-ppc64 if requested.

Can you confirm that the sign extension behaviour is still correct for both 
qemu-system-ppc and qemu-system-ppc64? If so I'm happy to give it a R-B tag.


ATB,

Mark.

Re: [PATCH v8 0/5] Mac Old World ROM experiment (ppc/mac_* clean ups and loading binary ROM)

Posted by Philippe Mathieu-Daudé 3 days ago
On 10/16/20 11:58 AM, Mark Cave-Ayland wrote:
> On 16/10/2020 00:47, BALATON Zoltan via wrote:
> 
>> This is the cut down version of the earlier series omitting unfinished
>> patches that I plan to rework later and rebased to Mark's qemu-macppc
>> branch. Compared to v7 the only change is the cast to (target_ulong)
>> from (uint32_t) as requested by Mark in patch 1.
> 
> FWIW the reason for suggesting the cast to target_ulong is so that the 
> same code works for both qemu-system-ppc and qemu-system-ppc64. For 
> qemu-system-ppc that should correctly drop the sign extension from 
> 32-bit, whilst still allowing someone to load a 64-bit ELF into 
> qemu-system-ppc64 if requested.

IMO this is part of a bigger design problem. Not all
machines main bus is 64-bit. I did some experiments
but changing that involves a lot of work.

Re: [PATCH v8 0/5] Mac Old World ROM experiment (ppc/mac_* clean ups and loading binary ROM)

Posted by BALATON Zoltan via 2 days ago
On Fri, 16 Oct 2020, Philippe Mathieu-Daudé wrote:
> On 10/16/20 11:58 AM, Mark Cave-Ayland wrote:
>> On 16/10/2020 00:47, BALATON Zoltan via wrote:
>> 
>>> This is the cut down version of the earlier series omitting unfinished
>>> patches that I plan to rework later and rebased to Mark's qemu-macppc
>>> branch. Compared to v7 the only change is the cast to (target_ulong)
>>> from (uint32_t) as requested by Mark in patch 1.
>> 
>> FWIW the reason for suggesting the cast to target_ulong is so that the same 
>> code works for both qemu-system-ppc and qemu-system-ppc64. For 
>> qemu-system-ppc that should correctly drop the sign extension from 32-bit, 
>> whilst still allowing someone to load a 64-bit ELF into qemu-system-ppc64 
>> if requested.
>
> IMO this is part of a bigger design problem. Not all
> machines main bus is 64-bit. I did some experiments
> but changing that involves a lot of work.

Did not want to reply to this to not bring it to your attention before 
patch gets in finally but it's too late...

Not sure what you refer to but in this particular case the problem only 
seems to be load_elf loading 32 bit ELF files returning sign extended 64 
bit address which looks bogus but since this function is widely used I did 
not feel confident enough to propose a patch to load_elf.

By the way, also the parameters of load_elf could take a clean up to 
remove all the mostly NULL values as I've pointed out before:

https://lists.nongnu.org/archive/html/qemu-devel/2019-01/msg03427.html

but all this could wait until later, these don't seem to be urgent 
problems to prevent moving mac machines forward now and could all be 
addressen in separate elf loading series. So just note the problem and 
move on for now please.

Reagards.
BALATON Zoltan

Re: [PATCH v8 0/5] Mac Old World ROM experiment (ppc/mac_* clean ups and loading binary ROM)

Posted by Philippe Mathieu-Daudé 2 days ago
On 10/17/20 6:39 PM, BALATON Zoltan via wrote:
> On Fri, 16 Oct 2020, Philippe Mathieu-Daudé wrote:
>> On 10/16/20 11:58 AM, Mark Cave-Ayland wrote:
>>> On 16/10/2020 00:47, BALATON Zoltan via wrote:
>>>
>>>> This is the cut down version of the earlier series omitting unfinished
>>>> patches that I plan to rework later and rebased to Mark's qemu-macppc
>>>> branch. Compared to v7 the only change is the cast to (target_ulong)
>>>> from (uint32_t) as requested by Mark in patch 1.
>>>
>>> FWIW the reason for suggesting the cast to target_ulong is so that 
>>> the same code works for both qemu-system-ppc and qemu-system-ppc64. 
>>> For qemu-system-ppc that should correctly drop the sign extension 
>>> from 32-bit, whilst still allowing someone to load a 64-bit ELF into 
>>> qemu-system-ppc64 if requested.
>>
>> IMO this is part of a bigger design problem. Not all
>> machines main bus is 64-bit. I did some experiments
>> but changing that involves a lot of work.
> 
> Did not want to reply to this to not bring it to your attention before 
> patch gets in finally but it's too late...
> 
> Not sure what you refer to

I refer to having machines with a N-bit main bus using a N-bit main bus
(currently all main busses are 64-bit wide).

Some 32-bit machines have access to 64-bit busses, some don't.

I have been wondering about it because of the AVR CPU which
uses a pair of busses, each less than 32-bit.

If one day I can finish my work there, the Old World mac might
benefit from it.

> but in this particular case the problem only 
> seems to be load_elf loading 32 bit ELF files returning sign extended 64 
> bit address which looks bogus but since this function is widely used I 
> did not feel confident enough to propose a patch to load_elf.
> 
> By the way, also the parameters of load_elf could take a clean up to 
> remove all the mostly NULL values as I've pointed out before:
> 
> https://lists.nongnu.org/archive/html/qemu-devel/2019-01/msg03427.html
> 
> but all this could wait until later, these don't seem to be urgent 
> problems to prevent moving mac machines forward now and could all be 
> addressen in separate elf loading series. So just note the problem and 
> move on for now please.
> 
> Reagards.
> BALATON Zoltan

Re: [PATCH v8 0/5] Mac Old World ROM experiment (ppc/mac_* clean ups and loading binary ROM)

Posted by BALATON Zoltan via 3 days ago
On Fri, 16 Oct 2020, Mark Cave-Ayland wrote:
> On 16/10/2020 00:47, BALATON Zoltan via wrote:
>> This is the cut down version of the earlier series omitting unfinished
>> patches that I plan to rework later and rebased to Mark's qemu-macppc
>> branch. Compared to v7 the only change is the cast to (target_ulong)
>> from (uint32_t) as requested by Mark in patch 1.
>
> FWIW the reason for suggesting the cast to target_ulong is so that the same 
> code works for both qemu-system-ppc and qemu-system-ppc64. For 
> qemu-system-ppc that should correctly drop the sign extension from 32-bit, 
> whilst still allowing someone to load a 64-bit ELF into qemu-system-ppc64 if 
> requested.
>
> Can you confirm that the sign extension behaviour is still correct for both 
> qemu-system-ppc and qemu-system-ppc64? If so I'm happy to give it a R-B tag.

I've tried it now again with both ppc and ppc64: both OpenBIOS and a G3 
beige ROM can be loaded with qemu-system-ppc but qemu-system-ppc64 fails 
with OpenBIOS when casting to target_ulong (i think because target_ulong 
is 64 bit there but g3beige is still 32 bit but I haven't throughly 
debugged it). But everything works with my original uint32_t cast, so 
ditch it and use my original version. Should I resubmit or you can fix up? 
(I think I wait until it's clear if this will be taken by David or you and 
send a fixed version cc-ing David if this is decided to go through the PPC 
queue.)

Regards,
BALATON Zoltan

Re: [PATCH v8 0/5] Mac Old World ROM experiment (ppc/mac_* clean ups and loading binary ROM)

Posted by Mark Cave-Ayland 2 days ago
On 16/10/2020 13:19, BALATON Zoltan via wrote:

> On Fri, 16 Oct 2020, Mark Cave-Ayland wrote:
>> On 16/10/2020 00:47, BALATON Zoltan via wrote:
>>> This is the cut down version of the earlier series omitting unfinished
>>> patches that I plan to rework later and rebased to Mark's qemu-macppc
>>> branch. Compared to v7 the only change is the cast to (target_ulong)
>>> from (uint32_t) as requested by Mark in patch 1.
>>
>> FWIW the reason for suggesting the cast to target_ulong is so that the same code 
>> works for both qemu-system-ppc and qemu-system-ppc64. For qemu-system-ppc that 
>> should correctly drop the sign extension from 32-bit, whilst still allowing someone 
>> to load a 64-bit ELF into qemu-system-ppc64 if requested.
>>
>> Can you confirm that the sign extension behaviour is still correct for both 
>> qemu-system-ppc and qemu-system-ppc64? If so I'm happy to give it a R-B tag.
> 
> I've tried it now again with both ppc and ppc64: both OpenBIOS and a G3 beige ROM can 
> be loaded with qemu-system-ppc but qemu-system-ppc64 fails with OpenBIOS when casting 
> to target_ulong (i think because target_ulong is 64 bit there but g3beige is still 32 
> bit but I haven't throughly debugged it). But everything works with my original 
> uint32_t cast, so ditch it and use my original version. Should I resubmit or you can 
> fix up? (I think I wait until it's clear if this will be taken by David or you and 
> send a fixed version cc-ing David if this is decided to go through the PPC queue.)

Hmmm yes I see that qemu-system-ppc64 fails because target_ulong will always 
represent a 64-bit quantity, even if you request a 32-bit CPU. Rather than add some 
CPU-specific code let's keep the uint32_t cast for now as I would hope it is unlikely 
someone would load an ELF in high memory, and perhaps the sign-extended address bug 
will get fixed later.

With the cast reverted to uint32_t then for patches 1 and 2:
Reviewed-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>

If you can send a v9 with the cast fixed I'll apply this to my qemu-macppc branch 
right away.


ATB,

Mark.

Re: [PATCH v8 0/5] Mac Old World ROM experiment (ppc/mac_* clean ups and loading binary ROM)

Posted by BALATON Zoltan via 2 days ago
On Sat, 17 Oct 2020, Mark Cave-Ayland wrote:
> On 16/10/2020 13:19, BALATON Zoltan via wrote:
>> On Fri, 16 Oct 2020, Mark Cave-Ayland wrote:
>>> On 16/10/2020 00:47, BALATON Zoltan via wrote:
>>>> This is the cut down version of the earlier series omitting unfinished
>>>> patches that I plan to rework later and rebased to Mark's qemu-macppc
>>>> branch. Compared to v7 the only change is the cast to (target_ulong)
>>>> from (uint32_t) as requested by Mark in patch 1.
>>> 
>>> FWIW the reason for suggesting the cast to target_ulong is so that the 
>>> same code works for both qemu-system-ppc and qemu-system-ppc64. For 
>>> qemu-system-ppc that should correctly drop the sign extension from 32-bit, 
>>> whilst still allowing someone to load a 64-bit ELF into qemu-system-ppc64 
>>> if requested.
>>> 
>>> Can you confirm that the sign extension behaviour is still correct for 
>>> both qemu-system-ppc and qemu-system-ppc64? If so I'm happy to give it a 
>>> R-B tag.
>> 
>> I've tried it now again with both ppc and ppc64: both OpenBIOS and a G3 
>> beige ROM can be loaded with qemu-system-ppc but qemu-system-ppc64 fails 
>> with OpenBIOS when casting to target_ulong (i think because target_ulong is 
>> 64 bit there but g3beige is still 32 bit but I haven't throughly debugged 
>> it). But everything works with my original uint32_t cast, so ditch it and 
>> use my original version. Should I resubmit or you can fix up? (I think I 
>> wait until it's clear if this will be taken by David or you and send a 
>> fixed version cc-ing David if this is decided to go through the PPC queue.)
>
> Hmmm yes I see that qemu-system-ppc64 fails because target_ulong will always 
> represent a 64-bit quantity, even if you request a 32-bit CPU. Rather than 
> add some CPU-specific code let's keep the uint32_t cast for now as I would 
> hope it is unlikely someone would load an ELF in high memory, and perhaps the 
> sign-extended address bug will get fixed later.
>
> With the cast reverted to uint32_t then for patches 1 and 2:
> Reviewed-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>
> If you can send a v9 with the cast fixed I'll apply this to my qemu-macppc 
> branch right away.

You could've really just edit the single cast in patch 1 before applying 
to change the it back but I've resent the changed patch 1 as v9 also 
adding your R-b for your convenience. Other patches are unchanged so you 
can take the v8 for those, I haven't resent those, let me know if you want 
the whole series but this is really getting much more work that it should 
be for such a simple change. (There is no cast in patch 2 as I've already 
stated several times.)

Regards,
BALATON Zoltan

Re: [PATCH v8 0/5] Mac Old World ROM experiment (ppc/mac_* clean ups and loading binary ROM)

Posted by Mark Cave-Ayland 1 day ago
On 17/10/2020 16:56, BALATON Zoltan via wrote:

>> If you can send a v9 with the cast fixed I'll apply this to my qemu-macppc branch 
>> right away.
> 
> You could've really just edit the single cast in patch 1 before applying to change 
> the it back but I've resent the changed patch 1 as v9 also adding your R-b for your 
> convenience. Other patches are unchanged so you can take the v8 for those, I haven't 
> resent those, let me know if you want the whole series but this is really getting 
> much more work that it should be for such a simple change. (There is no cast in patch 
> 2 as I've already stated several times.)

Thanks - this has been included in the PR I just sent.


ATB,

Mark.