QEMU-Devel Archive on lore.kernel.org
 help / color / Atom feed
* [Qemu-devel] [PATCH-for-4.2 v2 0/6] s390x/mmu: Storage key reference and change bit handling
@ 2019-08-14  7:23 David Hildenbrand
  2019-08-14  7:23 ` [Qemu-devel] [PATCH-for-4.2 v2 1/6] s390x/mmu: ASC selection in s390_cpu_get_phys_page_debug() David Hildenbrand
                   ` (6 more replies)
  0 siblings, 7 replies; 20+ messages in thread
From: David Hildenbrand @ 2019-08-14  7:23 UTC (permalink / raw)
  To: qemu-devel
  Cc: Thomas Huth, Janosch Frank, David Hildenbrand, Cornelia Huck,
	Halil Pasic, Christian Borntraeger, qemu-s390x,
	Richard Henderson

The first two patches are modified patches from:
    [PATCH-for-4.2 v1 0/9] s390x: MMU changes and extensions

This series primarily fixes minor things in the storage key handling code
in the MMU and implements fairly reliable reference and change bit handling
for TCG. To track the reference and change bit, we have to invalidate
TLB entries whenever the storage key is changed by the guest and make sure
not TLB entry is writable in case the storage key does not indicate a
change already.

With this series, the kvm-unit-test "skey" now passes. \o/

v1 -> v2:
- "s390x/tcg: Rework MMU selection for instruction fetches"
-- Cleanup return value handling
- Added RB's

David Hildenbrand (6):
  s390x/mmu: ASC selection in s390_cpu_get_phys_page_debug()
  s390x/tcg: Rework MMU selection for instruction fetches
  s390x/tcg: Flush the TLB of all CPUs on SSKE and RRBE
  s390x/mmu: Trace the right value if setting/getting the storage key
    fails
  s390x/mmu: Better storage key reference and change bit handling
  s390x/mmu: Factor out storage key handling

 target/s390x/cpu.h        |   7 ++
 target/s390x/helper.c     |   5 ++
 target/s390x/mem_helper.c |   4 ++
 target/s390x/mmu_helper.c | 133 ++++++++++++++++++++++++--------------
 4 files changed, 99 insertions(+), 50 deletions(-)

-- 
2.21.0



^ permalink raw reply	[flat|nested] 20+ messages in thread

* [Qemu-devel] [PATCH-for-4.2 v2 1/6] s390x/mmu: ASC selection in s390_cpu_get_phys_page_debug()
  2019-08-14  7:23 [Qemu-devel] [PATCH-for-4.2 v2 0/6] s390x/mmu: Storage key reference and change bit handling David Hildenbrand
@ 2019-08-14  7:23 ` David Hildenbrand
  2019-08-14  7:23 ` [Qemu-devel] [PATCH-for-4.2 v2 2/6] s390x/tcg: Rework MMU selection for instruction fetches David Hildenbrand
                   ` (5 subsequent siblings)
  6 siblings, 0 replies; 20+ messages in thread
From: David Hildenbrand @ 2019-08-14  7:23 UTC (permalink / raw)
  To: qemu-devel
  Cc: Thomas Huth, Janosch Frank, David Hildenbrand, Cornelia Huck,
	Halil Pasic, Christian Borntraeger, qemu-s390x,
	Richard Henderson

Let's select the ASC before calling the function. This is a prepararion
to remove the ASC magic depending on the access mode from mmu_translate.

There is currently no way to distinguish if we have code or data access.
For now, we were using code access, because especially when debugging with
the gdbstub, we want to read and disassemble what we single-step.

Reviewed-by: Thomas Huth <thuth@redhat.com>
Reviewed-by: Cornelia Huck <cohuck@redhat.com>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
 target/s390x/helper.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/target/s390x/helper.c b/target/s390x/helper.c
index 13ae9909ad..c5fb8966b6 100644
--- a/target/s390x/helper.c
+++ b/target/s390x/helper.c
@@ -58,6 +58,11 @@ hwaddr s390_cpu_get_phys_page_debug(CPUState *cs, vaddr vaddr)
         vaddr &= 0x7fffffff;
     }
 
+    /* We want to read the code (e.g., see what we are single-stepping).*/
+    if (asc != PSW_ASC_HOME) {
+        asc = PSW_ASC_PRIMARY;
+    }
+
     if (mmu_translate(env, vaddr, MMU_INST_FETCH, asc, &raddr, &prot, false)) {
         return -1;
     }
-- 
2.21.0



^ permalink raw reply	[flat|nested] 20+ messages in thread

* [Qemu-devel] [PATCH-for-4.2 v2 2/6] s390x/tcg: Rework MMU selection for instruction fetches
  2019-08-14  7:23 [Qemu-devel] [PATCH-for-4.2 v2 0/6] s390x/mmu: Storage key reference and change bit handling David Hildenbrand
  2019-08-14  7:23 ` [Qemu-devel] [PATCH-for-4.2 v2 1/6] s390x/mmu: ASC selection in s390_cpu_get_phys_page_debug() David Hildenbrand
