[PATCH v5 0/2] target/s390x: Implement the MVPG condition-code-option bit

[PATCH v5 0/2] target/s390x: Implement the MVPG condition-code-option bit

[David Hildenbrand]

Based on work from Richard and Thomas.

v4 -> v5:
- Don't realy on TLB_INVALID_MASK
-- Check against tlb_fill_exc and return the exception right away
- Handle !CONFIG_USER_ONLY
-- Check against haddr
-- Properly store vaddr to env->__excp_addr and return PGM_ADDRESSING
- Exclude tlb_fill_tec/tlb_fill_exc for CONFIG_USER_ONLY
- While at it, tackle r1/r2 indication as well

The ifdeffery in patch #1 is a bit ugly but IMHO but still tolerable.

KVM unit tests continue working as expected.

David Hildenbrand (1):
  target/s390x: Store r1/r2 for page-translation exceptions during MVPG

Richard Henderson (1):
  target/s390x: Implement the MVPG condition-code-option bit

 target/s390x/cpu.h         |   5 ++
 target/s390x/excp_helper.c |   3 +
 target/s390x/helper.h      |   2 +-
 target/s390x/insn-data.def |   2 +-
 target/s390x/mem_helper.c  | 146 +++++++++++++++++++++++++++++--------
 target/s390x/translate.c   |   7 +-
 6 files changed, 133 insertions(+), 32 deletions(-)

-- 
2.29.2

[PATCH v5 1/2] target/s390x: Implement the MVPG condition-code-option bit

[David Hildenbrand]

From: Richard Henderson <richard.henderson@linaro.org>

If the CCO bit is set, MVPG should not generate an exception but
report page translation faults via a CC code.

Create a new helper, access_prepare_nf, which can use probe_access_flags
in non-faulting mode, and then handle watchpoints.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
[thuth: Added logic to still inject protection exceptions]
Signed-off-by: Thomas Huth <thuth@redhat.com>
[david: Look at env->tlb_fill_exc to determine if there was an exception]
Signed-off-by: David Hildenbrand <david@redhat.com>
---
 target/s390x/cpu.h         |   5 ++
 target/s390x/excp_helper.c |   3 +
 target/s390x/mem_helper.c  | 124 ++++++++++++++++++++++++++++++-------
 3 files changed, 110 insertions(+), 22 deletions(-)

diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
index 60d434d5ed..468b4430f3 100644
--- a/target/s390x/cpu.h
+++ b/target/s390x/cpu.h
@@ -114,6 +114,11 @@ struct CPUS390XState {
 
     uint64_t diag318_info;
 
+#if !defined(CONFIG_USER_ONLY)
+    uint64_t tlb_fill_tec;   /* translation exception code during tlb_fill */
+    int tlb_fill_exc;        /* exception number seen during tlb_fill */
+#endif
+
     /* Fields up to this point are cleared by a CPU reset */
     struct {} end_reset_fields;
 
diff --git a/target/s390x/excp_helper.c b/target/s390x/excp_helper.c
index ce16af394b..c48cd6b46f 100644
--- a/target/s390x/excp_helper.c
+++ b/target/s390x/excp_helper.c
@@ -164,6 +164,9 @@ bool s390_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
         tec = 0; /* unused */
     }
 
