[PATCH 0/2] target/riscv: write mstatus.GVA on every trap into M-mode

[PATCH 0/2] target/riscv: write mstatus.GVA on every trap into M-mode

[tewaro]

Hi,

Based-on: <cover.1793677944.git.tewaro@theorem.dev>
("target/riscv: fix mtinst/htinst for G-stage faults on VS-stage A/D
updates", whose hyp-guest.h and hyp-guest.S this test reuses)

Patch 1 fixes mstatus.GVA. On every trap into M-mode, GVA should be
set to 1 if mtval holds a guest virtual address and to 0 otherwise [1].
QEMU does not clear gva or set GVA for particular faults.

Both date back to 9034e90ad9. The fix sets mstatus.GVA from write_gva,
the value QEMU already computes for hstatus.GVA.

Patch 2 adds a test, all 4 pass on upstream Spike, all fail on master.

This bug was found while examining <cover.1793677944.git.tewaro@theorem.dev>
and writing a generalized test. Claude was used to diagnose the issue. All
commits were human written.

[1] Privileged ISA v1.12, 8.2.1, the description of GVA and the note
    under hstatus.GVA
https://github.com/riscv/riscv-isa-manual/releases/tag/Priv-v1.12

tewaro (2):
  target/riscv: write mstatus.GVA on every trap into M-mode
  tests/tcg/riscv64: test mstatus.GVA on traps into M-mode

 target/riscv/tcg/cpu_helper.c        |  4 +-
 tests/tcg/riscv64/system/meson.build | 21 ++++++++
 tests/tcg/riscv64/test-mstatus-gva.S | 75 ++++++++++++++++++++++++++++
 3 files changed, 97 insertions(+), 3 deletions(-)
 create mode 100644 tests/tcg/riscv64/test-mstatus-gva.S

-- 
2.54.0 (Apple Git-157)

[PATCH 1/2] target/riscv: write mstatus.GVA on every trap into M-mode

[tewaro]

On every trap into M-mode, mstatus.GVA should be set to 1 if mtval
holds a guest virtual address and to 0 otherwise [1].
riscv_cpu_do_interrupt() only ever sets it, and only when V = 1 and
tval is nonzero. That has two effects:

- GVA is never cleared. After one guest-page fault, every later trap
  into M-mode, including an ecall, reports GVA = 1.
- GVA stays 0 when HLV, HLVX or HSV faults, because those run with
  V = 0. The spec names this case ("SPV=0 but GVA=1").

The HS-mode path already computes write_gva for hstatus.GVA. Use it
for mstatus.GVA too.

[1] Privileged ISA v1.12, 8.2.1
https://github.com/riscv/riscv-isa-manual/releases/tag/Priv-v1.12

Resolves: https://gitlab.com/qemu-project/qemu/-/issues/4594
Signed-off-by: tewaro <tewaro@theorem.dev>
---
 target/riscv/tcg/cpu_helper.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/target/riscv/tcg/cpu_helper.c b/target/riscv/tcg/cpu_helper.c
index 063525cfc5..55b370294a 100644
--- a/target/riscv/tcg/cpu_helper.c
+++ b/target/riscv/tcg/cpu_helper.c
@@ -2416,9 +2416,7 @@ void riscv_cpu_do_interrupt(CPUState *cs)
             }
             env->mstatus = set_field(env->mstatus, MSTATUS_MPV,
                                      env->virt_enabled);
-            if (env->virt_enabled && tval) {
-                env->mstatus = set_field(env->mstatus, MSTATUS_GVA, 1);
-            }
+            env->mstatus = set_field(env->mstatus, MSTATUS_GVA, write_gva);
 
             mtval2 = env->guest_phys_fault_addr;
 
-- 
2.54.0 (Apple Git-157)

[PATCH 2/2] tests/tcg/riscv64: test mstatus.GVA on traps into M-mode

[tewaro]

Every trap into M-mode writes mstatus.GVA. If mtval holds a guest virtual
address then mstatus.GVA should be 1 otherwise it should be 0.

test-mstatus-gva checks 4 cases:

  case     access  from
  -------  ------  ----
  load     ld      VS
  load-vu  ld      VU
  hlv      hlv.d   HS
  hsv      hsv.d   HS

Each case makes a guest-page fault and checks that GVA is 1. It then
runs an ecall in M-mode, whose trap has no guest address, and checks
that GVA is 0. Without the previous patch QEMU fails all 4:

- load and load-vu: GVA stays 1 after the ecall
- hlv and hsv: GVA is 0 after the fault

Each case is its own test, built with the -D flags listed in
meson.build, and exits with 0 or the number of the first check that
failed. It uses hyp-guest.h and hyp-guest.S from the previous series.

Signed-off-by: tewaro <tewaro@theorem.dev>
---
 tests/tcg/riscv64/system/meson.build | 21 ++++++++
 tests/tcg/riscv64/test-mstatus-gva.S | 75 ++++++++++++++++++++++++++++
 2 files changed, 96 insertions(+)
 create mode 100644 tests/tcg/riscv64/test-mstatus-gva.S

