1. Patchew
  2. QEMU
  3. [PATCH v2 0/9] target/hppa qemu v8.2 regression fixes
  4. View patch

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

deller@kernel.org posted 9 patches 10 months, 3 weeks ago
Maintainers: Richard Henderson <richard.henderson@linaro.org>, Helge Deller <deller@gmx.de>
There is a newer version of this series
[PATCH v2 4/9] target/hppa: Fix PDC address translation on PA2.0 with PSW.W=0
Posted by deller@kernel.org 10 months, 3 weeks ago 
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;
-- 
2.43.0
Re: [PATCH v2 4/9] target/hppa: Fix PDC address translation on PA2.0 with PSW.W=0
Posted by Richard Henderson 10 months, 3 weeks ago 
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.


r~
Re: [PATCH v2 4/9] target/hppa: Fix PDC address translation on PA2.0 with PSW.W=0
Posted by Helge Deller 10 months, 3 weeks ago 
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.

Helge
Re: [PATCH v2 4/9] target/hppa: Fix PDC address translation on PA2.0 with PSW.W=0
Posted by Richard Henderson 10 months, 3 weeks ago 
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~
Re: [PATCH v2 4/9] target/hppa: Fix PDC address translation on PA2.0 with PSW.W=0
Posted by Helge Deller 10 months, 3 weeks ago 
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
Re: [PATCH v2 4/9] target/hppa: Fix PDC address translation on PA2.0 with PSW.W=0
Posted by Richard Henderson 10 months, 3 weeks ago 
On 1/10/24 08:06, Helge Deller wrote:
>> 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).

Ok, fair enough.  I just wish HP had been more accurate in their diagrams.  :-)


r~

Patchew 2.1-devel-b67e4c2

© 2016 - 2024 Red Hat, Inc.