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

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

[Alistair Francis]

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

[PATCH 1/2] accel: tcg: Allow forcing a store fault on read ops

[Alistair Francis]

From: Alistair Francis <alistair.francis@wdc.com>

When performing atomic operations TCG will do a read operation then a
write operation. This results in a MMU_DATA_LOAD fault if the address is
invalid.

For some platforms (such as RISC-V) we should produce a store fault if
an atomic operation fails. This patch adds a new MemOp (MO_WRITE_FAULT)
that allows us to indicate that the operation should produce a
MMU_DATA_STORE access type if the operation faults.

Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
---
 include/exec/memop.h |  2 ++
 accel/tcg/cputlb.c   | 11 +++++++++--
 2 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/include/exec/memop.h b/include/exec/memop.h
index 2a885f3917..93ae1b6a2e 100644
--- a/include/exec/memop.h
+++ b/include/exec/memop.h
@@ -81,6 +81,8 @@ typedef enum MemOp {
     MO_ALIGN_32 = 5 << MO_ASHIFT,
     MO_ALIGN_64 = 6 << MO_ASHIFT,
 
+    MO_WRITE_FAULT = 0x100,
+
     /* Combinations of the above, for ease of use.  */
     MO_UB    = MO_8,
     MO_UW    = MO_16,
diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
index 5e0d0eebc3..320555d5e9 100644
--- a/accel/tcg/cputlb.c
+++ b/accel/tcg/cputlb.c
@@ -1362,8 +1362,15 @@ static uint64_t io_readx(CPUArchState *env, CPUIOTLBEntry *iotlbentry,
             section->offset_within_address_space -
             section->offset_within_region;
 
-        cpu_transaction_failed(cpu, physaddr, addr, memop_size(op), access_type,
-                               mmu_idx, iotlbentry->attrs, r, retaddr);
+        if (op & MO_WRITE_FAULT) {
+            cpu_transaction_failed(cpu, physaddr, addr, memop_size(op),
+                                   MMU_DATA_STORE, mmu_idx, iotlbentry->attrs,
+                                   r, retaddr);
+        } else {
+            cpu_transaction_failed(cpu, physaddr, addr, memop_size(op),
+                                   access_type, mmu_idx, iotlbentry->attrs,
+                                   r, retaddr);
+        }
     }
     if (locked) {
         qemu_mutex_unlock_iothread();
-- 
2.31.1

[PATCH 2/2] targett/riscv: rva: Correctly generate a store/amo fault

[Alistair Francis]

From: Alistair Francis <alistair.francis@wdc.com>

If the atomic operation fails we want to generate a MMU_DATA_STORE
access type so we can produce a RISCV_EXCP_STORE_AMO_ACCESS_FAULT for
the guest.

Resolves: https://gitlab.com/qemu-project/qemu/-/issues/594
Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
---
 target/riscv/insn_trans/trans_rva.c.inc | 56 ++++++++++++++++---------
 1 file changed, 37 insertions(+), 19 deletions(-)

diff --git a/target/riscv/insn_trans/trans_rva.c.inc b/target/riscv/insn_trans/trans_rva.c.inc
index 45db82c9be..be5c94803b 100644
--- a/target/riscv/insn_trans/trans_rva.c.inc
+++ b/target/riscv/insn_trans/trans_rva.c.inc
@@ -93,7 +93,7 @@ static bool gen_amo(DisasContext *ctx, arg_atomic *a,
 static bool trans_lr_w(DisasContext *ctx, arg_lr_w *a)
 {
     REQUIRE_EXT(ctx, RVA);
-    return gen_lr(ctx, a, (MO_ALIGN | MO_TESL));
+    return gen_lr(ctx, a, (MO_ALIGN  | MO_TESL));
 }
 
 static bool trans_sc_w(DisasContext *ctx, arg_sc_w *a)
@@ -105,55 +105,64 @@ static bool trans_sc_w(DisasContext *ctx, arg_sc_w *a)
 static bool trans_amoswap_w(DisasContext *ctx, arg_amoswap_w *a)
 {
     REQUIRE_EXT(ctx, RVA);
-    return gen_amo(ctx, a, &tcg_gen_atomic_xchg_tl, (MO_ALIGN | MO_TESL));
+    return gen_amo(ctx, a, &tcg_gen_atomic_xchg_tl,
+                   (MO_ALIGN | MO_WRITE_FAULT | MO_TESL));
 }
 
 static bool trans_amoadd_w(DisasContext *ctx, arg_amoadd_w *a)
 {
     REQUIRE_EXT(ctx, RVA);
-    return gen_amo(ctx, a, &tcg_gen_atomic_fetch_add_tl, (MO_ALIGN | MO_TESL));
+    return gen_amo(ctx, a, &tcg_gen_atomic_fetch_add_tl,
+                   (MO_ALIGN | MO_WRITE_FAULT | MO_TESL));
 }
 
 static bool trans_amoxor_w(DisasContext *ctx, arg_amoxor_w *a)
 {
     REQUIRE_EXT(ctx, RVA);
-    return gen_amo(ctx, a, &tcg_gen_atomic_fetch_xor_tl, (MO_ALIGN | MO_TESL));
+    return gen_amo(ctx, a, &tcg_gen_atomic_fetch_xor_tl,
+                   (MO_ALIGN | MO_WRITE_FAULT | MO_TESL));
 }
 
 static bool trans_amoand_w(DisasContext *ctx, arg_amoand_w *a)
 {
     REQUIRE_EXT(ctx, RVA);
-    return gen_amo(ctx, a, &tcg_gen_atomic_fetch_and_tl, (MO_ALIGN | MO_TESL));
+    return gen_amo(ctx, a, &tcg_gen_atomic_fetch_and_tl,
+                   (MO_ALIGN | MO_WRITE_FAULT | MO_TESL));
 }
 
 static bool trans_amoor_w(DisasContext *ctx, arg_amoor_w *a)
 {
     REQUIRE_EXT(ctx, RVA);
-    return gen_amo(ctx, a, &tcg_gen_atomic_fetch_or_tl, (MO_ALIGN | MO_TESL));
+    return gen_amo(ctx, a, &tcg_gen_atomic_fetch_or_tl,
+                   (MO_ALIGN | MO_WRITE_FAULT | MO_TESL));
 }
 
 static bool trans_amomin_w(DisasContext *ctx, arg_amomin_w *a)
 {
     REQUIRE_EXT(ctx, RVA);
-    return gen_amo(ctx, a, &tcg_gen_atomic_fetch_smin_tl, (MO_ALIGN | MO_TESL));
+    return gen_amo(ctx, a, &tcg_gen_atomic_fetch_smin_tl,
+                   (MO_ALIGN | MO_WRITE_FAULT | MO_TESL));
 }
 
 static bool trans_amomax_w(DisasContext *ctx, arg_amomax_w *a)
 {
     REQUIRE_EXT(ctx, RVA);
-    return gen_amo(ctx, a, &tcg_gen_atomic_fetch_smax_tl, (MO_ALIGN | MO_TESL));
+    return gen_amo(ctx, a, &tcg_gen_atomic_fetch_smax_tl,
+                   (MO_ALIGN | MO_WRITE_FAULT | MO_TESL));
 }
 
 static bool trans_amominu_w(DisasContext *ctx, arg_amominu_w *a)
 {
     REQUIRE_EXT(ctx, RVA);
-    return gen_amo(ctx, a, &tcg_gen_atomic_fetch_umin_tl, (MO_ALIGN | MO_TESL));
+    return gen_amo(ctx, a, &tcg_gen_atomic_fetch_umin_tl,
+                   (MO_ALIGN | MO_WRITE_FAULT | MO_TESL));
 }
 
 static bool trans_amomaxu_w(DisasContext *ctx, arg_amomaxu_w *a)
 {
     REQUIRE_EXT(ctx, RVA);
-    return gen_amo(ctx, a, &tcg_gen_atomic_fetch_umax_tl, (MO_ALIGN | MO_TESL));
+    return gen_amo(ctx, a, &tcg_gen_atomic_fetch_umax_tl,
+                   (MO_ALIGN | MO_WRITE_FAULT | MO_TESL));
 }
 
 static bool trans_lr_d(DisasContext *ctx, arg_lr_d *a)
@@ -171,53 +180,62 @@ static bool trans_sc_d(DisasContext *ctx, arg_sc_d *a)
 static bool trans_amoswap_d(DisasContext *ctx, arg_amoswap_d *a)
 {
     REQUIRE_64BIT(ctx);
-    return gen_amo(ctx, a, &tcg_gen_atomic_xchg_tl, (MO_ALIGN | MO_TEUQ));
+    return gen_amo(ctx, a, &tcg_gen_atomic_xchg_tl,
+                   (MO_ALIGN | MO_WRITE_FAULT | MO_TEUQ));
 }
 
 static bool trans_amoadd_d(DisasContext *ctx, arg_amoadd_d *a)
 {
     REQUIRE_64BIT(ctx);
-    return gen_amo(ctx, a, &tcg_gen_atomic_fetch_add_tl, (MO_ALIGN | MO_TEUQ));
+    return gen_amo(ctx, a, &tcg_gen_atomic_fetch_add_tl,
+                   (MO_ALIGN | MO_WRITE_FAULT | MO_TEUQ));
 }
 
 static bool trans_amoxor_d(DisasContext *ctx, arg_amoxor_d *a)
 {
     REQUIRE_64BIT(ctx);
-    return gen_amo(ctx, a, &tcg_gen_atomic_fetch_xor_tl, (MO_ALIGN | MO_TEUQ));
+    return gen_amo(ctx, a, &tcg_gen_atomic_fetch_xor_tl,
+                   (MO_ALIGN | MO_WRITE_FAULT | MO_TEUQ));
 }
 
 static bool trans_amoand_d(DisasContext *ctx, arg_amoand_d *a)
 {
     REQUIRE_64BIT(ctx);
-    return gen_amo(ctx, a, &tcg_gen_atomic_fetch_and_tl, (MO_ALIGN | MO_TEUQ));
+    return gen_amo(ctx, a, &tcg_gen_atomic_fetch_and_tl,
+                   (MO_ALIGN | MO_WRITE_FAULT | MO_TEUQ));
 }
 
 static bool trans_amoor_d(DisasContext *ctx, arg_amoor_d *a)
 {
     REQUIRE_64BIT(ctx);
-    return gen_amo(ctx, a, &tcg_gen_atomic_fetch_or_tl, (MO_ALIGN | MO_TEUQ));
+    return gen_amo(ctx, a, &tcg_gen_atomic_fetch_or_tl,
+                   (MO_ALIGN | MO_WRITE_FAULT | MO_TEUQ));
 }
 
 static bool trans_amomin_d(DisasContext *ctx, arg_amomin_d *a)
 {
     REQUIRE_64BIT(ctx);
-    return gen_amo(ctx, a, &tcg_gen_atomic_fetch_smin_tl, (MO_ALIGN | MO_TEUQ));
+    return gen_amo(ctx, a, &tcg_gen_atomic_fetch_smin_tl,
+                   (MO_ALIGN | MO_WRITE_FAULT | MO_TEUQ));
 }
 
 static bool trans_amomax_d(DisasContext *ctx, arg_amomax_d *a)
 {
     REQUIRE_64BIT(ctx);
-    return gen_amo(ctx, a, &tcg_gen_atomic_fetch_smax_tl, (MO_ALIGN | MO_TEUQ));
+    return gen_amo(ctx, a, &tcg_gen_atomic_fetch_smax_tl,
+                   (MO_ALIGN | MO_WRITE_FAULT | MO_TEUQ));
 }
 
 static bool trans_amominu_d(DisasContext *ctx, arg_amominu_d *a)
 {
     REQUIRE_64BIT(ctx);
-    return gen_amo(ctx, a, &tcg_gen_atomic_fetch_umin_tl, (MO_ALIGN | MO_TEUQ));
+    return gen_amo(ctx, a, &tcg_gen_atomic_fetch_umin_tl,
+                   (MO_ALIGN | MO_WRITE_FAULT | MO_TEUQ));
 }
 
 static bool trans_amomaxu_d(DisasContext *ctx, arg_amomaxu_d *a)
 {
     REQUIRE_64BIT(ctx);
-    return gen_amo(ctx, a, &tcg_gen_atomic_fetch_umax_tl, (MO_ALIGN | MO_TEUQ));
+    return gen_amo(ctx, a, &tcg_gen_atomic_fetch_umax_tl,
+                   (MO_ALIGN | MO_WRITE_FAULT | MO_TEUQ));
 }
-- 
2.31.1

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

[LIU Zhiwei]

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(-)
>

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

[LIU Zhiwei]

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(-)
>

Re: [PATCH 2/2] targett/riscv: rva: Correctly generate a store/amo fault

[LIU Zhiwei]

On 2022/1/24 上午8:59, Alistair Francis wrote:
> From: Alistair Francis <alistair.francis@wdc.com>
>
> If the atomic operation fails we want to generate a MMU_DATA_STORE
> access type so we can produce a RISCV_EXCP_STORE_AMO_ACCESS_FAULT for
> the guest.
>
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/594
> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> ---
>   target/riscv/insn_trans/trans_rva.c.inc | 56 ++++++++++++++++---------
>   1 file changed, 37 insertions(+), 19 deletions(-)
>
> diff --git a/target/riscv/insn_trans/trans_rva.c.inc b/target/riscv/insn_trans/trans_rva.c.inc
> index 45db82c9be..be5c94803b 100644
> --- a/target/riscv/insn_trans/trans_rva.c.inc
> +++ b/target/riscv/insn_trans/trans_rva.c.inc
> @@ -93,7 +93,7 @@ static bool gen_amo(DisasContext *ctx, arg_atomic *a,
>   static bool trans_lr_w(DisasContext *ctx, arg_lr_w *a)
>   {
>       REQUIRE_EXT(ctx, RVA);
> -    return gen_lr(ctx, a, (MO_ALIGN | MO_TESL));
> +    return gen_lr(ctx, a, (MO_ALIGN  | MO_TESL));

Typo.  This changes nothing.

Besides, I think we can add MO_WRITE_FAULT for SC in this patch or 
another patch.

Otherwise,

Reviewed-by: LIU Zhiwei<zhiwei_liu@c-sky.com>

Thanks,
Zhiwei

>   }
>   
>   static bool trans_sc_w(DisasContext *ctx, arg_sc_w *a)
> @@ -105,55 +105,64 @@ static bool trans_sc_w(DisasContext *ctx, arg_sc_w *a)
>   static bool trans_amoswap_w(DisasContext *ctx, arg_amoswap_w *a)
>   {
>       REQUIRE_EXT(ctx, RVA);
> -    return gen_amo(ctx, a, &tcg_gen_atomic_xchg_tl, (MO_ALIGN | MO_TESL));
> +    return gen_amo(ctx, a, &tcg_gen_atomic_xchg_tl,
> +                   (MO_ALIGN | MO_WRITE_FAULT | MO_TESL));
>   }
>   
>   static bool trans_amoadd_w(DisasContext *ctx, arg_amoadd_w *a)
>   {
>       REQUIRE_EXT(ctx, RVA);
> -    return gen_amo(ctx, a, &tcg_gen_atomic_fetch_add_tl, (MO_ALIGN | MO_TESL));
> +    return gen_amo(ctx, a, &tcg_gen_atomic_fetch_add_tl,
> +                   (MO_ALIGN | MO_WRITE_FAULT | MO_TESL));
>   }
>   
>   static bool trans_amoxor_w(DisasContext *ctx, arg_amoxor_w *a)
>   {
>       REQUIRE_EXT(ctx, RVA);
> -    return gen_amo(ctx, a, &tcg_gen_atomic_fetch_xor_tl, (MO_ALIGN | MO_TESL));
> +    return gen_amo(ctx, a, &tcg_gen_atomic_fetch_xor_tl,
> +                   (MO_ALIGN | MO_WRITE_FAULT | MO_TESL));
>   }
>   
>   static bool trans_amoand_w(DisasContext *ctx, arg_amoand_w *a)
>   {
>       REQUIRE_EXT(ctx, RVA);
> -    return gen_amo(ctx, a, &tcg_gen_atomic_fetch_and_tl, (MO_ALIGN | MO_TESL));
> +    return gen_amo(ctx, a, &tcg_gen_atomic_fetch_and_tl,
> +                   (MO_ALIGN | MO_WRITE_FAULT | MO_TESL));
>   }
>   
>   static bool trans_amoor_w(DisasContext *ctx, arg_amoor_w *a)
>   {
>       REQUIRE_EXT(ctx, RVA);
> -    return gen_amo(ctx, a, &tcg_gen_atomic_fetch_or_tl, (MO_ALIGN | MO_TESL));
> +    return gen_amo(ctx, a, &tcg_gen_atomic_fetch_or_tl,
> +                   (MO_ALIGN | MO_WRITE_FAULT | MO_TESL));
>   }
>   
>   static bool trans_amomin_w(DisasContext *ctx, arg_amomin_w *a)
>   {
>       REQUIRE_EXT(ctx, RVA);
> -    return gen_amo(ctx, a, &tcg_gen_atomic_fetch_smin_tl, (MO_ALIGN | MO_TESL));
> +    return gen_amo(ctx, a, &tcg_gen_atomic_fetch_smin_tl,
> +                   (MO_ALIGN | MO_WRITE_FAULT | MO_TESL));
>   }
>   
>   static bool trans_amomax_w(DisasContext *ctx, arg_amomax_w *a)
>   {
>       REQUIRE_EXT(ctx, RVA);
> -    return gen_amo(ctx, a, &tcg_gen_atomic_fetch_smax_tl, (MO_ALIGN | MO_TESL));
> +    return gen_amo(ctx, a, &tcg_gen_atomic_fetch_smax_tl,
> +                   (MO_ALIGN | MO_WRITE_FAULT | MO_TESL));
>   }
>   
>   static bool trans_amominu_w(DisasContext *ctx, arg_amominu_w *a)
>   {
>       REQUIRE_EXT(ctx, RVA);
> -    return gen_amo(ctx, a, &tcg_gen_atomic_fetch_umin_tl, (MO_ALIGN | MO_TESL));
> +    return gen_amo(ctx, a, &tcg_gen_atomic_fetch_umin_tl,
> +                   (MO_ALIGN | MO_WRITE_FAULT | MO_TESL));
>   }
>   
>   static bool trans_amomaxu_w(DisasContext *ctx, arg_amomaxu_w *a)
>   {
>       REQUIRE_EXT(ctx, RVA);
> -    return gen_amo(ctx, a, &tcg_gen_atomic_fetch_umax_tl, (MO_ALIGN | MO_TESL));
> +    return gen_amo(ctx, a, &tcg_gen_atomic_fetch_umax_tl,
> +                   (MO_ALIGN | MO_WRITE_FAULT | MO_TESL));
>   }
>   
>   static bool trans_lr_d(DisasContext *ctx, arg_lr_d *a)
> @@ -171,53 +180,62 @@ static bool trans_sc_d(DisasContext *ctx, arg_sc_d *a)
>   static bool trans_amoswap_d(DisasContext *ctx, arg_amoswap_d *a)
>   {
>       REQUIRE_64BIT(ctx);
> -    return gen_amo(ctx, a, &tcg_gen_atomic_xchg_tl, (MO_ALIGN | MO_TEUQ));
> +    return gen_amo(ctx, a, &tcg_gen_atomic_xchg_tl,
> +                   (MO_ALIGN | MO_WRITE_FAULT | MO_TEUQ));
>   }
>   
>   static bool trans_amoadd_d(DisasContext *ctx, arg_amoadd_d *a)
>   {
>       REQUIRE_64BIT(ctx);
> -    return gen_amo(ctx, a, &tcg_gen_atomic_fetch_add_tl, (MO_ALIGN | MO_TEUQ));
> +    return gen_amo(ctx, a, &tcg_gen_atomic_fetch_add_tl,
> +                   (MO_ALIGN | MO_WRITE_FAULT | MO_TEUQ));
>   }
>   
>   static bool trans_amoxor_d(DisasContext *ctx, arg_amoxor_d *a)
>   {
>       REQUIRE_64BIT(ctx);
> -    return gen_amo(ctx, a, &tcg_gen_atomic_fetch_xor_tl, (MO_ALIGN | MO_TEUQ));
> +    return gen_amo(ctx, a, &tcg_gen_atomic_fetch_xor_tl,
> +                   (MO_ALIGN | MO_WRITE_FAULT | MO_TEUQ));
>   }
>   
>   static bool trans_amoand_d(DisasContext *ctx, arg_amoand_d *a)
>   {
>       REQUIRE_64BIT(ctx);
> -    return gen_amo(ctx, a, &tcg_gen_atomic_fetch_and_tl, (MO_ALIGN | MO_TEUQ));
> +    return gen_amo(ctx, a, &tcg_gen_atomic_fetch_and_tl,
> +                   (MO_ALIGN | MO_WRITE_FAULT | MO_TEUQ));
>   }
>   
>   static bool trans_amoor_d(DisasContext *ctx, arg_amoor_d *a)
>   {
>       REQUIRE_64BIT(ctx);
> -    return gen_amo(ctx, a, &tcg_gen_atomic_fetch_or_tl, (MO_ALIGN | MO_TEUQ));
> +    return gen_amo(ctx, a, &tcg_gen_atomic_fetch_or_tl,
> +                   (MO_ALIGN | MO_WRITE_FAULT | MO_TEUQ));
>   }
>   
>   static bool trans_amomin_d(DisasContext *ctx, arg_amomin_d *a)
>   {
>       REQUIRE_64BIT(ctx);
> -    return gen_amo(ctx, a, &tcg_gen_atomic_fetch_smin_tl, (MO_ALIGN | MO_TEUQ));
> +    return gen_amo(ctx, a, &tcg_gen_atomic_fetch_smin_tl,
> +                   (MO_ALIGN | MO_WRITE_FAULT | MO_TEUQ));
>   }
>   
>   static bool trans_amomax_d(DisasContext *ctx, arg_amomax_d *a)
>   {
>       REQUIRE_64BIT(ctx);
> -    return gen_amo(ctx, a, &tcg_gen_atomic_fetch_smax_tl, (MO_ALIGN | MO_TEUQ));
> +    return gen_amo(ctx, a, &tcg_gen_atomic_fetch_smax_tl,
> +                   (MO_ALIGN | MO_WRITE_FAULT | MO_TEUQ));
>   }
>   
>   static bool trans_amominu_d(DisasContext *ctx, arg_amominu_d *a)
>   {
>       REQUIRE_64BIT(ctx);
> -    return gen_amo(ctx, a, &tcg_gen_atomic_fetch_umin_tl, (MO_ALIGN | MO_TEUQ));
> +    return gen_amo(ctx, a, &tcg_gen_atomic_fetch_umin_tl,
> +                   (MO_ALIGN | MO_WRITE_FAULT | MO_TEUQ));
>   }
>   
>   static bool trans_amomaxu_d(DisasContext *ctx, arg_amomaxu_d *a)
>   {
>       REQUIRE_64BIT(ctx);
> -    return gen_amo(ctx, a, &tcg_gen_atomic_fetch_umax_tl, (MO_ALIGN | MO_TEUQ));
> +    return gen_amo(ctx, a, &tcg_gen_atomic_fetch_umax_tl,
> +                   (MO_ALIGN | MO_WRITE_FAULT | MO_TEUQ));
>   }

Re: [PATCH 2/2] targett/riscv: rva: Correctly generate a store/amo fault

[LIU Zhiwei]

On 2022/1/24 上午8:59, Alistair Francis wrote:
> From: Alistair Francis <alistair.francis@wdc.com>
>
> If the atomic operation fails we want to generate a MMU_DATA_STORE
> access type so we can produce a RISCV_EXCP_STORE_AMO_ACCESS_FAULT for
> the guest.
>
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/594
> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> ---
>   target/riscv/insn_trans/trans_rva.c.inc | 56 ++++++++++++++++---------
>   1 file changed, 37 insertions(+), 19 deletions(-)
>
> diff --git a/target/riscv/insn_trans/trans_rva.c.inc b/target/riscv/insn_trans/trans_rva.c.inc
> index 45db82c9be..be5c94803b 100644
> --- a/target/riscv/insn_trans/trans_rva.c.inc
> +++ b/target/riscv/insn_trans/trans_rva.c.inc
> @@ -93,7 +93,7 @@ static bool gen_amo(DisasContext *ctx, arg_atomic *a,
>   static bool trans_lr_w(DisasContext *ctx, arg_lr_w *a)
>   {
>       REQUIRE_EXT(ctx, RVA);
> -    return gen_lr(ctx, a, (MO_ALIGN | MO_TESL));
> +    return gen_lr(ctx, a, (MO_ALIGN  | MO_TESL));

Typo.  This changes nothing.

Besides, I think we can add MO_WRITE_FAULT for SC in this patch or 
another patch.

Otherwise,

Reviewed-by: LIU Zhiwei<zhiwei_liu@c-sky.com>

Thanks,
Zhiwei

>   }
>   
>   static bool trans_sc_w(DisasContext *ctx, arg_sc_w *a)
> @@ -105,55 +105,64 @@ static bool trans_sc_w(DisasContext *ctx, arg_sc_w *a)
>   static bool trans_amoswap_w(DisasContext *ctx, arg_amoswap_w *a)
>   {
>       REQUIRE_EXT(ctx, RVA);
> -    return gen_amo(ctx, a, &tcg_gen_atomic_xchg_tl, (MO_ALIGN | MO_TESL));
> +    return gen_amo(ctx, a, &tcg_gen_atomic_xchg_tl,
> +                   (MO_ALIGN | MO_WRITE_FAULT | MO_TESL));
>   }
>   
>   static bool trans_amoadd_w(DisasContext *ctx, arg_amoadd_w *a)
>   {
>       REQUIRE_EXT(ctx, RVA);
> -    return gen_amo(ctx, a, &tcg_gen_atomic_fetch_add_tl, (MO_ALIGN | MO_TESL));
> +    return gen_amo(ctx, a, &tcg_gen_atomic_fetch_add_tl,
> +                   (MO_ALIGN | MO_WRITE_FAULT | MO_TESL));
>   }
>   
>   static bool trans_amoxor_w(DisasContext *ctx, arg_amoxor_w *a)
>   {
>       REQUIRE_EXT(ctx, RVA);
> -    return gen_amo(ctx, a, &tcg_gen_atomic_fetch_xor_tl, (MO_ALIGN | MO_TESL));
> +    return gen_amo(ctx, a, &tcg_gen_atomic_fetch_xor_tl,
> +                   (MO_ALIGN | MO_WRITE_FAULT | MO_TESL));
>   }
>   
>   static bool trans_amoand_w(DisasContext *ctx, arg_amoand_w *a)
>   {
>       REQUIRE_EXT(ctx, RVA);
> -    return gen_amo(ctx, a, &tcg_gen_atomic_fetch_and_tl, (MO_ALIGN | MO_TESL));
> +    return gen_amo(ctx, a, &tcg_gen_atomic_fetch_and_tl,
> +                   (MO_ALIGN | MO_WRITE_FAULT | MO_TESL));
>   }
>   
>   static bool trans_amoor_w(DisasContext *ctx, arg_amoor_w *a)
>   {
>       REQUIRE_EXT(ctx, RVA);
> -    return gen_amo(ctx, a, &tcg_gen_atomic_fetch_or_tl, (MO_ALIGN | MO_TESL));
> +    return gen_amo(ctx, a, &tcg_gen_atomic_fetch_or_tl,
> +                   (MO_ALIGN | MO_WRITE_FAULT | MO_TESL));
>   }
>   
>   static bool trans_amomin_w(DisasContext *ctx, arg_amomin_w *a)
>   {
>       REQUIRE_EXT(ctx, RVA);
> -    return gen_amo(ctx, a, &tcg_gen_atomic_fetch_smin_tl, (MO_ALIGN | MO_TESL));
> +    return gen_amo(ctx, a, &tcg_gen_atomic_fetch_smin_tl,
> +                   (MO_ALIGN | MO_WRITE_FAULT | MO_TESL));
>   }
>   
>   static bool trans_amomax_w(DisasContext *ctx, arg_amomax_w *a)
>   {
>       REQUIRE_EXT(ctx, RVA);
> -    return gen_amo(ctx, a, &tcg_gen_atomic_fetch_smax_tl, (MO_ALIGN | MO_TESL));
> +    return gen_amo(ctx, a, &tcg_gen_atomic_fetch_smax_tl,
> +                   (MO_ALIGN | MO_WRITE_FAULT | MO_TESL));
>   }
>   
>   static bool trans_amominu_w(DisasContext *ctx, arg_amominu_w *a)
>   {
>       REQUIRE_EXT(ctx, RVA);
> -    return gen_amo(ctx, a, &tcg_gen_atomic_fetch_umin_tl, (MO_ALIGN | MO_TESL));
> +    return gen_amo(ctx, a, &tcg_gen_atomic_fetch_umin_tl,
> +                   (MO_ALIGN | MO_WRITE_FAULT | MO_TESL));
>   }
>   
>   static bool trans_amomaxu_w(DisasContext *ctx, arg_amomaxu_w *a)
>   {
>       REQUIRE_EXT(ctx, RVA);
> -    return gen_amo(ctx, a, &tcg_gen_atomic_fetch_umax_tl, (MO_ALIGN | MO_TESL));
> +    return gen_amo(ctx, a, &tcg_gen_atomic_fetch_umax_tl,
> +                   (MO_ALIGN | MO_WRITE_FAULT | MO_TESL));
>   }
>   
>   static bool trans_lr_d(DisasContext *ctx, arg_lr_d *a)
> @@ -171,53 +180,62 @@ static bool trans_sc_d(DisasContext *ctx, arg_sc_d *a)
>   static bool trans_amoswap_d(DisasContext *ctx, arg_amoswap_d *a)
>   {
>       REQUIRE_64BIT(ctx);
> -    return gen_amo(ctx, a, &tcg_gen_atomic_xchg_tl, (MO_ALIGN | MO_TEUQ));
> +    return gen_amo(ctx, a, &tcg_gen_atomic_xchg_tl,
> +                   (MO_ALIGN | MO_WRITE_FAULT | MO_TEUQ));
>   }
>   
>   static bool trans_amoadd_d(DisasContext *ctx, arg_amoadd_d *a)
>   {
>       REQUIRE_64BIT(ctx);
> -    return gen_amo(ctx, a, &tcg_gen_atomic_fetch_add_tl, (MO_ALIGN | MO_TEUQ));
> +    return gen_amo(ctx, a, &tcg_gen_atomic_fetch_add_tl,
> +                   (MO_ALIGN | MO_WRITE_FAULT | MO_TEUQ));
>   }
>   
>   static bool trans_amoxor_d(DisasContext *ctx, arg_amoxor_d *a)
>   {
>       REQUIRE_64BIT(ctx);
> -    return gen_amo(ctx, a, &tcg_gen_atomic_fetch_xor_tl, (MO_ALIGN | MO_TEUQ));
> +    return gen_amo(ctx, a, &tcg_gen_atomic_fetch_xor_tl,
> +                   (MO_ALIGN | MO_WRITE_FAULT | MO_TEUQ));
>   }
>   
>   static bool trans_amoand_d(DisasContext *ctx, arg_amoand_d *a)
>   {
>       REQUIRE_64BIT(ctx);
> -    return gen_amo(ctx, a, &tcg_gen_atomic_fetch_and_tl, (MO_ALIGN | MO_TEUQ));
> +    return gen_amo(ctx, a, &tcg_gen_atomic_fetch_and_tl,
> +                   (MO_ALIGN | MO_WRITE_FAULT | MO_TEUQ));
>   }
>   
>   static bool trans_amoor_d(DisasContext *ctx, arg_amoor_d *a)
>   {
>       REQUIRE_64BIT(ctx);
> -    return gen_amo(ctx, a, &tcg_gen_atomic_fetch_or_tl, (MO_ALIGN | MO_TEUQ));
> +    return gen_amo(ctx, a, &tcg_gen_atomic_fetch_or_tl,
> +                   (MO_ALIGN | MO_WRITE_FAULT | MO_TEUQ));
>   }
>   
>   static bool trans_amomin_d(DisasContext *ctx, arg_amomin_d *a)
>   {
>       REQUIRE_64BIT(ctx);
> -    return gen_amo(ctx, a, &tcg_gen_atomic_fetch_smin_tl, (MO_ALIGN | MO_TEUQ));
> +    return gen_amo(ctx, a, &tcg_gen_atomic_fetch_smin_tl,
> +                   (MO_ALIGN | MO_WRITE_FAULT | MO_TEUQ));
>   }
>   
>   static bool trans_amomax_d(DisasContext *ctx, arg_amomax_d *a)
>   {
>       REQUIRE_64BIT(ctx);
> -    return gen_amo(ctx, a, &tcg_gen_atomic_fetch_smax_tl, (MO_ALIGN | MO_TEUQ));
> +    return gen_amo(ctx, a, &tcg_gen_atomic_fetch_smax_tl,
> +                   (MO_ALIGN | MO_WRITE_FAULT | MO_TEUQ));
>   }
>   
>   static bool trans_amominu_d(DisasContext *ctx, arg_amominu_d *a)
>   {
>       REQUIRE_64BIT(ctx);
> -    return gen_amo(ctx, a, &tcg_gen_atomic_fetch_umin_tl, (MO_ALIGN | MO_TEUQ));
> +    return gen_amo(ctx, a, &tcg_gen_atomic_fetch_umin_tl,
> +                   (MO_ALIGN | MO_WRITE_FAULT | MO_TEUQ));
>   }
>   
>   static bool trans_amomaxu_d(DisasContext *ctx, arg_amomaxu_d *a)
>   {
>       REQUIRE_64BIT(ctx);
> -    return gen_amo(ctx, a, &tcg_gen_atomic_fetch_umax_tl, (MO_ALIGN | MO_TEUQ));
> +    return gen_amo(ctx, a, &tcg_gen_atomic_fetch_umax_tl,
> +                   (MO_ALIGN | MO_WRITE_FAULT | MO_TEUQ));
>   }

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~

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~

Re: [PATCH 2/2] targett/riscv: rva: Correctly generate a store/amo fault

[Weiwei Li]

It seems that target is miswritten to "targett"  in commit message.

Regards,

Weiwei Li

在 2022/1/24 上午8:59, Alistair Francis 写道:
> From: Alistair Francis <alistair.francis@wdc.com>
>
> If the atomic operation fails we want to generate a MMU_DATA_STORE
> access type so we can produce a RISCV_EXCP_STORE_AMO_ACCESS_FAULT for
> the guest.
>
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/594
> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> ---
>   target/riscv/insn_trans/trans_rva.c.inc | 56 ++++++++++++++++---------
>   1 file changed, 37 insertions(+), 19 deletions(-)
>
> diff --git a/target/riscv/insn_trans/trans_rva.c.inc b/target/riscv/insn_trans/trans_rva.c.inc
> index 45db82c9be..be5c94803b 100644
> --- a/target/riscv/insn_trans/trans_rva.c.inc
> +++ b/target/riscv/insn_trans/trans_rva.c.inc
> @@ -93,7 +93,7 @@ static bool gen_amo(DisasContext *ctx, arg_atomic *a,
>   static bool trans_lr_w(DisasContext *ctx, arg_lr_w *a)
>   {
>       REQUIRE_EXT(ctx, RVA);
> -    return gen_lr(ctx, a, (MO_ALIGN | MO_TESL));
> +    return gen_lr(ctx, a, (MO_ALIGN  | MO_TESL));
>   }
>   
>   static bool trans_sc_w(DisasContext *ctx, arg_sc_w *a)
> @@ -105,55 +105,64 @@ static bool trans_sc_w(DisasContext *ctx, arg_sc_w *a)
>   static bool trans_amoswap_w(DisasContext *ctx, arg_amoswap_w *a)
>   {
>       REQUIRE_EXT(ctx, RVA);
> -    return gen_amo(ctx, a, &tcg_gen_atomic_xchg_tl, (MO_ALIGN | MO_TESL));
> +    return gen_amo(ctx, a, &tcg_gen_atomic_xchg_tl,
> +                   (MO_ALIGN | MO_WRITE_FAULT | MO_TESL));
>   }
>   
>   static bool trans_amoadd_w(DisasContext *ctx, arg_amoadd_w *a)
>   {
>       REQUIRE_EXT(ctx, RVA);
> -    return gen_amo(ctx, a, &tcg_gen_atomic_fetch_add_tl, (MO_ALIGN | MO_TESL));
> +    return gen_amo(ctx, a, &tcg_gen_atomic_fetch_add_tl,
> +                   (MO_ALIGN | MO_WRITE_FAULT | MO_TESL));
>   }
>   
>   static bool trans_amoxor_w(DisasContext *ctx, arg_amoxor_w *a)
>   {
>       REQUIRE_EXT(ctx, RVA);
> -    return gen_amo(ctx, a, &tcg_gen_atomic_fetch_xor_tl, (MO_ALIGN | MO_TESL));
> +    return gen_amo(ctx, a, &tcg_gen_atomic_fetch_xor_tl,
> +                   (MO_ALIGN | MO_WRITE_FAULT | MO_TESL));
>   }
>   
>   static bool trans_amoand_w(DisasContext *ctx, arg_amoand_w *a)
>   {
>       REQUIRE_EXT(ctx, RVA);
> -    return gen_amo(ctx, a, &tcg_gen_atomic_fetch_and_tl, (MO_ALIGN | MO_TESL));
> +    return gen_amo(ctx, a, &tcg_gen_atomic_fetch_and_tl,
> +                   (MO_ALIGN | MO_WRITE_FAULT | MO_TESL));
>   }
>   
>   static bool trans_amoor_w(DisasContext *ctx, arg_amoor_w *a)
>   {
>       REQUIRE_EXT(ctx, RVA);
> -    return gen_amo(ctx, a, &tcg_gen_atomic_fetch_or_tl, (MO_ALIGN | MO_TESL));
> +    return gen_amo(ctx, a, &tcg_gen_atomic_fetch_or_tl,
> +                   (MO_ALIGN | MO_WRITE_FAULT | MO_TESL));
>   }
>   
>   static bool trans_amomin_w(DisasContext *ctx, arg_amomin_w *a)
>   {
>       REQUIRE_EXT(ctx, RVA);
> -    return gen_amo(ctx, a, &tcg_gen_atomic_fetch_smin_tl, (MO_ALIGN | MO_TESL));
> +    return gen_amo(ctx, a, &tcg_gen_atomic_fetch_smin_tl,
> +                   (MO_ALIGN | MO_WRITE_FAULT | MO_TESL));
>   }
>   
>   static bool trans_amomax_w(DisasContext *ctx, arg_amomax_w *a)
>   {
>       REQUIRE_EXT(ctx, RVA);
> -    return gen_amo(ctx, a, &tcg_gen_atomic_fetch_smax_tl, (MO_ALIGN | MO_TESL));
> +    return gen_amo(ctx, a, &tcg_gen_atomic_fetch_smax_tl,
> +                   (MO_ALIGN | MO_WRITE_FAULT | MO_TESL));
>   }
>   
>   static bool trans_amominu_w(DisasContext *ctx, arg_amominu_w *a)
>   {
>       REQUIRE_EXT(ctx, RVA);
> -    return gen_amo(ctx, a, &tcg_gen_atomic_fetch_umin_tl, (MO_ALIGN | MO_TESL));
> +    return gen_amo(ctx, a, &tcg_gen_atomic_fetch_umin_tl,
> +                   (MO_ALIGN | MO_WRITE_FAULT | MO_TESL));
>   }
>   
>   static bool trans_amomaxu_w(DisasContext *ctx, arg_amomaxu_w *a)
>   {
>       REQUIRE_EXT(ctx, RVA);
> -    return gen_amo(ctx, a, &tcg_gen_atomic_fetch_umax_tl, (MO_ALIGN | MO_TESL));
> +    return gen_amo(ctx, a, &tcg_gen_atomic_fetch_umax_tl,
> +                   (MO_ALIGN | MO_WRITE_FAULT | MO_TESL));
>   }
>   
>   static bool trans_lr_d(DisasContext *ctx, arg_lr_d *a)
> @@ -171,53 +180,62 @@ static bool trans_sc_d(DisasContext *ctx, arg_sc_d *a)
>   static bool trans_amoswap_d(DisasContext *ctx, arg_amoswap_d *a)
>   {
>       REQUIRE_64BIT(ctx);
> -    return gen_amo(ctx, a, &tcg_gen_atomic_xchg_tl, (MO_ALIGN | MO_TEUQ));
> +    return gen_amo(ctx, a, &tcg_gen_atomic_xchg_tl,
> +                   (MO_ALIGN | MO_WRITE_FAULT | MO_TEUQ));
>   }
>   
>   static bool trans_amoadd_d(DisasContext *ctx, arg_amoadd_d *a)
>   {
>       REQUIRE_64BIT(ctx);
> -    return gen_amo(ctx, a, &tcg_gen_atomic_fetch_add_tl, (MO_ALIGN | MO_TEUQ));
> +    return gen_amo(ctx, a, &tcg_gen_atomic_fetch_add_tl,
> +                   (MO_ALIGN | MO_WRITE_FAULT | MO_TEUQ));
>   }
>   
>   static bool trans_amoxor_d(DisasContext *ctx, arg_amoxor_d *a)
>   {
>       REQUIRE_64BIT(ctx);
> -    return gen_amo(ctx, a, &tcg_gen_atomic_fetch_xor_tl, (MO_ALIGN | MO_TEUQ));
> +    return gen_amo(ctx, a, &tcg_gen_atomic_fetch_xor_tl,
> +                   (MO_ALIGN | MO_WRITE_FAULT | MO_TEUQ));
>   }
>   
>   static bool trans_amoand_d(DisasContext *ctx, arg_amoand_d *a)
>   {
>       REQUIRE_64BIT(ctx);
> -    return gen_amo(ctx, a, &tcg_gen_atomic_fetch_and_tl, (MO_ALIGN | MO_TEUQ));
> +    return gen_amo(ctx, a, &tcg_gen_atomic_fetch_and_tl,
> +                   (MO_ALIGN | MO_WRITE_FAULT | MO_TEUQ));
>   }
>   
>   static bool trans_amoor_d(DisasContext *ctx, arg_amoor_d *a)
>   {
>       REQUIRE_64BIT(ctx);
> -    return gen_amo(ctx, a, &tcg_gen_atomic_fetch_or_tl, (MO_ALIGN | MO_TEUQ));
> +    return gen_amo(ctx, a, &tcg_gen_atomic_fetch_or_tl,
> +                   (MO_ALIGN | MO_WRITE_FAULT | MO_TEUQ));
>   }
>   
>   static bool trans_amomin_d(DisasContext *ctx, arg_amomin_d *a)
>   {
>       REQUIRE_64BIT(ctx);
> -    return gen_amo(ctx, a, &tcg_gen_atomic_fetch_smin_tl, (MO_ALIGN | MO_TEUQ));
> +    return gen_amo(ctx, a, &tcg_gen_atomic_fetch_smin_tl,
> +                   (MO_ALIGN | MO_WRITE_FAULT | MO_TEUQ));
>   }
>   
>   static bool trans_amomax_d(DisasContext *ctx, arg_amomax_d *a)
>   {
>       REQUIRE_64BIT(ctx);
> -    return gen_amo(ctx, a, &tcg_gen_atomic_fetch_smax_tl, (MO_ALIGN | MO_TEUQ));
> +    return gen_amo(ctx, a, &tcg_gen_atomic_fetch_smax_tl,
> +                   (MO_ALIGN | MO_WRITE_FAULT | MO_TEUQ));
>   }
>   
>   static bool trans_amominu_d(DisasContext *ctx, arg_amominu_d *a)
>   {
>       REQUIRE_64BIT(ctx);
> -    return gen_amo(ctx, a, &tcg_gen_atomic_fetch_umin_tl, (MO_ALIGN | MO_TEUQ));
> +    return gen_amo(ctx, a, &tcg_gen_atomic_fetch_umin_tl,
> +                   (MO_ALIGN | MO_WRITE_FAULT | MO_TEUQ));
>   }
>   
>   static bool trans_amomaxu_d(DisasContext *ctx, arg_amomaxu_d *a)
>   {
>       REQUIRE_64BIT(ctx);
> -    return gen_amo(ctx, a, &tcg_gen_atomic_fetch_umax_tl, (MO_ALIGN | MO_TEUQ));
> +    return gen_amo(ctx, a, &tcg_gen_atomic_fetch_umax_tl,
> +                   (MO_ALIGN | MO_WRITE_FAULT | MO_TEUQ));
>   }

Re: [PATCH 2/2] targett/riscv: rva: Correctly generate a store/amo fault

[Weiwei Li]

It seems that target is miswritten to "targett"  in commit message.

Regards,

Weiwei Li

在 2022/1/24 上午8:59, Alistair Francis 写道:
> From: Alistair Francis <alistair.francis@wdc.com>
>
> If the atomic operation fails we want to generate a MMU_DATA_STORE
> access type so we can produce a RISCV_EXCP_STORE_AMO_ACCESS_FAULT for
> the guest.
>
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/594
> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> ---
>   target/riscv/insn_trans/trans_rva.c.inc | 56 ++++++++++++++++---------
>   1 file changed, 37 insertions(+), 19 deletions(-)
>
> diff --git a/target/riscv/insn_trans/trans_rva.c.inc b/target/riscv/insn_trans/trans_rva.c.inc
> index 45db82c9be..be5c94803b 100644
> --- a/target/riscv/insn_trans/trans_rva.c.inc
> +++ b/target/riscv/insn_trans/trans_rva.c.inc
> @@ -93,7 +93,7 @@ static bool gen_amo(DisasContext *ctx, arg_atomic *a,
>   static bool trans_lr_w(DisasContext *ctx, arg_lr_w *a)
>   {
>       REQUIRE_EXT(ctx, RVA);
> -    return gen_lr(ctx, a, (MO_ALIGN | MO_TESL));
> +    return gen_lr(ctx, a, (MO_ALIGN  | MO_TESL));
>   }
>   
>   static bool trans_sc_w(DisasContext *ctx, arg_sc_w *a)
> @@ -105,55 +105,64 @@ static bool trans_sc_w(DisasContext *ctx, arg_sc_w *a)
>   static bool trans_amoswap_w(DisasContext *ctx, arg_amoswap_w *a)
>   {
>       REQUIRE_EXT(ctx, RVA);
> -    return gen_amo(ctx, a, &tcg_gen_atomic_xchg_tl, (MO_ALIGN | MO_TESL));
> +    return gen_amo(ctx, a, &tcg_gen_atomic_xchg_tl,
> +                   (MO_ALIGN | MO_WRITE_FAULT | MO_TESL));
>   }
>   
>   static bool trans_amoadd_w(DisasContext *ctx, arg_amoadd_w *a)
>   {
>       REQUIRE_EXT(ctx, RVA);
> -    return gen_amo(ctx, a, &tcg_gen_atomic_fetch_add_tl, (MO_ALIGN | MO_TESL));
> +    return gen_amo(ctx, a, &tcg_gen_atomic_fetch_add_tl,
> +                   (MO_ALIGN | MO_WRITE_FAULT | MO_TESL));
>   }
>   
>   static bool trans_amoxor_w(DisasContext *ctx, arg_amoxor_w *a)
>   {
>       REQUIRE_EXT(ctx, RVA);
> -    return gen_amo(ctx, a, &tcg_gen_atomic_fetch_xor_tl, (MO_ALIGN | MO_TESL));
> +    return gen_amo(ctx, a, &tcg_gen_atomic_fetch_xor_tl,
> +                   (MO_ALIGN | MO_WRITE_FAULT | MO_TESL));
>   }
>   
>   static bool trans_amoand_w(DisasContext *ctx, arg_amoand_w *a)
>   {
>       REQUIRE_EXT(ctx, RVA);
> -    return gen_amo(ctx, a, &tcg_gen_atomic_fetch_and_tl, (MO_ALIGN | MO_TESL));
> +    return gen_amo(ctx, a, &tcg_gen_atomic_fetch_and_tl,
> +                   (MO_ALIGN | MO_WRITE_FAULT | MO_TESL));
>   }
>   
>   static bool trans_amoor_w(DisasContext *ctx, arg_amoor_w *a)
>   {
>       REQUIRE_EXT(ctx, RVA);
> -    return gen_amo(ctx, a, &tcg_gen_atomic_fetch_or_tl, (MO_ALIGN | MO_TESL));
> +    return gen_amo(ctx, a, &tcg_gen_atomic_fetch_or_tl,
> +                   (MO_ALIGN | MO_WRITE_FAULT | MO_TESL));
>   }
>   
>   static bool trans_amomin_w(DisasContext *ctx, arg_amomin_w *a)
>   {
>       REQUIRE_EXT(ctx, RVA);
> -    return gen_amo(ctx, a, &tcg_gen_atomic_fetch_smin_tl, (MO_ALIGN | MO_TESL));
> +    return gen_amo(ctx, a, &tcg_gen_atomic_fetch_smin_tl,
> +                   (MO_ALIGN | MO_WRITE_FAULT | MO_TESL));
>   }
>   
>   static bool trans_amomax_w(DisasContext *ctx, arg_amomax_w *a)
>   {
>       REQUIRE_EXT(ctx, RVA);
> -    return gen_amo(ctx, a, &tcg_gen_atomic_fetch_smax_tl, (MO_ALIGN | MO_TESL));
> +    return gen_amo(ctx, a, &tcg_gen_atomic_fetch_smax_tl,
> +                   (MO_ALIGN | MO_WRITE_FAULT | MO_TESL));
>   }
>   
>   static bool trans_amominu_w(DisasContext *ctx, arg_amominu_w *a)
>   {
>       REQUIRE_EXT(ctx, RVA);
> -    return gen_amo(ctx, a, &tcg_gen_atomic_fetch_umin_tl, (MO_ALIGN | MO_TESL));
> +    return gen_amo(ctx, a, &tcg_gen_atomic_fetch_umin_tl,
> +                   (MO_ALIGN | MO_WRITE_FAULT | MO_TESL));
>   }
>   
>   static bool trans_amomaxu_w(DisasContext *ctx, arg_amomaxu_w *a)
>   {
>       REQUIRE_EXT(ctx, RVA);
> -    return gen_amo(ctx, a, &tcg_gen_atomic_fetch_umax_tl, (MO_ALIGN | MO_TESL));
> +    return gen_amo(ctx, a, &tcg_gen_atomic_fetch_umax_tl,
> +                   (MO_ALIGN | MO_WRITE_FAULT | MO_TESL));
>   }
>   
>   static bool trans_lr_d(DisasContext *ctx, arg_lr_d *a)
> @@ -171,53 +180,62 @@ static bool trans_sc_d(DisasContext *ctx, arg_sc_d *a)
>   static bool trans_amoswap_d(DisasContext *ctx, arg_amoswap_d *a)
>   {
>       REQUIRE_64BIT(ctx);
> -    return gen_amo(ctx, a, &tcg_gen_atomic_xchg_tl, (MO_ALIGN | MO_TEUQ));
> +    return gen_amo(ctx, a, &tcg_gen_atomic_xchg_tl,
> +                   (MO_ALIGN | MO_WRITE_FAULT | MO_TEUQ));
>   }
>   
>   static bool trans_amoadd_d(DisasContext *ctx, arg_amoadd_d *a)
>   {
>       REQUIRE_64BIT(ctx);
> -    return gen_amo(ctx, a, &tcg_gen_atomic_fetch_add_tl, (MO_ALIGN | MO_TEUQ));
> +    return gen_amo(ctx, a, &tcg_gen_atomic_fetch_add_tl,
> +                   (MO_ALIGN | MO_WRITE_FAULT | MO_TEUQ));
>   }
>   
>   static bool trans_amoxor_d(DisasContext *ctx, arg_amoxor_d *a)
>   {
>       REQUIRE_64BIT(ctx);
> -    return gen_amo(ctx, a, &tcg_gen_atomic_fetch_xor_tl, (MO_ALIGN | MO_TEUQ));
> +    return gen_amo(ctx, a, &tcg_gen_atomic_fetch_xor_tl,
> +                   (MO_ALIGN | MO_WRITE_FAULT | MO_TEUQ));
>   }
>   
>   static bool trans_amoand_d(DisasContext *ctx, arg_amoand_d *a)
>   {
>       REQUIRE_64BIT(ctx);
> -    return gen_amo(ctx, a, &tcg_gen_atomic_fetch_and_tl, (MO_ALIGN | MO_TEUQ));
> +    return gen_amo(ctx, a, &tcg_gen_atomic_fetch_and_tl,
> +                   (MO_ALIGN | MO_WRITE_FAULT | MO_TEUQ));
>   }
>   
>   static bool trans_amoor_d(DisasContext *ctx, arg_amoor_d *a)
>   {
>       REQUIRE_64BIT(ctx);
> -    return gen_amo(ctx, a, &tcg_gen_atomic_fetch_or_tl, (MO_ALIGN | MO_TEUQ));
> +    return gen_amo(ctx, a, &tcg_gen_atomic_fetch_or_tl,
> +                   (MO_ALIGN | MO_WRITE_FAULT | MO_TEUQ));
>   }
>   
>   static bool trans_amomin_d(DisasContext *ctx, arg_amomin_d *a)
>   {
>       REQUIRE_64BIT(ctx);
> -    return gen_amo(ctx, a, &tcg_gen_atomic_fetch_smin_tl, (MO_ALIGN | MO_TEUQ));
> +    return gen_amo(ctx, a, &tcg_gen_atomic_fetch_smin_tl,
> +                   (MO_ALIGN | MO_WRITE_FAULT | MO_TEUQ));
>   }
>   
>   static bool trans_amomax_d(DisasContext *ctx, arg_amomax_d *a)
>   {
>       REQUIRE_64BIT(ctx);
> -    return gen_amo(ctx, a, &tcg_gen_atomic_fetch_smax_tl, (MO_ALIGN | MO_TEUQ));
> +    return gen_amo(ctx, a, &tcg_gen_atomic_fetch_smax_tl,
> +                   (MO_ALIGN | MO_WRITE_FAULT | MO_TEUQ));
>   }
>   
>   static bool trans_amominu_d(DisasContext *ctx, arg_amominu_d *a)
>   {
>       REQUIRE_64BIT(ctx);
> -    return gen_amo(ctx, a, &tcg_gen_atomic_fetch_umin_tl, (MO_ALIGN | MO_TEUQ));
> +    return gen_amo(ctx, a, &tcg_gen_atomic_fetch_umin_tl,
> +                   (MO_ALIGN | MO_WRITE_FAULT | MO_TEUQ));
>   }
>   
>   static bool trans_amomaxu_d(DisasContext *ctx, arg_amomaxu_d *a)
>   {
>       REQUIRE_64BIT(ctx);
> -    return gen_amo(ctx, a, &tcg_gen_atomic_fetch_umax_tl, (MO_ALIGN | MO_TEUQ));
> +    return gen_amo(ctx, a, &tcg_gen_atomic_fetch_umax_tl,
> +                   (MO_ALIGN | MO_WRITE_FAULT | MO_TEUQ));
>   }

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~

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~

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

[Richard Henderson]

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~

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

[Richard Henderson]

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~

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~

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~

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

[Richard Henderson]

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~

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

[Richard Henderson]

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~