[PATCH v2 0/4] target/avr: Few fixes

[PATCH v2 0/4] target/avr: Few fixes

[Philippe Mathieu-Daudé]

Since v1:
- added missing 'Fix store instructions display order'

Few fixes on top of the AVR merger series Thomas sent yesterday:
https://www.mail-archive.com/qemu-devel@nongnu.org/msg720089.html
Based-on: <20200705140315.260514-1-huth@tuxfamily.org>

Philippe Mathieu-Daudé (4):
  target/avr/cpu: Drop tlb_flush() in avr_cpu_reset()
  target/avr/cpu: Fix $PC displayed address
  target/avr/disas: Fix store instructions display order
  target/avr/translate: Fix SBRC/SBRS instructions

 target/avr/cpu.c       |  4 +---
 target/avr/disas.c     | 20 ++++++++++----------
 target/avr/translate.c |  4 ++--
 3 files changed, 13 insertions(+), 15 deletions(-)

-- 
2.21.3

[PATCH v2 1/4] target/avr/cpu: Drop tlb_flush() in avr_cpu_reset()

[Philippe Mathieu-Daudé]

Since commit 1f5c00cfdb tlb_flush() is called from cpu_common_reset().

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 target/avr/cpu.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/target/avr/cpu.c b/target/avr/cpu.c
index 4e4dd4f6aa..50fb1c378b 100644
--- a/target/avr/cpu.c
+++ b/target/avr/cpu.c
@@ -78,8 +78,6 @@ static void avr_cpu_reset(DeviceState *ds)
     env->skip = 0;
 
     memset(env->r, 0, sizeof(env->r));
-
-    tlb_flush(cs);
 }
 
 static void avr_cpu_disas_set_info(CPUState *cpu, disassemble_info *info)
-- 
2.21.3

Re: [PATCH v2 1/4] target/avr/cpu: Drop tlb_flush() in avr_cpu_reset()

[Thomas Huth]

Am Tue,  7 Jul 2020 09:00:18 +0200
schrieb Philippe Mathieu-Daudé <f4bug@amsat.org>:

> Since commit 1f5c00cfdb tlb_flush() is called from cpu_common_reset().
> 
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  target/avr/cpu.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/target/avr/cpu.c b/target/avr/cpu.c
> index 4e4dd4f6aa..50fb1c378b 100644
> --- a/target/avr/cpu.c
> +++ b/target/avr/cpu.c
> @@ -78,8 +78,6 @@ static void avr_cpu_reset(DeviceState *ds)
>      env->skip = 0;
>  
>      memset(env->r, 0, sizeof(env->r));
> -
> -    tlb_flush(cs);
>  }
>  
>  static void avr_cpu_disas_set_info(CPUState *cpu, disassemble_info
> *info)

Reviewed-by: Thomas Huth <huth@tuxfamily.org>

Re: [PATCH v2 1/4] target/avr/cpu: Drop tlb_flush() in avr_cpu_reset()

[Richard Henderson]

On 7/7/20 12:00 AM, Philippe Mathieu-Daudé wrote:
> Since commit 1f5c00cfdb tlb_flush() is called from cpu_common_reset().
> 
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  target/avr/cpu.c | 2 --
>  1 file changed, 2 deletions(-)

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

r~

[PATCH v2 2/4] target/avr/cpu: Fix $PC displayed address

[Philippe Mathieu-Daudé]

$PC is 16-bit wide. Other registers display addresses on a byte
granularity.
To have a coherent ouput, display $PC using byte granularity too.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 target/avr/cpu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/avr/cpu.c b/target/avr/cpu.c
index 50fb1c378b..9be464991f 100644
--- a/target/avr/cpu.c
+++ b/target/avr/cpu.c
@@ -151,7 +151,7 @@ static void avr_cpu_dump_state(CPUState *cs, FILE *f, int flags)
     int i;
 
     qemu_fprintf(f, "\n");