@ 2019-08-14  7:23 ` David Hildenbrand
  2019-08-14 17:44   ` [Qemu-devel] [qemu-s390x] " Thomas Huth
  2019-08-15 15:43   ` [Qemu-devel] " Cornelia Huck
  2019-08-14  7:23 ` [Qemu-devel] [PATCH-for-4.2 v2 3/6] s390x/tcg: Flush the TLB of all CPUs on SSKE and RRBE David Hildenbrand
                   ` (4 subsequent siblings)
  6 siblings, 2 replies; 20+ messages in thread
From: David Hildenbrand @ 2019-08-14  7:23 UTC (permalink / raw)
  To: qemu-devel
  Cc: Thomas Huth, Janosch Frank, David Hildenbrand, Cornelia Huck,
	Halil Pasic, Christian Borntraeger, qemu-s390x,
	Richard Henderson

Instructions are always fetched from primary address space, except when
in home address mode. Perform the selection directly in cpu_mmu_index().

get_mem_index() is only used to perform data access, instructions are
fetched via cpu_lduw_code(), which translates to cpu_mmu_index(env, true).

We don't care about restricting the access permissions of the TLB
entries anymore, as we no longer enter PRIMARY entries into the
SECONDARY MMU. Cleanup related code a bit.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 target/s390x/cpu.h        |  7 +++++++
 target/s390x/mmu_helper.c | 38 +++++++++++++++-----------------------
 2 files changed, 22 insertions(+), 23 deletions(-)

diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
index a606547b4d..c34992bb2e 100644
--- a/target/s390x/cpu.h
+++ b/target/s390x/cpu.h
@@ -332,6 +332,13 @@ static inline int cpu_mmu_index(CPUS390XState *env, bool ifetch)
         return MMU_REAL_IDX;
     }
 
+    if (ifetch) {
+        if ((env->psw.mask & PSW_MASK_ASC) == PSW_ASC_HOME) {
+            return MMU_HOME_IDX;
+        }
+        return MMU_PRIMARY_IDX;
+    }
+
     switch (env->psw.mask & PSW_MASK_ASC) {
     case PSW_ASC_PRIMARY:
         return MMU_PRIMARY_IDX;
diff --git a/target/s390x/mmu_helper.c b/target/s390x/mmu_helper.c
index 6e9c4d6151..c34e8d2021 100644
--- a/target/s390x/mmu_helper.c
+++ b/target/s390x/mmu_helper.c
@@ -349,8 +349,9 @@ int mmu_translate(CPUS390XState *env, target_ulong vaddr, int rw, uint64_t asc,
 {
     static S390SKeysState *ss;
     static S390SKeysClass *skeyclass;
-    int r = -1;
+    uint64_t asce;
     uint8_t key;
+    int r;
 
     if (unlikely(!ss)) {
         ss = s390_get_skeys_device();
@@ -380,36 +381,21 @@ int mmu_translate(CPUS390XState *env, target_ulong vaddr, int rw, uint64_t asc,
 
     if (!(env->psw.mask & PSW_MASK_DAT)) {
         *raddr = vaddr;
-        r = 0;
-        goto out;
+        goto nodat;
     }
 
     switch (asc) {
     case PSW_ASC_PRIMARY:
         PTE_DPRINTF("%s: asc=primary\n", __func__);
-        r = mmu_translate_asce(env, vaddr, asc, env->cregs[1], raddr, flags,
-                               rw, exc);
+        asce = env->cregs[1];
         break;
     case PSW_ASC_HOME:
         PTE_DPRINTF("%s: asc=home\n", __func__);
-        r = mmu_translate_asce(env, vaddr, asc, env->cregs[13], raddr, flags,
-                               rw, exc);
+        asce = env->cregs[13];
         break;
     case PSW_ASC_SECONDARY:
         PTE_DPRINTF("%s: asc=secondary\n", __func__);
-        /*
-         * Instruction: Primary
-         * Data: Secondary
-         */
-        if (rw == MMU_INST_FETCH) {
-            r = mmu_translate_asce(env, vaddr, PSW_ASC_PRIMARY, env->cregs[1],
-                                   raddr, flags, rw, exc);
-            *flags &= ~(PAGE_READ | PAGE_WRITE);
-        } else {
-            r = mmu_translate_asce(env, vaddr, PSW_ASC_SECONDARY, env->cregs[7],
-                                   raddr, flags, rw, exc);
-            *flags &= ~(PAGE_EXEC);
-        }
+        asce = env->cregs[7];
         break;
     case PSW_ASC_ACCREG:
     default:
@@ -417,11 +403,17 @@ int mmu_translate(CPUS390XState *env, target_ulong vaddr, int rw, uint64_t asc,
         break;
     }
 
- out:
+    /* perform the DAT translation */
+    r = mmu_translate_asce(env, vaddr, asc, asce, raddr, flags, rw, exc);
+    if (r) {
+        return r;
+    }
+
+nodat:
     /* Convert real address -> absolute address */
     *raddr = mmu_real2abs(env, *raddr);
 
-    if (r == 0 && *raddr < ram_size) {
+    if (*raddr < ram_size) {
         if (skeyclass->get_skeys(ss, *raddr / TARGET_PAGE_SIZE, 1, &key)) {
             trace_get_skeys_nonzero(r);
             return 0;
@@ -441,7 +433,7 @@ int mmu_translate(CPUS390XState *env, target_ulong vaddr, int rw, uint64_t asc,
         }
     }
 
-    return r;
+    return 0;
 }
 
 /**
-- 
2.21.0



^ permalink raw reply	[flat|nested] 20+ messages in thread

* [Qemu-devel] [PATCH-for-4.2 v2 3/6] s390x/tcg: Flush the TLB of all CPUs on SSKE and RRBE
  2019-08-14  7:23 [Qemu-devel] [PATCH-for-4.2 v2 0/6] s390x/mmu: Storage key reference and change bit handling David Hildenbrand
  2019-08-14  7:23 ` [Qemu-devel] [PATCH-for-4.2 v2 1/6] s390x/mmu: ASC selection in s390_cpu_get_phys_page_debug() David Hildenbrand
  2019-08-14  7:23 ` [Qemu-devel] [PATCH-for-4.2 v2 2/6] s390x/tcg: Rework MMU selection for instruction fetches David Hildenbrand
@ 2019-08-14  7:23 ` David Hildenbrand
  2019-08-14 10:06   ` Alex Bennée
  2019-08-14  7:23 ` [Qemu-devel] [PATCH-for-4.2 v2 4/6] s390x/mmu: Trace the right value if setting/getting the storage key fails David Hildenbrand
                   ` (3 subsequent siblings)
  6 siblings, 1 reply; 20+ messages in thread
From: David Hildenbrand @ 2019-08-14  7:23 UTC (permalink / raw)
  To: qemu-devel
  Cc: Thomas Huth, Janosch Frank, David Hildenbrand, Cornelia Huck,
	Halil Pasic, Christian Borntraeger, qemu-s390x,
	Richard Henderson

Whenever we modify a storage key, we shuld flush the TLBs of all CPUs,
so the MMU fault handling code can properly consider the changed storage
key (to e.g., properly set the reference and change bit on the next
accesses).

These functions are barely used in modern Linux guests, so the performance
implications are neglectable for now.

This is a preparation for better reference and change bit handling for
TCG, which will require more MMU changes.

Reviewed-by: Cornelia Huck <cohuck@redhat.com>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
 target/s390x/mem_helper.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/target/s390x/mem_helper.c b/target/s390x/mem_helper.c
index 29d9eaa5b7..ed54265e03 100644
--- a/target/s390x/mem_helper.c
+++ b/target/s390x/mem_helper.c
@@ -1815,6 +1815,8 @@ void HELPER(sske)(CPUS390XState *env, uint64_t r1, uint64_t r2)
 
     key = (uint8_t) r1;
     skeyclass->set_skeys(ss, addr / TARGET_PAGE_SIZE, 1, &key);
+    /* TODO: Flush only entries with this target address */
+    tlb_flush_all_cpus_synced(env_cpu(env));
 }
 
 /* reset reference bit extended */
@@ -1843,6 +1845,8 @@ uint32_t HELPER(rrbe)(CPUS390XState *env, uint64_t r2)
     if (skeyclass->set_skeys(ss, r2 / TARGET_PAGE_SIZE, 1, &key)) {
         return 0;
     }
+    /* TODO: Flush only entries with this target address */
+    tlb_flush_all_cpus_synced(env_cpu(env));
 
     /*
      * cc
-- 
2.21.0



^ permalink raw reply	[flat|nested] 20+ messages in thread

* [Qemu-devel] [PATCH-for-4.2 v2 4/6] s390x/mmu: Trace the right value if setting/getting the storage key fails
  2019-08-14  7:23 [Qemu-devel] [PATCH-for-4.2 v2 0/6] s390x/mmu: Storage key reference and change bit handling David Hildenbrand
                   ` (2 preceding siblings ...)
  2019-08-14  7:23 ` [Qemu-devel] [PATCH-for-4.2 v2 3/6] s390x/tcg: Flush the TLB of all CPUs on SSKE and RRBE David Hildenbrand
@ 2019-08-14  7:23 ` David Hildenbrand
  2019-08-14 17:50   ` [Qemu-devel] [qemu-s390x] " Thomas Huth
  2019-08-14  7:23 ` [Qemu-devel] [PATCH-for-4.2 v2 5/6] s390x/mmu: Better storage key reference and change bit handling David Hildenbrand
                   ` (2 subsequent siblings)
  6 siblings, 1 reply; 20+ messages in thread
From: David Hildenbrand @ 2019-08-14  7:23 UTC (permalink / raw)
  To: qemu-devel
  Cc: Thomas Huth, Janosch Frank, David Hildenbrand, Cornelia Huck,
	Halil Pasic, Christian Borntraeger, qemu-s390x,
	Richard Henderson

We want to trace the actual return value, not "0".

Reviewed-by: Cornelia Huck <cohuck@redhat.com>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
 target/s390x/mmu_helper.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/target/s390x/mmu_helper.c b/target/s390x/mmu_helper.c
index c34e8d2021..d22c6b9c81 100644
--- a/target/s390x/mmu_helper.c
+++ b/target/s390x/mmu_helper.c
@@ -414,7 +414,8 @@ nodat:
     *raddr = mmu_real2abs(env, *raddr);
 
     if (*raddr < ram_size) {
-        if (skeyclass->get_skeys(ss, *raddr / TARGET_PAGE_SIZE, 1, &key)) {
+        r = skeyclass->get_skeys(ss, *raddr / TARGET_PAGE_SIZE, 1, &key);
+        if (r) {
             trace_get_skeys_nonzero(r);
             return 0;
         }
@@ -427,7 +428,8 @@ nodat:
             key |= SK_C;
         }
 
-        if (skeyclass->set_skeys(ss, *raddr / TARGET_PAGE_SIZE, 1, &key)) {
+        r = skeyclass->set_skeys(ss, *raddr / TARGET_PAGE_SIZE, 1, &key);
+        if (r) {
             trace_set_skeys_nonzero(r);
             return 0;
         }
-- 
2.21.0



^ permalink raw reply	[flat|nested] 20+ messages in thread

* [Qemu-devel] [PATCH-for-4.2 v2 5/6] s390x/mmu: Better storage key reference and change bit handling
  2019-08-14  7:23 [Qemu-devel] [PATCH-for-4.2 v2 0/6] s390x/mmu: Storage key reference and change bit handling David Hildenbrand
                   ` (3 preceding siblings ...)
  2019-08-14  7:23 ` [Qemu-devel] [PATCH-for-4.2 v2 4/6] s390x/mmu: Trace the right value if setting/getting the storage key fails David Hildenbrand
@ 2019-08-14  7:23 ` David Hildenbrand
  2019-08-14  7:23 ` [Qemu-devel] [PATCH-for-4.2 v2 6/6] s390x/mmu: Factor out storage key handling David Hildenbrand
  2019-08-19 16:36 ` [Qemu-devel] [PATCH-for-4.2 v2 0/6] s390x/mmu: Storage key reference and change bit handling Cornelia Huck
  6 siblings, 0 replies; 20+ messages in thread
