diff mbox series

target: hppa: Fix unaligned double word accesses for hppa64

Message ID 20240216053415.2163286-1-linux@roeck-us.net (mailing list archive)
State New, archived
Headers show
Series target: hppa: Fix unaligned double word accesses for hppa64 | expand

Commit Message

Guenter Roeck Feb. 16, 2024, 5:34 a.m. UTC
Unaligned 64-bit accesses were found in Linux to clobber carry bits,
resulting in bad results if an arithmetic operation involving a
carry bit was executed after an unaligned 64-bit operation.

hppa 2.0 defines additional carry bits in PSW register bits 32..39.
When restoring PSW after executing an unaligned instruction trap,
those bits were not cleared and ended up to be active all the time.
Clearing bit 32..39 in psw prior to restoring it solves the problem.

Fixes: 931adff31478 ("target/hppa: Update cpu_hppa_get/put_psw for hppa64")
Cc: Richard Henderson <richard.henderson@linaro.org>
Cc: Charlie Jenkins <charlie@rivosinc.com>
Cc: Helge Deller <deller@gmx.de>
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 target/hppa/helper.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

Comments

Charlie Jenkins Feb. 16, 2024, 5:58 a.m. UTC | #1
On Thu, Feb 15, 2024 at 09:34:15PM -0800, Guenter Roeck wrote:
> Unaligned 64-bit accesses were found in Linux to clobber carry bits,
> resulting in bad results if an arithmetic operation involving a
> carry bit was executed after an unaligned 64-bit operation.
> 
> hppa 2.0 defines additional carry bits in PSW register bits 32..39.
> When restoring PSW after executing an unaligned instruction trap,
> those bits were not cleared and ended up to be active all the time.
> Clearing bit 32..39 in psw prior to restoring it solves the problem.
> 
> Fixes: 931adff31478 ("target/hppa: Update cpu_hppa_get/put_psw for hppa64")
> Cc: Richard Henderson <richard.henderson@linaro.org>
> Cc: Charlie Jenkins <charlie@rivosinc.com>
> Cc: Helge Deller <deller@gmx.de>
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> ---
>  target/hppa/helper.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/target/hppa/helper.c b/target/hppa/helper.c
> index 859644c47a..7b798d1227 100644
> --- a/target/hppa/helper.c
> +++ b/target/hppa/helper.c
> @@ -76,7 +76,12 @@ void cpu_hppa_put_psw(CPUHPPAState *env, target_ulong psw)
>      }
>      psw &= ~reserved;
>  
> -    env->psw = psw & ~(PSW_N | PSW_V | PSW_CB);
> +    if (hppa_is_pa20(env)) {
> +        env->psw = psw & ~(PSW_N | PSW_V | PSW_CB | 0xff00000000ull);

I thought there was something fishy in this function but was slow on the
uptake...

How about defining a new macro (PSW_CB_HIGH) to hold this value?

- Charlie

> +    } else {
> +        env->psw = psw & ~(PSW_N | PSW_V | PSW_CB);
> +    }
> +
>      env->psw_n = (psw / PSW_N) & 1;
>      env->psw_v = -((psw / PSW_V) & 1);
>  
> -- 
> 2.39.2
>
Richard Henderson Feb. 16, 2024, 6:16 a.m. UTC | #2
On 2/15/24 19:34, Guenter Roeck wrote:
> -    env->psw = psw & ~(PSW_N | PSW_V | PSW_CB);
> +    if (hppa_is_pa20(env)) {
> +        env->psw = psw & ~(PSW_N | PSW_V | PSW_CB | 0xff00000000ull);
> +    } else {
> +        env->psw = psw & ~(PSW_N | PSW_V | PSW_CB);
> +    }

There are never any bits above 31 set in env->psw, because all of the CB bits are supposed 
to be stored in env->psw_cb.  Thus

   env->psw = psw & (uint32_t)~(...)

with no need for the pa20 check.

