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
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~
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
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,
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~
© 2016 - 2026 Red Hat, Inc.