[Qemu-devel] [PATCH v3 33/50] target/riscv: fetch code with translator_ld

Alex Bennée posted 50 patches 6 years, 8 months ago
[Qemu-devel] [PATCH v3 33/50] target/riscv: fetch code with translator_ld
Posted by Alex Bennée 6 years, 8 months ago
From: "Emilio G. Cota" <cota@braap.org>

Signed-off-by: Emilio G. Cota <cota@braap.org>
---
 target/riscv/translate.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/riscv/translate.c b/target/riscv/translate.c
index 313c27b700..899abf41fa 100644
--- a/target/riscv/translate.c
+++ b/target/riscv/translate.c
@@ -793,7 +793,7 @@ static void riscv_tr_translate_insn(DisasContextBase *dcbase, CPUState *cpu)
     DisasContext *ctx = container_of(dcbase, DisasContext, base);
     CPURISCVState *env = cpu->env_ptr;
 
-    ctx->opcode = cpu_ldl_code(env, ctx->base.pc_next);
+    ctx->opcode = translator_ldl(env, ctx->base.pc_next);
     decode_opc(ctx);
     ctx->base.pc_next = ctx->pc_succ_insn;
 
-- 
2.20.1


Re: [Qemu-devel] [PATCH v3 33/50] target/riscv: fetch code with translator_ld
Posted by Richard Henderson 6 years, 7 months ago
On 6/14/19 10:11 AM, Alex Bennée wrote:
> +++ b/target/riscv/translate.c
> @@ -793,7 +793,7 @@ static void riscv_tr_translate_insn(DisasContextBase *dcbase, CPUState *cpu)
>      DisasContext *ctx = container_of(dcbase, DisasContext, base);
>      CPURISCVState *env = cpu->env_ptr;
>  
> -    ctx->opcode = cpu_ldl_code(env, ctx->base.pc_next);
> +    ctx->opcode = translator_ldl(env, ctx->base.pc_next);

I'll note for the riscv folks that this is an existing bug, reading too much in
the case of an RVC instruction.  This could well matter for the last 2-byte
instruction at the end of a page.

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


r~

Re: [Qemu-devel] [PATCH v3 33/50] target/riscv: fetch code with translator_ld
Posted by Palmer Dabbelt 6 years, 7 months ago
On Mon, 17 Jun 2019 15:38:45 PDT (-0700), richard.henderson@linaro.org wrote:
> On 6/14/19 10:11 AM, Alex Bennée wrote:
>> +++ b/target/riscv/translate.c
>> @@ -793,7 +793,7 @@ static void riscv_tr_translate_insn(DisasContextBase *dcbase, CPUState *cpu)
>>      DisasContext *ctx = container_of(dcbase, DisasContext, base);
>>      CPURISCVState *env = cpu->env_ptr;
>>
>> -    ctx->opcode = cpu_ldl_code(env, ctx->base.pc_next);
>> +    ctx->opcode = translator_ldl(env, ctx->base.pc_next);
>
> I'll note for the riscv folks that this is an existing bug, reading too much in
> the case of an RVC instruction.  This could well matter for the last 2-byte
> instruction at the end of a page.
>
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

Thanks for pointing this out.  I'm checking the ISA semantics with Andrew to
make sure I've got it right, as there's some implicit wording in the document
that doesn't quite do what I'd expect it to.

Re: [Qemu-devel] [PATCH v3 33/50] target/riscv: fetch code with translator_ld
Posted by Alistair Francis 6 years, 4 months ago
On Wed, Jun 19, 2019 at 3:50 AM Palmer Dabbelt <palmer@sifive.com> wrote:
>
> On Mon, 17 Jun 2019 15:38:45 PDT (-0700), richard.henderson@linaro.org wrote:
> > On 6/14/19 10:11 AM, Alex Bennée wrote:
> >> +++ b/target/riscv/translate.c
> >> @@ -793,7 +793,7 @@ static void riscv_tr_translate_insn(DisasContextBase *dcbase, CPUState *cpu)
> >>      DisasContext *ctx = container_of(dcbase, DisasContext, base);
> >>      CPURISCVState *env = cpu->env_ptr;
> >>
> >> -    ctx->opcode = cpu_ldl_code(env, ctx->base.pc_next);
> >> +    ctx->opcode = translator_ldl(env, ctx->base.pc_next);
> >
> > I'll note for the riscv folks that this is an existing bug, reading too much in
> > the case of an RVC instruction.  This could well matter for the last 2-byte
> > instruction at the end of a page.
> >
> > Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
>
> Thanks for pointing this out.  I'm checking the ISA semantics with Andrew to
> make sure I've got it right, as there's some implicit wording in the document
> that doesn't quite do what I'd expect it to.

Did we figure out what to do here?

Alistair

>