Re: [PATCH v2 4/9] target/hppa: Fix PDC address translation on PA2.0 with PSW.W=0

[Helge Deller <deller@gmx.de>]

On 1/9/24 17:18, Richard Henderson wrote:
> On 1/9/24 22:22, Helge Deller wrote:
>> On 1/9/24 10:14, Richard Henderson wrote:
>>> On 1/8/24 00:22, deller@kernel.org wrote:
>>>> From: Helge Deller <deller@gmx.de>
>>>>
>>>> Fix the address translation for PDC space on PA2.0 if PSW.W=0.
>>>> Basically, for any address in the 32-bit PDC range from 0xf0000000 to
>>>> 0xf1000000 keep the lower 32-bits and just set the upper 32-bits to
>>>> 0xfffffff0.
>>>>
>>>> This mapping fixes the emulated power button in PDC space for 32- and
>>>> 64-bit machines and is how the physical C3700 machine seems to map
>>>> PDC.
>>>>
>>>> Signed-off-by: Helge Deller <deller@gmx.de>
>>>> ---
>>>>   target/hppa/mem_helper.c | 2 +-
>>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/target/hppa/mem_helper.c b/target/hppa/mem_helper.c
>>>> index 08abd1a9f9..011b192406 100644
>>>> --- a/target/hppa/mem_helper.c
>>>> +++ b/target/hppa/mem_helper.c
>>>> @@ -56,7 +56,7 @@ hwaddr hppa_abs_to_phys_pa2_w0(vaddr addr)
>>>>           addr = (int32_t)addr;
>>>>       } else {
>>>>           /* PDC address space */
>>>> -        addr &= MAKE_64BIT_MASK(0, 24);
>>>> +        addr = (uint32_t)addr;
>>>>           addr |= -1ull << (TARGET_PHYS_ADDR_SPACE_BITS - 4);
>>>>       }
>>>>       return addr;
>>>
>>> I believe this to be incorrect, as it contradicts Figures H-10 and H-11.
>>
>> Yes, but that seems to be how it's really implemented on physical hardware.
>> We have seen other figures as well, which didn't reflect the real world either.
>> IMHO we can revert if it really turns out to be wrong and when we
>> get a better solution.
>
> What evidence?  So far, all I can see is for your seabios button, which doesn't run on physical hardware.

You are wrong on this.
My Seabios just mimics the real hardware. And the hardware has such a button
which is reported back by the PDC firmware.
Here is what the Linux kernel reports on *physical* hardware:
64-bit kernel -> powersw: Soft power switch at 0xfffffff0f0400804 enabled.
32-bit kernel -> powersw: Soft power switch at 0xf0400804 enabled
Just look at the old dmesg from another user (with Linux kernel 2.6.16):
http://ftp.parisc-linux.org/dmesg/dmesg_C3700.txt
(search for "power" in that log).

As you can see, even the real C3700 reports the power button inside the
firmware region. And on 64-bit the higher 32-bits are at 0xfffffff0.
That's exactly what I do with this patch.

> In any case, there is a comment just above pointing to the spec, which you are now deviating from.  You need to expand that comment to say why and how.

Ok.

Helge