From: David Hildenbrand @ 2019-08-14  7:23 UTC (permalink / raw)
  To: qemu-devel
  Cc: Thomas Huth, Janosch Frank, David Hildenbrand, Cornelia Huck,
	Halil Pasic, Christian Borntraeger, qemu-s390x,
	Richard Henderson

Any access sets the reference bit. In case we have a read-fault, we
should not allow writes to the TLB entry if the change bit was not
already set.

This is a preparation for proper storage-key reference/change bit handling
in TCG and a fix for KVM whereby read accesses would set the change
bit (old KVM versions without the ioctl to carry out the translation).

Reviewed-by: Cornelia Huck <cohuck@redhat.com>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
 target/s390x/mmu_helper.c | 24 +++++++++++++++++++-----
 1 file changed, 19 insertions(+), 5 deletions(-)

diff --git a/target/s390x/mmu_helper.c b/target/s390x/mmu_helper.c
index d22c6b9c81..6cc81a29b6 100644
--- a/target/s390x/mmu_helper.c
+++ b/target/s390x/mmu_helper.c
@@ -420,14 +420,28 @@ nodat:
             return 0;
         }
 
-        if (*flags & PAGE_READ) {
-            key |= SK_R;
-        }
-
-        if (*flags & PAGE_WRITE) {
+        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);
-- 
2.21.0



^ permalink raw reply	[flat|nested] 20+ messages in thread

* [Qemu-devel] [PATCH-for-4.2 v2 6/6] s390x/mmu: Factor out storage key handling
  2019-08-14  7:23 [Qemu-devel] [PATCH-for-4.2 v2 0/6] s390x/mmu: Storage key reference and change bit handling David Hildenbrand
                   ` (4 preceding siblings ...)
  2019-08-14  7:23 ` [Qemu-devel] [PATCH-for-4.2 v2 5/6] s390x/mmu: Better storage key reference and change bit handling David Hildenbrand
@ 2019-08-14  7:23 ` David Hildenbrand
  2019-08-14 18:01   ` [Qemu-devel] [qemu-s390x] " Thomas Huth
  2019-08-19 16:36 ` [Qemu-devel] [PATCH-for-4.2 v2 0/6] s390x/mmu: Storage key reference and change bit handling Cornelia Huck
  6 siblings, 1 reply; 20+ messages in thread
From: David Hildenbrand @ 2019-08-14  7:23 UTC (permalink / raw)
  To: qemu-devel
  Cc: Thomas Huth, Janosch Frank, David Hildenbrand, Cornelia Huck,
	Halil Pasic, Christian Borntraeger, qemu-s390x,
	Richard Henderson

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

Reviewed-by: Cornelia Huck <cohuck@redhat.com>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
 target/s390x/mmu_helper.c | 113 +++++++++++++++++++++++---------------
 1 file changed, 69 insertions(+), 44 deletions(-)

diff --git a/target/s390x/mmu_helper.c b/target/s390x/mmu_helper.c
index 6cc81a29b6..e125837d68 100644
--- a/target/s390x/mmu_helper.c
+++ b/target/s390x/mmu_helper.c
@@ -334,6 +334,73 @@ 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(!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.
+     */
+    if (addr < ram_size) {
+        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 +414,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 +473,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 +591,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



^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [Qemu-devel] [PATCH-for-4.2 v2 3/6] s390x/tcg: Flush the TLB of all CPUs on SSKE and RRBE
  2019-08-14  7:23 ` [Qemu-devel] [PATCH-for-4.2 v2 3/6] s390x/tcg: Flush the TLB of all CPUs on SSKE and RRBE David Hildenbrand
@ 2019-08-14 10:06   ` Alex Bennée
  2019-08-14 10:21     ` David Hildenbrand
  0 siblings, 1 reply; 20+ messages in thread
From: Alex Bennée @ 2019-08-14 10:06 UTC (permalink / raw)
  To: qemu-devel
  Cc: Thomas Huth, Janosch Frank, David Hildenbrand, Cornelia Huck,
	Halil Pasic, Christian Borntraeger, qemu-s390x,
	Richard Henderson


David Hildenbrand <david@redhat.com> writes:

> Whenever we modify a storage key, we shuld flush the TLBs of all CPUs,
> so the MMU fault handling code can properly consider the changed storage
> key (to e.g., properly set the reference and change bit on the next
> accesses).
>
> These functions are barely used in modern Linux guests, so the performance
> implications are neglectable for now.
>
> This is a preparation for better reference and change bit handling for
> TCG, which will require more MMU changes.
>
> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  target/s390x/mem_helper.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/target/s390x/mem_helper.c b/target/s390x/mem_helper.c
> index 29d9eaa5b7..ed54265e03 100644
> --- a/target/s390x/mem_helper.c
> +++ b/target/s390x/mem_helper.c
> @@ -1815,6 +1815,8 @@ void HELPER(sske)(CPUS390XState *env, uint64_t r1, uint64_t r2)
>
>      key = (uint8_t) r1;
>      skeyclass->set_skeys(ss, addr / TARGET_PAGE_SIZE, 1, &key);
> +    /* TODO: Flush only entries with this target address */
> +    tlb_flush_all_cpus_synced(env_cpu(env));

