Skip to contentSkip to conversation

s390x/tcg: fix some accesses using low address

PATCH In review David Hildenbrand 4 participants 11 messages Sep 26, 2017 – Sep 27, 2017

lore
Share & tools
lore

David Hildenbrand

author
#0 lore raw

cover s390x/tcg: fix some accesses using low address

Conny reported that a guest Linux currently spits out:
"The s390-virtio transport is deprecated. Please switch to a modern host providing virtio-ccw."

This is strange, because we have no legacy virtio transport in QEMU :)

Main problem is, that a page is tested for acccess, and the test does not fail. All memory access variants like stb_phys() will never fail. On invalid memory, they simply do nothing.

Linux uses lura for this check. Let's introduce a _real MMU that handles prefixing and can later be used for things like low address protection.

The other BUG is that a check for invalid memory is wrong. Therefore, Patch 1 alone makes the guest crash when trying to initialize the virtio transport. Crazy stuff.

David Hildenbrand (6):
s390x/tcg: fix checking for invalid memory check s390x/tcg: add MMU for real addresses s390x/tcg: make lura(g) use the new _real mmu. s390x/tcg: make stora(g) use the new _real mmu s390x/tcg: make testblock use the new _real mmu s390x/tcg: make idte/ipte use the new _real mmu

 target/s390x/cpu.h         |  4 +++-
 target/s390x/excp_helper.c | 29 +++++++++++++++++++----------
 target/s390x/internal.h    |  2 ++
 target/s390x/mem_helper.c  | 36 +++++++++++-------------------------
 target/s390x/mmu_helper.c  | 19 +++++++++++++++++++
 target/s390x/translate.c   |  4 ----
 6 files changed, 54 insertions(+), 40 deletions(-)
Signature · 1 line
2.13.5
author
#1 lore raw

PATCH 1/6 s390x/tcg: fix checking for invalid memory check

It should have been a >=, but let's directly perform a proper access check to also be able to deal with hotplugged memory later.

Signed-off-by David Hildenbrand <[email protected]>
---
 target/s390x/excp_helper.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
target/s390x/excp_helper.c +3 -1
index 470cf8f5bc..308605d9ed 100644
@@ -26,6 +26,7 @@
 #include "exec/exec-all.h"
 #include "exec/cpu_ldst.h"
 #include "hw/s390x/ioinst.h"
+#include "exec/address-spaces.h"
 #ifndef CONFIG_USER_ONLY
 #include "sysemu/sysemu.h"
 #endif

@@ -108,7 +109,8 @@ int s390_cpu_handle_mmu_fault(CPUState *cs, vaddr orig_vaddr,
     }
 
     /* check out of RAM access */