+    env->tlb_fill_exc = excp;
+    env->tlb_fill_tec = tec;
+
     if (!excp) {
         qemu_log_mask(CPU_LOG_MMU,
                       "%s: set tlb %" PRIx64 " -> %" PRIx64 " (%x)\n",
diff --git a/target/s390x/mem_helper.c b/target/s390x/mem_helper.c
index 25cfede806..ebb55884c9 100644
--- a/target/s390x/mem_helper.c
+++ b/target/s390x/mem_helper.c
@@ -130,28 +130,93 @@ typedef struct S390Access {
     int mmu_idx;
 } S390Access;
 
-static S390Access access_prepare(CPUS390XState *env, vaddr vaddr, int size,
-                                 MMUAccessType access_type, int mmu_idx,
-                                 uintptr_t ra)
+/*
+ * With nofault=1, return the PGM_ exception that would have been injected
+ * into the guest; return 0 if no exception was detected.
+ *
+ * For !CONFIG_USER_ONLY, the TEC is stored stored to env->tlb_fill_tec.
+ * For CONFIG_USER_ONLY, the faulting address is stored to env->__excp_addr.
+ */
+static int access_prepare_nf(S390Access *access, CPUS390XState *env,
+                             bool nofault, vaddr vaddr1, int size,
+                             MMUAccessType access_type,
+                             int mmu_idx, uintptr_t ra)
 {
-    S390Access access = {
-        .vaddr1 = vaddr,
-        .size1 = MIN(size, -(vaddr | TARGET_PAGE_MASK)),
-        .mmu_idx = mmu_idx,
-    };
+    void *haddr1, *haddr2 = NULL;
+    int size1, size2;
+    vaddr vaddr2 = 0;
+    int flags;
+
+    assert(size > 0 && size <= 4096);
 
-    g_assert(size > 0 && size <= 4096);
-    access.haddr1 = probe_access(env, access.vaddr1, access.size1, access_type,
-                                 mmu_idx, ra);
+    size1 = MIN(size, -(vaddr1 | TARGET_PAGE_MASK)),
+    size2 = size - size1;
 
-    if (unlikely(access.size1 != size)) {
+#if !defined(CONFIG_USER_ONLY)
+    /*
+     * For !CONFIG_USER_ONLY, we cannot rely on TLB_INVALID_MASK or haddr==NULL
+     * to detect if there was an exception during tlb_fill().
+     */
+    env->tlb_fill_exc = 0;
+#endif
+    flags = probe_access_flags(env, vaddr1, access_type, mmu_idx,
+                               nofault, &haddr1, ra);
+#if !defined(CONFIG_USER_ONLY)
+    if (env->tlb_fill_exc) {
+        return env->tlb_fill_exc;
+    }
+#else
+    if (!haddr1) {
+        env->__excp_addr = vaddr1;
+        return PGM_ADDRESSING;
+    }
+#endif
+    if (unlikely(size2)) {
         /* The access crosses page boundaries. */
-        access.vaddr2 = wrap_address(env, vaddr + access.size1);
-        access.size2 = size - access.size1;
-        access.haddr2 = probe_access(env, access.vaddr2, access.size2,
-                                     access_type, mmu_idx, ra);
+        vaddr2 = wrap_address(env, vaddr1 + size1);
+        flags |= probe_access_flags(env, vaddr2, access_type, mmu_idx,
+                                    nofault, &haddr2, ra);
+#if !defined(CONFIG_USER_ONLY)
+        if (env->tlb_fill_exc) {
+            return env->tlb_fill_exc;
+        }
+#else
+        if (!haddr2) {
+            env->__excp_addr = vaddr2;
+            return PGM_ADDRESSING;
+        }
+#endif
     }
-    return access;
+
+    if (unlikely(flags & TLB_WATCHPOINT)) {
+        /* S390 does not presently use transaction attributes. */
+        cpu_check_watchpoint(env_cpu(env), vaddr1, size,
+                             MEMTXATTRS_UNSPECIFIED,
+                             (access_type == MMU_DATA_STORE
+                              ? BP_MEM_WRITE : BP_MEM_READ), ra);
+    }
+
+    *access = (S390Access) {
+        .vaddr1 = vaddr1,
+        .vaddr2 = vaddr2,
+        .haddr1 = haddr1,
+        .haddr2 = haddr2,
+        .size1 = size1,
+        .size2 = size2,
+        .mmu_idx = mmu_idx
+    };
+    return 0;
+}
+
+static S390Access access_prepare(CPUS390XState *env, vaddr vaddr, int size,
+                                 MMUAccessType access_type, int mmu_idx,
+                                 uintptr_t ra)
+{
+    S390Access ret;
+    int exc = access_prepare_nf(&ret, env, false, vaddr, size,
+                                access_type, mmu_idx, ra);
+    assert(!exc);
+    return ret;
 }
 
 /* Helper to handle memset on a single page. */
@@ -845,8 +910,10 @@ uint32_t HELPER(mvpg)(CPUS390XState *env, uint64_t r0, uint64_t r1, uint64_t r2)
     const int mmu_idx = cpu_mmu_index(env, false);
     const bool f = extract64(r0, 11, 1);
     const bool s = extract64(r0, 10, 1);
+    const bool cco = extract64(r0, 8, 1);
     uintptr_t ra = GETPC();
     S390Access srca, desta;
+    int exc;
 
     if ((f && s) || extract64(r0, 12, 4)) {
         tcg_s390_program_interrupt(env, PGM_SPECIFICATION, GETPC());
@@ -858,13 +925,26 @@ uint32_t HELPER(mvpg)(CPUS390XState *env, uint64_t r0, uint64_t r1, uint64_t r2)
     /*
      * TODO:
      * - Access key handling
-     * - CC-option with surpression of page-translation exceptions
      * - Store r1/r2 register identifiers at real location 162
      */
-    srca = access_prepare(env, r2, TARGET_PAGE_SIZE, MMU_DATA_LOAD, mmu_idx,
-                          ra);
-    desta = access_prepare(env, r1, TARGET_PAGE_SIZE, MMU_DATA_STORE, mmu_idx,
-                           ra);
+    exc = access_prepare_nf(&srca, env, cco, r2, TARGET_PAGE_SIZE,
+                            MMU_DATA_LOAD, mmu_idx, ra);
+    if (exc) {
+        return 2;
+    }
+    exc = access_prepare_nf(&desta, env, cco, r1, TARGET_PAGE_SIZE,
+                            MMU_DATA_STORE, mmu_idx, ra);
+    if (exc) {
+#if !defined(CONFIG_USER_ONLY)
+        if (exc == PGM_PROTECTION) {
+            stq_phys(env_cpu(env)->as,
+                     env->psa + offsetof(LowCore, trans_exc_code),
+                     env->tlb_fill_tec);
+            tcg_s390_program_interrupt(env, PGM_PROTECTION, ra);
+        }
+#endif
+        return 1;
+    }
     access_memmove(env, &desta, &srca, ra);
     return 0; /* data moved */
 }
-- 
2.29.2

[PATCH v5 2/2] target/s390x: Store r1/r2 for page-translation exceptions during MVPG

[David Hildenbrand]

The PoP states:

    When EDAT-1 does not apply, and a program interruption due to a
    page-translation exception is recognized by the MOVE PAGE
    instruction, the contents of the R1 field of the instruction are
    stored in bit positions 0-3 of location 162, and the contents of
    the R2 field are stored in bit positions 4-7.

    If [...] an ASCE-type, region-first-translation,
    region-second-translation, region-third-translation, or
    segment-translation exception was recognized, the contents of
    location 162 are unpredictable.

So we have to write r1/r2 into the lowcore on page-translation
exceptions. Simply handle all exceptions inside our mvpg helper now.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 target/s390x/helper.h      |  2 +-
 target/s390x/insn-data.def |  2 +-
 target/s390x/mem_helper.c  | 44 ++++++++++++++++++++++----------------
 target/s390x/translate.c   |  7 +++++-
 4 files changed, 34 insertions(+), 21 deletions(-)

diff --git a/target/s390x/helper.h b/target/s390x/helper.h
index 55bd1551e6..d4e4f3388f 100644
--- a/target/s390x/helper.h
+++ b/target/s390x/helper.h
@@ -18,7 +18,7 @@ DEF_HELPER_3(srstu, void, env, i32, i32)
 DEF_HELPER_4(clst, i64, env, i64, i64, i64)
 DEF_HELPER_FLAGS_4(mvn, TCG_CALL_NO_WG, void, env, i32, i64, i64)
 DEF_HELPER_FLAGS_4(mvo, TCG_CALL_NO_WG, void, env, i32, i64, i64)
-DEF_HELPER_FLAGS_4(mvpg, TCG_CALL_NO_WG, i32, env, i64, i64, i64)
+DEF_HELPER_FLAGS_4(mvpg, TCG_CALL_NO_WG, i32, env, i64, i32, i32)
 DEF_HELPER_FLAGS_4(mvz, TCG_CALL_NO_WG, void, env, i32, i64, i64)
 DEF_HELPER_3(mvst, i32, env, i32, i32)
 DEF_HELPER_4(ex, void, env, i32, i64, i64)
diff --git a/target/s390x/insn-data.def b/target/s390x/insn-data.def
index e5b6efabf3..0bb1886a2e 100644
--- a/target/s390x/insn-data.def
+++ b/target/s390x/insn-data.def
@@ -641,7 +641,7 @@
 /* MOVE NUMERICS */
     C(0xd100, MVN,     SS_a,  Z,   la1, a2, 0, 0, mvn, 0)
 /* MOVE PAGE */
-    C(0xb254, MVPG,    RRE,   Z,   r1_o, r2_o, 0, 0, mvpg, 0)
+    C(0xb254, MVPG,    RRE,   Z,   0, 0, 0, 0, mvpg, 0)
 /* MOVE STRING */
     C(0xb255, MVST,    RRE,   Z,   0, 0, 0, 0, mvst, 0)
 /* MOVE WITH OPTIONAL SPECIFICATION */
diff --git a/target/s390x/mem_helper.c b/target/s390x/mem_helper.c
index ebb55884c9..432c1a4954 100644
--- a/target/s390x/mem_helper.c
+++ b/target/s390x/mem_helper.c
@@ -905,8 +905,10 @@ uint64_t HELPER(clst)(CPUS390XState *env, uint64_t c, uint64_t s1, uint64_t s2)
 }
 
 /* move page */
-uint32_t HELPER(mvpg)(CPUS390XState *env, uint64_t r0, uint64_t r1, uint64_t r2)
+uint32_t HELPER(mvpg)(CPUS390XState *env, uint64_t r0, uint32_t r1, uint32_t r2)
 {
+    const uint64_t src = get_address(env, r2) & TARGET_PAGE_MASK;
+    const uint64_t dst = get_address(env, r1) & TARGET_PAGE_MASK;
     const int mmu_idx = cpu_mmu_index(env, false);
     const bool f = extract64(r0, 11, 1);
     const bool s = extract64(r0, 10, 1);
@@ -919,34 +921,40 @@ uint32_t HELPER(mvpg)(CPUS390XState *env, uint64_t r0, uint64_t r1, uint64_t r2)
         tcg_s390_program_interrupt(env, PGM_SPECIFICATION, GETPC());
     }
 
-    r1 = wrap_address(env, r1 & TARGET_PAGE_MASK);
-    r2 = wrap_address(env, r2 & TARGET_PAGE_MASK);
-
     /*
-     * TODO:
-     * - Access key handling
-     * - Store r1/r2 register identifiers at real location 162
+     * We always manually handle exceptions such that we can properly store
+     * r1/r2 to the lowcore on page-translation exceptions.
+     *
+     * TODO: Access key handling
      */
-    exc = access_prepare_nf(&srca, env, cco, r2, TARGET_PAGE_SIZE,
+    exc = access_prepare_nf(&srca, env, true, src, TARGET_PAGE_SIZE,
                             MMU_DATA_LOAD, mmu_idx, ra);
     if (exc) {
-        return 2;
+        if (cco) {
+            return 2;
+        }
+        goto inject_exc;
     }
-    exc = access_prepare_nf(&desta, env, cco, r1, TARGET_PAGE_SIZE,
+    exc = access_prepare_nf(&desta, env, true, dst, TARGET_PAGE_SIZE,
                             MMU_DATA_STORE, mmu_idx, ra);
     if (exc) {
-#if !defined(CONFIG_USER_ONLY)
-        if (exc == PGM_PROTECTION) {
-            stq_phys(env_cpu(env)->as,
-                     env->psa + offsetof(LowCore, trans_exc_code),
-                     env->tlb_fill_tec);
-            tcg_s390_program_interrupt(env, PGM_PROTECTION, ra);
+        if (cco && exc != PGM_PROTECTION) {
+            return 1;
         }
-#endif
-        return 1;
+        goto inject_exc;
     }
     access_memmove(env, &desta, &srca, ra);
     return 0; /* data moved */
+inject_exc:
+#if !defined(CONFIG_USER_ONLY)
+    stq_phys(env_cpu(env)->as, env->psa + offsetof(LowCore, trans_exc_code),
+             env->tlb_fill_tec);
+    if (exc == PGM_PAGE_TRANS) {
+        stb_phys(env_cpu(env)->as, env->psa + offsetof(LowCore, op_access_id),
+                 r1 << 4 | r2);
+    }
+#endif
+    tcg_s390_program_interrupt(env, exc, ra);
 }
 
 /* string copy */
diff --git a/target/s390x/translate.c b/target/s390x/translate.c
index 61dd0341e4..4f953ddfba 100644
--- a/target/s390x/translate.c
+++ b/target/s390x/translate.c
@@ -3513,7 +3513,12 @@ static DisasJumpType op_mvo(DisasContext *s, DisasOps *o)
 
 static DisasJumpType op_mvpg(DisasContext *s, DisasOps *o)
 {
-    gen_helper_mvpg(cc_op, cpu_env, regs[0], o->in1, o->in2);
+    TCGv_i32 t1 = tcg_const_i32(get_field(s, r1));
+    TCGv_i32 t2 = tcg_const_i32(get_field(s, r2));
+
+    gen_helper_mvpg(cc_op, cpu_env, regs[0], t1, t2);
+    tcg_temp_free_i32(t1);
+    tcg_temp_free_i32(t2);
     set_cc_static(s);
     return DISAS_NEXT;
 }
-- 
2.29.2

Re: [PATCH v5 2/2] target/s390x: Store r1/r2 for page-translation exceptions during MVPG

[David Hildenbrand]

On 11.03.21 17:17, David Hildenbrand wrote:
> The PoP states:
> 
>      When EDAT-1 does not apply, and a program interruption due to a
>      page-translation exception is recognized by the MOVE PAGE
>      instruction, the contents of the R1 field of the instruction are
>      stored in bit positions 0-3 of location 162, and the contents of
>      the R2 field are stored in bit positions 4-7.
> 
>      If [...] an ASCE-type, region-first-translation,
>      region-second-translation, region-third-translation, or
>      segment-translation exception was recognized, the contents of
>      location 162 are unpredictable.
> 
> So we have to write r1/r2 into the lowcore on page-translation
> exceptions. Simply handle all exceptions inside our mvpg helper now.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>   target/s390x/helper.h      |  2 +-
>   target/s390x/insn-data.def |  2 +-
>   target/s390x/mem_helper.c  | 44 ++++++++++++++++++++++----------------
>   target/s390x/translate.c   |  7 +++++-
>   4 files changed, 34 insertions(+), 21 deletions(-)
> 
> diff --git a/target/s390x/helper.h b/target/s390x/helper.h
> index 55bd1551e6..d4e4f3388f 100644
> --- a/target/s390x/helper.h
> +++ b/target/s390x/helper.h
> @@ -18,7 +18,7 @@ DEF_HELPER_3(srstu, void, env, i32, i32)
>   DEF_HELPER_4(clst, i64, env, i64, i64, i64)
>   DEF_HELPER_FLAGS_4(mvn, TCG_CALL_NO_WG, void, env, i32, i64, i64)
>   DEF_HELPER_FLAGS_4(mvo, TCG_CALL_NO_WG, void, env, i32, i64, i64)
> -DEF_HELPER_FLAGS_4(mvpg, TCG_CALL_NO_WG, i32, env, i64, i64, i64)
> +DEF_HELPER_FLAGS_4(mvpg, TCG_CALL_NO_WG, i32, env, i64, i32, i32)
>   DEF_HELPER_FLAGS_4(mvz, TCG_CALL_NO_WG, void, env, i32, i64, i64)
>   DEF_HELPER_3(mvst, i32, env, i32, i32)
>   DEF_HELPER_4(ex, void, env, i32, i64, i64)
> diff --git a/target/s390x/insn-data.def b/target/s390x/insn-data.def
> index e5b6efabf3..0bb1886a2e 100644
> --- a/target/s390x/insn-data.def
> +++ b/target/s390x/insn-data.def
> @@ -641,7 +641,7 @@
>   /* MOVE NUMERICS */
>       C(0xd100, MVN,     SS_a,  Z,   la1, a2, 0, 0, mvn, 0)
>   /* MOVE PAGE */
> -    C(0xb254, MVPG,    RRE,   Z,   r1_o, r2_o, 0, 0, mvpg, 0)
> +    C(0xb254, MVPG,    RRE,   Z,   0, 0, 0, 0, mvpg, 0)
>   /* MOVE STRING */
>       C(0xb255, MVST,    RRE,   Z,   0, 0, 0, 0, mvst, 0)
>   /* MOVE WITH OPTIONAL SPECIFICATION */
> diff --git a/target/s390x/mem_helper.c b/target/s390x/mem_helper.c
> index ebb55884c9..432c1a4954 100644
> --- a/target/s390x/mem_helper.c
> +++ b/target/s390x/mem_helper.c
> @@ -905,8 +905,10 @@ uint64_t HELPER(clst)(CPUS390XState *env, uint64_t c, uint64_t s1, uint64_t s2)
>   }
>   
>   /* move page */
> -uint32_t HELPER(mvpg)(CPUS390XState *env, uint64_t r0, uint64_t r1, uint64_t r2)
> +uint32_t HELPER(mvpg)(CPUS390XState *env, uint64_t r0, uint32_t r1, uint32_t r2)
>   {
> +    const uint64_t src = get_address(env, r2) & TARGET_PAGE_MASK;
> +    const uint64_t dst = get_address(env, r1) & TARGET_PAGE_MASK;
>       const int mmu_idx = cpu_mmu_index(env, false);
>       const bool f = extract64(r0, 11, 1);
>       const bool s = extract64(r0, 10, 1);
> @@ -919,34 +921,40 @@ uint32_t HELPER(mvpg)(CPUS390XState *env, uint64_t r0, uint64_t r1, uint64_t r2)
>           tcg_s390_program_interrupt(env, PGM_SPECIFICATION, GETPC());
>       }
>   
> -    r1 = wrap_address(env, r1 & TARGET_PAGE_MASK);
> -    r2 = wrap_address(env, r2 & TARGET_PAGE_MASK);
> -
>       /*
> -     * TODO:
> -     * - Access key handling
> -     * - Store r1/r2 register identifiers at real location 162
> +     * We always manually handle exceptions such that we can properly store
> +     * r1/r2 to the lowcore on page-translation exceptions.
> +     *
> +     * TODO: Access key handling
>        */
> -    exc = access_prepare_nf(&srca, env, cco, r2, TARGET_PAGE_SIZE,
> +    exc = access_prepare_nf(&srca, env, true, src, TARGET_PAGE_SIZE,
>                               MMU_DATA_LOAD, mmu_idx, ra);
>       if (exc) {
> -        return 2;
> +        if (cco) {
> +            return 2;
> +        }
> +        goto inject_exc;
>       }
> -    exc = access_prepare_nf(&desta, env, cco, r1, TARGET_PAGE_SIZE,
> +    exc = access_prepare_nf(&desta, env, true, dst, TARGET_PAGE_SIZE,
>                               MMU_DATA_STORE, mmu_idx, ra);
>       if (exc) {
> -#if !defined(CONFIG_USER_ONLY)
> -        if (exc == PGM_PROTECTION) {
> -            stq_phys(env_cpu(env)->as,
> -                     env->psa + offsetof(LowCore, trans_exc_code),
> -                     env->tlb_fill_tec);
> -            tcg_s390_program_interrupt(env, PGM_PROTECTION, ra);
> +        if (cco && exc != PGM_PROTECTION) {
> +            return 1;
>           }
> -#endif
> -        return 1;
> +        goto inject_exc;
>       }
>       access_memmove(env, &desta, &srca, ra);
>       return 0; /* data moved */
> +inject_exc:
> +#if !defined(CONFIG_USER_ONLY)
> +    stq_phys(env_cpu(env)->as, env->psa + offsetof(LowCore, trans_exc_code),
> +             env->tlb_fill_tec);

Just realized, we want a

if (exc != PGM_ADDRESSING) {

for this case. But let's hear other feedback first :)


-- 
Thanks,

David / dhildenb

Re: [PATCH v5 1/2] target/s390x: Implement the MVPG condition-code-option bit

[Richard Henderson]

On 3/11/21 10:17 AM, David Hildenbrand wrote:
> +#if !defined(CONFIG_USER_ONLY)
> +    if (env->tlb_fill_exc) {
> +        return env->tlb_fill_exc;
> +    }
> +#else
> +    if (!haddr1) {
> +        env->__excp_addr = vaddr1;
> +        return PGM_ADDRESSING;
> +    }
> +#endif

For user-only, we can rely on TLB_INVALID_MASK, and check once for both pages.


> @@ -858,13 +925,26 @@ uint32_t HELPER(mvpg)(CPUS390XState *env, uint64_t r0, uint64_t r1, uint64_t r2)
>       /*
>        * TODO:
>        * - Access key handling
> -     * - CC-option with surpression of page-translation exceptions
>        * - Store r1/r2 register identifiers at real location 162
>        */
> -    srca = access_prepare(env, r2, TARGET_PAGE_SIZE, MMU_DATA_LOAD, mmu_idx,
> -                          ra);
> -    desta = access_prepare(env, r1, TARGET_PAGE_SIZE, MMU_DATA_STORE, mmu_idx,
> -                           ra);
> +    exc = access_prepare_nf(&srca, env, cco, r2, TARGET_PAGE_SIZE,
> +                            MMU_DATA_LOAD, mmu_idx, ra);
> +    if (exc) {
> +        return 2;
> +    }
> +    exc = access_prepare_nf(&desta, env, cco, r1, TARGET_PAGE_SIZE,
> +                            MMU_DATA_STORE, mmu_idx, ra);
> +    if (exc) {
> +#if !defined(CONFIG_USER_ONLY)
> +        if (exc == PGM_PROTECTION) {
> +            stq_phys(env_cpu(env)->as,
> +                     env->psa + offsetof(LowCore, trans_exc_code),
> +                     env->tlb_fill_tec);
> +            tcg_s390_program_interrupt(env, PGM_PROTECTION, ra);
> +        }
> +#endif
> +        return 1;
> +    }
>       access_memmove(env, &desta, &srca, ra);
>       return 0; /* data moved */
>   }

If we're going to have an ifdef at all here, it should be around the entire 
helper -- this is a privileged operation.


r~

Re: [PATCH v5 1/2] target/s390x: Implement the MVPG condition-code-option bit

[David Hildenbrand]

On 11.03.21 18:02, Richard Henderson wrote:
> On 3/11/21 10:17 AM, David Hildenbrand wrote:
>> +#if !defined(CONFIG_USER_ONLY)
>> +    if (env->tlb_fill_exc) {
>> +        return env->tlb_fill_exc;
>> +    }
>> +#else
>> +    if (!haddr1) {
>> +        env->__excp_addr = vaddr1;
>> +        return PGM_ADDRESSING;
>> +    }
>> +#endif
> 
> For user-only, we can rely on TLB_INVALID_MASK, and check once for both pages.

Then, I cannot set the proper vaddr1 vs. vaddr2 (see patch #2).


>> @@ -858,13 +925,26 @@ uint32_t HELPER(mvpg)(CPUS390XState *env, uint64_t r0, uint64_t r1, uint64_t r2)
>>        /*
>>         * TODO:
>>         * - Access key handling
>> -     * - CC-option with surpression of page-translation exceptions
>>         * - Store r1/r2 register identifiers at real location 162
>>         */
>> -    srca = access_prepare(env, r2, TARGET_PAGE_SIZE, MMU_DATA_LOAD, mmu_idx,
>> -                          ra);
>> -    desta = access_prepare(env, r1, TARGET_PAGE_SIZE, MMU_DATA_STORE, mmu_idx,
>> -                           ra);
>> +    exc = access_prepare_nf(&srca, env, cco, r2, TARGET_PAGE_SIZE,
>> +                            MMU_DATA_LOAD, mmu_idx, ra);
>> +    if (exc) {
>> +        return 2;
>> +    }
>> +    exc = access_prepare_nf(&desta, env, cco, r1, TARGET_PAGE_SIZE,
>> +                            MMU_DATA_STORE, mmu_idx, ra);
>> +    if (exc) {
>> +#if !defined(CONFIG_USER_ONLY)
>> +        if (exc == PGM_PROTECTION) {
>> +            stq_phys(env_cpu(env)->as,
>> +                     env->psa + offsetof(LowCore, trans_exc_code),
>> +                     env->tlb_fill_tec);
>> +            tcg_s390_program_interrupt(env, PGM_PROTECTION, ra);
>> +        }
>> +#endif
>> +        return 1;
>> +    }
>>        access_memmove(env, &desta, &srca, ra);
>>        return 0; /* data moved */
>>    }
> 
> If we're going to have an ifdef at all here, it should be around the entire
> helper -- this is a privileged operation.


Privileged operation (access key specified, and
selected PSW-key-mask bit is zero in the prob-
lem state)

Without an access key in GR0, we're using the PSW key - which should 
always work, no?

What am I missing?

-- 
Thanks,

David / dhildenb

Re: [PATCH v5 1/2] target/s390x: Implement the MVPG condition-code-option bit

[Richard Henderson]

On 3/11/21 11:12 AM, David Hildenbrand wrote:
> Without an access key in GR0, we're using the PSW key - which should always 
> work, no?
> 
> What am I missing?

Nothing.  Insufficiently close reading on my part.


r~

Re: [PATCH v5 1/2] target/s390x: Implement the MVPG condition-code-option bit

[Richard Henderson]

On 3/11/21 10:17 AM, David Hildenbrand wrote:
> +#if !defined(CONFIG_USER_ONLY)
> +    /*
> +     * For !CONFIG_USER_ONLY, we cannot rely on TLB_INVALID_MASK or haddr==NULL
> +     * to detect if there was an exception during tlb_fill().
> +     */
> +    env->tlb_fill_exc = 0;
> +#endif
> +    flags = probe_access_flags(env, vaddr1, access_type, mmu_idx,
> +                               nofault, &haddr1, ra);
> +#if !defined(CONFIG_USER_ONLY)
> +    if (env->tlb_fill_exc) {
> +        return env->tlb_fill_exc;
> +    }
> +#else
> +    if (!haddr1) {
> +        env->__excp_addr = vaddr1;
> +        return PGM_ADDRESSING;
> +    }
> +#endif

The assumption of PGM_ADDRESSING is incorrect here -- it could still be 
PGM_PROTECTION, depending on how the page is mapped.

I guess this should be done like

#ifdef CONFIG_USER_ONLY
     flags = page_get_flags(vaddr1);
     if (!flags & (access_type == MMU_DATA_LOAD
                   ? PAGE_READ : PAGE_WRITE)) {
         env->__excp_addr = vaddr1;
         if (nofault) {
             return (flags & PAGE_VALID
                 ? PGM_PROTECTION : PGM_ADDRESSING);
         }
         raise exception.
     }
     haddr1 = g2h(vaddr1);
#else
     env->tlb_fill_exc = 0;
     flags = probe_access_flags(...);
     if (env->tlb_fill_exc) {
         return env->tlb_fill_exc;
     }
#endif

which is pretty ugly, but no worse than what you have above.


r~

Re: [PATCH v5 1/2] target/s390x: Implement the MVPG condition-code-option bit

[David Hildenbrand]

On 11.03.21 18:52, Richard Henderson wrote:
> On 3/11/21 10:17 AM, David Hildenbrand wrote:
>> +#if !defined(CONFIG_USER_ONLY)
>> +    /*
>> +     * For !CONFIG_USER_ONLY, we cannot rely on TLB_INVALID_MASK or haddr==NULL
>> +     * to detect if there was an exception during tlb_fill().
>> +     */
>> +    env->tlb_fill_exc = 0;
>> +#endif
>> +    flags = probe_access_flags(env, vaddr1, access_type, mmu_idx,
>> +                               nofault, &haddr1, ra);
>> +#if !defined(CONFIG_USER_ONLY)
>> +    if (env->tlb_fill_exc) {
>> +        return env->tlb_fill_exc;
>> +    }
>> +#else
>> +    if (!haddr1) {
>> +        env->__excp_addr = vaddr1;
>> +        return PGM_ADDRESSING;
>> +    }
>> +#endif
> 
> The assumption of PGM_ADDRESSING is incorrect here -- it could still be
> PGM_PROTECTION, depending on how the page is mapped.
> 

Interesting, I was only looking at the s390x tlb_fill() implementation. 
But I assume these checks are performed in common code.

> I guess this should be done like
> 
> #ifdef CONFIG_USER_ONLY
>       flags = page_get_flags(vaddr1);
>       if (!flags & (access_type == MMU_DATA_LOAD
>                     ? PAGE_READ : PAGE_WRITE)) {
>           env->__excp_addr = vaddr1;
>           if (nofault) {
>               return (flags & PAGE_VALID
>                   ? PGM_PROTECTION : PGM_ADDRESSING);
>           }
>           raise exception.
>       }
>       haddr1 = g2h(vaddr1);
> #else
>       env->tlb_fill_exc = 0;
>       flags = probe_access_flags(...);
>       if (env->tlb_fill_exc) {
>           return env->tlb_fill_exc;
>       }
> #endif
> 
> which is pretty ugly, but no worse than what you have above.

Thanks, maybe I can factor that out in a nice way. I guess we could do 
the access via probe_access_flags() and only on error do the 
page_get_flags()?


-- 
Thanks,

David / dhildenb

Re: [PATCH v5 1/2] target/s390x: Implement the MVPG condition-code-option bit

[Richard Henderson]

On 3/11/21 12:16 PM, David Hildenbrand wrote:
> On 11.03.21 18:52, Richard Henderson wrote:
>> On 3/11/21 10:17 AM, David Hildenbrand wrote:
>>> +#if !defined(CONFIG_USER_ONLY)
>>> +    /*
>>> +     * For !CONFIG_USER_ONLY, we cannot rely on TLB_INVALID_MASK or 
>>> haddr==NULL
>>> +     * to detect if there was an exception during tlb_fill().
>>> +     */
>>> +    env->tlb_fill_exc = 0;
>>> +#endif
>>> +    flags = probe_access_flags(env, vaddr1, access_type, mmu_idx,
>>> +                               nofault, &haddr1, ra);
>>> +#if !defined(CONFIG_USER_ONLY)
>>> +    if (env->tlb_fill_exc) {
>>> +        return env->tlb_fill_exc;
>>> +    }
>>> +#else
>>> +    if (!haddr1) {
>>> +        env->__excp_addr = vaddr1;
>>> +        return PGM_ADDRESSING;
>>> +    }
>>> +#endif
>>
>> The assumption of PGM_ADDRESSING is incorrect here -- it could still be
>> PGM_PROTECTION, depending on how the page is mapped.
>>
> 
> Interesting, I was only looking at the s390x tlb_fill() implementation. But I 
> assume these checks are performed in common code.

Actually, no.  It's a common bug in our linux-user targets, where we don't fill 
in the SIGSEGV si_code correctly.

See e.g. 8db94ab4e5d.

> Thanks, maybe I can factor that out in a nice way. I guess we could do the 
> access via probe_access_flags() and only on error do the page_get_flags()?

Yes, we could do that.  It's certainly better for !nofault, which is the 
common-case user of this function.


r~