Skip to contentSkip to conversation

RISC-V: Respect fences for user-only emulators

PATCH New Palmer Dabbelt 3 participants 4 messages Nov 9, 2018 – Nov 10, 2018 RISC-V

lore
Share & tools
lore
#0 lore raw

PATCH 1/1 RISC-V: Respect fences for user-only emulators

I'd like to target this for 3.1: while it's a pretty esoteric bug the risk of breaking anything is low and tracking down a manifestation of the bug would be a nightmare.

I don't think the performance improvement alluded to will be a 3.1 candidiate, and as a result I probably won't get around to it until later.

#1 lore raw

PATCH 1/1 RISC-V: Respect fences for user-only emulators

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.

---
 target/riscv/translate.c | 2 --
 1 file changed, 2 deletions(-)
target/riscv/translate.c +0 -2
index 18d7b6d1471d..624d1c679a84 100644
@@ -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,
Signature · 1 line
2.18.1
Reviewed-by on v1 1/1
#2 lore raw
Quoted 6 lines · 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.

Quoted 8 lines

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

s/though/thought/g

Quoted 6 lines

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.

Alistair

Quoted 28 lines
---
 target/riscv/translate.c | 2 --
 1 file changed, 2 deletions(-)
target/riscv/translate.c +0 -3
index 18d7b6d1471d..624d1c679a84 100644
@@ -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,
--

2.18.1

Reviewed-by on v1 1/1
#3 lore raw
In reply to Palmer Dabbelt
Quoted 16 lines · 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.

Quoted 6 lines

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

---
 target/riscv/translate.c | 2 --
 1 file changed, 2 deletions(-)

r~