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

[Alistair Francis]

On Wed, Jan 26, 2022 at 10:09 AM Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> On 1/24/22 4:17 PM, LIU Zhiwei wrote:
> >
> > On 2022/1/24 上午8:59, Alistair Francis wrote:
> >> From: Alistair Francis <alistair.francis@wdc.com>
> >>
> >> This series adds a MO_ op to specify that a load instruction should
> >> produce a store fault. This is used on RISC-V to produce a store/amo
> >> fault when an atomic access fails.
> >
> > Hi Alistair,
> >
> > As Richard said,  we  can address this issue in two ways, probe_read(I 
> > think probe_write
> > is typo)
>
> It is not a typo: we want to verify that the memory is writable before we 
> perform the
> load.  This will raise a write fault on a no-access page before a read fault 
> would be
> generated by the load.  This may still generate the wrong fault for a 
> write-only page.
> (Is such a page permission encoding possible with RISCV?  Not all cpus 
> support that, since

It's not. RISC-V doesn't have write only pages, at least not in the
current priv spec (maybe some extension allows it).

> at first blush it seems to be mostly useless.  But some do, and a generic tcg 
> feature
> should be designed with those in mind.)
>
> > In my opinion use MO_op in io_readx may be not right because the issue is 
> > not only with IO
> > access. And MO_ op in io_readx is too later because the exception has been 
> > created when
> > tlb_fill.
>
> You are correct that changing only io_readx is insufficient.  Very much so.
>
> 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.

So we are already performing a write permission check, if that fails
on RISC-V we correctly generate the RISCV_EXCP_STORE_AMO_ACCESS_FAULT
fault. I guess on some architectures there might be a specific atomic
fault, which we will still not correctly trigger though.

The part we are interested in is the load, and ensuring that we
generate a store fault if that fails. At least for RISC-V.

> memory not just MMIO.  Which will require changes across all tcg/arch/, as 
> well as in all
> of the memory access helpers in accel/tcg/.

Argh, yeah

>
> We may not want to add this check along the normal hot path of a normal load, 
> but create

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?

> separate helpers for "load with write-permission-check".  And we should 
> answer the

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?

Alistair

> question of whether it should really be "load with 
> read-write-permission-check", which
> will make the changes to tcg/arch/ harder.
>
>
> r~