[Qemu-devel] [PR RFC] RISC-V Patches for 3.1-rc2

[Qemu-devel] [PR RFC] RISC-V Patches for 3.1-rc2

[Palmer Dabbelt]

The following changes since commit cb968d275c145467c8b385a3618a207ec111eab1:

  Update version for v3.1.0-rc1 release (2018-11-13 18:16:14 +0000)

are available in the Git repository at:

  git://github.com/riscv/riscv-qemu.git tags/riscv-for-master-3.1-rc2

for you to fetch changes up to 3502dc824a7b0218abb49f4350e80a49829748cf:

  RISC-V: Respect fences for user-only emulators (2018-11-13 15:12:15 -0800)

----------------------------------------------------------------
RISC-V Patches for 3.1-rc2

This pull request contains four patches that aren't really related to
each other aside from all being bug fixes that I think should go in for
3.1.0:

* The second half of Alistair's memory leak patch set that I missed last
  week.
* A fix to make fclass.d availiable only on RV64IFD systems (without
  this it's availiable on RV32IFD systems, truncating the result).
* A fix to make sfence.vm availiable only in priv-1.9.1, and sfence.vma
  only availiable in priv-1.10.
* A change to respect fences in user-mode emulators, which were
  previously treated as NOPs.

As usual, this builds and boot Linux for me.  I don't think I have
anything else planned for 3.1.0, but I may be wrong as things are a bit
hectic this week.

----------------------------------------------------------------
Alistair Francis (1):
      hw/riscv/virt: Free the test device tree node name

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

Palmer Dabbelt (1):
      RISC-V: Respect fences for user-only emulators

 hw/riscv/virt.c          |  1 +
 target/riscv/translate.c | 24 ++++++++++++++++--------
 2 files changed, 17 insertions(+), 8 deletions(-)

[Qemu-devel] [PULL 1/4] hw/riscv/virt: Free the test device tree node name

[Palmer Dabbelt]

From: Alistair Francis <Alistair.Francis@wdc.com>

Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
Signed-off-by: Palmer Dabbelt <palmer@sifive.com>
---
 hw/riscv/virt.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
index 4a137a503c8a..2b38f890702c 100644
--- a/hw/riscv/virt.c
+++ b/hw/riscv/virt.c
@@ -240,6 +240,7 @@ static void *create_fdt(RISCVVirtState *s, const struct MemmapEntry *memmap,
     qemu_fdt_setprop_cells(fdt, nodename, "reg",
         0x0, memmap[VIRT_TEST].base,
         0x0, memmap[VIRT_TEST].size);
+    g_free(nodename);
 
     nodename = g_strdup_printf("/uart@%lx",
         (long)memmap[VIRT_UART0].base);
-- 
2.18.1

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

[Palmer Dabbelt]

From: Bastian Koppelmann <kbastian@mail.uni-paderborn.de>

Signed-off-by: Bastian Koppelmann <kbastian@mail.uni-paderborn.de>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Signed-off-by: Palmer Dabbelt <palmer@sifive.com>
---
 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 18d7b6d1471d..5359088e24bc 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.18.1

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

[Palmer Dabbelt]

From: Bastian Koppelmann <kbastian@mail.uni-paderborn.de>

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: Richard Henderson <richard.henderson@linaro.org>
Signed-off-by: Palmer Dabbelt <palmer@sifive.com>
---
 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 5359088e24bc..f44eb9c41b48 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.18.1

[Qemu-devel] [PULL 4/4] RISC-V: Respect fences for user-only emulators

[Palmer Dabbelt]

Our current fence implementation ignores fences for the user-only
configurations.  This is incorrect but unlikely to manifest: it requires
multi-threaded user-only code that takes advantage of the weakness in
the host's memory model and can be inlined by TCG.

This patch simply treats fences the same way for all our emulators.
I've given it to testing as I don't want to construct a test that would
actually trigger the failure.

Our fence implementation has an additional deficiency where we map all
RISC-V fences to full fences.  Now that we have a formal memory model
for RISC-V we can start to take advantage of the strength bits on our
fence instructions.  This requires a bit more though, so I'm going to
split it out because the implementation is still correct without taking
advantage of these weaker fences.

Thanks to Richard Henderson for pointing out both of the issues.

Signed-off-by: Palmer Dabbelt <palmer@sifive.com>
Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/riscv/translate.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/target/riscv/translate.c b/target/riscv/translate.c
index f44eb9c41b48..312bf298b3c2 100644
--- a/target/riscv/translate.c
+++ b/target/riscv/translate.c
@@ -1776,7 +1776,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 */
@@ -1787,7 +1786,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,
-- 
2.18.1

Re: [Qemu-devel] [PULL 4/4] RISC-V: Respect fences for user-only emulators

[Michael Clark]

Nits. Please improve your commit message to make them more impersonal...

On Wed, Nov 14, 2018 at 12:52 PM Palmer Dabbelt <palmer@sifive.com> wrote:

> Our current fence implementation ignores fences for the user-only
> configurations.  This is incorrect but unlikely to manifest: it requires
> multi-threaded user-only code that takes advantage of the weakness in
> the host's memory model and can be inlined by TCG.
>

The RISC-V fence implementation...


> This patch simply treats fences the same way for all our emulators.
> I've given it to testing as I don't want to construct a test that would
> actually trigger the failure.
>

Testing has been limited to... ? Reproducer?


> Our fence implementation has an additional deficiency where we map all
> RISC-V fences to full fences.  Now that we have a formal memory model
> for RISC-V we can start to take advantage of the strength bits on our
> fence instructions.  This requires a bit more though, so I'm going to
> split it out because the implementation is still correct without taking
> advantage of these weaker fences.
>

The fence implementation...

A formal memory model for RISC-V allows...


> Thanks to Richard Henderson for pointing out both of the issues.
>
> Signed-off-by: Palmer Dabbelt <palmer@sifive.com>
> Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


Assuming you fix up the commit message. I am not sure if I am present in
any of the commit messages I wrote, however perhaps thats just a matter
style with respect to writing (or re-writing) history.

Reviewed-by: Michael Clark <mjc@sifive.com>

---
>  target/riscv/translate.c | 2 --
>  1 file changed, 2 deletions(-)
>
> diff --git a/target/riscv/translate.c b/target/riscv/translate.c
> index f44eb9c41b48..312bf298b3c2 100644
> --- a/target/riscv/translate.c
> +++ b/target/riscv/translate.c
> @@ -1776,7 +1776,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 */
> @@ -1787,7 +1786,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,
> --
> 2.18.1
>
>
>

Re: [Qemu-devel] [PR RFC] RISC-V Patches for 3.1-rc2

[Michael Clark]

On Wed, Nov 14, 2018 at 12:52 PM Palmer Dabbelt <palmer@sifive.com> wrote:

> The following changes since commit
> cb968d275c145467c8b385a3618a207ec111eab1:
>
>   Update version for v3.1.0-rc1 release (2018-11-13 18:16:14 +0000)
>
> are available in the Git repository at:
>
>   git://github.com/riscv/riscv-qemu.git tags/riscv-for-master-3.1-rc2
>
> for you to fetch changes up to 3502dc824a7b0218abb49f4350e80a49829748cf:
>
>   RISC-V: Respect fences for user-only emulators (2018-11-13 15:12:15
> -0800)
>
> ----------------------------------------------------------------
> RISC-V Patches for 3.1-rc2
>
> This pull request contains four patches that aren't really related to
> each other aside from all being bug fixes that I think should go in for
> 3.1.0:
>

Here's I again. I guess its a somewhat arbitrary set of fixes. Although one
could say the same about the set of fixes in the github repo.

At least you don't have review feedback asking you split a patch up to make
it easier to review, after its already been reviewed (what I would call
"make work").

No objections from me. We just have to get these patches into the other
tree which also has RISC-V bug fixes (but I don't have an opinion about
which bugs are more or less critical). There will be some minor merge
conflicts because the RISC-V tree has writable misa suppoer. I'm planning
to rebase at 3.1.0, just so we don't trample on each other. It's probably
going to be quite a lot of work to get the patches in that tree here
because my patches tend to attract a lot of pedantic feedback. i.e. !! to
normalize a scalar into a bool and ~ to broadcast bit 1 e.g. C bitwise
logic that you find frequently in spike; has to be dumbed down into macro
wrappers. Feedback. These changes to keep people happy are the reason why
we broke things for folk like changing gp$ to __globalPointer$ (in
GCC/binutils). Suddendly all earlier asm is broken because someone wants to
give feedback about how RISC-V should be; someone who does not need to bear
the burden of those changes.

Change simply because we can make you change stuff to break things for
RISC-V users because we think those changes are a good idea. Don't get me
wrong. We get good feedback (in QEMU, mostly from RIchard Henderson), but
its not always the case. VSPILL/VFILL and vaddsz sp is a good idea. I need
to find that email of Richard's on the GCC list and forward it to Krste. I
hope the feedback makes it to folk working on the Vector spec because as
far as I can tell it is a good idea.

Apologies for the cynicism. I'll get over it.

* The second half of Alistair's memory leak patch set that I missed last
>   week.
> * A fix to make fclass.d availiable only on RV64IFD systems (without
>   this it's availiable on RV32IFD systems, truncating the result).
> * A fix to make sfence.vm availiable only in priv-1.9.1, and sfence.vma
>   only availiable in priv-1.10.
> * A change to respect fences in user-mode emulators, which were
>   previously treated as NOPs.
>
> As usual, this builds and boot Linux for me.  I don't think I have
> anything else planned for 3.1.0, but I may be wrong as things are a bit
> hectic this week.
>
> ----------------------------------------------------------------
> Alistair Francis (1):
>       hw/riscv/virt: Free the test device tree node name
>
> 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
>
> Palmer Dabbelt (1):
>       RISC-V: Respect fences for user-only emulators
>
>  hw/riscv/virt.c          |  1 +
>  target/riscv/translate.c | 24 ++++++++++++++++--------
>  2 files changed, 17 insertions(+), 8 deletions(-)
>
>
>