-    if (raddr > ram_size) {
+    if (!address_space_access_valid(&address_space_memory, raddr,
+                                    TARGET_PAGE_SIZE, rw)) {
         DPRINTF("%s: raddr %" PRIx64 " > ram_size %" PRIx64 "\n", __func__,
                 (uint64_t)raddr, (uint64_t)ram_size);
         trigger_pgm_exception(env, PGM_ADDRESSING, ILEN_AUTO);
Signature · 1 line
2.13.5
author
#2 lore raw

PATCH 2/6 s390x/tcg: add MMU for real addresses

In reply to David Hildenbrand

This makes it easy to access real addresses (prefix) and in addition checks for valid memory addresses, which is missing when using e.g. stl_phys().

We can later reuse it to implement low address protection checks (then we might even decide to introduce yet another MMU for absolute addresses, just for handling storage keys and low address protection).

Signed-off-by David Hildenbrand <[email protected]>
---
 target/s390x/cpu.h         |  4 +++-
 target/s390x/excp_helper.c | 25 ++++++++++++++++---------
 target/s390x/internal.h    |  2 ++
 target/s390x/mmu_helper.c  | 19 +++++++++++++++++++
 4 files changed, 40 insertions(+), 10 deletions(-)
target/s390x/cpu.h +3 -1
index 9b549dc491..42b4e813e4 100644
@@ -43,12 +43,13 @@
 
 #include "fpu/softfloat.h"
 
-#define NB_MMU_MODES 3
+#define NB_MMU_MODES 4
 #define TARGET_INSN_START_EXTRA_WORDS 1
 
 #define MMU_MODE0_SUFFIX _primary
 #define MMU_MODE1_SUFFIX _secondary
 #define MMU_MODE2_SUFFIX _home
+#define MMU_MODE3_SUFFIX _real
 
 #define MMU_USER_IDX 0
 

@@ -349,6 +350,7 @@ extern const struct VMStateDescription vmstate_s390_cpu;
 #define MMU_PRIMARY_IDX         0
 #define MMU_SECONDARY_IDX       1
 #define MMU_HOME_IDX            2
+#define MMU_REAL_IDX            3
 
 static inline int cpu_mmu_index(CPUS390XState *env, bool ifetch)
 {
target/s390x/excp_helper.c +16 -9
index 308605d9ed..3e4349d00b 100644
@@ -88,8 +88,8 @@ int s390_cpu_handle_mmu_fault(CPUState *cs, vaddr orig_vaddr,
 {
     S390CPU *cpu = S390_CPU(cs);
     CPUS390XState *env = &cpu->env;
-    uint64_t asc = cpu_mmu_idx_to_asc(mmu_idx);
     target_ulong vaddr, raddr;
+    uint64_t asc;
     int prot;
 
     DPRINTF("%s: address 0x%" VADDR_PRIx " rw %d mmu_idx %d\n",

@@ -98,14 +98,21 @@ int s390_cpu_handle_mmu_fault(CPUState *cs, vaddr orig_vaddr,
     orig_vaddr &= TARGET_PAGE_MASK;
     vaddr = orig_vaddr;
 
-    /* 31-Bit mode */
-    if (!(env->psw.mask & PSW_MASK_64)) {
-        vaddr &= 0x7fffffff;
-    }
-
-    if (mmu_translate(env, vaddr, rw, asc, &raddr, &prot, true)) {
-        /* Translation ended in exception */
-        return 1;
+    if (mmu_idx < MMU_REAL_IDX) {
+        asc = cpu_mmu_idx_to_asc(mmu_idx);
+        /* 31-Bit mode */
+        if (!(env->psw.mask & PSW_MASK_64)) {
+            vaddr &= 0x7fffffff;
+        }
+        if (mmu_translate(env, vaddr, rw, asc, &raddr, &prot, true)) {
+            return 1;
+        }
+    } else if (mmu_idx == MMU_REAL_IDX) {
+        if (mmu_translate_real(env, vaddr, rw, &raddr, &prot)) {
+            return 1;
+        }
+    } else {
+        abort();
     }
 
     /* check out of RAM access */
target/s390x/internal.h +2 -0
index bc8f83129a..1a83e559e3 100644
@@ -375,6 +375,8 @@ target_ulong mmu_real2abs(CPUS390XState *env, target_ulong raddr);
 /* mmu_helper.c */
 int mmu_translate(CPUS390XState *env, target_ulong vaddr, int rw, uint64_t asc,
                   target_ulong *raddr, int *flags, bool exc);
+int mmu_translate_real(CPUS390XState *env, target_ulong raddr, int rw,
+                       target_ulong *addr, int *flags);
 
 
 /* misc_helper.c */
target/s390x/mmu_helper.c +19 -0
index b528c5921d..98c58fc7c9 100644
@@ -497,3 +497,22 @@ int s390_cpu_virt_mem_rw(S390CPU *cpu, vaddr laddr, uint8_t ar, void *hostbuf,
     g_free(pages);
     return ret;
 }
+
+/**
+ * Translate a real address into a physical (absolute) address.
+ * @param raddr  the real address
+ * @param rw     0 = read, 1 = write, 2 = code fetch
+ * @param addr  the translated address is stored to this pointer
+ * @param flags  the PAGE_READ/WRITE/EXEC flags are stored to this pointer
+ * @return       0 if the translation was successful, < 0 if a fault occurred
+ */
+int mmu_translate_real(CPUS390XState *env, target_ulong raddr, int rw,
+                       target_ulong *addr, int *flags)
+{
+    /* TODO: low address protection once we flush the tlb on cr changes */
+    *flags = PAGE_READ | PAGE_WRITE;
+    *addr = mmu_real2abs(env, raddr);
+
+    /* TODO: storage key handling */
+    return 0;
+}
Signature · 1 line
2.13.5
author
#3 lore raw

Re: [Qemu-devel] [PATCH v1 2/6] s390x/tcg: add MMU for real addresses

Quoted 6 lines

+/**
+ * Translate a real address into a physical (absolute) address.
+ * @param raddr the real address
+ * @param rw 0 = read, 1 = write, 2 = code fetch
+ * @param addr the translated address is stored to this pointer

----------------->^
Conny, if you want you can also insert one additional space here :)

Quoted 14 lines

+ * @param flags the PAGE_READ/WRITE/EXEC flags are stored to this pointer
+ * @return 0 if the translation was successful, < 0 if a fault occurred
+ */

target/s390x/mmu_helper.c +10 -0
+int mmu_translate_real(CPUS390XState *env, target_ulong raddr, int rw,
+                       target_ulong *addr, int *flags)
+{
+    /* TODO: low address protection once we flush the tlb on cr changes */
+    *flags = PAGE_READ | PAGE_WRITE;
+    *addr = mmu_real2abs(env, raddr);
+
+    /* TODO: storage key handling */
+    return 0;
+}
Signature · 3 lines
Thanks,

David
author
#4 lore raw

PATCH 3/6 s390x/tcg: make lura(g) use the new _real mmu.

In reply to David Hildenbrand

Looks like, lurag was not loading 64bit but only 32bit.

As we properly handle the return address now, we can drop potential_page_fault().

Signed-off-by David Hildenbrand <[email protected]>
---
 target/s390x/mem_helper.c | 7 ++-----
 target/s390x/translate.c  | 2 --
 2 files changed, 2 insertions(+), 7 deletions(-)
target/s390x/mem_helper.c +2 -5
index ec4760e390..f8525e05b3 100644
@@ -1973,16 +1973,13 @@ void HELPER(purge)(CPUS390XState *env)
 /* load using real address */
 uint64_t HELPER(lura)(CPUS390XState *env, uint64_t addr)
 {
-    CPUState *cs = CPU(s390_env_get_cpu(env));
 
-    return (uint32_t)ldl_phys(cs->as, wrap_address(env, addr));
+    return cpu_ldl_real_ra(env, wrap_address(env, addr), GETPC());
 }
 
 uint64_t HELPER(lurag)(CPUS390XState *env, uint64_t addr)
 {
-    CPUState *cs = CPU(s390_env_get_cpu(env));
-
-    return ldq_phys(cs->as, wrap_address(env, addr));
+    return cpu_ldq_real_ra(env, wrap_address(env, addr), GETPC());
 }
 
 /* store using real address */
target/s390x/translate.c +0 -2
index 5abd34fb34..d0ce14d120 100644
@@ -2915,7 +2915,6 @@ static ExitStatus op_lpq(DisasContext *s, DisasOps *o)
 static ExitStatus op_lura(DisasContext *s, DisasOps *o)
 {
     check_privileged(s);
-    potential_page_fault(s);
     gen_helper_lura(o->out, cpu_env, o->in2);
     return NO_EXIT;
 }

@@ -2923,7 +2922,6 @@ static ExitStatus op_lura(DisasContext *s, DisasOps *o)
 static ExitStatus op_lurag(DisasContext *s, DisasOps *o)
 {
     check_privileged(s);
-    potential_page_fault(s);
     gen_helper_lurag(o->out, cpu_env, o->in2);
     return NO_EXIT;
 }
Signature · 1 line
2.13.5
author
#5 lore raw

PATCH 4/6 s390x/tcg: make stora(g) use the new _real mmu

In reply to David Hildenbrand

As we properly handle the return address now, we can drop potential_page_fault().

Signed-off-by David Hildenbrand <[email protected]>
---
 target/s390x/mem_helper.c | 8 ++------
 target/s390x/translate.c  | 2 --
 2 files changed, 2 insertions(+), 8 deletions(-)
target/s390x/mem_helper.c +2 -6
index f8525e05b3..34ffc282bf 100644
@@ -1985,9 +1985,7 @@ uint64_t HELPER(lurag)(CPUS390XState *env, uint64_t addr)
 /* store using real address */
 void HELPER(stura)(CPUS390XState *env, uint64_t addr, uint64_t v1)
 {
-    CPUState *cs = CPU(s390_env_get_cpu(env));
-
-    stl_phys(cs->as, wrap_address(env, addr), (uint32_t)v1);
+    cpu_stl_real_ra(env, wrap_address(env, addr), (uint32_t)v1, GETPC());
 
     if ((env->psw.mask & PSW_MASK_PER) &&
         (env->cregs[9] & PER_CR9_EVENT_STORE) &&

@@ -2000,9 +1998,7 @@ void HELPER(stura)(CPUS390XState *env, uint64_t addr, uint64_t v1)
 
 void HELPER(sturg)(CPUS390XState *env, uint64_t addr, uint64_t v1)
 {
-    CPUState *cs = CPU(s390_env_get_cpu(env));
-
-    stq_phys(cs->as, wrap_address(env, addr), v1);
+    cpu_stq_real_ra(env, wrap_address(env, addr), v1, GETPC());
 
     if ((env->psw.mask & PSW_MASK_PER) &&
         (env->cregs[9] & PER_CR9_EVENT_STORE) &&
target/s390x/translate.c +0 -2
index d0ce14d120..ee93fce482 100644
@@ -4063,7 +4063,6 @@ static ExitStatus op_stnosm(DisasContext *s, DisasOps *o)
 static ExitStatus op_stura(DisasContext *s, DisasOps *o)
 {
     check_privileged(s);
-    potential_page_fault(s);
     gen_helper_stura(cpu_env, o->in2, o->in1);
     return NO_EXIT;
 }

@@ -4071,7 +4070,6 @@ static ExitStatus op_stura(DisasContext *s, DisasOps *o)
 static ExitStatus op_sturg(DisasContext *s, DisasOps *o)
 {
     check_privileged(s);
-    potential_page_fault(s);
     gen_helper_sturg(cpu_env, o->in2, o->in1);
     return NO_EXIT;
 }
Signature · 1 line
2.13.5
author
#6 lore raw

PATCH 5/6 s390x/tcg: make testblock use the new _real mmu

In reply to David Hildenbrand

Low address protection checks will be moved into the mmu later.

Signed-off-by David Hildenbrand <[email protected]>
---
 target/s390x/mem_helper.c | 12 ++----------
 1 file changed, 2 insertions(+), 10 deletions(-)
target/s390x/mem_helper.c +2 -10
index 34ffc282bf..4cf4f29171 100644
@@ -1702,17 +1702,9 @@ uint32_t HELPER(testblock)(CPUS390XState *env, uint64_t real_addr)
 {
     uintptr_t ra = GETPC();
     CPUState *cs = CPU(s390_env_get_cpu(env));
-    uint64_t abs_addr;
     int i;
 
-    real_addr = wrap_address(env, real_addr);
-    abs_addr = mmu_real2abs(env, real_addr) & TARGET_PAGE_MASK;
-    if (!address_space_access_valid(&address_space_memory, abs_addr,
-                                    TARGET_PAGE_SIZE, true)) {
-        cpu_restore_state(cs, ra);
-        program_interrupt(env, PGM_ADDRESSING, 4);
-        return 1;
-    }
+    real_addr = wrap_address(env, real_addr) & TARGET_PAGE_MASK;
 
     /* Check low-address protection */
     if ((env->cregs[0] & CR0_LOWPROT) && real_addr < 0x2000) {

@@ -1722,7 +1714,7 @@ uint32_t HELPER(testblock)(CPUS390XState *env, uint64_t real_addr)
     }
 
     for (i = 0; i < TARGET_PAGE_SIZE; i += 8) {
-        stq_phys(cs->as, abs_addr + i, 0);
+        cpu_stq_real_ra(env, real_addr + i, 0, ra);
     }
 
     return 0;
Signature · 1 line
2.13.5
author
#7 lore raw

PATCH 6/6 s390x/tcg: make idte/ipte use the new _real mmu

In reply to David Hildenbrand

We don't wrap addresses in the mmu for the _real case, therefore the behavior should be unchanged.

Signed-off-by David Hildenbrand <[email protected]>
---
 target/s390x/mem_helper.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)
target/s390x/mem_helper.c +5 -4
index 4cf4f29171..010f509ac5 100644
@@ -1889,11 +1889,11 @@ void HELPER(idte)(CPUS390XState *env, uint64_t r1, uint64_t r2, uint32_t m4)
         for (i = 0; i < entries; i++) {
             /* addresses are not wrapped in 24/31bit mode but table index is */
             raddr = table + ((index + i) & 0x7ff) * sizeof(entry);
-            entry = ldq_phys(cs->as, raddr);
+            entry = cpu_ldq_real_ra(env, raddr, ra);
             if (!(entry & _REGION_ENTRY_INV)) {
                 /* we are allowed to not store if already invalid */
                 entry |= _REGION_ENTRY_INV;
-                stq_phys(cs->as, raddr, entry);
+                cpu_stq_real_ra(env, raddr, entry, ra);
             }
         }
     }

@@ -1911,6 +1911,7 @@ void HELPER(ipte)(CPUS390XState *env, uint64_t pto, uint64_t vaddr,
                   uint32_t m4)
 {
     CPUState *cs = CPU(s390_env_get_cpu(env));
+    const uintptr_t ra = GETPC();
     uint64_t page = vaddr & TARGET_PAGE_MASK;
     uint64_t pte_addr, pte;
 

@@ -1919,9 +1920,9 @@ void HELPER(ipte)(CPUS390XState *env, uint64_t pto, uint64_t vaddr,
     pte_addr += (vaddr & VADDR_PX) >> 9;
 
     /* Mark the page table entry as invalid */
-    pte = ldq_phys(cs->as, pte_addr);
+    pte = cpu_ldq_real_ra(env, pte_addr, ra);
     pte |= _PAGE_INVALID;
-    stq_phys(cs->as, pte_addr, pte);
+    cpu_stq_real_ra(env, pte_addr, pte, ra);
 
     /* XXX we exploit the fact that Linux passes the exact virtual
        address here - it's not obliged to! */
Signature · 1 line
2.13.5
Reviewed-by on the v1 series
#8 lore raw
In reply to David Hildenbrand
Quoted 7 lines · David Hildenbrand

David Hildenbrand (6):
s390x/tcg: fix checking for invalid memory check s390x/tcg: add MMU for real addresses s390x/tcg: make lura(g) use the new _real mmu. s390x/tcg: make stora(g) use the new _real mmu s390x/tcg: make testblock use the new _real mmu s390x/tcg: make idte/ipte use the new _real mmu

Reviewed-by Richard Henderson <[email protected]>

Watch out for a couple of blank lines left over while deleting stuff. E.g.

{

code
}

r~

Reviewed-by
#9 lore raw
Quoted 9 lines · Richard Henderson
Quoted 7 lines · David Hildenbrand

David Hildenbrand (6):
s390x/tcg: fix checking for invalid memory check s390x/tcg: add MMU for real addresses s390x/tcg: make lura(g) use the new _real mmu. s390x/tcg: make stora(g) use the new _real mmu s390x/tcg: make testblock use the new _real mmu s390x/tcg: make idte/ipte use the new _real mmu

Quoted 3 lines
Reviewed-by Richard Henderson <[email protected]>

Watch out for a couple of blank lines left over while deleting stuff.

Yes, the HELPER(lura) hunk in the third patch has a superfluous empty line now. Apart from that, the series also looks good to me:

Reviewed-by Thomas Huth <[email protected]>
#10 lore raw
In reply to David Hildenbrand
Quoted 10 lines · On Tue, 26 Sep 2017 20:33:12 +0200 David Hildenbrand <[email protected]> wrote:

Conny reported that a guest Linux currently spits out:
"The s390-virtio transport is deprecated. Please switch to a modern host providing virtio-ccw."

This is strange, because we have no legacy virtio transport in QEMU :)

Main problem is, that a page is tested for acccess, and the test does not fail. All memory access variants like stb_phys() will never fail. On invalid memory, they simply do nothing.

Quoted 6 lines

Linux uses lura for this check. Let's introduce a _real MMU that handles prefixing and can later be used for things like low address protection.

The other BUG is that a check for invalid memory is wrong. Therefore, Patch 1 alone makes the guest crash when trying to initialize the virtio transport. Crazy stuff.

Indeed, thanks for hunting this down! No strange messages for me any more :)

Fixed up the empty line in lura and applied to s390-next.