Doesn't:

  tlb_flush_page_all_cpus_synced(env_cpu(env), addr & TARGET_PAGE_MASK);

do what you want here?

>  }
>
>  /* reset reference bit extended */
> @@ -1843,6 +1845,8 @@ uint32_t HELPER(rrbe)(CPUS390XState *env, uint64_t r2)
>      if (skeyclass->set_skeys(ss, r2 / TARGET_PAGE_SIZE, 1, &key)) {
>          return 0;
>      }
> +    /* TODO: Flush only entries with this target address */
> +    tlb_flush_all_cpus_synced(env_cpu(env));
>
>      /*
>       * cc


--
Alex Bennée


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [Qemu-devel] [PATCH-for-4.2 v2 3/6] s390x/tcg: Flush the TLB of all CPUs on SSKE and RRBE
  2019-08-14 10:06   ` Alex Bennée
@ 2019-08-14 10:21     ` David Hildenbrand
  2019-08-14 10:44       ` Alex Bennée
  0 siblings, 1 reply; 20+ messages in thread
From: David Hildenbrand @ 2019-08-14 10:21 UTC (permalink / raw)
  To: Alex Bennée, qemu-devel
  Cc: Thomas Huth, Janosch Frank, Cornelia Huck, Halil Pasic,
	Christian Borntraeger, qemu-s390x, Richard Henderson

On 14.08.19 12:06, Alex Bennée wrote:
> 
> David Hildenbrand <david@redhat.com> writes:
> 
>> Whenever we modify a storage key, we shuld flush the TLBs of all CPUs,
>> so the MMU fault handling code can properly consider the changed storage
>> key (to e.g., properly set the reference and change bit on the next
>> accesses).
>>
>> These functions are barely used in modern Linux guests, so the performance
>> implications are neglectable for now.
>>
>> This is a preparation for better reference and change bit handling for
>> TCG, which will require more MMU changes.
>>
>> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
>> Signed-off-by: David Hildenbrand <david@redhat.com>
>> ---
>>  target/s390x/mem_helper.c | 4 ++++
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/target/s390x/mem_helper.c b/target/s390x/mem_helper.c
>> index 29d9eaa5b7..ed54265e03 100644
>> --- a/target/s390x/mem_helper.c
>> +++ b/target/s390x/mem_helper.c
>> @@ -1815,6 +1815,8 @@ void HELPER(sske)(CPUS390XState *env, uint64_t r1, uint64_t r2)
>>
>>      key = (uint8_t) r1;
>>      skeyclass->set_skeys(ss, addr / TARGET_PAGE_SIZE, 1, &key);
>> +    /* TODO: Flush only entries with this target address */
>> +    tlb_flush_all_cpus_synced(env_cpu(env));
> 
> Doesn't:
> 
>   tlb_flush_page_all_cpus_synced(env_cpu(env), addr & TARGET_PAGE_MASK);
> 
> do what you want here?

I would have to flush all TLB entries that target this physical page,
not the entry of the single virtual page. So that does, unfortunately,
not work.

-- 

Thanks,

David / dhildenb


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [Qemu-devel] [PATCH-for-4.2 v2 3/6] s390x/tcg: Flush the TLB of all CPUs on SSKE and RRBE
  2019-08-14 10:21     ` David Hildenbrand
@ 2019-08-14 10:44       ` Alex Bennée
  2019-08-14 10:51         ` David Hildenbrand
  0 siblings, 1 reply; 20+ messages in thread
From: Alex Bennée @ 2019-08-14 10:44 UTC (permalink / raw)
  To: David Hildenbrand
  Cc: Thomas Huth, Janosch Frank, Cornelia Huck, qemu-devel,
	Halil Pasic, Christian Borntraeger, qemu-s390x,
	Richard Henderson


David Hildenbrand <david@redhat.com> writes:

> On 14.08.19 12:06, Alex Bennée wrote:
>>
>> David Hildenbrand <david@redhat.com> writes:
>>
>>> Whenever we modify a storage key, we shuld flush the TLBs of all CPUs,
>>> so the MMU fault handling code can properly consider the changed storage
>>> key (to e.g., properly set the reference and change bit on the next
>>> accesses).
>>>
>>> These functions are barely used in modern Linux guests, so the performance
>>> implications are neglectable for now.
>>>
>>> This is a preparation for better reference and change bit handling for
>>> TCG, which will require more MMU changes.
>>>
>>> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
>>> Signed-off-by: David Hildenbrand <david@redhat.com>
>>> ---
>>>  target/s390x/mem_helper.c | 4 ++++
>>>  1 file changed, 4 insertions(+)
>>>
>>> diff --git a/target/s390x/mem_helper.c b/target/s390x/mem_helper.c
>>> index 29d9eaa5b7..ed54265e03 100644
>>> --- a/target/s390x/mem_helper.c
>>> +++ b/target/s390x/mem_helper.c
>>> @@ -1815,6 +1815,8 @@ void HELPER(sske)(CPUS390XState *env, uint64_t r1, uint64_t r2)
>>>
>>>      key = (uint8_t) r1;
>>>      skeyclass->set_skeys(ss, addr / TARGET_PAGE_SIZE, 1, &key);
>>> +    /* TODO: Flush only entries with this target address */
>>> +    tlb_flush_all_cpus_synced(env_cpu(env));
>>
>> Doesn't:
>>
>>   tlb_flush_page_all_cpus_synced(env_cpu(env), addr & TARGET_PAGE_MASK);
>>
>> do what you want here?
>
> I would have to flush all TLB entries that target this physical page,
> not the entry of the single virtual page. So that does, unfortunately,
> not work.

Ahh I see. Well maybe that should be the comment instead:

  /*
   * As we can only flush by virtual address and not all the entries
   * that point to a physical address we have to flush the whole TLB
   * here.
   */
   tlb_flush_all_cpus_synced(env_cpu(env));

?


--
Alex Bennée


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [Qemu-devel] [PATCH-for-4.2 v2 3/6] s390x/tcg: Flush the TLB of all CPUs on SSKE and RRBE
  2019-08-14 10:44       ` Alex Bennée
