[Qemu-devel] [PATCH 0/2] target/riscv: Bugfixes found in decodetree conversion

[Qemu-devel] [PATCH 0/2] target/riscv: Bugfixes found in decodetree conversion

[Bastian Koppelmann]

Hi,

while going through the reviews of the riscv-decodetree patches, two bugs came
up that I fix here. There is one more problem [1] mentioned by Richard but 
I don't have the time to investigate it further.

[1] https://patchwork.kernel.org/patch/10650293/

Bastian Koppelmann (2):
  target/riscv: Fix FCLASS_D being treated as RV64 only
  target/riscv: Fix sfence.vm/a both available in any priv version

 target/riscv/translate.c | 22 ++++++++++++++++------
 1 file changed, 16 insertions(+), 6 deletions(-)

-- 
2.19.1

[Qemu-devel] [PATCH 1/2] target/riscv: Fix FCLASS_D being treated as RV64 only

[Bastian Koppelmann]

Signed-off-by: Bastian Koppelmann <kbastian@mail.uni-paderborn.de>
---
 target/riscv/translate.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/target/riscv/translate.c b/target/riscv/translate.c
index 18d7b6d147..5359088e24 100644
--- a/target/riscv/translate.c
+++ b/target/riscv/translate.c
@@ -1237,13 +1237,14 @@ static void gen_fp_arith(DisasContext *ctx, uint32_t opc, int rd,
         tcg_temp_free(t0);
         break;
 
-#if defined(TARGET_RISCV64)
     case OPC_RISC_FMV_X_D:
         /* also OPC_RISC_FCLASS_D */
         switch (rm) {
+#if defined(TARGET_RISCV64)
         case 0: /* FMV */
             gen_set_gpr(rd, cpu_fpr[rs1]);
             break;
+#endif
         case 1:
             t0 = tcg_temp_new();
             gen_helper_fclass_d(t0, cpu_fpr[rs1]);
@@ -1255,6 +1256,7 @@ static void gen_fp_arith(DisasContext *ctx, uint32_t opc, int rd,
         }
         break;
 
+#if defined(TARGET_RISCV64)
     case OPC_RISC_FMV_D_X:
         t0 = tcg_temp_new();
         gen_get_gpr(t0, rs1);
-- 
2.19.1

Re: [Qemu-devel] [PATCH 1/2] target/riscv: Fix FCLASS_D being treated as RV64 only

[Richard Henderson]

On 11/8/18 1:06 PM, Bastian Koppelmann wrote:
> Signed-off-by: Bastian Koppelmann <kbastian@mail.uni-paderborn.de>
> ---
>  target/riscv/translate.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)

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


r~

Re: [Qemu-devel] [PATCH 1/2] target/riscv: Fix FCLASS_D being treated as RV64 only

[Alistair Francis]

On Thu, Nov 8, 2018 at 4:07 AM Bastian Koppelmann
<kbastian@mail.uni-paderborn.de> wrote:
>
> Signed-off-by: Bastian Koppelmann <kbastian@mail.uni-paderborn.de>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  target/riscv/translate.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/target/riscv/translate.c b/target/riscv/translate.c
> index 18d7b6d147..5359088e24 100644
> --- a/target/riscv/translate.c
> +++ b/target/riscv/translate.c
> @@ -1237,13 +1237,14 @@ static void gen_fp_arith(DisasContext *ctx, uint32_t opc, int rd,
>          tcg_temp_free(t0);
>          break;
>
> -#if defined(TARGET_RISCV64)
>      case OPC_RISC_FMV_X_D:
>          /* also OPC_RISC_FCLASS_D */
>          switch (rm) {
> +#if defined(TARGET_RISCV64)
>          case 0: /* FMV */
>              gen_set_gpr(rd, cpu_fpr[rs1]);
>              break;
> +#endif
>          case 1:
>              t0 = tcg_temp_new();
>              gen_helper_fclass_d(t0, cpu_fpr[rs1]);
> @@ -1255,6 +1256,7 @@ static void gen_fp_arith(DisasContext *ctx, uint32_t opc, int rd,
>          }
>          break;
>
> +#if defined(TARGET_RISCV64)
>      case OPC_RISC_FMV_D_X:
>          t0 = tcg_temp_new();
>          gen_get_gpr(t0, rs1);
> --
> 2.19.1
>
>

[Qemu-devel] [PATCH 2/2] target/riscv: Fix sfence.vm/a both available in any priv version

[Bastian Koppelmann]

sfence.vm has been replaced in priv v1.10 spec by sfence.vma.

Reported-by: Richard Henderson <richard.henderson@linaro.org>
Signed-off-by: Bastian Koppelmann <kbastian@mail.uni-paderborn.de>
---
 target/riscv/translate.c | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/target/riscv/translate.c b/target/riscv/translate.c
index 5359088e24..f44eb9c41b 100644
--- a/target/riscv/translate.c
+++ b/target/riscv/translate.c
@@ -1292,10 +1292,14 @@ static void gen_system(CPURISCVState *env, DisasContext *ctx, uint32_t opc,
 #ifndef CONFIG_USER_ONLY
     /* Extract funct7 value and check whether it matches SFENCE.VMA */
     if ((opc == OPC_RISC_ECALL) && ((csr >> 5) == 9)) {
-        /* sfence.vma */
-        /* TODO: handle ASID specific fences */
-        gen_helper_tlb_flush(cpu_env);
-        return;
+        if (env->priv_ver == PRIV_VERSION_1_10_0) {
+            /* sfence.vma */
+            /* TODO: handle ASID specific fences */
+            gen_helper_tlb_flush(cpu_env);
+            return;
+        } else {
+            gen_exception_illegal(ctx);
+        }
     }
 #endif
 
@@ -1342,7 +1346,11 @@ static void gen_system(CPURISCVState *env, DisasContext *ctx, uint32_t opc,
             gen_helper_wfi(cpu_env);
             break;
         case 0x104: /* SFENCE.VM */
-            gen_helper_tlb_flush(cpu_env);
+            if (env->priv_ver <= PRIV_VERSION_1_09_1) {
+                gen_helper_tlb_flush(cpu_env);
+            } else {
+                gen_exception_illegal(ctx);
+            }
             break;
 #endif
         default:
-- 
2.19.1

Re: [Qemu-devel] [PATCH 2/2] target/riscv: Fix sfence.vm/a both available in any priv version

[Richard Henderson]

On 11/8/18 1:06 PM, Bastian Koppelmann wrote:
> sfence.vm has been replaced in priv v1.10 spec by sfence.vma.
> 
> Reported-by: Richard Henderson <richard.henderson@linaro.org>
> Signed-off-by: Bastian Koppelmann <kbastian@mail.uni-paderborn.de>
> ---
>  target/riscv/translate.c | 18 +++++++++++++-----
>  1 file changed, 13 insertions(+), 5 deletions(-)

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


r~

Re: [Qemu-devel] [PATCH 2/2] target/riscv: Fix sfence.vm/a both available in any priv version

[Alistair Francis]

On Thu, Nov 8, 2018 at 4:07 AM Bastian Koppelmann
<kbastian@mail.uni-paderborn.de> wrote:
>
> sfence.vm has been replaced in priv v1.10 spec by sfence.vma.
>
> Reported-by: Richard Henderson <richard.henderson@linaro.org>
> Signed-off-by: Bastian Koppelmann <kbastian@mail.uni-paderborn.de>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  target/riscv/translate.c | 18 +++++++++++++-----
>  1 file changed, 13 insertions(+), 5 deletions(-)
>
> diff --git a/target/riscv/translate.c b/target/riscv/translate.c
> index 5359088e24..f44eb9c41b 100644
> --- a/target/riscv/translate.c
> +++ b/target/riscv/translate.c
> @@ -1292,10 +1292,14 @@ static void gen_system(CPURISCVState *env, DisasContext *ctx, uint32_t opc,
>  #ifndef CONFIG_USER_ONLY
>      /* Extract funct7 value and check whether it matches SFENCE.VMA */
>      if ((opc == OPC_RISC_ECALL) && ((csr >> 5) == 9)) {
> -        /* sfence.vma */
> -        /* TODO: handle ASID specific fences */
> -        gen_helper_tlb_flush(cpu_env);
> -        return;
> +        if (env->priv_ver == PRIV_VERSION_1_10_0) {
> +            /* sfence.vma */
> +            /* TODO: handle ASID specific fences */
> +            gen_helper_tlb_flush(cpu_env);
> +            return;
> +        } else {
> +            gen_exception_illegal(ctx);
> +        }
>      }
>  #endif
>
> @@ -1342,7 +1346,11 @@ static void gen_system(CPURISCVState *env, DisasContext *ctx, uint32_t opc,
>              gen_helper_wfi(cpu_env);
>              break;
>          case 0x104: /* SFENCE.VM */
> -            gen_helper_tlb_flush(cpu_env);
> +            if (env->priv_ver <= PRIV_VERSION_1_09_1) {
> +                gen_helper_tlb_flush(cpu_env);
> +            } else {
> +                gen_exception_illegal(ctx);
> +            }
>              break;
>  #endif
>          default:
> --
> 2.19.1
>
>

Re: [Qemu-devel] [PATCH 0/2] target/riscv: Bugfixes found in decodetree conversion

[Richard Henderson]

On 11/8/18 1:06 PM, Bastian Koppelmann wrote:
> while going through the reviews of the riscv-decodetree patches, two bugs came
> up that I fix here. There is one more problem [1] mentioned by Richard but 
> I don't have the time to investigate it further.
> 
> [1] https://patchwork.kernel.org/patch/10650293/

That one's not a bug, but an optimization.

The other bug mentioned is shrw and shaw not sign-extending the result.


r~

Re: [Qemu-devel] [PATCH 0/2] target/riscv: Bugfixes found in decodetree conversion

[Bastian Koppelmann]

On 11/8/18 4:53 PM, Richard Henderson wrote:
> On 11/8/18 1:06 PM, Bastian Koppelmann wrote:
>> while going through the reviews of the riscv-decodetree patches, two bugs came
>> up that I fix here. There is one more problem [1] mentioned by Richard but
>> I don't have the time to investigate it further.
>>
>> [1] https://patchwork.kernel.org/patch/10650293/
> That one's not a bug, but an optimization.
>
> The other bug mentioned is shrw and shaw not sign-extending the result.


That was a bug I introduced during the conversion to decodetree.


Cheers,

Bastian

Re: [Qemu-devel] [PATCH 0/2] target/riscv: Bugfixes found in decodetree conversion

[Palmer Dabbelt]

On Thu, 08 Nov 2018 09:29:26 PST (-0800), Bastian Koppelmann wrote:
>
> On 11/8/18 4:53 PM, Richard Henderson wrote:
>> On 11/8/18 1:06 PM, Bastian Koppelmann wrote:
>>> while going through the reviews of the riscv-decodetree patches, two bugs came
>>> up that I fix here. There is one more problem [1] mentioned by Richard but
>>> I don't have the time to investigate it further.
>>>
>>> [1] https://patchwork.kernel.org/patch/10650293/
>> That one's not a bug, but an optimization.
>>
>> The other bug mentioned is shrw and shaw not sign-extending the result.
>
>
> That was a bug I introduced during the conversion to decodetree.

My understand of that patch feedback is that there are two issues:

* We don't take advantage of the ordering bits on fences, which could allow us 
  to emit less conservative fences.  This would presumably increase 
  performance.
* We treat fences as NOPs under "#ifndef CONFIG_USER_ONLY", which is a bug.

Am I wrong about that second one?  I think the fix should look something like 
this, which I haven't even tried to compile

diff --git a/target/riscv/translate.c b/target/riscv/translate.c
index 18d7b6d1471d..624d1c679a84 100644
--- a/target/riscv/translate.c
+++ b/target/riscv/translate.c
@@ -1766,7 +1766,6 @@ static void decode_RV32_64G(CPURISCVState *env, DisasContext *ctx)
                      GET_RM(ctx->opcode));
         break;
     case OPC_RISC_FENCE:
-#ifndef CONFIG_USER_ONLY
         if (ctx->opcode & 0x1000) {
             /* FENCE_I is a no-op in QEMU,
              * however we need to end the translation block */
@@ -1777,7 +1776,6 @@ static void decode_RV32_64G(CPURISCVState *env, DisasContext *ctx)
             /* FENCE is a full memory barrier. */
             tcg_gen_mb(TCG_MO_ALL | TCG_BAR_SC);
         }
-#endif
         break;
     case OPC_RISC_SYSTEM:
         gen_system(env, ctx, MASK_OP_SYSTEM(ctx->opcode), rd, rs1,

Re: [Qemu-devel] [PATCH 0/2] target/riscv: Bugfixes found in decodetree conversion

[Richard Henderson]

On 11/8/18 8:12 PM, Palmer Dabbelt wrote:
> * We don't take advantage of the ordering bits on fences, which could allow us
>  to emit less conservative fences.  This would presumably increase  performance.

Yes.

> * We treat fences as NOPs under "#ifndef CONFIG_USER_ONLY", which is a bug.

Ah yes, I'd forgotten this one.  This is a bug, in that multi-threaded user
programs do not get the memory ordering that they requested.

Hard to see, obviously, since the emulator has its own memory operations,
barriers, and locks.  But once we have a lot of the hot path of the user
program compiled, there's a lot less of that.


>     case OPC_RISC_FENCE:
> -#ifndef CONFIG_USER_ONLY
>         if (ctx->opcode & 0x1000) {
>             /* FENCE_I is a no-op in QEMU,
>              * however we need to end the translation block */
> @@ -1777,7 +1776,6 @@ static void decode_RV32_64G(CPURISCVState *env,
> DisasContext *ctx)
>             /* FENCE is a full memory barrier. */
>             tcg_gen_mb(TCG_MO_ALL | TCG_BAR_SC);
>         }
> -#endif
>         break;

Yes, this is minimally correct.


r~