From: Alistair Francis <alistair.francis@wdc.com>
The RISC-V spec states:
"For the purposes of memory protection, a failed SC.W may be treated
like a store."
So if the comparison in sc.w fails we should still check for alignment
and do a probe access to check permissions.
Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/3323
Resoves: https://gitlab.com/qemu-project/qemu/-/work_items/3136
Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
---
target/riscv/helper.h | 3 +++
target/riscv/op_helper.c | 14 ++++++++++++++
target/riscv/insn_trans/trans_rva.c.inc | 6 ++++++
3 files changed, 23 insertions(+)
diff --git a/target/riscv/helper.h b/target/riscv/helper.h
index b785456ee0..af6cfcfc27 100644
--- a/target/riscv/helper.h
+++ b/target/riscv/helper.h
@@ -1289,3 +1289,6 @@ DEF_HELPER_4(vsm4r_vs, void, ptr, ptr, env, i32)
#ifndef CONFIG_USER_ONLY
DEF_HELPER_1(ssamoswap_disabled, void, env)
#endif
+
+/* Zalrsc" SC write probe */
+DEF_HELPER_FLAGS_3(sc_probe_write, TCG_CALL_NO_WG, void, env, tl, tl)
diff --git a/target/riscv/op_helper.c b/target/riscv/op_helper.c
index 6ccc127c30..b569366369 100644
--- a/target/riscv/op_helper.c
+++ b/target/riscv/op_helper.c
@@ -281,6 +281,20 @@ void helper_cbo_inval(CPURISCVState *env, target_ulong address)
/* We don't emulate the cache-hierarchy, so we're done. */
}
+void helper_sc_probe_write(CPURISCVState *env, target_ulong addr,
+ target_ulong size)
+{
+ uintptr_t ra = GETPC();
+ int mmu_idx = riscv_env_mmu_index(env, false);
+
+ if (addr & (size - 1)) {
+ env->badaddr = addr;
+ riscv_raise_exception(env, RISCV_EXCP_STORE_AMO_ADDR_MIS, ra);
+ }
+
+ probe_write(env, addr, size, mmu_idx, ra);
+}
+
#ifndef CONFIG_USER_ONLY
target_ulong helper_sret(CPURISCVState *env)
diff --git a/target/riscv/insn_trans/trans_rva.c.inc b/target/riscv/insn_trans/trans_rva.c.inc
index a7a3278d24..62c0fe673d 100644
--- a/target/riscv/insn_trans/trans_rva.c.inc
+++ b/target/riscv/insn_trans/trans_rva.c.inc
@@ -90,6 +90,12 @@ static bool gen_sc(DisasContext *ctx, arg_atomic *a, MemOp mop)
*/
TCGBar bar_strl = (ctx->ztso || a->rl) ? TCG_BAR_STRL : 0;
tcg_gen_mb(TCG_MO_ALL + a->aq * TCG_BAR_LDAQ + bar_strl);
+ /*
+ * "For the purposes of memory protection, a failed SC.W may be treated
+ * like a store." so let's check the write access permissions
+ */
+ gen_helper_sc_probe_write(tcg_env, src1,
+ tcg_constant_tl(memop_size(mop)));
gen_set_gpr(ctx, a->rd, tcg_constant_tl(1));
gen_set_label(l2);
--
2.53.0
On 4/7/2026 1:36 AM, alistair23@gmail.com wrote:
> From: Alistair Francis <alistair.francis@wdc.com>
>
> The RISC-V spec states:
>
> "For the purposes of memory protection, a failed SC.W may be treated
> like a store."
>
> So if the comparison in sc.w fails we should still check for alignment
> and do a probe access to check permissions.
>
> Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/3323
> Resoves: https://gitlab.com/qemu-project/qemu/-/work_items/3136
Typo: "Resoves"
Also, IIRC the bug URL that automatically closes Gitlab issues when
merging the patch is on the format:
"Resolves: https://gitlab.com/qemu-project/qemu/-/issues/(number)"
Maybe this URL with "work_items" also works, but even with the regular
URL Gitlab fails to autoclose the bug sometimes. I suggest changing the
"work_items" to "issues" in all those URLs to be safe.
That said,
Reviewed-by: Daniel Henrique Barboza <daniel.barboza@oss.qualcomm.com>
> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> ---
> target/riscv/helper.h | 3 +++
> target/riscv/op_helper.c | 14 ++++++++++++++
> target/riscv/insn_trans/trans_rva.c.inc | 6 ++++++
> 3 files changed, 23 insertions(+)
>
> diff --git a/target/riscv/helper.h b/target/riscv/helper.h
> index b785456ee0..af6cfcfc27 100644
> --- a/target/riscv/helper.h
> +++ b/target/riscv/helper.h
> @@ -1289,3 +1289,6 @@ DEF_HELPER_4(vsm4r_vs, void, ptr, ptr, env, i32)
> #ifndef CONFIG_USER_ONLY
> DEF_HELPER_1(ssamoswap_disabled, void, env)
> #endif
> +
> +/* Zalrsc" SC write probe */
> +DEF_HELPER_FLAGS_3(sc_probe_write, TCG_CALL_NO_WG, void, env, tl, tl)
> diff --git a/target/riscv/op_helper.c b/target/riscv/op_helper.c
> index 6ccc127c30..b569366369 100644
> --- a/target/riscv/op_helper.c
> +++ b/target/riscv/op_helper.c
> @@ -281,6 +281,20 @@ void helper_cbo_inval(CPURISCVState *env, target_ulong address)
> /* We don't emulate the cache-hierarchy, so we're done. */
> }
>
> +void helper_sc_probe_write(CPURISCVState *env, target_ulong addr,
> + target_ulong size)
> +{
> + uintptr_t ra = GETPC();
> + int mmu_idx = riscv_env_mmu_index(env, false);
> +
> + if (addr & (size - 1)) {
> + env->badaddr = addr;
> + riscv_raise_exception(env, RISCV_EXCP_STORE_AMO_ADDR_MIS, ra);
> + }
> +
> + probe_write(env, addr, size, mmu_idx, ra);
> +}
> +
> #ifndef CONFIG_USER_ONLY
>
> target_ulong helper_sret(CPURISCVState *env)
> diff --git a/target/riscv/insn_trans/trans_rva.c.inc b/target/riscv/insn_trans/trans_rva.c.inc
> index a7a3278d24..62c0fe673d 100644
> --- a/target/riscv/insn_trans/trans_rva.c.inc
> +++ b/target/riscv/insn_trans/trans_rva.c.inc
> @@ -90,6 +90,12 @@ static bool gen_sc(DisasContext *ctx, arg_atomic *a, MemOp mop)
> */
> TCGBar bar_strl = (ctx->ztso || a->rl) ? TCG_BAR_STRL : 0;
> tcg_gen_mb(TCG_MO_ALL + a->aq * TCG_BAR_LDAQ + bar_strl);
> + /*
> + * "For the purposes of memory protection, a failed SC.W may be treated
> + * like a store." so let's check the write access permissions
> + */
> + gen_helper_sc_probe_write(tcg_env, src1,
> + tcg_constant_tl(memop_size(mop)));
> gen_set_gpr(ctx, a->rd, tcg_constant_tl(1));
>
> gen_set_label(l2);
On Tue, Apr 7, 2026 at 7:38 PM Daniel Henrique Barboza
<daniel.barboza@oss.qualcomm.com> wrote:
>
>
>
> On 4/7/2026 1:36 AM, alistair23@gmail.com wrote:
> > From: Alistair Francis <alistair.francis@wdc.com>
> >
> > The RISC-V spec states:
> >
> > "For the purposes of memory protection, a failed SC.W may be treated
> > like a store."
> >
> > So if the comparison in sc.w fails we should still check for alignment
> > and do a probe access to check permissions.
> >
> > Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/3323
> > Resoves: https://gitlab.com/qemu-project/qemu/-/work_items/3136
>
> Typo: "Resoves"
>
>
> Also, IIRC the bug URL that automatically closes Gitlab issues when
> merging the patch is on the format:
>
> "Resolves: https://gitlab.com/qemu-project/qemu/-/issues/(number)"
Interestingly trying to access
https://gitlab.com/qemu-project/qemu/-/issues/3196
takes me to
https://gitlab.com/qemu-project/qemu/-/work_items/3196
So I think Gitlab is changing from issues to work_items
>
>
> Maybe this URL with "work_items" also works, but even with the regular
> URL Gitlab fails to autoclose the bug sometimes. I suggest changing the
> "work_items" to "issues" in all those URLs to be safe.
Maybe using "issues" is the problem?
I'm thinking of leaving these as-is, as issues seem to be replaced now.
I even see https://docs.gitlab.com/development/work_items/#migration-strategy
and https://gitlab.com/groups/gitlab-org/-/work_items/6033 with more
details on this
Alistair
>
>
> That said,
>
>
> Reviewed-by: Daniel Henrique Barboza <daniel.barboza@oss.qualcomm.com>
>
> > Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> > ---
> > target/riscv/helper.h | 3 +++
> > target/riscv/op_helper.c | 14 ++++++++++++++
> > target/riscv/insn_trans/trans_rva.c.inc | 6 ++++++
> > 3 files changed, 23 insertions(+)
> >
> > diff --git a/target/riscv/helper.h b/target/riscv/helper.h
> > index b785456ee0..af6cfcfc27 100644
> > --- a/target/riscv/helper.h
> > +++ b/target/riscv/helper.h
> > @@ -1289,3 +1289,6 @@ DEF_HELPER_4(vsm4r_vs, void, ptr, ptr, env, i32)
> > #ifndef CONFIG_USER_ONLY
> > DEF_HELPER_1(ssamoswap_disabled, void, env)
> > #endif
> > +
> > +/* Zalrsc" SC write probe */
> > +DEF_HELPER_FLAGS_3(sc_probe_write, TCG_CALL_NO_WG, void, env, tl, tl)
> > diff --git a/target/riscv/op_helper.c b/target/riscv/op_helper.c
> > index 6ccc127c30..b569366369 100644
> > --- a/target/riscv/op_helper.c
> > +++ b/target/riscv/op_helper.c
> > @@ -281,6 +281,20 @@ void helper_cbo_inval(CPURISCVState *env, target_ulong address)
> > /* We don't emulate the cache-hierarchy, so we're done. */
> > }
> >
> > +void helper_sc_probe_write(CPURISCVState *env, target_ulong addr,
> > + target_ulong size)
> > +{
> > + uintptr_t ra = GETPC();
> > + int mmu_idx = riscv_env_mmu_index(env, false);
> > +
> > + if (addr & (size - 1)) {
> > + env->badaddr = addr;
> > + riscv_raise_exception(env, RISCV_EXCP_STORE_AMO_ADDR_MIS, ra);
> > + }
> > +
> > + probe_write(env, addr, size, mmu_idx, ra);
> > +}
> > +
> > #ifndef CONFIG_USER_ONLY
> >
> > target_ulong helper_sret(CPURISCVState *env)
> > diff --git a/target/riscv/insn_trans/trans_rva.c.inc b/target/riscv/insn_trans/trans_rva.c.inc
> > index a7a3278d24..62c0fe673d 100644
> > --- a/target/riscv/insn_trans/trans_rva.c.inc
> > +++ b/target/riscv/insn_trans/trans_rva.c.inc
> > @@ -90,6 +90,12 @@ static bool gen_sc(DisasContext *ctx, arg_atomic *a, MemOp mop)
> > */
> > TCGBar bar_strl = (ctx->ztso || a->rl) ? TCG_BAR_STRL : 0;
> > tcg_gen_mb(TCG_MO_ALL + a->aq * TCG_BAR_LDAQ + bar_strl);
> > + /*
> > + * "For the purposes of memory protection, a failed SC.W may be treated
> > + * like a store." so let's check the write access permissions
> > + */
> > + gen_helper_sc_probe_write(tcg_env, src1,
> > + tcg_constant_tl(memop_size(mop)));
> > gen_set_gpr(ctx, a->rd, tcg_constant_tl(1));
> >
> > gen_set_label(l2);
>
On 4/9/2026 2:38 AM, Alistair Francis wrote:
> On Tue, Apr 7, 2026 at 7:38 PM Daniel Henrique Barboza
> <daniel.barboza@oss.qualcomm.com> wrote:
>>
>>
>>
>> On 4/7/2026 1:36 AM, alistair23@gmail.com wrote:
>>> From: Alistair Francis <alistair.francis@wdc.com>
>>>
>>> The RISC-V spec states:
>>>
>>> "For the purposes of memory protection, a failed SC.W may be treated
>>> like a store."
>>>
>>> So if the comparison in sc.w fails we should still check for alignment
>>> and do a probe access to check permissions.
>>>
>>> Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/3323
>>> Resoves: https://gitlab.com/qemu-project/qemu/-/work_items/3136
>>
>> Typo: "Resoves"
>>
>>
>> Also, IIRC the bug URL that automatically closes Gitlab issues when
>> merging the patch is on the format:
>>
>> "Resolves: https://gitlab.com/qemu-project/qemu/-/issues/(number)"
>
> Interestingly trying to access
>
> https://gitlab.com/qemu-project/qemu/-/issues/3196
>
> takes me to
>
> https://gitlab.com/qemu-project/qemu/-/work_items/3196
>
> So I think Gitlab is changing from issues to work_items
>
>>
>>
>> Maybe this URL with "work_items" also works, but even with the regular
>> URL Gitlab fails to autoclose the bug sometimes. I suggest changing the
>> "work_items" to "issues" in all those URLs to be safe.
>
> Maybe using "issues" is the problem?
>
> I'm thinking of leaving these as-is, as issues seem to be replaced now.
>
> I even see https://docs.gitlab.com/development/work_items/#migration-strategy
> and https://gitlab.com/groups/gitlab-org/-/work_items/6033 with more
> details on this
Seems like you're ahead of the curve then :D let's leave it as is and
see if the bug autocloses when the patches are merged.
Thanks,
Daniel
>
> Alistair
>
>>
>>
>> That said,
>>
>>
>> Reviewed-by: Daniel Henrique Barboza <daniel.barboza@oss.qualcomm.com>
>>
>>> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
>>> ---
>>> target/riscv/helper.h | 3 +++
>>> target/riscv/op_helper.c | 14 ++++++++++++++
>>> target/riscv/insn_trans/trans_rva.c.inc | 6 ++++++
>>> 3 files changed, 23 insertions(+)
>>>
>>> diff --git a/target/riscv/helper.h b/target/riscv/helper.h
>>> index b785456ee0..af6cfcfc27 100644
>>> --- a/target/riscv/helper.h
>>> +++ b/target/riscv/helper.h
>>> @@ -1289,3 +1289,6 @@ DEF_HELPER_4(vsm4r_vs, void, ptr, ptr, env, i32)
>>> #ifndef CONFIG_USER_ONLY
>>> DEF_HELPER_1(ssamoswap_disabled, void, env)
>>> #endif
>>> +
>>> +/* Zalrsc" SC write probe */
>>> +DEF_HELPER_FLAGS_3(sc_probe_write, TCG_CALL_NO_WG, void, env, tl, tl)
>>> diff --git a/target/riscv/op_helper.c b/target/riscv/op_helper.c
>>> index 6ccc127c30..b569366369 100644
>>> --- a/target/riscv/op_helper.c
>>> +++ b/target/riscv/op_helper.c
>>> @@ -281,6 +281,20 @@ void helper_cbo_inval(CPURISCVState *env, target_ulong address)
>>> /* We don't emulate the cache-hierarchy, so we're done. */
>>> }
>>>
>>> +void helper_sc_probe_write(CPURISCVState *env, target_ulong addr,
>>> + target_ulong size)
>>> +{
>>> + uintptr_t ra = GETPC();
>>> + int mmu_idx = riscv_env_mmu_index(env, false);
>>> +
>>> + if (addr & (size - 1)) {
>>> + env->badaddr = addr;
>>> + riscv_raise_exception(env, RISCV_EXCP_STORE_AMO_ADDR_MIS, ra);
>>> + }
>>> +
>>> + probe_write(env, addr, size, mmu_idx, ra);
>>> +}
>>> +
>>> #ifndef CONFIG_USER_ONLY
>>>
>>> target_ulong helper_sret(CPURISCVState *env)
>>> diff --git a/target/riscv/insn_trans/trans_rva.c.inc b/target/riscv/insn_trans/trans_rva.c.inc
>>> index a7a3278d24..62c0fe673d 100644
>>> --- a/target/riscv/insn_trans/trans_rva.c.inc
>>> +++ b/target/riscv/insn_trans/trans_rva.c.inc
>>> @@ -90,6 +90,12 @@ static bool gen_sc(DisasContext *ctx, arg_atomic *a, MemOp mop)
>>> */
>>> TCGBar bar_strl = (ctx->ztso || a->rl) ? TCG_BAR_STRL : 0;
>>> tcg_gen_mb(TCG_MO_ALL + a->aq * TCG_BAR_LDAQ + bar_strl);
>>> + /*
>>> + * "For the purposes of memory protection, a failed SC.W may be treated
>>> + * like a store." so let's check the write access permissions
>>> + */
>>> + gen_helper_sc_probe_write(tcg_env, src1,
>>> + tcg_constant_tl(memop_size(mop)));
>>> gen_set_gpr(ctx, a->rd, tcg_constant_tl(1));
>>>
>>> gen_set_label(l2);
>>
On Thu, Apr 9, 2026 at 8:02 PM Daniel Henrique Barboza <daniel.barboza@oss.qualcomm.com> wrote: > > > > On 4/9/2026 2:38 AM, Alistair Francis wrote: > > On Tue, Apr 7, 2026 at 7:38 PM Daniel Henrique Barboza > > <daniel.barboza@oss.qualcomm.com> wrote: > >> > >> > >> > >> On 4/7/2026 1:36 AM, alistair23@gmail.com wrote: > >>> From: Alistair Francis <alistair.francis@wdc.com> > >>> > >>> The RISC-V spec states: > >>> > >>> "For the purposes of memory protection, a failed SC.W may be treated > >>> like a store." > >>> > >>> So if the comparison in sc.w fails we should still check for alignment > >>> and do a probe access to check permissions. > >>> > >>> Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/3323 > >>> Resoves: https://gitlab.com/qemu-project/qemu/-/work_items/3136 > >> > >> Typo: "Resoves" > >> > >> > >> Also, IIRC the bug URL that automatically closes Gitlab issues when > >> merging the patch is on the format: > >> > >> "Resolves: https://gitlab.com/qemu-project/qemu/-/issues/(number)" > > > > Interestingly trying to access > > > > https://gitlab.com/qemu-project/qemu/-/issues/3196 > > > > takes me to > > > > https://gitlab.com/qemu-project/qemu/-/work_items/3196 > > > > So I think Gitlab is changing from issues to work_items > > > >> > >> > >> Maybe this URL with "work_items" also works, but even with the regular > >> URL Gitlab fails to autoclose the bug sometimes. I suggest changing the > >> "work_items" to "issues" in all those URLs to be safe. > > > > Maybe using "issues" is the problem? > > > > I'm thinking of leaving these as-is, as issues seem to be replaced now. > > > > I even see https://docs.gitlab.com/development/work_items/#migration-strategy > > and https://gitlab.com/groups/gitlab-org/-/work_items/6033 with more > > details on this > > Seems like you're ahead of the curve then :D let's leave it as is and > see if the bug autocloses when the patches are merged. I didn't know that when I started, I just tried to open the issue link and it went to work_items :) Seems like it does work though: https://gitlab.com/qemu-project/qemu/-/work_items/2796 Alistair
On 7/4/26 11:38, Daniel Henrique Barboza wrote:
>
>
> On 4/7/2026 1:36 AM, alistair23@gmail.com wrote:
>> From: Alistair Francis <alistair.francis@wdc.com>
>>
>> The RISC-V spec states:
>>
>> "For the purposes of memory protection, a failed SC.W may be treated
>> like a store."
>>
>> So if the comparison in sc.w fails we should still check for alignment
>> and do a probe access to check permissions.
>>
>> Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/3323
>> Resoves: https://gitlab.com/qemu-project/qemu/-/work_items/3136
>
> Typo: "Resoves"
>
>
> Also, IIRC the bug URL that automatically closes Gitlab issues when
> merging the patch is on the format:
>
> "Resolves: https://gitlab.com/qemu-project/qemu/-/issues/(number)"
>
>
> Maybe this URL with "work_items" also works, but even with the regular
> URL Gitlab fails to autoclose the bug sometimes. I suggest changing the
> "work_items" to "issues" in all those URLs to be safe.
FWIW I concur :)
>
>
> That said,
>
>
> Reviewed-by: Daniel Henrique Barboza <daniel.barboza@oss.qualcomm.com>
>
>> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
>> ---
>> target/riscv/helper.h | 3 +++
>> target/riscv/op_helper.c | 14 ++++++++++++++
>> target/riscv/insn_trans/trans_rva.c.inc | 6 ++++++
>> 3 files changed, 23 insertions(+)
>>
>> diff --git a/target/riscv/helper.h b/target/riscv/helper.h
>> index b785456ee0..af6cfcfc27 100644
>> --- a/target/riscv/helper.h
>> +++ b/target/riscv/helper.h
>> @@ -1289,3 +1289,6 @@ DEF_HELPER_4(vsm4r_vs, void, ptr, ptr, env, i32)
>> #ifndef CONFIG_USER_ONLY
>> DEF_HELPER_1(ssamoswap_disabled, void, env)
>> #endif
>> +
>> +/* Zalrsc" SC write probe */
>> +DEF_HELPER_FLAGS_3(sc_probe_write, TCG_CALL_NO_WG, void, env, tl, tl)
>> diff --git a/target/riscv/op_helper.c b/target/riscv/op_helper.c
>> index 6ccc127c30..b569366369 100644
>> --- a/target/riscv/op_helper.c
>> +++ b/target/riscv/op_helper.c
>> @@ -281,6 +281,20 @@ void helper_cbo_inval(CPURISCVState *env,
>> target_ulong address)
>> /* We don't emulate the cache-hierarchy, so we're done. */
>> }
>> +void helper_sc_probe_write(CPURISCVState *env, target_ulong addr,
>> + target_ulong size)
We could directly use uint64_t for @size. Then after the release the
@addr argument will be converted to vaddr type [*]. We could do that
directly now:
DEF_HELPER_FLAGS_3(sc_probe_write, TCG_CALL_NO_WG, void, env, vaddr, i64)
void helper_sc_probe_write(CPURISCVState *env, vaddr addr,
uint64_t size)
[*]
https://lore.kernel.org/qemu-devel/20260401143456.79843-1-philmd@linaro.org/
>> +{
>> + uintptr_t ra = GETPC();
>> + int mmu_idx = riscv_env_mmu_index(env, false);
>> +
>> + if (addr & (size - 1)) {
>> + env->badaddr = addr;
>> + riscv_raise_exception(env, RISCV_EXCP_STORE_AMO_ADDR_MIS, ra);
>> + }
>> +
>> + probe_write(env, addr, size, mmu_idx, ra);
>> +}
On Wed, Apr 8, 2026 at 6:48 PM Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> On 7/4/26 11:38, Daniel Henrique Barboza wrote:
> >
> >
> > On 4/7/2026 1:36 AM, alistair23@gmail.com wrote:
> >> From: Alistair Francis <alistair.francis@wdc.com>
> >>
> >> The RISC-V spec states:
> >>
> >> "For the purposes of memory protection, a failed SC.W may be treated
> >> like a store."
> >>
> >> So if the comparison in sc.w fails we should still check for alignment
> >> and do a probe access to check permissions.
> >>
> >> Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/3323
> >> Resoves: https://gitlab.com/qemu-project/qemu/-/work_items/3136
> >
> > Typo: "Resoves"
> >
> >
> > Also, IIRC the bug URL that automatically closes Gitlab issues when
> > merging the patch is on the format:
> >
> > "Resolves: https://gitlab.com/qemu-project/qemu/-/issues/(number)"
> >
> >
> > Maybe this URL with "work_items" also works, but even with the regular
> > URL Gitlab fails to autoclose the bug sometimes. I suggest changing the
> > "work_items" to "issues" in all those URLs to be safe.
>
> FWIW I concur :)
>
> >
> >
> > That said,
> >
> >
> > Reviewed-by: Daniel Henrique Barboza <daniel.barboza@oss.qualcomm.com>
> >
> >> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> >> ---
> >> target/riscv/helper.h | 3 +++
> >> target/riscv/op_helper.c | 14 ++++++++++++++
> >> target/riscv/insn_trans/trans_rva.c.inc | 6 ++++++
> >> 3 files changed, 23 insertions(+)
> >>
> >> diff --git a/target/riscv/helper.h b/target/riscv/helper.h
> >> index b785456ee0..af6cfcfc27 100644
> >> --- a/target/riscv/helper.h
> >> +++ b/target/riscv/helper.h
> >> @@ -1289,3 +1289,6 @@ DEF_HELPER_4(vsm4r_vs, void, ptr, ptr, env, i32)
> >> #ifndef CONFIG_USER_ONLY
> >> DEF_HELPER_1(ssamoswap_disabled, void, env)
> >> #endif
> >> +
> >> +/* Zalrsc" SC write probe */
> >> +DEF_HELPER_FLAGS_3(sc_probe_write, TCG_CALL_NO_WG, void, env, tl, tl)
> >> diff --git a/target/riscv/op_helper.c b/target/riscv/op_helper.c
> >> index 6ccc127c30..b569366369 100644
> >> --- a/target/riscv/op_helper.c
> >> +++ b/target/riscv/op_helper.c
> >> @@ -281,6 +281,20 @@ void helper_cbo_inval(CPURISCVState *env,
> >> target_ulong address)
> >> /* We don't emulate the cache-hierarchy, so we're done. */
> >> }
> >> +void helper_sc_probe_write(CPURISCVState *env, target_ulong addr,
> >> + target_ulong size)
>
> We could directly use uint64_t for @size. Then after the release the
> @addr argument will be converted to vaddr type [*]. We could do that
> directly now:
>
>
> DEF_HELPER_FLAGS_3(sc_probe_write, TCG_CALL_NO_WG, void, env, vaddr, i64)
>
> void helper_sc_probe_write(CPURISCVState *env, vaddr addr,
> uint64_t size)
That fails for RV32 linux-user
../target/riscv/insn_trans/trans_rva.c.inc:97:40: error: passing
argument 2 of ‘gen_helper_sc_probe_write’ from incompatible pointer
type [-Wincompatible-po
inter-types]
97 | gen_helper_sc_probe_write(tcg_env, src1,
| ^~~~
| |
| TCGv {aka struct TCGv_i32_d *}
In file included from
/var/mnt/scratch/alistair/software/qemu/include/qemu/osdep.h:53,
from ../target/riscv/translate.c:19:
/var/mnt/scratch/alistair/software/qemu/include/exec/helper-head.h.inc:128:57:
note: expected ‘TCGv_i64’ {aka ‘struct TCGv_i64_d *’} but argument is
of type
‘TCGv’ {aka ‘struct TCGv_i32_d *’}
128 | #define dh_arg_decl(t, n) glue(TCGv_, dh_alias(t)) glue(arg, n)
| ^
/var/mnt/scratch/alistair/software/qemu/include/qemu/compiler.h:29:21:
note: in definition of macro ‘xglue’
29 | #define xglue(x, y) x ## y
| ^
/var/mnt/scratch/alistair/software/qemu/include/exec/helper-head.h.inc:128:52:
note: in expansion of macro ‘glue’
128 | #define dh_arg_decl(t, n) glue(TCGv_, dh_alias(t)) glue(arg, n)
| ^~~~
/var/mnt/scratch/alistair/software/qemu/include/exec/helper-gen.h.inc:44:25:
note: in expansion of macro ‘dh_arg_decl’
44 | dh_arg_decl(t1, 1), dh_arg_decl(t2, 2), dh_arg_decl(t3, 3)) \
| ^~~~~~~~~~~
../target/riscv/helper.h:1294:1: note: in expansion of macro
‘DEF_HELPER_FLAGS_3’
1294 | DEF_HELPER_FLAGS_3(sc_probe_write, TCG_CALL_NO_WG, void, env, vaddr, i64)
| ^~~~~~~~~~~~~~~~~~
In file included from
/var/mnt/scratch/alistair/software/qemu/include/tcg/tcg-op-common.h:11,
from
/var/mnt/scratch/alistair/software/qemu/include/tcg/tcg-op.h:11,
from ../target/riscv/translate.c:22:
/var/mnt/scratch/alistair/software/qemu/include/tcg/tcg.h:202:28:
note: ‘TCGv_i64’ declared here
202 | typedef struct TCGv_i64_d *TCGv_i64;
| ^~~~~~~~
/var/mnt/scratch/alistair/software/qemu/include/tcg/tcg-op.h:32:18:
note: ‘TCGv’ declared here
32 | typedef TCGv_i32 TCGv;
| ^~~~
Am I missing something?
Alistair
>
>
> [*]
> https://lore.kernel.org/qemu-devel/20260401143456.79843-1-philmd@linaro.org/
>
> >> +{
> >> + uintptr_t ra = GETPC();
> >> + int mmu_idx = riscv_env_mmu_index(env, false);
> >> +
> >> + if (addr & (size - 1)) {
> >> + env->badaddr = addr;
> >> + riscv_raise_exception(env, RISCV_EXCP_STORE_AMO_ADDR_MIS, ra);
> >> + }
> >> +
> >> + probe_write(env, addr, size, mmu_idx, ra);
> >> +}
On 9/4/26 08:21, Alistair Francis wrote:
> On Wed, Apr 8, 2026 at 6:48 PM Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>>
>> On 7/4/26 11:38, Daniel Henrique Barboza wrote:
>>>
>>>
>>> On 4/7/2026 1:36 AM, alistair23@gmail.com wrote:
>>>> From: Alistair Francis <alistair.francis@wdc.com>
>>>>
>>>> The RISC-V spec states:
>>>>
>>>> "For the purposes of memory protection, a failed SC.W may be treated
>>>> like a store."
>>>>
>>>> So if the comparison in sc.w fails we should still check for alignment
>>>> and do a probe access to check permissions.
>>>>
>>>> Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/3323
>>>> Resoves: https://gitlab.com/qemu-project/qemu/-/work_items/3136
>>>
>>> Typo: "Resoves"
>>>
>>>
>>> Also, IIRC the bug URL that automatically closes Gitlab issues when
>>> merging the patch is on the format:
>>>
>>> "Resolves: https://gitlab.com/qemu-project/qemu/-/issues/(number)"
>>>
>>>
>>> Maybe this URL with "work_items" also works, but even with the regular
>>> URL Gitlab fails to autoclose the bug sometimes. I suggest changing the
>>> "work_items" to "issues" in all those URLs to be safe.
>>
>> FWIW I concur :)
>>
>>>
>>>
>>> That said,
>>>
>>>
>>> Reviewed-by: Daniel Henrique Barboza <daniel.barboza@oss.qualcomm.com>
>>>
>>>> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
>>>> ---
>>>> target/riscv/helper.h | 3 +++
>>>> target/riscv/op_helper.c | 14 ++++++++++++++
>>>> target/riscv/insn_trans/trans_rva.c.inc | 6 ++++++
>>>> 3 files changed, 23 insertions(+)
>>>>
>>>> diff --git a/target/riscv/helper.h b/target/riscv/helper.h
>>>> index b785456ee0..af6cfcfc27 100644
>>>> --- a/target/riscv/helper.h
>>>> +++ b/target/riscv/helper.h
>>>> @@ -1289,3 +1289,6 @@ DEF_HELPER_4(vsm4r_vs, void, ptr, ptr, env, i32)
>>>> #ifndef CONFIG_USER_ONLY
>>>> DEF_HELPER_1(ssamoswap_disabled, void, env)
>>>> #endif
>>>> +
>>>> +/* Zalrsc" SC write probe */
>>>> +DEF_HELPER_FLAGS_3(sc_probe_write, TCG_CALL_NO_WG, void, env, tl, tl)
>>>> diff --git a/target/riscv/op_helper.c b/target/riscv/op_helper.c
>>>> index 6ccc127c30..b569366369 100644
>>>> --- a/target/riscv/op_helper.c
>>>> +++ b/target/riscv/op_helper.c
>>>> @@ -281,6 +281,20 @@ void helper_cbo_inval(CPURISCVState *env,
>>>> target_ulong address)
>>>> /* We don't emulate the cache-hierarchy, so we're done. */
>>>> }
>>>> +void helper_sc_probe_write(CPURISCVState *env, target_ulong addr,
>>>> + target_ulong size)
>>
>> We could directly use uint64_t for @size. Then after the release the
>> @addr argument will be converted to vaddr type [*]. We could do that
>> directly now:
>>
>>
>> DEF_HELPER_FLAGS_3(sc_probe_write, TCG_CALL_NO_WG, void, env, vaddr, i64)
>>
>> void helper_sc_probe_write(CPURISCVState *env, vaddr addr,
>> uint64_t size)
>
> That fails for RV32 linux-user
>
> ../target/riscv/insn_trans/trans_rva.c.inc:97:40: error: passing
> argument 2 of ‘gen_helper_sc_probe_write’ from incompatible pointer
> type [-Wincompatible-po
> inter-types]
> 97 | gen_helper_sc_probe_write(tcg_env, src1,
> | ^~~~
> | |
> | TCGv {aka struct TCGv_i32_d *}
> In file included from
> /var/mnt/scratch/alistair/software/qemu/include/qemu/osdep.h:53,
> from ../target/riscv/translate.c:19:
> /var/mnt/scratch/alistair/software/qemu/include/exec/helper-head.h.inc:128:57:
> note: expected ‘TCGv_i64’ {aka ‘struct TCGv_i64_d *’} but argument is
> of type
> ‘TCGv’ {aka ‘struct TCGv_i32_d *’}
> 128 | #define dh_arg_decl(t, n) glue(TCGv_, dh_alias(t)) glue(arg, n)
> | ^
> /var/mnt/scratch/alistair/software/qemu/include/qemu/compiler.h:29:21:
> note: in definition of macro ‘xglue’
> 29 | #define xglue(x, y) x ## y
> | ^
> /var/mnt/scratch/alistair/software/qemu/include/exec/helper-head.h.inc:128:52:
> note: in expansion of macro ‘glue’
> 128 | #define dh_arg_decl(t, n) glue(TCGv_, dh_alias(t)) glue(arg, n)
> | ^~~~
> /var/mnt/scratch/alistair/software/qemu/include/exec/helper-gen.h.inc:44:25:
> note: in expansion of macro ‘dh_arg_decl’
> 44 | dh_arg_decl(t1, 1), dh_arg_decl(t2, 2), dh_arg_decl(t3, 3)) \
> | ^~~~~~~~~~~
> ../target/riscv/helper.h:1294:1: note: in expansion of macro
> ‘DEF_HELPER_FLAGS_3’
> 1294 | DEF_HELPER_FLAGS_3(sc_probe_write, TCG_CALL_NO_WG, void, env, vaddr, i64)
> | ^~~~~~~~~~~~~~~~~~
> In file included from
> /var/mnt/scratch/alistair/software/qemu/include/tcg/tcg-op-common.h:11,
> from
> /var/mnt/scratch/alistair/software/qemu/include/tcg/tcg-op.h:11,
> from ../target/riscv/translate.c:22:
> /var/mnt/scratch/alistair/software/qemu/include/tcg/tcg.h:202:28:
> note: ‘TCGv_i64’ declared here
> 202 | typedef struct TCGv_i64_d *TCGv_i64;
> | ^~~~~~~~
> /var/mnt/scratch/alistair/software/qemu/include/tcg/tcg-op.h:32:18:
> note: ‘TCGv’ declared here
> 32 | typedef TCGv_i32 TCGv;
> | ^~~~
>
> Am I missing something?
Oh, RISCV needs more work than :( Discard my suggestion then,
thanks for trying it and sorry for the delay.
Regards,
Phil.
On Tue, Apr 07, 2026 at 02:36:10PM +1000, alistair23@gmail.com wrote:
> From: Alistair Francis <alistair.francis@wdc.com>
>
> The RISC-V spec states:
>
> "For the purposes of memory protection, a failed SC.W may be treated
> like a store."
>
> So if the comparison in sc.w fails we should still check for alignment
> and do a probe access to check permissions.
>
> Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/3323
> Resoves: https://gitlab.com/qemu-project/qemu/-/work_items/3136
> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> ---
> target/riscv/helper.h | 3 +++
> target/riscv/op_helper.c | 14 ++++++++++++++
> target/riscv/insn_trans/trans_rva.c.inc | 6 ++++++
> 3 files changed, 23 insertions(+)
>
> diff --git a/target/riscv/helper.h b/target/riscv/helper.h
> index b785456ee0..af6cfcfc27 100644
> --- a/target/riscv/helper.h
> +++ b/target/riscv/helper.h
> @@ -1289,3 +1289,6 @@ DEF_HELPER_4(vsm4r_vs, void, ptr, ptr, env, i32)
> #ifndef CONFIG_USER_ONLY
> DEF_HELPER_1(ssamoswap_disabled, void, env)
> #endif
> +
> +/* Zalrsc" SC write probe */
stray `"` after Zalrsc.
other LGTM :)
Reviewed-by: Chao Liu <chao.liu.zevorn@gmail.com>
Thanks,
Chao
> +DEF_HELPER_FLAGS_3(sc_probe_write, TCG_CALL_NO_WG, void, env, tl, tl)
> diff --git a/target/riscv/op_helper.c b/target/riscv/op_helper.c
> index 6ccc127c30..b569366369 100644
> --- a/target/riscv/op_helper.c
> +++ b/target/riscv/op_helper.c
> @@ -281,6 +281,20 @@ void helper_cbo_inval(CPURISCVState *env, target_ulong address)
> /* We don't emulate the cache-hierarchy, so we're done. */
> }
>
> +void helper_sc_probe_write(CPURISCVState *env, target_ulong addr,
> + target_ulong size)
> +{
> + uintptr_t ra = GETPC();
> + int mmu_idx = riscv_env_mmu_index(env, false);
> +
> + if (addr & (size - 1)) {
> + env->badaddr = addr;
> + riscv_raise_exception(env, RISCV_EXCP_STORE_AMO_ADDR_MIS, ra);
> + }
> +
> + probe_write(env, addr, size, mmu_idx, ra);
> +}
> +
> #ifndef CONFIG_USER_ONLY
>
> target_ulong helper_sret(CPURISCVState *env)
> diff --git a/target/riscv/insn_trans/trans_rva.c.inc b/target/riscv/insn_trans/trans_rva.c.inc
> index a7a3278d24..62c0fe673d 100644
> --- a/target/riscv/insn_trans/trans_rva.c.inc
> +++ b/target/riscv/insn_trans/trans_rva.c.inc
> @@ -90,6 +90,12 @@ static bool gen_sc(DisasContext *ctx, arg_atomic *a, MemOp mop)
> */
> TCGBar bar_strl = (ctx->ztso || a->rl) ? TCG_BAR_STRL : 0;
> tcg_gen_mb(TCG_MO_ALL + a->aq * TCG_BAR_LDAQ + bar_strl);
> + /*
> + * "For the purposes of memory protection, a failed SC.W may be treated
> + * like a store." so let's check the write access permissions
> + */
> + gen_helper_sc_probe_write(tcg_env, src1,
> + tcg_constant_tl(memop_size(mop)));
> gen_set_gpr(ctx, a->rd, tcg_constant_tl(1));
>
> gen_set_label(l2);
> --
> 2.53.0
>
© 2016 - 2026 Red Hat, Inc.