On Wed, Jan 26, 2022 at 10:09 AM Richard Henderson <richard.hender...@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.fran...@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~
