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

[Richard Henderson <richard.henderson@linaro.org>]

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.

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.


r~