From Unpriviledged ISA manual:
"For LR and SC, the Zalrsc extension requires that the address held in rs1 be
naturally aligned to the size of the operand (i.e., eight-byte aligned for
doublewords and four-byte aligned for words). If the address is not naturally
aligned, an address-misaligned exception or an access-fault exception will be
generated. The access-fault exception can be generated for a memory access that
would otherwise be able to complete except for the misalignment, if the
misaligned access should not be emulated."
Here nothing checks that the address is naturally aligned, so this fixes that
wrong behavior by raising address-misaligned exception if the address in rs1
is not naturally aligned.
Signed-off-by: Frederic Konrad <fkonrad@amd.com>
---
target/riscv/insn_trans/trans_rva.c.inc | 19 +++++++++++++++++++
1 file changed, 19 insertions(+)
diff --git a/target/riscv/insn_trans/trans_rva.c.inc b/target/riscv/insn_trans/trans_rva.c.inc
index 9cf3ae8019..30a047164c 100644
--- a/target/riscv/insn_trans/trans_rva.c.inc
+++ b/target/riscv/insn_trans/trans_rva.c.inc
@@ -58,11 +58,30 @@ static bool gen_lr(DisasContext *ctx, arg_atomic *a, MemOp mop)
static bool gen_sc(DisasContext *ctx, arg_atomic *a, MemOp mop)
{
TCGv dest, src1, src2;
+ TCGv tmp = tcg_temp_new();
+ TCGLabel *l3 = gen_new_label();
TCGLabel *l1 = gen_new_label();
TCGLabel *l2 = gen_new_label();
decode_save_opc(ctx, 0);
src1 = get_address(ctx, a->rs1, 0);
+ /*
+ * A misaligned store trap should be triggered even if the store should
+ * fail due to the reservation.
+ */
+ tcg_gen_andi_tl(tmp, src1, ~((uint64_t)0) << memop_alignment_bits(mop));
+ tcg_gen_brcond_tl(TCG_COND_EQ, tmp, src1, l3);
+
+ /*
+ * Store the faulty address, and the actual PC. Then generate the
+ * exception.
+ */
+ tcg_gen_st_tl(src1, tcg_env, offsetof(CPURISCVState, badaddr));
+ gen_pc_plus_diff(cpu_pc, ctx, 0);
+ gen_helper_raise_exception(tcg_env,
+ tcg_constant_i32(RISCV_EXCP_STORE_AMO_ADDR_MIS));
+
+ gen_set_label(l3);
tcg_gen_brcond_tl(TCG_COND_NE, load_res, src1, l1);
/*
--
2.43.5On 12/11/24 15:19, Frederic Konrad wrote:
> + /*
> + * A misaligned store trap should be triggered even if the store should
> + * fail due to the reservation.
> + */
> + tcg_gen_andi_tl(tmp, src1, ~((uint64_t)0) << memop_alignment_bits(mop));
The constant is incorrect for testing the low bits.
> + tcg_gen_brcond_tl(TCG_COND_EQ, tmp, src1, l3);
Best to make the fallthrough path be the common case, as we will optimize across the
extended basic block.
Use test-style comparison:
tcg_gen_brcondi_tl(TCG_COND_TSTNE, src1, memop_size(mop) - 1, l_misalign);
r~
+CC maintainers
> -----Original Message-----
> From: qemu-devel-bounces+fkonrad=amd.com@nongnu.org <qemu-devel-bounces+fkonrad=amd.com@nongnu.org> On
> Behalf Of Richard Henderson
> Sent: 11 December 2024 22:43
> To: qemu-devel@nongnu.org
> Subject: Re: [PATCH 3/3] target/riscv: fix the trap generation for conditional store
>
> On 12/11/24 15:19, Frederic Konrad wrote:
> > + /*
> > + * A misaligned store trap should be triggered even if the store should
> > + * fail due to the reservation.
> > + */
> > + tcg_gen_andi_tl(tmp, src1, ~((uint64_t)0) << memop_alignment_bits(mop));
>
> The constant is incorrect for testing the low bits.
Hmm, I don't get it, basically with that I'm trying to do:
MO_8: src1 == (src1 & 0xFFFFFFFF)
MO_16: src1 == (src1 & 0xFFFFFFFE)
MO_32: src1 == (src1 & 0xFFFFFFFC)
etc
what am I missing?
>
> > + tcg_gen_brcond_tl(TCG_COND_EQ, tmp, src1, l3);
>
> Best to make the fallthrough path be the common case, as we will optimize across the
> extended basic block.
>
> Use test-style comparison:
>
> tcg_gen_brcondi_tl(TCG_COND_TSTNE, src1, memop_size(mop) - 1, l_misalign);
In any case that one looks better thanks for the tips!
About making the fallthrough path be the common case, If I do it I'll need to jump anyway and the
end of this instruction ie:
if not aligned go to misaligned label:
...
do the normal operation
...
go to done label
misaligned label
...
trigger the exception
...
done label
Is that what you had in mind?
Thanks,
Fred
>
>
>
> r~
On 12/13/24 10:31, Konrad, Frederic wrote:
> +CC maintainers
>
>> -----Original Message-----
>> From: qemu-devel-bounces+fkonrad=amd.com@nongnu.org <qemu-devel-bounces+fkonrad=amd.com@nongnu.org> On
>> Behalf Of Richard Henderson
>> Sent: 11 December 2024 22:43
>> To: qemu-devel@nongnu.org
>> Subject: Re: [PATCH 3/3] target/riscv: fix the trap generation for conditional store
>>
>> On 12/11/24 15:19, Frederic Konrad wrote:
>>> + /*
>>> + * A misaligned store trap should be triggered even if the store should
>>> + * fail due to the reservation.
>>> + */
>>> + tcg_gen_andi_tl(tmp, src1, ~((uint64_t)0) << memop_alignment_bits(mop));
>>
>> The constant is incorrect for testing the low bits.
>
> Hmm, I don't get it, basically with that I'm trying to do:
> MO_8: src1 == (src1 & 0xFFFFFFFF)
> MO_16: src1 == (src1 & 0xFFFFFFFE)
> MO_32: src1 == (src1 & 0xFFFFFFFC)
> etc
>
> what am I missing?
>
>>
>>> + tcg_gen_brcond_tl(TCG_COND_EQ, tmp, src1, l3);
Ah, I missed the form of the equality. I had been expecting
tmp = src1 & 1
brcond tmp != 0
and so mis-read the mask.
> About making the fallthrough path be the common case, If I do it I'll need to jump anyway and the
> end of this instruction ie:
>
> if not aligned go to misaligned label:
> ...
> do the normal operation
> ...
> go to done label
> misaligned label
> ...
> trigger the exception
> ...
> done label
>
> Is that what you had in mind?
There is another code block in there. We can sort as
if not aligned goto misaligned;
if reservation mismatch goto mismatch
normal operation
goto done
misaligned:
raise exception
mismatch:
mb, set failure
done:
clear reservation
so that we save adding a branch on the mismatch path.
r~
© 2016 - 2025 Red Hat, Inc.