[Qemu-devel] [PATCH v1 0/6] s390x/tcg: fix some accesses using low address

[Qemu-devel] [PATCH v1 0/6] s390x/tcg: fix some accesses using low address

[David Hildenbrand]

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(-)

-- 
2.13.5

[Qemu-devel] [PATCH v1 1/6] s390x/tcg: fix checking for invalid memory check

[David Hildenbrand]

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 <david@redhat.com>
---
 target/s390x/excp_helper.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/target/s390x/excp_helper.c b/target/s390x/excp_helper.c
index 470cf8f5bc..308605d9ed 100644
--- a/target/s390x/excp_helper.c
+++ b/target/s390x/excp_helper.c
@@ -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);
-- 
2.13.5

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

[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 <david@redhat.com>
---
 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(-)

diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
index 9b549dc491..42b4e813e4 100644
--- a/target/s390x/cpu.h
+++ b/target/s390x/cpu.h
@@ -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)
 {
diff --git a/target/s390x/excp_helper.c b/target/s390x/excp_helper.c
index 308605d9ed..3e4349d00b 100644
--- a/target/s390x/excp_helper.c
+++ b/target/s390x/excp_helper.c
@@ -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 */
diff --git a/target/s390x/internal.h b/target/s390x/internal.h
index bc8f83129a..1a83e559e3 100644
--- a/target/s390x/internal.h
+++ b/target/s390x/internal.h
@@ -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 */
diff --git a/target/s390x/mmu_helper.c b/target/s390x/mmu_helper.c
index b528c5921d..98c58fc7c9 100644
--- a/target/s390x/mmu_helper.c
+++ b/target/s390x/mmu_helper.c
@@ -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;
+}
-- 
2.13.5

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

[David Hildenbrand]

> +/**
> + * 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 :)

> + * @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;
> +}
> 


-- 

Thanks,

David

[Qemu-devel] [PATCH v1 3/6] s390x/tcg: make lura(g) use the new _real mmu.

[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 <david@redhat.com>
---
 target/s390x/mem_helper.c | 7 ++-----
 target/s390x/translate.c  | 2 --
 2 files changed, 2 insertions(+), 7 deletions(-)

diff --git a/target/s390x/mem_helper.c b/target/s390x/mem_helper.c
index ec4760e390..f8525e05b3 100644
--- a/target/s390x/mem_helper.c
+++ b/target/s390x/mem_helper.c
@@ -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 */
diff --git a/target/s390x/translate.c b/target/s390x/translate.c
index 5abd34fb34..d0ce14d120 100644
--- a/target/s390x/translate.c
+++ b/target/s390x/translate.c
@@ -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;
 }
-- 
2.13.5

[Qemu-devel] [PATCH v1 4/6] s390x/tcg: make stora(g) use the new _real mmu

[David Hildenbrand]

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

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 target/s390x/mem_helper.c | 8 ++------
 target/s390x/translate.c  | 2 --
 2 files changed, 2 insertions(+), 8 deletions(-)

diff --git a/target/s390x/mem_helper.c b/target/s390x/mem_helper.c
index f8525e05b3..34ffc282bf 100644
--- a/target/s390x/mem_helper.c
+++ b/target/s390x/mem_helper.c
@@ -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) &&
diff --git a/target/s390x/translate.c b/target/s390x/translate.c
index d0ce14d120..ee93fce482 100644
--- a/target/s390x/translate.c
+++ b/target/s390x/translate.c
@@ -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;
 }
-- 
2.13.5

[Qemu-devel] [PATCH v1 5/6] s390x/tcg: make testblock use the new _real mmu

[David Hildenbrand]

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

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

diff --git a/target/s390x/mem_helper.c b/target/s390x/mem_helper.c
index 34ffc282bf..4cf4f29171 100644
--- a/target/s390x/mem_helper.c
+++ b/target/s390x/mem_helper.c
@@ -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;
-- 
2.13.5

[Qemu-devel] [PATCH v1 6/6] s390x/tcg: make idte/ipte use the new _real mmu

[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 <david@redhat.com>
---
 target/s390x/mem_helper.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/target/s390x/mem_helper.c b/target/s390x/mem_helper.c
index 4cf4f29171..010f509ac5 100644
--- a/target/s390x/mem_helper.c
+++ b/target/s390x/mem_helper.c
@@ -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! */
-- 
2.13.5

Re: [Qemu-devel] [PATCH v1 0/6] s390x/tcg: fix some accesses using low address

[Richard Henderson]

On 09/26/2017 11:33 AM, David Hildenbrand wrote:
> 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 <richard.henderson@linaro.org>

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

{

  code
}


r~

Re: [Qemu-devel] [PATCH v1 0/6] s390x/tcg: fix some accesses using low address

[Thomas Huth]

On 27.09.2017 01:10, Richard Henderson wrote:
> On 09/26/2017 11:33 AM, David Hildenbrand wrote:
>> 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 <richard.henderson@linaro.org>
> 
> 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 <thuth@redhat.com>

Re: [Qemu-devel] [PATCH v1 0/6] s390x/tcg: fix some accesses using low address

[Cornelia Huck]

On Tue, 26 Sep 2017 20:33:12 +0200
David Hildenbrand <david@redhat.com> 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.
> 
> 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.