-    qemu_fprintf(f, "PC:    %06x\n", env->pc_w);
+    qemu_fprintf(f, "PC:    %06x\n", env->pc_w * 2);
     qemu_fprintf(f, "SP:      %04x\n", env->sp);
     qemu_fprintf(f, "rampD:     %02x\n", env->rampD >> 16);
     qemu_fprintf(f, "rampX:     %02x\n", env->rampX >> 16);
-- 
2.21.3

Re: [PATCH v2 2/4] target/avr/cpu: Fix $PC displayed address

[Thomas Huth]

Am Tue,  7 Jul 2020 09:00:19 +0200
schrieb Philippe Mathieu-Daudé <f4bug@amsat.org>:

> $PC is 16-bit wide. Other registers display addresses on a byte
> granularity.
> To have a coherent ouput, display $PC using byte granularity too.
> 
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  target/avr/cpu.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/target/avr/cpu.c b/target/avr/cpu.c
> index 50fb1c378b..9be464991f 100644
> --- a/target/avr/cpu.c
> +++ b/target/avr/cpu.c
> @@ -151,7 +151,7 @@ static void avr_cpu_dump_state(CPUState *cs, FILE
> *f, int flags) int i;
>  
>      qemu_fprintf(f, "\n");
> -    qemu_fprintf(f, "PC:    %06x\n", env->pc_w);
> +    qemu_fprintf(f, "PC:    %06x\n", env->pc_w * 2);
>      qemu_fprintf(f, "SP:      %04x\n", env->sp);
>      qemu_fprintf(f, "rampD:     %02x\n", env->rampD >> 16);
>      qemu_fprintf(f, "rampX:     %02x\n", env->rampX >> 16);

Reviewed-by: Thomas Huth <huth@tuxfamily.org>

Re: [PATCH v2 2/4] target/avr/cpu: Fix $PC displayed address

[Richard Henderson]

On 7/7/20 12:00 AM, Philippe Mathieu-Daudé wrote:
> $PC is 16-bit wide. Other registers display addresses on a byte
> granularity.
> To have a coherent ouput, display $PC using byte granularity too.
> 
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  target/avr/cpu.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

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

r~

[PATCH v2 3/4] target/avr/disas: Fix store instructions display order

[Philippe Mathieu-Daudé]

While LOAD instructions use the target register as first
argument, STORE instructions use it as second argument:

  LD Rd, X        // Rd <- (X)

  ST Y, Rd        // (Y) <- Rr

Reported-by: Joaquin de Andres <me@xcancerberox.com.ar>
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 target/avr/disas.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/target/avr/disas.c b/target/avr/disas.c
index 23bd9919ed..7fa6e8711b 100644
--- a/target/avr/disas.c
+++ b/target/avr/disas.c
@@ -196,16 +196,16 @@ INSN(LDZ2,   "r%d, Z+", a->rd)
 INSN(LDZ3,   "r%d, -Z", a->rd)
 INSN(LDDY,   "r%d, Y+%d", a->rd, a->imm)
 INSN(LDDZ,   "r%d, Z+%d", a->rd, a->imm)
-INSN(STS,    "r%d, %d", a->rd, a->imm)
-INSN(STX1,   "r%d, X", a->rr)
-INSN(STX2,   "r%d, X+", a->rr)
-INSN(STX3,   "r%d, -X", a->rr)
-INSN(STY2,   "r%d, Y+", a->rd)
-INSN(STY3,   "r%d, -Y", a->rd)
-INSN(STZ2,   "r%d, Z+", a->rd)
-INSN(STZ3,   "r%d, -Z", a->rd)
-INSN(STDY,   "r%d, Y+%d", a->rd, a->imm)
-INSN(STDZ,   "r%d, Z+%d", a->rd, a->imm)
+INSN(STS,    "%d, r%d", a->imm, a->rd)
+INSN(STX1,   "X, r%d", a->rr)
+INSN(STX2,   "X+, r%d", a->rr)
+INSN(STX3,   "-X, r%d", a->rr)
+INSN(STY2,   "Y+, r%d", a->rd)
+INSN(STY3,   "-Y, r%d", a->rd)
+INSN(STZ2,   "Z+, r%d", a->rd)
+INSN(STZ3,   "-Z, r%d", a->rd)
+INSN(STDY,   "Y+%d, r%d", a->imm, a->rd)
+INSN(STDZ,   "Z+%d, r%d", a->imm, a->rd)
 INSN(LPM1,   "")
 INSN(LPM2,   "r%d, Z", a->rd)
 INSN(LPMX,   "r%d, Z+", a->rd)
