Re: [PATCH 0/2] RISC-V: Correctly generate store/amo faults

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

On 2/1/22 15:40, Alistair Francis wrote:
>> Alistair, you're only changing the reporting of MMIO faults for which read permission is
>> missing.  Importantly, the actual permission check is done elsewhere, and you aren't
>> changing that to perform a write access check.  Also, you very much need to handle normal
> 
> I'm a little confused with this part.
> 
> Looking at tcg_gen_atomic_cmpxchg_i64() for example we either:
>   1. call tcg_gen_qemu_ld_i64() then tcg_gen_qemu_st_i64()
>   2. call table_cmpxchg[] which eventually calls atomic_mmu_lookup()
>   3. call tcg_gen_atomic_cmpxchg_i32() which is pretty much the same as
> the above two
> 
> That means in both cases we end up performing a load or tlb_fill(..,
> MMU_DATA_LOAD, ..) operation as well as a store operation.

Yep...

> So we are already performing a write permission check...

... but we're doing so *after* the load.

Which means that for a completely unmapped page (as opposed to a read-only page) we will 
generate a read fault, which generates RISCV_EXCP_LOAD_ACCESS_FAULT and *not* 
RISCV_EXCP_STORE_AMO_ACCESS_FAULT.

So we need to check for write permission first, before performing the load.

> Can't we just do the check in the slow path? By the time we get to the
> fast path shouldn't we already have permissions?

No, the fast path performs the permissions check on one bit [rwx] depending on which tlb 
comparator it loads.

> As in add a new INDEX_op_qemu_ld_write_perm_i32/i64, make edits to
> atomic_mmu_lookup() and all of the plumbing for those?

That's one possibility, sure.


r~