diff mbox series

[3/3] target/hppa: fix building gva for wide mode

Message ID 20240324080945.991100-4-svens@stackframe.org (mailing list archive)
State New, archived
Headers show
Series few hppa fixes for 64bit mode | expand

Commit Message

Sven Schnelle March 24, 2024, 8:09 a.m. UTC
64 Bit hppa no longer has a fixed 32/32 bit split between space and
offset. Instead it uses 42 bits for the offset. The lower 10 bits of
the space are always zero, leaving 22 bits actually used. Simply or
the values together to build the gva.

Signed-off-by: Sven Schnelle <svens@stackframe.org>
---
 target/hppa/mem_helper.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

Comments

Helge Deller March 24, 2024, 4:28 p.m. UTC | #1
On 3/24/24 09:09, Sven Schnelle wrote:
> 64 Bit hppa no longer has a fixed 32/32 bit split between space and
> offset. Instead it uses 42 bits for the offset. The lower 10 bits of
> the space are always zero, leaving 22 bits actually used. Simply or
> the values together to build the gva.
>
> Signed-off-by: Sven Schnelle <svens@stackframe.org>

Reviewed-by: Helge Deller <deller@gmx.de>
Tested-by: Helge Deller <deller@gmx.de>

Helge

> ---
>   target/hppa/mem_helper.c | 13 +++++++------
>   1 file changed, 7 insertions(+), 6 deletions(-)
>
> diff --git a/target/hppa/mem_helper.c b/target/hppa/mem_helper.c
> index 84785b5a5c..6f895fced7 100644
> --- a/target/hppa/mem_helper.c
> +++ b/target/hppa/mem_helper.c
> @@ -523,13 +523,16 @@ void HELPER(itlbp_pa11)(CPUHPPAState *env, target_ulong addr, target_ulong reg)
>   }
>
>   static void itlbt_pa20(CPUHPPAState *env, target_ulong r1,
> -                       target_ulong r2, vaddr va_b)
> +                       target_ulong r2, uint64_t spc, uint64_t off)
>   {
>       HPPATLBEntry *ent;
> -    vaddr va_e;
> +    vaddr va_b, va_e;
>       uint64_t va_size;
>       int mask_shift;
>
> +    va_b = off & gva_offset_mask(env->psw);
> +    va_b |= spc << 32;
> +
>       mask_shift = 2 * (r1 & 0xf);
>       va_size = (uint64_t)TARGET_PAGE_SIZE << mask_shift;
>       va_b &= -va_size;
> @@ -569,14 +572,12 @@ static void itlbt_pa20(CPUHPPAState *env, target_ulong r1,
>
>   void HELPER(idtlbt_pa20)(CPUHPPAState *env, target_ulong r1, target_ulong r2)
>   {
> -    vaddr va_b = deposit64(env->cr[CR_IOR], 32, 32, env->cr[CR_ISR]);
> -    itlbt_pa20(env, r1, r2, va_b);
> +    itlbt_pa20(env, r1, r2, env->cr[CR_ISR], env->cr[CR_IOR]);
>   }
>
>   void HELPER(iitlbt_pa20)(CPUHPPAState *env, target_ulong r1, target_ulong r2)
>   {
> -    vaddr va_b = deposit64(env->cr[CR_IIAOQ], 32, 32, env->cr[CR_IIASQ]);
> -    itlbt_pa20(env, r1, r2, va_b);
> +    itlbt_pa20(env, r1, r2, env->cr[CR_IIASQ], env->cr[CR_IIAOQ]);
>   }
>
>   /* Purge (Insn/Data) TLB. */
Richard Henderson March 24, 2024, 5:20 p.m. UTC | #2
On 3/23/24 22:09, Sven Schnelle wrote:
> 64 Bit hppa no longer has a fixed 32/32 bit split between space and
> offset. Instead it uses 42 bits for the offset. The lower 10 bits of
> the space are always zero, leaving 22 bits actually used. Simply or
> the values together to build the gva.
> 
> Signed-off-by: Sven Schnelle <svens@stackframe.org>
> ---
>   target/hppa/mem_helper.c | 13 +++++++------
>   1 file changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/target/hppa/mem_helper.c b/target/hppa/mem_helper.c
> index 84785b5a5c..6f895fced7 100644
> --- a/target/hppa/mem_helper.c
> +++ b/target/hppa/mem_helper.c
> @@ -523,13 +523,16 @@ void HELPER(itlbp_pa11)(CPUHPPAState *env, target_ulong addr, target_ulong reg)
>   }
>   
>   static void itlbt_pa20(CPUHPPAState *env, target_ulong r1,
> -                       target_ulong r2, vaddr va_b)
> +                       target_ulong r2, uint64_t spc, uint64_t off)
>   {
>       HPPATLBEntry *ent;
> -    vaddr va_e;
> +    vaddr va_b, va_e;
>       uint64_t va_size;
>       int mask_shift;
>   
> +    va_b = off & gva_offset_mask(env->psw);
> +    va_b |= spc << 32;

Indeed, this ought to be using

     va_b = hppa_form_gva_psw(env->psw, off, spc << 32);

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


r~
Richard Henderson March 24, 2024, 9:39 p.m. UTC | #3
On 3/23/24 22:09, Sven Schnelle wrote:
> 64 Bit hppa no longer has a fixed 32/32 bit split between space and
> offset. Instead it uses 42 bits for the offset. The lower 10 bits of
> the space are always zero, leaving 22 bits actually used. Simply or
> the values together to build the gva.
> 
> Signed-off-by: Sven Schnelle <svens@stackframe.org>
> ---
>   target/hppa/mem_helper.c | 13 +++++++------
>   1 file changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/target/hppa/mem_helper.c b/target/hppa/mem_helper.c
> index 84785b5a5c..6f895fced7 100644
> --- a/target/hppa/mem_helper.c
> +++ b/target/hppa/mem_helper.c
> @@ -523,13 +523,16 @@ void HELPER(itlbp_pa11)(CPUHPPAState *env, target_ulong addr, target_ulong reg)
>   }
>   
>   static void itlbt_pa20(CPUHPPAState *env, target_ulong r1,
> -                       target_ulong r2, vaddr va_b)
> +                       target_ulong r2, uint64_t spc, uint64_t off)
>   {
>       HPPATLBEntry *ent;
> -    vaddr va_e;
> +    vaddr va_b, va_e;
>       uint64_t va_size;
>       int mask_shift;
>   
> +    va_b = off & gva_offset_mask(env->psw);
> +    va_b |= spc << 32;

Actually, no, these instructions don't form a GVA in the normal fashion:

space ← ISR;
offset ← cat(ISR{32..63},IOR{32..63});
VIRTUAL_ADDR ← (space<<32) | (offset);

> -    vaddr va_b = deposit64(env->cr[CR_IOR], 32, 32, env->cr[CR_ISR]);

But this is wrong too.

Since the input here is from the result of a previous memory access, which populated 
ISR+IOR, I presume any masking would be done from the prior memory access.

We do need to fix the merge, per the docs though.


r~
Richard Henderson March 24, 2024, 11:38 p.m. UTC | #4
On 3/24/24 11:39, Richard Henderson wrote:
> On 3/23/24 22:09, Sven Schnelle wrote:
>> 64 Bit hppa no longer has a fixed 32/32 bit split between space and
>> offset. Instead it uses 42 bits for the offset. The lower 10 bits of
>> the space are always zero, leaving 22 bits actually used. Simply or
>> the values together to build the gva.
>>
>> Signed-off-by: Sven Schnelle <svens@stackframe.org>
>> ---
>>   target/hppa/mem_helper.c | 13 +++++++------
>>   1 file changed, 7 insertions(+), 6 deletions(-)
>>
>> diff --git a/target/hppa/mem_helper.c b/target/hppa/mem_helper.c
>> index 84785b5a5c..6f895fced7 100644
>> --- a/target/hppa/mem_helper.c
>> +++ b/target/hppa/mem_helper.c
>> @@ -523,13 +523,16 @@ void HELPER(itlbp_pa11)(CPUHPPAState *env, target_ulong addr, 
>> target_ulong reg)
>>   }
>>   static void itlbt_pa20(CPUHPPAState *env, target_ulong r1,
>> -                       target_ulong r2, vaddr va_b)
>> +                       target_ulong r2, uint64_t spc, uint64_t off)
>>   {
>>       HPPATLBEntry *ent;
>> -    vaddr va_e;
>> +    vaddr va_b, va_e;
>>       uint64_t va_size;
>>       int mask_shift;
>> +    va_b = off & gva_offset_mask(env->psw);
>> +    va_b |= spc << 32;
> 
> Actually, no, these instructions don't form a GVA in the normal fashion:
> 
> space ← ISR;
> offset ← cat(ISR{32..63},IOR{32..63});
> VIRTUAL_ADDR ← (space<<32) | (offset);
> 
>> -    vaddr va_b = deposit64(env->cr[CR_IOR], 32, 32, env->cr[CR_ISR]);
> 
> But this is wrong too.

Actually, no.  The

   VIRTUAL_ADDR = (space << 32) | offset

line constructs a 96-bit virtual address.  The low 32 bits of ISR have already replaced 
the high 32 bits of IOR, so this line really only adds the high 32 bits of space as bits 
[96:64] of the full virtual address.

Truncated to 64 bits, the deposit as written is exactly right.


r~
diff mbox series

Patch

diff --git a/target/hppa/mem_helper.c b/target/hppa/mem_helper.c
index 84785b5a5c..6f895fced7 100644
--- a/target/hppa/mem_helper.c
+++ b/target/hppa/mem_helper.c
@@ -523,13 +523,16 @@  void HELPER(itlbp_pa11)(CPUHPPAState *env, target_ulong addr, target_ulong reg)
 }
 
 static void itlbt_pa20(CPUHPPAState *env, target_ulong r1,
-                       target_ulong r2, vaddr va_b)
+                       target_ulong r2, uint64_t spc, uint64_t off)
 {
     HPPATLBEntry *ent;
-    vaddr va_e;
+    vaddr va_b, va_e;
     uint64_t va_size;
     int mask_shift;
 
+    va_b = off & gva_offset_mask(env->psw);
+    va_b |= spc << 32;
+
     mask_shift = 2 * (r1 & 0xf);
     va_size = (uint64_t)TARGET_PAGE_SIZE << mask_shift;
     va_b &= -va_size;
@@ -569,14 +572,12 @@  static void itlbt_pa20(CPUHPPAState *env, target_ulong r1,
 
 void HELPER(idtlbt_pa20)(CPUHPPAState *env, target_ulong r1, target_ulong r2)
 {
-    vaddr va_b = deposit64(env->cr[CR_IOR], 32, 32, env->cr[CR_ISR]);
-    itlbt_pa20(env, r1, r2, va_b);
+    itlbt_pa20(env, r1, r2, env->cr[CR_ISR], env->cr[CR_IOR]);
 }
 
 void HELPER(iitlbt_pa20)(CPUHPPAState *env, target_ulong r1, target_ulong r2)
 {
-    vaddr va_b = deposit64(env->cr[CR_IIAOQ], 32, 32, env->cr[CR_IIASQ]);
-    itlbt_pa20(env, r1, r2, va_b);
+    itlbt_pa20(env, r1, r2, env->cr[CR_IIASQ], env->cr[CR_IIAOQ]);
 }
 
 /* Purge (Insn/Data) TLB. */