Re: [PATCH] target/riscv: rvzicbo: Fixup CBO extension register calculation

[Philippe Mathieu-Daudé]

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(-)