target/avr: Use vaddr type for $PC jumps

[PATCH] target/avr: Use vaddr type for $PC jumps

Posted by Philippe Mathieu-Daudé 1 month ago

translator_use_goto_tb() expects a vaddr type since commit
b1c09220b4c ("accel/tcg: Replace target_ulong with vaddr in
translator*()").

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 target/avr/translate.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/target/avr/translate.c b/target/avr/translate.c
index 804b0b21dbd..20191055861 100644
--- a/target/avr/translate.c
+++ b/target/avr/translate.c
@@ -87,7 +87,7 @@ struct DisasContext {
     CPUAVRState *env;
     CPUState *cs;
 
-    target_long npc;
+    vaddr npc;
     uint32_t opcode;
 
     /* Routine used to access memory */
@@ -981,7 +981,7 @@ static void gen_pop_ret(DisasContext *ctx, TCGv ret)
     }
 }
 
-static void gen_goto_tb(DisasContext *ctx, int n, target_ulong dest)
+static void gen_goto_tb(DisasContext *ctx, int n, vaddr dest)
 {
     const TranslationBlock *tb = ctx->base.tb;
 
@@ -1004,7 +1004,7 @@ static void gen_goto_tb(DisasContext *ctx, int n, target_ulong dest)
  */
 static bool trans_RJMP(DisasContext *ctx, arg_RJMP *a)
 {
-    int dst = ctx->npc + a->imm;
+    vaddr dst = ctx->npc + a->imm;
 
     gen_goto_tb(ctx, 0, dst);
 
@@ -1072,8 +1072,8 @@ static bool trans_JMP(DisasContext *ctx, arg_JMP *a)
  */
 static bool trans_RCALL(DisasContext *ctx, arg_RCALL *a)
 {
-    int ret = ctx->npc;
-    int dst = ctx->npc + a->imm;
+    vaddr ret = ctx->npc;
+    vaddr dst = ctx->npc + a->imm;
 
     gen_push_ret(ctx, ret);
     gen_goto_tb(ctx, 0, dst);
@@ -1094,7 +1094,7 @@ static bool trans_ICALL(DisasContext *ctx, arg_ICALL *a)
         return true;
     }
 
-    int ret = ctx->npc;
+    vaddr ret = ctx->npc;
 
     gen_push_ret(ctx, ret);
     gen_jmp_z(ctx);
@@ -1136,8 +1136,8 @@ static bool trans_CALL(DisasContext *ctx, arg_CALL *a)
         return true;
     }
 
-    int Imm = a->imm;
-    int ret = ctx->npc;
+    vaddr Imm = a->imm;
+    vaddr ret = ctx->npc;
 
     gen_push_ret(ctx, ret);
     gen_goto_tb(ctx, 0, Imm);
@@ -2743,7 +2743,7 @@ static void avr_tr_translate_insn(DisasContextBase *dcbase, CPUState *cs)
     }
 
     if (ctx->base.is_jmp == DISAS_NEXT) {
-        target_ulong page_first = ctx->base.pc_first & TARGET_PAGE_MASK;
+        vaddr page_first = ctx->base.pc_first & TARGET_PAGE_MASK;
 
         if ((ctx->base.pc_next - page_first) >= TARGET_PAGE_SIZE - 4) {
             ctx->base.is_jmp = DISAS_TOO_MANY;
-- 
2.51.0

Re: [PATCH] target/avr: Use vaddr type for $PC jumps

Posted by Richard Henderson 1 month ago

On 10/9/25 13:05, Philippe Mathieu-Daudé wrote:
> translator_use_goto_tb() expects a vaddr type since commit
> b1c09220b4c ("accel/tcg: Replace target_ulong with vaddr in
> translator*()").
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   target/avr/translate.c | 18 +++++++++---------
>   1 file changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/target/avr/translate.c b/target/avr/translate.c
> index 804b0b21dbd..20191055861 100644
> --- a/target/avr/translate.c
> +++ b/target/avr/translate.c
> @@ -87,7 +87,7 @@ struct DisasContext {
>       CPUAVRState *env;
>       CPUState *cs;
>   
> -    target_long npc;
> +    vaddr npc;

Ah, here's where proper typing might have saved us a bug.

npc is not a virtual (or physical) address in the normal sense, it is a *word* address 
(i.e. byte address / 2).

So I think you should just use uint32_t here.

>       uint32_t opcode;
>   
>       /* Routine used to access memory */
> @@ -981,7 +981,7 @@ static void gen_pop_ret(DisasContext *ctx, TCGv ret)
>       }
>   }
>   
> -static void gen_goto_tb(DisasContext *ctx, int n, target_ulong dest)
> +static void gen_goto_tb(DisasContext *ctx, int n, vaddr dest)
>   {
>       const TranslationBlock *tb = ctx->base.tb;
>   
> @@ -1004,7 +1004,7 @@ static void gen_goto_tb(DisasContext *ctx, int n, target_ulong dest)
>    */
>   static bool trans_RJMP(DisasContext *ctx, arg_RJMP *a)
>   {
> -    int dst = ctx->npc + a->imm;
> +    vaddr dst = ctx->npc + a->imm;

And here...

>   
>       gen_goto_tb(ctx, 0, dst);

... and therefore also in the gen_goto_tb argument.

The bug can thus be said to be within gen_goto_tb, where we don't convert from word 
address to byte address.



r~

Re: [PATCH] target/avr: Use vaddr type for $PC jumps

Posted by Philippe Mathieu-Daudé 1 month ago

On 9/10/25 23:42, Richard Henderson wrote:
> On 10/9/25 13:05, Philippe Mathieu-Daudé wrote:
>> translator_use_goto_tb() expects a vaddr type since commit
>> b1c09220b4c ("accel/tcg: Replace target_ulong with vaddr in
>> translator*()").
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>>   target/avr/translate.c | 18 +++++++++---------
>>   1 file changed, 9 insertions(+), 9 deletions(-)
>>
>> diff --git a/target/avr/translate.c b/target/avr/translate.c
>> index 804b0b21dbd..20191055861 100644
>> --- a/target/avr/translate.c
>> +++ b/target/avr/translate.c
>> @@ -87,7 +87,7 @@ struct DisasContext {
>>       CPUAVRState *env;
>>       CPUState *cs;
>> -    target_long npc;
>> +    vaddr npc;
> 
> Ah, here's where proper typing might have saved us a bug.
> 
> npc is not a virtual (or physical) address in the normal sense, it is a 
> *word* address (i.e. byte address / 2).
> 
> So I think you should just use uint32_t here.

IIUC the field is 24-bit wide, OK.

> 
>>       uint32_t opcode;
>>       /* Routine used to access memory */
>> @@ -981,7 +981,7 @@ static void gen_pop_ret(DisasContext *ctx, TCGv ret)
>>       }
>>   }
>> -static void gen_goto_tb(DisasContext *ctx, int n, target_ulong dest)
>> +static void gen_goto_tb(DisasContext *ctx, int n, vaddr dest)
>>   {
>>       const TranslationBlock *tb = ctx->base.tb;
>> @@ -1004,7 +1004,7 @@ static void gen_goto_tb(DisasContext *ctx, int 
>> n, target_ulong dest)
>>    */
>>   static bool trans_RJMP(DisasContext *ctx, arg_RJMP *a)
>>   {
>> -    int dst = ctx->npc + a->imm;
>> +    vaddr dst = ctx->npc + a->imm;
> 
> And here...
> 
>>       gen_goto_tb(ctx, 0, dst);
> 
> ... and therefore also in the gen_goto_tb argument.
> 
> The bug can thus be said to be within gen_goto_tb, where we don't 
> convert from word address to byte address.

Oh I see. Then avr_tr_insn_start() is also buggy?

Re: [PATCH] target/avr: Use vaddr type for $PC jumps

Posted by Richard Henderson 1 month ago

On 10/9/25 19:56, Philippe Mathieu-Daudé wrote:
> Oh I see. Then avr_tr_insn_start() is also buggy?

Good question.  It depends on how we view things.

At least once upon a time, the values in insn_start were all private to the target.  They 
were only consumed by restore_state_to_opc.

Over time there has been creep, where the first argument might be assumed to be the 
virtual address.  E.g. plugins?  But beyond that?


r~