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

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

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