Re: [PATCH v6 12/16] target/riscv: AMO operations always raise store/AMO fault

[Richard Henderson <richard.henderson@linaro.org>]

On 8/22/24 10:58, Deepak Gupta wrote:
> On Thu, Aug 22, 2024 at 10:43:05AM +1000, Richard Henderson wrote:
>> On 8/22/24 07:50, Deepak Gupta wrote:
>>> @@ -1779,13 +1780,25 @@ void riscv_cpu_do_interrupt(CPUState *cs)
>>>              env->pc += 4;
>>>              return;
>>>          case RISCV_EXCP_LOAD_GUEST_ACCESS_FAULT:
>>> +            if (always_storeamo) {
>>> +                cause = RISCV_EXCP_STORE_GUEST_AMO_ACCESS_FAULT;
>>> +            }
>>> +            goto load_store_fault;
>>>          case RISCV_EXCP_STORE_GUEST_AMO_ACCESS_FAULT:
>>>          case RISCV_EXCP_LOAD_ADDR_MIS:
>>>          case RISCV_EXCP_STORE_AMO_ADDR_MIS:
>>>          case RISCV_EXCP_LOAD_ACCESS_FAULT:
>>> +            if (always_storeamo) {
>>> +                cause = RISCV_EXCP_STORE_AMO_ACCESS_FAULT;
>>> +            }
>>> +            goto load_store_fault;
>>>          case RISCV_EXCP_STORE_AMO_ACCESS_FAULT:
>>>          case RISCV_EXCP_LOAD_PAGE_FAULT:
>>>          case RISCV_EXCP_STORE_PAGE_FAULT:
>>> +            if (always_storeamo) {
>>> +                cause = RISCV_EXCP_STORE_PAGE_FAULT;
>>> +            }
>>> +        load_store_fault:
>>
>> These case labels need to be re-sorted;
> 
> Yeah it looks ugly but I didn't know what's expected. I'll sort cases.
> 
>> you're mising load/store when you're intending to check for load alone. 
> 
> I didn't get this.

Fall through of various case groups into the storeamo checks.
Only the first RISCV_EXCP_LOAD_GUEST_ACCESS_FAULT case is correct.

>> But perhaps in the end maybe just decode_save_opc(ctx, uw2) is better.
>>
>> I expect gen_cmpxchg also needs updating, though I don't have Zacas to hand.
> 
> I prefer decode_save_opc(ctx, uw2) but then
> 
> $git grep decode_save_opc | wc -l 38
> 
> I can update all these locations but it'll be handful.

That's fine.  Let's add the operand and update to pass 0 from existing sites as a separate 
patch.


r~