* [PATCH] target/riscv: rvzicbo: Fixup CBO extension register calculation
@ 2024-05-14 2:39 Alistair Francis
2024-05-14 7:09 ` Richard Henderson
2024-05-14 9:10 ` Daniel Henrique Barboza
0 siblings, 2 replies; 6+ messages in thread
From: Alistair Francis @ 2024-05-14 2:39 UTC (permalink / raw)
To: qemu-riscv, liwei1518, zhiwei_liu, palmer, bin.meng, dbarboza,
qemu-devel
Cc: alistair23, Alistair Francis, fabian.thomas, Bin Meng
When running the instruction
```
cbo.flush 0(x0)
```
QEMU would segfault.
The issue was in cpu_gpr[a->rs1] as QEMU does not have cpu_gpr[0]
allocated.
In order to fix this let's use the existing get_address()
helper. This also has the benefit of performing pointer mask
calculations on the address specified in rs1.
The pointer masking specificiation specifically states:
"""
Cache Management Operations: All instructions in Zicbom, Zicbop and Zicboz
"""
So this is the correct behaviour and we previously have been incorrectly
not masking the address.
Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
Reported-by: Fabian Thomas <fabian.thomas@cispa.de>
Fixes: e05da09b7cfd ("target/riscv: implement Zicbom extension")
---
target/riscv/insn_trans/trans_rvzicbo.c.inc | 16 ++++++++++++----
1 file changed, 12 insertions(+), 4 deletions(-)
diff --git a/target/riscv/insn_trans/trans_rvzicbo.c.inc b/target/riscv/insn_trans/trans_rvzicbo.c.inc
index d5d7095903..15711c3140 100644
--- a/target/riscv/insn_trans/trans_rvzicbo.c.inc
+++ b/target/riscv/insn_trans/trans_rvzicbo.c.inc
@@ -31,27 +31,35 @@
static bool trans_cbo_clean(DisasContext *ctx, arg_cbo_clean *a)
{
REQUIRE_ZICBOM(ctx);
- gen_helper_cbo_clean_flush(tcg_env, cpu_gpr[a->rs1]);
+ TCGv src = get_address(ctx, a->rs1, 0);
+
+ gen_helper_cbo_clean_flush(tcg_env, src);
return true;
}
static bool trans_cbo_flush(DisasContext *ctx, arg_cbo_flush *a)
{
REQUIRE_ZICBOM(ctx);
- gen_helper_cbo_clean_flush(tcg_env, cpu_gpr[a->rs1]);
+ TCGv src = get_address(ctx, a->rs1, 0);
+
+ gen_helper_cbo_clean_flush(tcg_env, src);
return true;
}
static bool trans_cbo_inval(DisasContext *ctx, arg_cbo_inval *a)
{
REQUIRE_ZICBOM(ctx);
- gen_helper_cbo_inval(tcg_env, cpu_gpr[a->rs1]);
+ TCGv src = get_address(ctx, a->rs1, 0);
+
+ gen_helper_cbo_inval(tcg_env, src);
return true;
}
static bool trans_cbo_zero(DisasContext *ctx, arg_cbo_zero *a)
{
REQUIRE_ZICBOZ(ctx);
- gen_helper_cbo_zero(tcg_env, cpu_gpr[a->rs1]);
+ TCGv src = get_address(ctx, a->rs1, 0);
+
+ gen_helper_cbo_zero(tcg_env, src);
return true;
}
--
2.45.0
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] target/riscv: rvzicbo: Fixup CBO extension register calculation
2024-05-14 2:39 [PATCH] target/riscv: rvzicbo: Fixup CBO extension register calculation Alistair Francis
@ 2024-05-14 7:09 ` Richard Henderson
2024-05-14 9:10 ` Daniel Henrique Barboza
1 sibling, 0 replies; 6+ messages in thread
From: Richard Henderson @ 2024-05-14 7:09 UTC (permalink / raw)
To: Alistair Francis, qemu-riscv, liwei1518, zhiwei_liu, palmer,
bin.meng, dbarboza, qemu-devel
Cc: Alistair Francis, fabian.thomas, Bin Meng
On 5/14/24 04:39, Alistair Francis wrote:
> When running the instruction
>
> ```
> cbo.flush 0(x0)
> ```
>
> QEMU would segfault.
>
> The issue was in cpu_gpr[a->rs1] as QEMU does not have cpu_gpr[0]
> allocated.
>
> In order to fix this let's use the existing get_address()
> helper. This also has the benefit of performing pointer mask
> calculations on the address specified in rs1.
>
> The pointer masking specificiation specifically states:
>
> """
> Cache Management Operations: All instructions in Zicbom, Zicbop and Zicboz
> """
>
> So this is the correct behaviour and we previously have been incorrectly
> not masking the address.
>
> Signed-off-by: Alistair Francis<alistair.francis@wdc.com>
> Reported-by: Fabian Thomas<fabian.thomas@cispa.de>
> Fixes: e05da09b7cfd ("target/riscv: implement Zicbom extension")
> ---
> target/riscv/insn_trans/trans_rvzicbo.c.inc | 16 ++++++++++++----
> 1 file changed, 12 insertions(+), 4 deletions(-)
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
r~
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] target/riscv: rvzicbo: Fixup CBO extension register calculation
2024-05-14 2:39 [PATCH] target/riscv: rvzicbo: Fixup CBO extension register calculation Alistair Francis
2024-05-14 7:09 ` Richard Henderson
@ 2024-05-14 9:10 ` Daniel Henrique Barboza
2024-05-16 5:09 ` Alistair Francis
1 sibling, 1 reply; 6+ messages in thread
From: Daniel Henrique Barboza @ 2024-05-14 9:10 UTC (permalink / raw)
To: Alistair Francis, qemu-riscv, liwei1518, zhiwei_liu, palmer,
bin.meng, qemu-devel
Cc: Alistair Francis, fabian.thomas, Bin Meng,
Philippe Mathieu-Daudé, Richard Henderson
On 5/13/24 23:39, Alistair Francis wrote:
> When running the instruction
>
> ```
> cbo.flush 0(x0)
> ```
>
> QEMU would segfault.
>
> The issue was in cpu_gpr[a->rs1] as QEMU does not have cpu_gpr[0]
> allocated.
>
> In order to fix this let's use the existing get_address()
> helper. This also has the benefit of performing pointer mask
> calculations on the address specified in rs1.
>
> The pointer masking specificiation specifically states:
>
> """
> Cache Management Operations: All instructions in Zicbom, Zicbop and Zicboz
> """
>
> So this is the correct behaviour and we previously have been incorrectly
> not masking the address.
>
> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> Reported-by: Fabian Thomas <fabian.thomas@cispa.de>
> Fixes: e05da09b7cfd ("target/riscv: implement Zicbom extension")
> ---
LGTM but I wonder if this is the same fix as this one sent by Phil a month
ago or so:
https://lore.kernel.org/qemu-riscv/20240419110514.69697-1-philmd@linaro.org/
("[PATCH] target/riscv: Use get_address() to get address with Zicbom extensions")
Thanks,
Daniel
> target/riscv/insn_trans/trans_rvzicbo.c.inc | 16 ++++++++++++----
> 1 file changed, 12 insertions(+), 4 deletions(-)
>
> diff --git a/target/riscv/insn_trans/trans_rvzicbo.c.inc b/target/riscv/insn_trans/trans_rvzicbo.c.inc
> index d5d7095903..15711c3140 100644
> --- a/target/riscv/insn_trans/trans_rvzicbo.c.inc
> +++ b/target/riscv/insn_trans/trans_rvzicbo.c.inc
> @@ -31,27 +31,35 @@
> static bool trans_cbo_clean(DisasContext *ctx, arg_cbo_clean *a)
> {
> REQUIRE_ZICBOM(ctx);
> - gen_helper_cbo_clean_flush(tcg_env, cpu_gpr[a->rs1]);
> + TCGv src = get_address(ctx, a->rs1, 0);
> +
> + gen_helper_cbo_clean_flush(tcg_env, src);
> return true;
> }
>
> static bool trans_cbo_flush(DisasContext *ctx, arg_cbo_flush *a)
> {
> REQUIRE_ZICBOM(ctx);
> - gen_helper_cbo_clean_flush(tcg_env, cpu_gpr[a->rs1]);
> + TCGv src = get_address(ctx, a->rs1, 0);
> +
> + gen_helper_cbo_clean_flush(tcg_env, src);
> return true;
> }
>
> static bool trans_cbo_inval(DisasContext *ctx, arg_cbo_inval *a)
> {
> REQUIRE_ZICBOM(ctx);
> - gen_helper_cbo_inval(tcg_env, cpu_gpr[a->rs1]);
> + TCGv src = get_address(ctx, a->rs1, 0);
> +
> + gen_helper_cbo_inval(tcg_env, src);
> return true;
> }
>
> static bool trans_cbo_zero(DisasContext *ctx, arg_cbo_zero *a)
> {
> REQUIRE_ZICBOZ(ctx);
> - gen_helper_cbo_zero(tcg_env, cpu_gpr[a->rs1]);
> + TCGv src = get_address(ctx, a->rs1, 0);
> +
> + gen_helper_cbo_zero(tcg_env, src);
> return true;
> }
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] target/riscv: rvzicbo: Fixup CBO extension register calculation
2024-05-14 9:10 ` Daniel Henrique Barboza
@ 2024-05-16 5:09 ` Alistair Francis
2024-06-04 8:32 ` Philippe Mathieu-Daudé
0 siblings, 1 reply; 6+ messages in thread
From: Alistair Francis @ 2024-05-16 5:09 UTC (permalink / raw)
To: Daniel Henrique Barboza
Cc: qemu-riscv, liwei1518, zhiwei_liu, palmer, bin.meng, qemu-devel,
Alistair Francis, fabian.thomas, Bin Meng,
Philippe Mathieu-Daudé, Richard Henderson
On Tue, May 14, 2024 at 7:11 PM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
>
>
> On 5/13/24 23:39, Alistair Francis wrote:
> > When running the instruction
> >
> > ```
> > cbo.flush 0(x0)
> > ```
> >
> > QEMU would segfault.
> >
> > The issue was in cpu_gpr[a->rs1] as QEMU does not have cpu_gpr[0]
> > allocated.
> >
> > In order to fix this let's use the existing get_address()
> > helper. This also has the benefit of performing pointer mask
> > calculations on the address specified in rs1.
> >
> > The pointer masking specificiation specifically states:
> >
> > """
> > Cache Management Operations: All instructions in Zicbom, Zicbop and Zicboz
> > """
> >
> > So this is the correct behaviour and we previously have been incorrectly
> > not masking the address.
> >
> > Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> > Reported-by: Fabian Thomas <fabian.thomas@cispa.de>
> > Fixes: e05da09b7cfd ("target/riscv: implement Zicbom extension")
> > ---
>
> LGTM but I wonder if this is the same fix as this one sent by Phil a month
> ago or so:
>
> https://lore.kernel.org/qemu-riscv/20240419110514.69697-1-philmd@linaro.org/
> ("[PATCH] target/riscv: Use get_address() to get address with Zicbom extensions")
It is the same fix!
I somehow missed that patch at the time. Sorry Philippe!
I'm going to merge this one as it includes the details about pointer
masking, which I think is useful as that's why we are using
get_address() instead of get_gpr()
Alistair
>
>
> Thanks,
>
> Daniel
>
> > target/riscv/insn_trans/trans_rvzicbo.c.inc | 16 ++++++++++++----
> > 1 file changed, 12 insertions(+), 4 deletions(-)
> >
> > diff --git a/target/riscv/insn_trans/trans_rvzicbo.c.inc b/target/riscv/insn_trans/trans_rvzicbo.c.inc
> > index d5d7095903..15711c3140 100644
> > --- a/target/riscv/insn_trans/trans_rvzicbo.c.inc
> > +++ b/target/riscv/insn_trans/trans_rvzicbo.c.inc
> > @@ -31,27 +31,35 @@
> > static bool trans_cbo_clean(DisasContext *ctx, arg_cbo_clean *a)
> > {
> > REQUIRE_ZICBOM(ctx);
> > - gen_helper_cbo_clean_flush(tcg_env, cpu_gpr[a->rs1]);
> > + TCGv src = get_address(ctx, a->rs1, 0);
> > +
> > + gen_helper_cbo_clean_flush(tcg_env, src);
> > return true;
> > }
> >
> > static bool trans_cbo_flush(DisasContext *ctx, arg_cbo_flush *a)
> > {
> > REQUIRE_ZICBOM(ctx);
> > - gen_helper_cbo_clean_flush(tcg_env, cpu_gpr[a->rs1]);
> > + TCGv src = get_address(ctx, a->rs1, 0);
> > +
> > + gen_helper_cbo_clean_flush(tcg_env, src);
> > return true;
> > }
> >
> > static bool trans_cbo_inval(DisasContext *ctx, arg_cbo_inval *a)
> > {
> > REQUIRE_ZICBOM(ctx);
> > - gen_helper_cbo_inval(tcg_env, cpu_gpr[a->rs1]);
> > + TCGv src = get_address(ctx, a->rs1, 0);
> > +
> > + gen_helper_cbo_inval(tcg_env, src);
> > return true;
> > }
> >
> > static bool trans_cbo_zero(DisasContext *ctx, arg_cbo_zero *a)
> > {
> > REQUIRE_ZICBOZ(ctx);
> > - gen_helper_cbo_zero(tcg_env, cpu_gpr[a->rs1]);
> > + TCGv src = get_address(ctx, a->rs1, 0);
> > +
> > + gen_helper_cbo_zero(tcg_env, src);
> > return true;
> > }
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] target/riscv: rvzicbo: Fixup CBO extension register calculation
2024-05-16 5:09 ` Alistair Francis
@ 2024-06-04 8:32 ` Philippe Mathieu-Daudé
2024-06-04 11:37 ` Philippe Mathieu-Daudé
0 siblings, 1 reply; 6+ messages in thread
From: Philippe Mathieu-Daudé @ 2024-06-04 8:32 UTC (permalink / raw)
To: Alistair Francis, Daniel Henrique Barboza
Cc: qemu-riscv, liwei1518, zhiwei_liu, palmer, bin.meng, qemu-devel,
Alistair Francis, fabian.thomas, Bin Meng, Richard Henderson
On 16/5/24 07:09, Alistair Francis wrote:
> On Tue, May 14, 2024 at 7:11 PM Daniel Henrique Barboza
> <dbarboza@ventanamicro.com> wrote:
>>
>>
>>
>> On 5/13/24 23:39, Alistair Francis wrote:
>>> When running the instruction
>>>
>>> ```
>>> cbo.flush 0(x0)
>>> ```
>>>
>>> QEMU would segfault.
>>>
>>> The issue was in cpu_gpr[a->rs1] as QEMU does not have cpu_gpr[0]
>>> allocated.
>>>
>>> In order to fix this let's use the existing get_address()
>>> helper. This also has the benefit of performing pointer mask
>>> calculations on the address specified in rs1.
>>>
>>> The pointer masking specificiation specifically states:
>>>
>>> """
>>> Cache Management Operations: All instructions in Zicbom, Zicbop and Zicboz
>>> """
>>>
>>> So this is the correct behaviour and we previously have been incorrectly
>>> not masking the address.
>>>
>>> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
>>> Reported-by: Fabian Thomas <fabian.thomas@cispa.de>
>>> Fixes: e05da09b7cfd ("target/riscv: implement Zicbom extension")
Reported-by: Zhiwei Jiang (姜智伟) <jiangzw@tecorigin.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>> ---
>>
>> LGTM but I wonder if this is the same fix as this one sent by Phil a month
>> ago or so:
>>
>> https://lore.kernel.org/qemu-riscv/20240419110514.69697-1-philmd@linaro.org/
>> ("[PATCH] target/riscv: Use get_address() to get address with Zicbom extensions")
>
> It is the same fix!
>
> I somehow missed that patch at the time. Sorry Philippe!
>
> I'm going to merge this one as it includes the details about pointer
> masking, which I think is useful as that's why we are using
> get_address() instead of get_gpr()
Fine by me :)
> Alistair
>
>>
>>
>> Thanks,
>>
>> Daniel
>>
>>> target/riscv/insn_trans/trans_rvzicbo.c.inc | 16 ++++++++++++----
>>> 1 file changed, 12 insertions(+), 4 deletions(-)
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] target/riscv: rvzicbo: Fixup CBO extension register calculation
2024-06-04 8:32 ` Philippe Mathieu-Daudé
@ 2024-06-04 11:37 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 6+ messages in thread
From: Philippe Mathieu-Daudé @ 2024-06-04 11:37 UTC (permalink / raw)
To: Alistair Francis, Daniel Henrique Barboza,
Zhiwei Jiang (姜智伟)
Cc: qemu-riscv, liwei1518, zhiwei_liu, palmer, bin.meng, qemu-devel,
Alistair Francis, fabian.thomas, Bin Meng, Richard Henderson
On 4/6/24 10:32, Philippe Mathieu-Daudé wrote:
> On 16/5/24 07:09, Alistair Francis wrote:
>> On Tue, May 14, 2024 at 7:11 PM Daniel Henrique Barboza
>> <dbarboza@ventanamicro.com> wrote:
>>>
>>>
>>>
>>> On 5/13/24 23:39, Alistair Francis wrote:
>>>> When running the instruction
>>>>
>>>> ```
>>>> cbo.flush 0(x0)
>>>> ```
>>>>
>>>> QEMU would segfault.
>>>>
>>>> The issue was in cpu_gpr[a->rs1] as QEMU does not have cpu_gpr[0]
>>>> allocated.
>>>>
>>>> In order to fix this let's use the existing get_address()
>>>> helper. This also has the benefit of performing pointer mask
>>>> calculations on the address specified in rs1.
>>>>
>>>> The pointer masking specificiation specifically states:
>>>>
>>>> """
>>>> Cache Management Operations: All instructions in Zicbom, Zicbop and
>>>> Zicboz
>>>> """
>>>>
>>>> So this is the correct behaviour and we previously have been
>>>> incorrectly
>>>> not masking the address.
>>>>
>>>> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
>>>> Reported-by: Fabian Thomas <fabian.thomas@cispa.de>
>>>> Fixes: e05da09b7cfd ("target/riscv: implement Zicbom extension")
>
> Reported-by: Zhiwei Jiang (姜智伟) <jiangzw@tecorigin.com>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Too late since merged as commit c5eb8d6336 ("target/riscv: rvzicbo:
Fixup CBO extension register calculation") but Cc Zhiwei Jiang to
notify it is now fixed.
>>>> ---
>>>
>>> LGTM but I wonder if this is the same fix as this one sent by Phil a
>>> month
>>> ago or so:
>>>
>>> https://lore.kernel.org/qemu-riscv/20240419110514.69697-1-philmd@linaro.org/
>>> ("[PATCH] target/riscv: Use get_address() to get address with Zicbom
>>> extensions")
>>
>> It is the same fix!
>>
>> I somehow missed that patch at the time. Sorry Philippe!
>>
>> I'm going to merge this one as it includes the details about pointer
>> masking, which I think is useful as that's why we are using
>> get_address() instead of get_gpr()
>
> Fine by me :)
>
>> Alistair
>>
>>>
>>>
>>> Thanks,
>>>
>>> Daniel
>>>
>>>> target/riscv/insn_trans/trans_rvzicbo.c.inc | 16 ++++++++++++----
>>>> 1 file changed, 12 insertions(+), 4 deletions(-)
>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2024-06-04 11:38 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-05-14 2:39 [PATCH] target/riscv: rvzicbo: Fixup CBO extension register calculation Alistair Francis
2024-05-14 7:09 ` Richard Henderson
2024-05-14 9:10 ` Daniel Henrique Barboza
2024-05-16 5:09 ` Alistair Francis
2024-06-04 8:32 ` Philippe Mathieu-Daudé
2024-06-04 11:37 ` Philippe Mathieu-Daudé
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).