diff --git a/tests/tcg/riscv64/system/meson.build b/tests/tcg/riscv64/system/meson.build
index 05ed8b4c8f..e1c478196b 100644
--- a/tests/tcg/riscv64/system/meson.build
+++ b/tests/tcg/riscv64/system/meson.build
@@ -130,6 +130,27 @@ foreach name, flags : gstage_tinst
   }
 endforeach
 
+# test-mstatus-gva.S: one build per case; the flags are documented there
+mstatus_gva = {
+  'load':      ['-DMODE=VS', '-DACCESS=ld t0, 0(a0)', '-DREG=3',
+                '-DFLAGS=PTE_R', '-DCAUSE=LOAD_GPF'],
+  'load-vu':   ['-DMODE=VU', '-DACCESS=ld t0, 0(a0)', '-DREG=4',
+                '-DFLAGS=(PTE_R|PTE_U)', '-DCAUSE=LOAD_GPF'],
+  'hlv':       ['-DMODE=HS', '-DACCESS=hlv.d t0, (a0)', '-DREG=5',
+                '-DFLAGS=PTE_R', '-DCAUSE=LOAD_GPF'],
+  'hsv':       ['-DMODE=HS', '-DACCESS=hsv.d zero, (a0)', '-DREG=6',
+                '-DFLAGS=(PTE_R|PTE_W|PTE_A)', '-DCAUSE=STORE_GPF'],
+}
+foreach name, flags : mstatus_gva
+  tests += {
+    'test-mstatus-gva.S': {
+      'exe_name': 'test-mstatus-gva-' + name,
+      'cflags': cflags + ['-march=rv64gch', hyp_guest] + flags,
+      'qemu_args': ['-cpu', 'rv64,h=true,svadu=true', qemu_args],
+    }
+  }
+endforeach
+
 tests += {
   'test-vle32ff.S': {
     'cflags': cflags + ['-march=rv64gcv'],
diff --git a/tests/tcg/riscv64/test-mstatus-gva.S b/tests/tcg/riscv64/test-mstatus-gva.S
new file mode 100644
index 0000000000..ec8b6df25a
--- /dev/null
+++ b/tests/tcg/riscv64/test-mstatus-gva.S
@@ -0,0 +1,75 @@
+/*
+ * Test that every trap into M-mode writes mstatus.GVA: 1 if mtval holds
+ * a guest virtual address, 0 otherwise.
+ *
+ * Each build is one case, set by -D flags in meson.build:
+ * - MODE: VS, VU or HS (HS runs hlv/hsv on the guest's behalf)
+ * - ACCESS: the instruction run on the start of region REG
+ * - REG: the region accessed
+ * - FLAGS: the guest's page-table entry for REG, with A or D clear, so
+ *   that the walk's A/D write faults on the read-only G-stage
+ * - CAUSE: the fault expected
+ *
+ * The trap handler checks, and the exit status is the first that fails:
+ *   1  mcause is CAUSE
+ *   2  GVA is 1, since mtval holds the guest address
+ *   3  after an ecall from M-mode, whose trap has no guest address, GVA
+ *      is 0
+ *
+ * `trap` makes checks 1 and 2, then runs the ecall, whose trap goes to
+ * `ecall_trap` for check 3.
+ *
+ * Privileged ISA v1.12: 8.2.1.
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+#include "hyp-guest.h"
+
+/* exit with status n unless reg == want */
+#define CHECK(n, reg, want) li t6, want; li a0, n; bne reg, t6, exit
+
+/* read mstatus.GVA into t5 */
+#define MSTATUS_GVA_SHIFT 38
+#define GVA csrr t5, mstatus; srli t5, t5, MSTATUS_GVA_SHIFT; andi t5, t5, 1
+
+        .option norvc
+        .text
+        .global test, trap, hs_trap
+test:
+        li      t0, VS_L1 + (REG) * 8   /* the guest's entry for REG */
+        li      t1, leaf(REGION(REG), FLAGS)
+        sd      t1, 0(t0)
+        li      a0, G_RO
+        call    set_tables_perm
+
+        li      a0, REGION(REG)
+        lla     a1, probe
+        li      a2, MODE
+        call    run_guest
+
+probe:
+        ACCESS
+        ecall
+
+        .balign 4
+trap:
+        csrr    t5, mcause
+        CHECK(1, t5, CAUSE)
+        GVA
+        CHECK(2, t5, 1)
+        lla     t0, ecall_trap          /* the ecall below traps there */
+        csrw    mtvec, t0
+        ecall
+
+        .balign 4
+ecall_trap:
+        GVA
+        CHECK(3, t5, 0)
+        li      a0, 0
+        j       exit
+
+/* nothing is delegated to HS-mode, so no trap should come here */
+        .balign 4
+hs_trap:
+        li      a0, 1
+        j       exit
-- 
2.54.0 (Apple Git-157)