@ 2019-08-14 10:51         ` David Hildenbrand
  0 siblings, 0 replies; 20+ messages in thread
From: David Hildenbrand @ 2019-08-14 10:51 UTC (permalink / raw)
  To: Alex Bennée
  Cc: Thomas Huth, Janosch Frank, Cornelia Huck, qemu-devel,
	Halil Pasic, Christian Borntraeger, qemu-s390x,
	Richard Henderson

On 14.08.19 12:44, Alex Bennée wrote:
> 
> David Hildenbrand <david@redhat.com> writes:
> 
>> On 14.08.19 12:06, Alex Bennée wrote:
>>>
>>> David Hildenbrand <david@redhat.com> writes:
>>>
>>>> Whenever we modify a storage key, we shuld flush the TLBs of all CPUs,
>>>> so the MMU fault handling code can properly consider the changed storage
>>>> key (to e.g., properly set the reference and change bit on the next
>>>> accesses).
>>>>
>>>> These functions are barely used in modern Linux guests, so the performance
>>>> implications are neglectable for now.
>>>>
>>>> This is a preparation for better reference and change bit handling for
>>>> TCG, which will require more MMU changes.
>>>>
>>>> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
>>>> Signed-off-by: David Hildenbrand <david@redhat.com>
>>>> ---
>>>>  target/s390x/mem_helper.c | 4 ++++
>>>>  1 file changed, 4 insertions(+)
>>>>
>>>> diff --git a/target/s390x/mem_helper.c b/target/s390x/mem_helper.c
>>>> index 29d9eaa5b7..ed54265e03 100644
>>>> --- a/target/s390x/mem_helper.c
>>>> +++ b/target/s390x/mem_helper.c
>>>> @@ -1815,6 +1815,8 @@ void HELPER(sske)(CPUS390XState *env, uint64_t r1, uint64_t r2)
>>>>
>>>>      key = (uint8_t) r1;
>>>>      skeyclass->set_skeys(ss, addr / TARGET_PAGE_SIZE, 1, &key);
>>>> +    /* TODO: Flush only entries with this target address */
>>>> +    tlb_flush_all_cpus_synced(env_cpu(env));
>>>
>>> Doesn't:
>>>
>>>   tlb_flush_page_all_cpus_synced(env_cpu(env), addr & TARGET_PAGE_MASK);
>>>
>>> do what you want here?
>>
>> I would have to flush all TLB entries that target this physical page,
>> not the entry of the single virtual page. So that does, unfortunately,
>> not work.
> 
> Ahh I see. Well maybe that should be the comment instead:
> 
>   /*
>    * As we can only flush by virtual address and not all the entries
>    * that point to a physical address we have to flush the whole TLB
>    * here.
>    */
>    tlb_flush_all_cpus_synced(env_cpu(env));
> 
> ?

I think "entries that target this address" makes it clear that we are
talking about the TLB entry target address, not the virtual source
address. But I can adjust it. Thanks.

-- 

Thanks,

David / dhildenb


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [Qemu-devel] [qemu-s390x] [PATCH-for-4.2 v2 2/6] s390x/tcg: Rework MMU selection for instruction fetches
  2019-08-14  7:23 ` [Qemu-devel] [PATCH-for-4.2 v2 2/6] s390x/tcg: Rework MMU selection for instruction fetches David Hildenbrand
@ 2019-08-14 17:44   ` " Thomas Huth
  2019-08-15 15:43   ` [Qemu-devel] " Cornelia Huck
  1 sibling, 0 replies; 20+ messages in thread
From: Thomas Huth @ 2019-08-14 17:44 UTC (permalink / raw)
  To: David Hildenbrand, qemu-devel
  Cc: Janosch Frank, Cornelia Huck, Halil Pasic, Christian Borntraeger,
	qemu-s390x, Richard Henderson

On 8/14/19 9:23 AM, David Hildenbrand wrote:
> Instructions are always fetched from primary address space, except when
> in home address mode. Perform the selection directly in cpu_mmu_index().
> 
> get_mem_index() is only used to perform data access, instructions are
> fetched via cpu_lduw_code(), which translates to cpu_mmu_index(env, true).
> 
> We don't care about restricting the access permissions of the TLB
> entries anymore, as we no longer enter PRIMARY entries into the
> SECONDARY MMU. Cleanup related code a bit.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  target/s390x/cpu.h        |  7 +++++++
>  target/s390x/mmu_helper.c | 38 +++++++++++++++-----------------------
>  2 files changed, 22 insertions(+), 23 deletions(-)

Reviewed-by: Thomas Huth <thuth@redhat.com>


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [Qemu-devel] [qemu-s390x] [PATCH-for-4.2 v2 4/6] s390x/mmu: Trace the right value if setting/getting the storage key fails
  2019-08-14  7:23 ` [Qemu-devel] [PATCH-for-4.2 v2 4/6] s390x/mmu: Trace the right value if setting/getting the storage key fails David Hildenbrand
@ 2019-08-14 17:50   ` " Thomas Huth
  2019-08-15 15:39     ` Cornelia Huck
  0 siblings, 1 reply; 20+ messages in thread
From: Thomas Huth @ 2019-08-14 17:50 UTC (permalink / raw)
  To: David Hildenbrand, qemu-devel
  Cc: Janosch Frank, Cornelia Huck, Halil Pasic, Christian Borntraeger,
	qemu-s390x, Richard Henderson

On 8/14/19 9:23 AM, David Hildenbrand wrote:
> We want to trace the actual return value, not "0".
> 
> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  target/s390x/mmu_helper.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/target/s390x/mmu_helper.c b/target/s390x/mmu_helper.c
> index c34e8d2021..d22c6b9c81 100644
> --- a/target/s390x/mmu_helper.c
> +++ b/target/s390x/mmu_helper.c
> @@ -414,7 +414,8 @@ nodat:
>      *raddr = mmu_real2abs(env, *raddr);
>  
>      if (*raddr < ram_size) {
> -        if (skeyclass->get_skeys(ss, *raddr / TARGET_PAGE_SIZE, 1, &key)) {
> +        r = skeyclass->get_skeys(ss, *raddr / TARGET_PAGE_SIZE, 1, &key);
> +        if (r) {
>              trace_get_skeys_nonzero(r);
>              return 0;
>          }
> @@ -427,7 +428,8 @@ nodat:
>              key |= SK_C;
>          }
>  
> -        if (skeyclass->set_skeys(ss, *raddr / TARGET_PAGE_SIZE, 1, &key)) {
> +        r = skeyclass->set_skeys(ss, *raddr / TARGET_PAGE_SIZE, 1, &key);
> +        if (r) {
>              trace_set_skeys_nonzero(r);
>              return 0;
>          }
> 

Fixes: 0f5f669147b52f89928bdf180165f74c4219210e
Reviewed-by: Thomas Huth <thuth@redhat.com>


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [Qemu-devel] [qemu-s390x] [PATCH-for-4.2 v2 6/6] s390x/mmu: Factor out storage key handling
  2019-08-14  7:23 ` [Qemu-devel] [PATCH-for-4.2 v2 6/6] s390x/mmu: Factor out storage key handling David Hildenbrand
@ 2019-08-14 18:01   ` " Thomas Huth
  2019-08-14 18:18     ` David Hildenbrand
  0 siblings, 1 reply; 20+ messages in thread
From: Thomas Huth @ 2019-08-14 18:01 UTC (permalink / raw)
  To: David Hildenbrand, qemu-devel
  Cc: Janosch Frank, Cornelia Huck, Halil Pasic, Christian Borntraeger,
	qemu-s390x, Richard Henderson