With that,

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~
Guenter Roeck Feb. 16, 2024, 6:21 a.m. UTC | #3
On 2/15/24 22:16, Richard Henderson wrote:
> On 2/15/24 19:34, Guenter Roeck wrote:
>> -    env->psw = psw & ~(PSW_N | PSW_V | PSW_CB);
>> +    if (hppa_is_pa20(env)) {
>> +        env->psw = psw & ~(PSW_N | PSW_V | PSW_CB | 0xff00000000ull);
>> +    } else {
>> +        env->psw = psw & ~(PSW_N | PSW_V | PSW_CB);
>> +    }
> 
> There are never any bits above 31 set in env->psw, because all of the CB bits are supposed to be stored in env->psw_cb.  Thus
> 
>    env->psw = psw & (uint32_t)~(...)
> 
> with no need for the pa20 check.
> 
> With that,
> 
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> 

sgtm. I'll test that and send v2 tomorrow (it is getting late).

Thanks,
Guenter
Helge Deller Feb. 16, 2024, 6:54 a.m. UTC | #4
On 2/16/24 06:58, Charlie Jenkins wrote:
> On Thu, Feb 15, 2024 at 09:34:15PM -0800, Guenter Roeck wrote:
>> Unaligned 64-bit accesses were found in Linux to clobber carry bits,
>> resulting in bad results if an arithmetic operation involving a
>> carry bit was executed after an unaligned 64-bit operation.
>>
>> hppa 2.0 defines additional carry bits in PSW register bits 32..39.
>> When restoring PSW after executing an unaligned instruction trap,
>> those bits were not cleared and ended up to be active all the time.
>> Clearing bit 32..39 in psw prior to restoring it solves the problem.
>>
>> Fixes: 931adff31478 ("target/hppa: Update cpu_hppa_get/put_psw for hppa64")
>> Cc: Richard Henderson <richard.henderson@linaro.org>
>> Cc: Charlie Jenkins <charlie@rivosinc.com>
>> Cc: Helge Deller <deller@gmx.de>
>> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
>> ---
>>   target/hppa/helper.c | 7 ++++++-
>>   1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/target/hppa/helper.c b/target/hppa/helper.c
>> index 859644c47a..7b798d1227 100644
>> --- a/target/hppa/helper.c
>> +++ b/target/hppa/helper.c
>> @@ -76,7 +76,12 @@ void cpu_hppa_put_psw(CPUHPPAState *env, target_ulong psw)
>>       }
>>       psw &= ~reserved;
>>
>> -    env->psw = psw & ~(PSW_N | PSW_V | PSW_CB);
>> +    if (hppa_is_pa20(env)) {
>> +        env->psw = psw & ~(PSW_N | PSW_V | PSW_CB | 0xff00000000ull);
>
> I thought there was something fishy in this function but was slow on the
> uptake...
>
> How about defining a new macro (PSW_CB_HIGH) to hold this value?

...and avoid the hppa_is_pa20() by using PSW_CB_HIGH unconditionally
on 32-bit too (which then gets optimized-out by the compiler).

Nice catch btw!
I wonder if this finally fixes 64-bit Linux kernels on qemu-hppa20....?

Helge

> - Charlie
>
>> +    } else {
>> +        env->psw = psw & ~(PSW_N | PSW_V | PSW_CB);
>> +    }
>> +
>>       env->psw_n = (psw / PSW_N) & 1;
>>       env->psw_v = -((psw / PSW_V) & 1);
>>
>> --
>> 2.39.2
>>
diff mbox series

Patch

diff --git a/target/hppa/helper.c b/target/hppa/helper.c
index 859644c47a..7b798d1227 100644
--- a/target/hppa/helper.c
+++ b/target/hppa/helper.c
@@ -76,7 +76,12 @@  void cpu_hppa_put_psw(CPUHPPAState *env, target_ulong psw)
     }
     psw &= ~reserved;
 
-    env->psw = psw & ~(PSW_N | PSW_V | PSW_CB);
+    if (hppa_is_pa20(env)) {
+        env->psw = psw & ~(PSW_N | PSW_V | PSW_CB | 0xff00000000ull);
+    } else {
+        env->psw = psw & ~(PSW_N | PSW_V | PSW_CB);
+    }
+
     env->psw_n = (psw / PSW_N) & 1;
     env->psw_v = -((psw / PSW_V) & 1);