-- 
2.21.3

Re: [PATCH v2 3/4] target/avr/disas: Fix store instructions display order

[Richard Henderson]

On 7/7/20 12:00 AM, Philippe Mathieu-Daudé wrote:
> While LOAD instructions use the target register as first
> argument, STORE instructions use it as second argument:
> 
>   LD Rd, X        // Rd <- (X)
> 
>   ST Y, Rd        // (Y) <- Rr
> 
> Reported-by: Joaquin de Andres <me@xcancerberox.com.ar>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  target/avr/disas.c | 20 ++++++++++----------
>  1 file changed, 10 insertions(+), 10 deletions(-)

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

r~

[RFC PATCH v2 4/4] target/avr/translate: Fix SBRC/SBRS instructions

[Philippe Mathieu-Daudé]

SBRC/SBRS instructions seem to be inverted.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 target/avr/translate.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/target/avr/translate.c b/target/avr/translate.c
index fe03e676df..2f77fe3ba7 100644
--- a/target/avr/translate.c
+++ b/target/avr/translate.c
@@ -1385,7 +1385,7 @@ static bool trans_SBRC(DisasContext *ctx, arg_SBRC *a)
 {
     TCGv Rr = cpu_r[a->rr];
 
-    ctx->skip_cond = TCG_COND_EQ;
+    ctx->skip_cond = TCG_COND_NE;
     ctx->skip_var0 = tcg_temp_new();
     ctx->free_skip_var0 = true;
 
@@ -1401,7 +1401,7 @@ static bool trans_SBRS(DisasContext *ctx, arg_SBRS *a)
 {
     TCGv Rr = cpu_r[a->rr];
 
-    ctx->skip_cond = TCG_COND_NE;
+    ctx->skip_cond = TCG_COND_EQ;
     ctx->skip_var0 = tcg_temp_new();
     ctx->free_skip_var0 = true;
 
-- 
2.21.3

Re: [RFC PATCH v2 4/4] target/avr/translate: Fix SBRC/SBRS instructions

[Richard Henderson]

On 7/7/20 12:00 AM, Philippe Mathieu-Daudé wrote:
> @@ -1385,7 +1385,7 @@ static bool trans_SBRC(DisasContext *ctx, arg_SBRC *a)
>  {
>      TCGv Rr = cpu_r[a->rr];
>  
> -    ctx->skip_cond = TCG_COND_EQ;
> +    ctx->skip_cond = TCG_COND_NE;
>      ctx->skip_var0 = tcg_temp_new();
>      ctx->free_skip_var0 = true;

This is wrong.  The next line is

>     tcg_gen_andi_tl(ctx->skip_var0, Rr, 1 << a->bit);

So we compute "var = R & bit", which should be zero for "Skip if Bit in
Register Clear".  Thus "var EQ 0" is the correct test.


> @@ -1401,7 +1401,7 @@ static bool trans_SBRS(DisasContext *ctx, arg_SBRS *a)
>  {
>      TCGv Rr = cpu_r[a->rr];
>  
> -    ctx->skip_cond = TCG_COND_NE;
> +    ctx->skip_cond = TCG_COND_EQ;
>      ctx->skip_var0 = tcg_temp_new();
>      ctx->free_skip_var0 = true;

Similarly this is "var NE 0".


r~

Re: [RFC PATCH v2 4/4] target/avr/translate: Fix SBRC/SBRS instructions

[Philippe Mathieu-Daudé]

On 7/7/20 6:34 PM, Richard Henderson wrote:
> On 7/7/20 12:00 AM, Philippe Mathieu-Daudé wrote:
>> @@ -1385,7 +1385,7 @@ static bool trans_SBRC(DisasContext *ctx, arg_SBRC *a)
>>  {
>>      TCGv Rr = cpu_r[a->rr];
>>  
>> -    ctx->skip_cond = TCG_COND_EQ;
>> +    ctx->skip_cond = TCG_COND_NE;
>>      ctx->skip_var0 = tcg_temp_new();
>>      ctx->free_skip_var0 = true;
> 
> This is wrong.  The next line is
> 
>>     tcg_gen_andi_tl(ctx->skip_var0, Rr, 1 << a->bit);
> 
> So we compute "var = R & bit", which should be zero for "Skip if Bit in
> Register Clear".  Thus "var EQ 0" is the correct test.

Thanks for verifying. If TCG is correct, then some hardware register
might have a bit flipped.

I couldn't run Sarah's test suite on Fedora 30:

/usr/lib/gcc/avr/9.2.0/../../../../avr/bin/ld: cannot find
crtatmega2560.o: No such file or directory
/usr/lib/gcc/avr/9.2.0/../../../../avr/bin/ld: cannot find -lm
/usr/lib/gcc/avr/9.2.0/../../../../avr/bin/ld: cannot find -lc
/usr/lib/gcc/avr/9.2.0/../../../../avr/bin/ld: cannot find -latmega2560
collect2: error: ld returned 1 exit status

I'll try on some Debian based host.

> 
>> @@ -1401,7 +1401,7 @@ static bool trans_SBRS(DisasContext *ctx, arg_SBRS *a)
>>  {
>>      TCGv Rr = cpu_r[a->rr];
>>  
>> -    ctx->skip_cond = TCG_COND_NE;
>> +    ctx->skip_cond = TCG_COND_EQ;
>>      ctx->skip_var0 = tcg_temp_new();
>>      ctx->free_skip_var0 = true;
> 
> Similarly this is "var NE 0".

Re: [RFC PATCH v2 4/4] target/avr/translate: Fix SBRC/SBRS instructions

[Richard Henderson]

On 7/7/20 9:58 AM, Philippe Mathieu-Daudé wrote:
> I couldn't run Sarah's test suite on Fedora 30:
> 
> /usr/lib/gcc/avr/9.2.0/../../../../avr/bin/ld: cannot find
> crtatmega2560.o: No such file or directory
> /usr/lib/gcc/avr/9.2.0/../../../../avr/bin/ld: cannot find -lm
> /usr/lib/gcc/avr/9.2.0/../../../../avr/bin/ld: cannot find -lc
> /usr/lib/gcc/avr/9.2.0/../../../../avr/bin/ld: cannot find -latmega2560
> collect2: error: ld returned 1 exit status
> 
> I'll try on some Debian based host.

I believe the debian avr-libc package will have those, and should be pulled in
by gcc-avr.



r~

Re: [PATCH v2 0/4] target/avr: Few fixes

[Philippe Mathieu-Daudé]

On 7/7/20 9:00 AM, Philippe Mathieu-Daudé wrote:
> Since v1:
> - added missing 'Fix store instructions display order'
> 
> Few fixes on top of the AVR merger series Thomas sent yesterday:
> https://www.mail-archive.com/qemu-devel@nongnu.org/msg720089.html
> Based-on: <20200705140315.260514-1-huth@tuxfamily.org>
> 
> Philippe Mathieu-Daudé (4):
>   target/avr/cpu: Drop tlb_flush() in avr_cpu_reset()
>   target/avr/cpu: Fix $PC displayed address
>   target/avr/disas: Fix store instructions display order
>   target/avr/translate: Fix SBRC/SBRS instructions

Thanks for the reviews, I am queuing the patches 1-3 and will send
a pull request for the AVR port shortly.

Regards,

Phil.