[Qemu-devel] [PATCH v3 6/6] s390x/mmu: Factor out storage key handling

[David Hildenbrand]

Factor it out, add a comment how it all works, and also use it in the
REAL MMU.

Reviewed-by: Cornelia Huck <address@hidden>
Reviewed-by: Thomas Huth <address@hidden>
Signed-off-by: David Hildenbrand <address@hidden>
---
 target/s390x/mmu_helper.c | 115 +++++++++++++++++++++++---------------
 1 file changed, 71 insertions(+), 44 deletions(-)

diff --git a/target/s390x/mmu_helper.c b/target/s390x/mmu_helper.c
index 6cc81a29b6..e9db31faf8 100644
--- a/target/s390x/mmu_helper.c
+++ b/target/s390x/mmu_helper.c
@@ -334,6 +334,75 @@ static int mmu_translate_asce(CPUS390XState *env, 
target_ulong vaddr,
     return r;
 }
 
+static void mmu_handle_skey(target_ulong addr, int rw, int *flags)
+{
+    static S390SKeysClass *skeyclass;
+    static S390SKeysState *ss;
+    uint8_t key;
+    int rc;
+
+    if (unlikely(addr >= ram_size)) {
+        return;
+    }
+
+    if (unlikely(!ss)) {
+        ss = s390_get_skeys_device();
+        skeyclass = S390_SKEYS_GET_CLASS(ss);
+    }
+
+    /*
+     * Whenever we create a new TLB entry, we set the storage key reference
+     * bit. In case we allow write accesses, we set the storage key change
+     * bit. Whenever the guest changes the storage key, we have to flush the
+     * TLBs of all CPUs (the whole TLB or all affected entries), so that the
+     * next reference/change will result in an MMU fault and make us properly
+     * update the storage key here.
+     *
+     * Note 1: "record of references ... is not necessarily accurate",
+     *         "change bit may be set in case no storing has occurred".
+     *         -> We can set reference/change bits even on exceptions.
+     * Note 2: certain accesses seem to ignore storage keys. For example,
+     *         DAT translation does not set reference bits for table accesses.
+     *
+     * TODO: key-controlled protection. Only CPU accesses make use of the
+     *       PSW key. CSS accesses are different - we have to pass in the key.
+     *
+     * TODO: we have races between getting and setting the key.
+     */
+    rc = skeyclass->get_skeys(ss, addr / TARGET_PAGE_SIZE, 1, &key);
+    if (rc) {
+        trace_get_skeys_nonzero(rc);
+        return;
+    }
+
+    switch (rw) {
+    case MMU_DATA_LOAD:
+    case MMU_INST_FETCH:
+        /*
+         * The TLB entry has to remain write-protected on read-faults if
+         * the storage key does not indicate a change already. Otherwise
+         * we might miss setting the change bit on write accesses.
+         */
+        if (!(key & SK_C)) {
+            *flags &= ~PAGE_WRITE;
+        }
+        break;
+    case MMU_DATA_STORE:
+        key |= SK_C;
+        break;
+    default:
+        g_assert_not_reached();
+    }
+
+    /* Any store/fetch sets the reference bit */
+    key |= SK_R;
+
+    rc = skeyclass->set_skeys(ss, addr / TARGET_PAGE_SIZE, 1, &key);
+    if (rc) {
+        trace_set_skeys_nonzero(rc);
+    }
+}
+
 /**
  * Translate a virtual (logical) address into a physical (absolute) address.
  * @param vaddr  the virtual address
@@ -347,16 +416,9 @@ static int mmu_translate_asce(CPUS390XState *env, 
target_ulong vaddr,
 int mmu_translate(CPUS390XState *env, target_ulong vaddr, int rw, uint64_t asc,
                   target_ulong *raddr, int *flags, bool exc)
 {
-    static S390SKeysState *ss;
-    static S390SKeysClass *skeyclass;
     uint64_t asce;
-    uint8_t key;
     int r;
 
-    if (unlikely(!ss)) {
-        ss = s390_get_skeys_device();
-        skeyclass = S390_SKEYS_GET_CLASS(ss);
-    }
 
     *flags = PAGE_READ | PAGE_WRITE | PAGE_EXEC;
     if (is_low_address(vaddr & TARGET_PAGE_MASK) && lowprot_enabled(env, asc)) 
{
@@ -413,42 +475,7 @@ nodat:
     /* Convert real address -> absolute address */
     *raddr = mmu_real2abs(env, *raddr);
 
-    if (*raddr < ram_size) {
-        r = skeyclass->get_skeys(ss, *raddr / TARGET_PAGE_SIZE, 1, &key);
-        if (r) {
-            trace_get_skeys_nonzero(r);
-            return 0;
-        }
-
-        switch (rw) {
-        case MMU_DATA_LOAD:
-        case MMU_INST_FETCH:
-            /*
-             * The TLB entry has to remain write-protected on read-faults if
-             * the storage key does not indicate a change already. Otherwise
-             * we might miss setting the change bit on write accesses.
-             */
-            if (!(key & SK_C)) {
-                *flags &= ~PAGE_WRITE;
-            }
-            break;
-        case MMU_DATA_STORE:
-            key |= SK_C;
-            break;
-        default:
-            g_assert_not_reached();
-        }
-
-        /* Any store/fetch sets the reference bit */
-        key |= SK_R;
-
-        r = skeyclass->set_skeys(ss, *raddr / TARGET_PAGE_SIZE, 1, &key);
-        if (r) {
-            trace_set_skeys_nonzero(r);
-            return 0;
-        }
-    }
-
+    mmu_handle_skey(*raddr, rw, flags);
     return 0;
 }
 
@@ -566,6 +593,6 @@ int mmu_translate_real(CPUS390XState *env, target_ulong 
raddr, int rw,
 
     *addr = mmu_real2abs(env, raddr & TARGET_PAGE_MASK);
 
-    /* TODO: storage key handling */
+    mmu_handle_skey(*addr, rw, flags);
     return 0;
 }
-- 
2.21.0