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

[Richard Henderson]

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 
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 
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/.

We may not want to add this check along the normal hot path of a normal load, but create 
separate helpers for "load with write-permission-check".  And we should answer the 
question of whether it should really be "load with read-write-permission-check", which 
will make the changes to tcg/arch/ harder.


r~