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.

This fixes: https://gitlab.com/qemu-project/qemu/-/issues/594

Alistair Francis (2):
  accel: tcg: Allow forcing a store fault on read ops
  targett/riscv: rva: Correctly generate a store/amo fault

 include/exec/memop.h                    |  2 +
 accel/tcg/cputlb.c                      | 11 ++++-
 target/riscv/insn_trans/trans_rva.c.inc | 56 ++++++++++++++++---------
 3 files changed, 48 insertions(+), 21 deletions(-)

-- 
2.31.1

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) or with another new MO_ op.

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.

Currently tlb_fill doesn't receive this parameter. Is it possible to add 
a new Memop parameter to  tlb_fill?

Thanks,
Zhiwei

>
> This fixes: https://gitlab.com/qemu-project/qemu/-/issues/594
>
> Alistair Francis (2):
>    accel: tcg: Allow forcing a store fault on read ops
>    targett/riscv: rva: Correctly generate a store/amo fault
>
>   include/exec/memop.h                    |  2 +
>   accel/tcg/cputlb.c                      | 11 ++++-
>   target/riscv/insn_trans/trans_rva.c.inc | 56 ++++++++++++++++---------
>   3 files changed, 48 insertions(+), 21 deletions(-)
>

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~

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~

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.

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

> 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~

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~

On 2/4/22 18:36, Alistair Francis wrote:
>> 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

No.

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

True.

But there it's not a permission problem, per se.  What are you supposed to get here on 
riscv?  On some other cpus you don't get a "normal" segv, but a machine check.  I suppose 
you still want to see "store" rather than "load" in reporting that...


>>> 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?

We do.  But you haven't changed any permissions checks, so you don't really know what 
you're getting -- you may not arrive at the slow path at all.


r~