On Wed, Feb 2, 2022 at 10:37 AM Richard Henderson <richard.hender...@linaro.org> wrote: > > 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.
Isn't that what this series does though, albeit for IO accesses only Using probe_write() solves part of this problem. If we have RAM at the address but no permissions to access it, then probe_write() will generate a store/AMO fault. But it won't help if nothing is mapped at that address. Let's say you are performing an atomic operation at an unmapped address 0x00, in M mode (so no MMU). probe_write() will eventually call riscv_cpu_tlb_fill() and get_physical_address(). On a system without an MMU and no PMP enforcement we get full read/write/execute permissions from riscv_cpu_tlb_fill(). So probe_write() succeeds. > > > 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. If you have permissions then that's fine. I thought we went via the slow path if the permission check fails? Alistair > > > 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~