On 8/14/19 9:23 AM, David Hildenbrand wrote:
> Factor it out, add a comment how it all works, and also use it in the
> REAL MMU.
> 
> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  target/s390x/mmu_helper.c | 113 +++++++++++++++++++++++---------------
>  1 file changed, 69 insertions(+), 44 deletions(-)
> 
> diff --git a/target/s390x/mmu_helper.c b/target/s390x/mmu_helper.c
> index 6cc81a29b6..e125837d68 100644
> --- a/target/s390x/mmu_helper.c
> +++ b/target/s390x/mmu_helper.c
> @@ -334,6 +334,73 @@ 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(!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.
> +     */
> +    if (addr < ram_size) {

If you want to get rid of some indentation, you could do an early return
if (addr >= ram_size) here instead.

Anyway, good idea to refactor this code, so also in its current shape:

Reviewed-by: Thomas Huth <thuth@redhat.com>


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [Qemu-devel] [qemu-s390x] [PATCH-for-4.2 v2 6/6] s390x/mmu: Factor out storage key handling
  2019-08-14 18:01   ` [Qemu-devel] [qemu-s390x] " Thomas Huth
@ 2019-08-14 18:18     ` David Hildenbrand
  0 siblings, 0 replies; 20+ messages in thread
From: David Hildenbrand @ 2019-08-14 18:18 UTC (permalink / raw)
  To: Thomas Huth, qemu-devel
  Cc: Janosch Frank, Cornelia Huck, Halil Pasic, Christian Borntraeger,
	qemu-s390x, Richard Henderson

On 14.08.19 20:01, Thomas Huth wrote:
> On 8/14/19 9:23 AM, David Hildenbrand wrote:
>> Factor it out, add a comment how it all works, and also use it in the
>> REAL MMU.
>>
>> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
>> Signed-off-by: David Hildenbrand <david@redhat.com>
>> ---
>>  target/s390x/mmu_helper.c | 113 +++++++++++++++++++++++---------------
>>  1 file changed, 69 insertions(+), 44 deletions(-)
>>
>> diff --git a/target/s390x/mmu_helper.c b/target/s390x/mmu_helper.c
>> index 6cc81a29b6..e125837d68 100644
>> --- a/target/s390x/mmu_helper.c
>> +++ b/target/s390x/mmu_helper.c
>> @@ -334,6 +334,73 @@ 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(!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.
>> +     */
>> +    if (addr < ram_size) {
> 
> If you want to get rid of some indentation, you could do an early return
> if (addr >= ram_size) here instead.

Right, that makes a lot of sense, will do this. Thanks!

> 
> Anyway, good idea to refactor this code, so also in its current shape:
> 
> Reviewed-by: Thomas Huth <thuth@redhat.com>
> 


-- 

Thanks,

David / dhildenb


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [Qemu-devel] [qemu-s390x] [PATCH-for-4.2 v2 4/6] s390x/mmu: Trace the right value if setting/getting the storage key fails
  2019-08-14 17:50   ` [Qemu-devel] [qemu-s390x] " Thomas Huth
@ 2019-08-15 15:39     ` Cornelia Huck
  0 siblings, 0 replies; 20+ messages in thread
From: Cornelia Huck @ 2019-08-15 15:39 UTC (permalink / raw)
  To: Thomas Huth
  Cc: Janosch Frank, David Hildenbrand, qemu-devel, Halil Pasic,
	Christian Borntraeger, qemu-s390x, Richard Henderson

On Wed, 14 Aug 2019 19:50:44 +0200
Thomas Huth <thuth@redhat.com> wrote:

> On 8/14/19 9:23 AM, David Hildenbrand wrote:
> > We want to trace the actual return value, not "0".
> > 
> > Reviewed-by: Cornelia Huck <cohuck@redhat.com>
> > Signed-off-by: David Hildenbrand <david@redhat.com>
> > ---
> >  target/s390x/mmu_helper.c | 6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)

> Fixes: 0f5f669147b52f89928bdf180165f74c4219210e

Better:
Fixes: 0f5f669147b5 ("s390x: Enable new s390-storage-keys device")

> Reviewed-by: Thomas Huth <thuth@redhat.com>



^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [Qemu-devel] [PATCH-for-4.2 v2 2/6] s390x/tcg: Rework MMU selection for instruction fetches
  2019-08-14  7:23 ` [Qemu-devel] [PATCH-for-4.2 v2 2/6] s390x/tcg: Rework MMU selection for instruction fetches David Hildenbrand
  2019-08-14 17:44   ` [Qemu-devel] [qemu-s390x] " Thomas Huth
@ 2019-08-15 15:43   ` " Cornelia Huck
  2019-08-15 16:52     ` David Hildenbrand
  1 sibling, 1 reply; 20+ messages in thread
From: Cornelia Huck @ 2019-08-15 15:43 UTC (permalink / raw)
  To: David Hildenbrand
  Cc: Thomas Huth, Janosch Frank, qemu-devel, Halil Pasic,
	Christian Borntraeger, qemu-s390x, Richard Henderson

On Wed, 14 Aug 2019 09:23:51 +0200
David Hildenbrand <david@redhat.com> wrote:

> Instructions are always fetched from primary address space, except when
> in home address mode. Perform the selection directly in cpu_mmu_index().
> 
> get_mem_index() is only used to perform data access, instructions are
> fetched via cpu_lduw_code(), which translates to cpu_mmu_index(env, true).
> 
> We don't care about restricting the access permissions of the TLB
> entries anymore, as we no longer enter PRIMARY entries into the
> SECONDARY MMU. Cleanup related code a bit.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  target/s390x/cpu.h        |  7 +++++++
>  target/s390x/mmu_helper.c | 38 +++++++++++++++-----------------------
>  2 files changed, 22 insertions(+), 23 deletions(-)
> 

(...)

> diff --git a/target/s390x/mmu_helper.c b/target/s390x/mmu_helper.c
> index 6e9c4d6151..c34e8d2021 100644
> --- a/target/s390x/mmu_helper.c
> +++ b/target/s390x/mmu_helper.c
> @@ -349,8 +349,9 @@ int mmu_translate(CPUS390XState *env, target_ulong vaddr, int rw, uint64_t asc,
>  {
>      static S390SKeysState *ss;
>      static S390SKeysClass *skeyclass;
> -    int r = -1;
> +    uint64_t asce;
>      uint8_t key;
> +    int r;
>  
>      if (unlikely(!ss)) {
>          ss = s390_get_skeys_device();
> @@ -380,36 +381,21 @@ int mmu_translate(CPUS390XState *env, target_ulong vaddr, int rw, uint64_t asc,
>  
>      if (!(env->psw.mask & PSW_MASK_DAT)) {
>          *raddr = vaddr;
> -        r = 0;
> -        goto out;
> +        goto nodat;
>      }
>  
>      switch (asc) {
>      case PSW_ASC_PRIMARY:
>          PTE_DPRINTF("%s: asc=primary\n", __func__);
> -        r = mmu_translate_asce(env, vaddr, asc, env->cregs[1], raddr, flags,
> -                               rw, exc);
> +        asce = env->cregs[1];
>          break;
>      case PSW_ASC_HOME:
>          PTE_DPRINTF("%s: asc=home\n", __func__);
> -        r = mmu_translate_asce(env, vaddr, asc, env->cregs[13], raddr, flags,
> -                               rw, exc);
> +        asce = env->cregs[13];
>          break;
>      case PSW_ASC_SECONDARY:
>          PTE_DPRINTF("%s: asc=secondary\n", __func__);
> -        /*
> -         * Instruction: Primary
> -         * Data: Secondary
> -         */
> -        if (rw == MMU_INST_FETCH) {
> -            r = mmu_translate_asce(env, vaddr, PSW_ASC_PRIMARY, env->cregs[1],
> -                                   raddr, flags, rw, exc);
> -            *flags &= ~(PAGE_READ | PAGE_WRITE);
> -        } else {
> -            r = mmu_translate_asce(env, vaddr, PSW_ASC_SECONDARY, env->cregs[7],
> -                                   raddr, flags, rw, exc);
> -            *flags &= ~(PAGE_EXEC);
> -        }
> +        asce = env->cregs[7];
>          break;
>      case PSW_ASC_ACCREG:
>      default:
> @@ -417,11 +403,17 @@ int mmu_translate(CPUS390XState *env, target_ulong vaddr, int rw, uint64_t asc,
>          break;
>      }
>  
> - out:
> +    /* perform the DAT translation */
> +    r = mmu_translate_asce(env, vaddr, asc, asce, raddr, flags, rw, exc);
> +    if (r) {
> +        return r;
> +    }
> +
> +nodat:
>      /* Convert real address -> absolute address */
>      *raddr = mmu_real2abs(env, *raddr);
>  
> -    if (r == 0 && *raddr < ram_size) {
> +    if (*raddr < ram_size) {
>          if (skeyclass->get_skeys(ss, *raddr / TARGET_PAGE_SIZE, 1, &key)) {
>              trace_get_skeys_nonzero(r);

I think you might up here with an uninitialized r before patch 4?

>              return 0;
> @@ -441,7 +433,7 @@ int mmu_translate(CPUS390XState *env, target_ulong vaddr, int rw, uint64_t asc,
>          }
>      }
>  
> -    return r;
> +    return 0;
>  }
>  
>  /**



^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [Qemu-devel] [PATCH-for-4.2 v2 2/6] s390x/tcg: Rework MMU selection for instruction fetches
  2019-08-15 15:43   ` [Qemu-devel] " Cornelia Huck
@ 2019-08-15 16:52     ` David Hildenbrand
  0 siblings, 0 replies; 20+ messages in thread
From: David Hildenbrand @ 2019-08-15 16:52 UTC (permalink / raw)
  To: Cornelia Huck
  Cc: Thomas Huth, Janosch Frank, qemu-devel, Halil Pasic,
	Christian Borntraeger, qemu-s390x, Richard Henderson

On 15.08.19 17:43, Cornelia Huck wrote:
> On Wed, 14 Aug 2019 09:23:51 +0200
> David Hildenbrand <david@redhat.com> wrote:
> 
>> Instructions are always fetched from primary address space, except when
>> in home address mode. Perform the selection directly in cpu_mmu_index().
>>
>> get_mem_index() is only used to perform data access, instructions are
>> fetched via cpu_lduw_code(), which translates to cpu_mmu_index(env, true).
>>
>> We don't care about restricting the access permissions of the TLB
>> entries anymore, as we no longer enter PRIMARY entries into the
>> SECONDARY MMU. Cleanup related code a bit.
>>
>> Signed-off-by: David Hildenbrand <david@redhat.com>
>> ---
>>  target/s390x/cpu.h        |  7 +++++++
>>  target/s390x/mmu_helper.c | 38 +++++++++++++++-----------------------
>>  2 files changed, 22 insertions(+), 23 deletions(-)
>>
> 
> (...)
> 
>> diff --git a/target/s390x/mmu_helper.c b/target/s390x/mmu_helper.c
>> index 6e9c4d6151..c34e8d2021 100644
>> --- a/target/s390x/mmu_helper.c
>> +++ b/target/s390x/mmu_helper.c
>> @@ -349,8 +349,9 @@ int mmu_translate(CPUS390XState *env, target_ulong vaddr, int rw, uint64_t asc,
>>  {
>>      static S390SKeysState *ss;
>>      static S390SKeysClass *skeyclass;
>> -    int r = -1;
>> +    uint64_t asce;
>>      uint8_t key;
>> +    int r;
>>  
>>      if (unlikely(!ss)) {
>>          ss = s390_get_skeys_device();
>> @@ -380,36 +381,21 @@ int mmu_translate(CPUS390XState *env, target_ulong vaddr, int rw, uint64_t asc,
>>  
>>      if (!(env->psw.mask & PSW_MASK_DAT)) {
>>          *raddr = vaddr;
>> -        r = 0;
>> -        goto out;
>> +        goto nodat;
>>      }
>>  
>>      switch (asc) {
>>      case PSW_ASC_PRIMARY:
>>          PTE_DPRINTF("%s: asc=primary\n", __func__);
>> -        r = mmu_translate_asce(env, vaddr, asc, env->cregs[1], raddr, flags,
>> -                               rw, exc);
>> +        asce = env->cregs[1];
>>          break;
>>      case PSW_ASC_HOME:
>>          PTE_DPRINTF("%s: asc=home\n", __func__);
>> -        r = mmu_translate_asce(env, vaddr, asc, env->cregs[13], raddr, flags,
>> -                               rw, exc);
>> +        asce = env->cregs[13];
>>          break;
>>      case PSW_ASC_SECONDARY:
>>          PTE_DPRINTF("%s: asc=secondary\n", __func__);
>> -        /*
>> -         * Instruction: Primary
>> -         * Data: Secondary
>> -         */
>> -        if (rw == MMU_INST_FETCH) {
>> -            r = mmu_translate_asce(env, vaddr, PSW_ASC_PRIMARY, env->cregs[1],
>> -                                   raddr, flags, rw, exc);
>> -            *flags &= ~(PAGE_READ | PAGE_WRITE);
>> -        } else {
>> -            r = mmu_translate_asce(env, vaddr, PSW_ASC_SECONDARY, env->cregs[7],
>> -                                   raddr, flags, rw, exc);
>> -            *flags &= ~(PAGE_EXEC);
>> -        }
>> +        asce = env->cregs[7];
>>          break;
>>      case PSW_ASC_ACCREG:
>>      default:
>> @@ -417,11 +403,17 @@ int mmu_translate(CPUS390XState *env, target_ulong vaddr, int rw, uint64_t asc,
>>          break;
>>      }
>>  
>> - out:
>> +    /* perform the DAT translation */
>> +    r = mmu_translate_asce(env, vaddr, asc, asce, raddr, flags, rw, exc);
>> +    if (r) {
>> +        return r;
>> +    }
>> +
>> +nodat:
>>      /* Convert real address -> absolute address */
>>      *raddr = mmu_real2abs(env, *raddr);
>>  
>> -    if (r == 0 && *raddr < ram_size) {
>> +    if (*raddr < ram_size) {
>>          if (skeyclass->get_skeys(ss, *raddr / TARGET_PAGE_SIZE, 1, &key)) {
>>              trace_get_skeys_nonzero(r);
> 
> I think you might up here with an uninitialized r before patch 4?

Right, will reshuffle. Thanks!

-- 

Thanks,

David / dhildenb


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [Qemu-devel] [PATCH-for-4.2 v2 0/6] s390x/mmu: Storage key reference and change bit handling
  2019-08-14  7:23 [Qemu-devel] [PATCH-for-4.2 v2 0/6] s390x/mmu: Storage key reference and change bit handling David Hildenbrand
                   ` (5 preceding siblings ...)
  2019-08-14  7:23 ` [Qemu-devel] [PATCH-for-4.2 v2 6/6] s390x/mmu: Factor out storage key handling David Hildenbrand
@ 2019-08-19 16:36 ` Cornelia Huck
  2019-08-19 16:37   ` Cornelia Huck
  6 siblings, 1 reply; 20+ messages in thread
From: Cornelia Huck @ 2019-08-19 16:36 UTC (permalink / raw)
  To: David Hildenbrand
  Cc: Thomas Huth, Janosch Frank, qemu-devel, Halil Pasic,
	Christian Borntraeger, qemu-s390x, Richard Henderson

On Wed, 14 Aug 2019 09:23:49 +0200
David Hildenbrand <david@redhat.com> wrote:

> The first two patches are modified patches from:
>     [PATCH-for-4.2 v1 0/9] s390x: MMU changes and extensions
> 
> This series primarily fixes minor things in the storage key handling code
> in the MMU and implements fairly reliable reference and change bit handling
> for TCG. To track the reference and change bit, we have to invalidate
> TLB entries whenever the storage key is changed by the guest and make sure
> not TLB entry is writable in case the storage key does not indicate a
> change already.
> 
> With this series, the kvm-unit-test "skey" now passes. \o/
> 
> v1 -> v2:
> - "s390x/tcg: Rework MMU selection for instruction fetches"
> -- Cleanup return value handling
> - Added RB's
> 
> David Hildenbrand (6):
>   s390x/mmu: ASC selection in s390_cpu_get_phys_page_debug()
>   s390x/tcg: Rework MMU selection for instruction fetches
>   s390x/tcg: Flush the TLB of all CPUs on SSKE and RRBE
>   s390x/mmu: Trace the right value if setting/getting the storage key
>     fails
>   s390x/mmu: Better storage key reference and change bit handling
>   s390x/mmu: Factor out storage key handling
> 
>  target/s390x/cpu.h        |   7 ++
>  target/s390x/helper.c     |   5 ++
>  target/s390x/mem_helper.c |   4 ++
>  target/s390x/mmu_helper.c | 133 ++++++++++++++++++++++++--------------
>  4 files changed, 99 insertions(+), 50 deletions(-)
> 

Thanks, applied.


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [Qemu-devel] [PATCH-for-4.2 v2 0/6] s390x/mmu: Storage key reference and change bit handling
  2019-08-19 16:36 ` [Qemu-devel] [PATCH-for-4.2 v2 0/6] s390x/mmu: Storage key reference and change bit handling Cornelia Huck
@ 2019-08-19 16:37   ` Cornelia Huck
  0 siblings, 0 replies; 20+ messages in thread
From: Cornelia Huck @ 2019-08-19 16:37 UTC (permalink / raw)
  To: David Hildenbrand
  Cc: Thomas Huth, Janosch Frank, qemu-devel, Halil Pasic,
	Christian Borntraeger, qemu-s390x, Richard Henderson

On Mon, 19 Aug 2019 18:36:23 +0200
Cornelia Huck <cohuck@redhat.com> wrote:

> On Wed, 14 Aug 2019 09:23:49 +0200
> David Hildenbrand <david@redhat.com> wrote:
> 
> > The first two patches are modified patches from:
> >     [PATCH-for-4.2 v1 0/9] s390x: MMU changes and extensions
> > 
> > This series primarily fixes minor things in the storage key handling code
> > in the MMU and implements fairly reliable reference and change bit handling
> > for TCG. To track the reference and change bit, we have to invalidate
> > TLB entries whenever the storage key is changed by the guest and make sure
> > not TLB entry is writable in case the storage key does not indicate a
> > change already.
> > 
> > With this series, the kvm-unit-test "skey" now passes. \o/
> > 
> > v1 -> v2:
> > - "s390x/tcg: Rework MMU selection for instruction fetches"
> > -- Cleanup return value handling
> > - Added RB's
> > 
> > David Hildenbrand (6):
> >   s390x/mmu: ASC selection in s390_cpu_get_phys_page_debug()
> >   s390x/tcg: Rework MMU selection for instruction fetches
> >   s390x/tcg: Flush the TLB of all CPUs on SSKE and RRBE
> >   s390x/mmu: Trace the right value if setting/getting the storage key
> >     fails
> >   s390x/mmu: Better storage key reference and change bit handling
> >   s390x/mmu: Factor out storage key handling
> > 
> >  target/s390x/cpu.h        |   7 ++
> >  target/s390x/helper.c     |   5 ++
> >  target/s390x/mem_helper.c |   4 ++
> >  target/s390x/mmu_helper.c | 133 ++++++++++++++++++++++++--------------
> >  4 files changed, 99 insertions(+), 50 deletions(-)
> >   
> 
> Thanks, applied.

No, not that one, v3. Argh.


^ permalink raw reply	[flat|nested] 20+ messages in thread

end of thread, back to index

Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-08-14  7:23 [Qemu-devel] [PATCH-for-4.2 v2 0/6] s390x/mmu: Storage key reference and change bit handling David Hildenbrand
2019-08-14  7:23 ` [Qemu-devel] [PATCH-for-4.2 v2 1/6] s390x/mmu: ASC selection in s390_cpu_get_phys_page_debug() David Hildenbrand
2019-08-14  7:23 ` [Qemu-devel] [PATCH-for-4.2 v2 2/6] s390x/tcg: Rework MMU selection for instruction fetches David Hildenbrand
2019-08-14 17:44   ` [Qemu-devel] [qemu-s390x] " Thomas Huth
2019-08-15 15:43   ` [Qemu-devel] " Cornelia Huck
2019-08-15 16:52     ` David Hildenbrand
2019-08-14  7:23 ` [Qemu-devel] [PATCH-for-4.2 v2 3/6] s390x/tcg: Flush the TLB of all CPUs on SSKE and RRBE David Hildenbrand
2019-08-14 10:06   ` Alex Bennée
2019-08-14 10:21     ` David Hildenbrand
2019-08-14 10:44       ` Alex Bennée
2019-08-14 10:51         ` David Hildenbrand
2019-08-14  7:23 ` [Qemu-devel] [PATCH-for-4.2 v2 4/6] s390x/mmu: Trace the right value if setting/getting the storage key fails David Hildenbrand
2019-08-14 17:50   ` [Qemu-devel] [qemu-s390x] " Thomas Huth
2019-08-15 15:39     ` Cornelia Huck
2019-08-14  7:23 ` [Qemu-devel] [PATCH-for-4.2 v2 5/6] s390x/mmu: Better storage key reference and change bit handling David Hildenbrand
2019-08-14  7:23 ` [Qemu-devel] [PATCH-for-4.2 v2 6/6] s390x/mmu: Factor out storage key handling David Hildenbrand
2019-08-14 18:01   ` [Qemu-devel] [qemu-s390x] " Thomas Huth
2019-08-14 18:18     ` David Hildenbrand
2019-08-19 16:36 ` [Qemu-devel] [PATCH-for-4.2 v2 0/6] s390x/mmu: Storage key reference and change bit handling Cornelia Huck
2019-08-19 16:37   ` Cornelia Huck

QEMU-Devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/qemu-devel/0 qemu-devel/git/0.git
	git clone --mirror https://lore.kernel.org/qemu-devel/1 qemu-devel/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 qemu-devel qemu-devel/ https://lore.kernel.org/qemu-devel \
		qemu-devel@nongnu.org qemu-devel@archiver.kernel.org
	public-inbox-index qemu-devel


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.nongnu.qemu-devel


AGPL code for this site: git clone https://public-inbox.org/ public-inbox