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

[Alistair Francis]

On Wed, Feb 2, 2022 at 10:37 AM Richard Henderson
<richard.henderson@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~