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

[David Hildenbrand <david@redhat.com>]

On 03.03.21 14:28, Thomas Huth wrote:
> 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>
> ---
>   v4: Add logic to inject protection exceptions if necessary
> 
>   target/s390x/cpu.h         |  3 ++
>   target/s390x/excp_helper.c |  3 ++
>   target/s390x/mem_helper.c  | 93 ++++++++++++++++++++++++++++----------
>   3 files changed, 76 insertions(+), 23 deletions(-)
> 
> diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
> index 60d434d5ed..825503c6c0 100644
> --- a/target/s390x/cpu.h
> +++ b/target/s390x/cpu.h
> @@ -114,6 +114,9 @@ struct CPUS390XState {
>   
>       uint64_t diag318_info;
>   
> +    uint64_t tlb_fill_tec;   /* translation exception code during tlb_fill */
> +    int tlb_fill_exc;        /* exception number seen during tlb_fill */
> +
>       /* 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..cf741541d3 100644
> --- a/target/s390x/mem_helper.c
> +++ b/target/s390x/mem_helper.c
> @@ -130,28 +130,62 @@ typedef struct S390Access {
>       int mmu_idx;
>   } S390Access;
>   
> +static bool access_prepare_nf(S390Access *access, CPUS390XState *env,
> +                              bool nofault, vaddr vaddr1, int size,
> +                              MMUAccessType access_type,
> +                              int mmu_idx, uintptr_t ra)
> +{
> +    void *haddr1, *haddr2 = NULL;
> +    int size1, size2;
> +    vaddr vaddr2 = 0;
> +    int flags;
> +
> +    assert(size > 0 && size <= 4096);
> +
> +    size1 = MIN(size, -(vaddr1 | TARGET_PAGE_MASK)),
> +    size2 = size - size1;
> +
> +    flags = probe_access_flags(env, vaddr1, access_type, mmu_idx,
> +                               nofault, &haddr1, ra);
> +    if (unlikely(size2)) {
> +        /* The access crosses page boundaries. */
> +        vaddr2 = wrap_address(env, vaddr1 + size1);
> +        flags |= probe_access_flags(env, vaddr2, access_type, mmu_idx,
> +                                    nofault, &haddr2, ra);
> +    }
> +
> +    if (unlikely(flags & TLB_INVALID_MASK)) {
> +        return false;
> +    }
> +    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 true;
> +}
> +
>   static S390Access access_prepare(CPUS390XState *env, vaddr vaddr, 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,
> -    };
> -
> -    g_assert(size > 0 && size <= 4096);
> -    access.haddr1 = probe_access(env, access.vaddr1, access.size1, access_type,
> -                                 mmu_idx, ra);
> -
> -    if (unlikely(access.size1 != size)) {
> -        /* 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);
> -    }
> -    return access;
> +    S390Access ret;
> +    bool ok = access_prepare_nf(&ret, env, false, vaddr, size,
> +                                access_type, mmu_idx, ra);
> +    assert(ok);
> +    return ret;
>   }
>   
>   /* Helper to handle memset on a single page. */
> @@ -845,8 +879,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;
> +    bool ok;
>   
>       if ((f && s) || extract64(r0, 12, 4)) {
>           tcg_s390_program_interrupt(env, PGM_SPECIFICATION, GETPC());
> @@ -858,13 +894,24 @@ 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);
> +    ok = access_prepare_nf(&srca, env, cco, r2, TARGET_PAGE_SIZE,
> +                           MMU_DATA_LOAD, mmu_idx, ra);
> +    if (!ok) {
> +        return 2;
> +    }
> +    ok = access_prepare_nf(&desta, env, cco, r1, TARGET_PAGE_SIZE,
> +                           MMU_DATA_STORE, mmu_idx, ra);
> +    if (!ok) {
> +        if (env->tlb_fill_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);
> +        }
> +        return 1;
> +    }
>       access_memmove(env, &desta, &srca, ra);
>       return 0; /* data moved */
>   }
> 

As talked with Thomas off-list, there is no trusting on host==NULL
as well (see comment in struct S390Access). host==NULL simply
means we have to do individual ld/st.

The following on top should work. Not perfect, but seems to get
the job done.


 From 056b3c9f2ffd43b10d8293e7143cf7af5d1d5022 Mon Sep 17 00:00:00 2001
From: David Hildenbrand <david@redhat.com>
Date: Thu, 11 Mar 2021 14:44:45 +0100
Subject: [PATCH] fixup

Signed-off-by: David Hildenbrand <david@redhat.com>
---
  target/s390x/mem_helper.c | 48 ++++++++++++++++++++++++---------------
  1 file changed, 30 insertions(+), 18 deletions(-)

diff --git a/target/s390x/mem_helper.c b/target/s390x/mem_helper.c
index cf741541d3..0a9b15ea90 100644
--- a/target/s390x/mem_helper.c
+++ b/target/s390x/mem_helper.c
@@ -130,10 +130,15 @@ typedef struct S390Access {
      int mmu_idx;
  } S390Access;
  
-static bool access_prepare_nf(S390Access *access, CPUS390XState *env,
-                              bool nofault, vaddr vaddr1, int size,
-                              MMUAccessType access_type,
-                              int mmu_idx, uintptr_t ra)
+/*
+ * With nofault=1, return the generated PGM_ exception that would have
+ * been injected into the guest (tec stored in env->tlb_fill_tec);
+ * return 0 if no exception was detected.
+ */
+static int access_prepare_nf(S390Access *access, CPUS390XState *env,
+                             bool nofault, vaddr vaddr1, int size,
+                             MMUAccessType access_type,
+                             int mmu_idx, uintptr_t ra)
  {
      void *haddr1, *haddr2 = NULL;
      int size1, size2;
@@ -145,18 +150,24 @@ static bool access_prepare_nf(S390Access *access, CPUS390XState *env,
      size1 = MIN(size, -(vaddr1 | TARGET_PAGE_MASK)),
      size2 = size - size1;
  
+    env->tlb_fill_exc = 0;
      flags = probe_access_flags(env, vaddr1, access_type, mmu_idx,
                                 nofault, &haddr1, ra);
+    if (env->tlb_fill_exc) {
+        /* We cannot rely on TLB_INVALID_MASK or haddr being NULL. */
+        return env->tlb_fill_exc;
+    }
      if (unlikely(size2)) {
          /* The access crosses page boundaries. */
          vaddr2 = wrap_address(env, vaddr1 + size1);
          flags |= probe_access_flags(env, vaddr2, access_type, mmu_idx,
                                      nofault, &haddr2, ra);
+        if (env->tlb_fill_exc) {
+            /* We cannot rely on TLB_INVALID_MASK or haddr being NULL. */
+            return env->tlb_fill_exc;
+        }
      }
  
-    if (unlikely(flags & TLB_INVALID_MASK)) {
-        return false;
-    }
      if (unlikely(flags & TLB_WATCHPOINT)) {
          /* S390 does not presently use transaction attributes. */
          cpu_check_watchpoint(env_cpu(env), vaddr1, size,
@@ -174,7 +185,7 @@ static bool access_prepare_nf(S390Access *access, CPUS390XState *env,
          .size2 = size2,
          .mmu_idx = mmu_idx
      };
-    return true;
+    return 0;
  }
  
  static S390Access access_prepare(CPUS390XState *env, vaddr vaddr, int size,
@@ -182,9 +193,9 @@ static S390Access access_prepare(CPUS390XState *env, vaddr vaddr, int size,
                                   uintptr_t ra)
  {
      S390Access ret;
-    bool ok = access_prepare_nf(&ret, env, false, vaddr, size,
+    int exc = access_prepare_nf(&ret, env, false, vaddr, size,
                                  access_type, mmu_idx, ra);
-    assert(ok);
+    assert(!exc);
      return ret;
  }
  
@@ -882,7 +893,7 @@ uint32_t HELPER(mvpg)(CPUS390XState *env, uint64_t r0, uint64_t r1, uint64_t r2)
      const bool cco = extract64(r0, 8, 1);
      uintptr_t ra = GETPC();
      S390Access srca, desta;
-    bool ok;
+    int exc;
  
      if ((f && s) || extract64(r0, 12, 4)) {
          tcg_s390_program_interrupt(env, PGM_SPECIFICATION, GETPC());
@@ -896,15 +907,16 @@ uint32_t HELPER(mvpg)(CPUS390XState *env, uint64_t r0, uint64_t r1, uint64_t r2)
       * - Access key handling
       * - Store r1/r2 register identifiers at real location 162
       */
-    ok = access_prepare_nf(&srca, env, cco, r2, TARGET_PAGE_SIZE,
-                           MMU_DATA_LOAD, mmu_idx, ra);
-    if (!ok) {
+    exc = access_prepare_nf(&srca, env, cco, r2, TARGET_PAGE_SIZE,
+                            MMU_DATA_LOAD, mmu_idx, ra);
+    if (exc) {
          return 2;
      }
-    ok = access_prepare_nf(&desta, env, cco, r1, TARGET_PAGE_SIZE,
-                           MMU_DATA_STORE, mmu_idx, ra);
-    if (!ok) {
-        if (env->tlb_fill_exc == PGM_PROTECTION) {
+    exc = access_prepare_nf(&desta, env, cco, r1, TARGET_PAGE_SIZE,
+                            MMU_DATA_STORE, mmu_idx, ra);
+    if (exc) {
+        fprintf(stderr, "Exception: %d\n", exc);
+        if (exc == PGM_PROTECTION) {
              stq_phys(env_cpu(env)->as,
                       env->psa + offsetof(LowCore, trans_exc_code),
                       env->tlb_fill_tec);
-- 
2.29.2



-- 
Thanks,

David / dhildenb