Re: [Qemu-devel] [PATCH 3/3] s390x/helper: Added format control bit to MMU translation

[Alexander Graf]

On 24.04.14 17:34, Jens Freimann wrote:
> From: Thomas Huth <address@hidden>
>
> With the EDAT-1 facility, the MMU translation can stop at the
> segment table already, pointing to a 1 MB block.
>
> Signed-off-by: Thomas Huth <address@hidden>
> Signed-off-by: Jens Freimann <address@hidden>
> Reviewed-by: David Hildenbrand <address@hidden>
> ---
>   target-s390x/cpu.h    | 4 ++++
>   target-s390x/helper.c | 4 ++++
>   2 files changed, 8 insertions(+)
>
> diff --git a/target-s390x/cpu.h b/target-s390x/cpu.h
> index f332d41..686d458 100644
> --- a/target-s390x/cpu.h
> +++ b/target-s390x/cpu.h
> @@ -267,6 +267,9 @@ typedef struct CPUS390XState {
>   #define FLAG_MASK_64            (PSW_MASK_64     >> 32)
>   #define FLAG_MASK_32            0x00001000
>   
> +/* Control register 0 bits */
> +#define CR0_EDAT                0x0000000000800000ULL
> +
>   static inline int cpu_mmu_index (CPUS390XState *env)
>   {
>       if (env->psw.mask & PSW_MASK_PSTATE) {
> @@ -924,6 +927,7 @@ struct sysib_322 {
>   #define _REGION_ENTRY_LENGTH    0x03      /* region third length              
> */
>   
>   #define _SEGMENT_ENTRY_ORIGIN   ~0x7ffULL /* segment table origin             */
> +#define _SEGMENT_ENTRY_FC       0x400     /* format control                   
> */
>   #define _SEGMENT_ENTRY_RO       0x200     /* page protection bit              
> */
>   #define _SEGMENT_ENTRY_INV      0x20      /* invalid segment table entry      
> */
>   
> diff --git a/target-s390x/helper.c b/target-s390x/helper.c
> index aa628b8..89dc6e7 100644
> --- a/target-s390x/helper.c
> +++ b/target-s390x/helper.c
> @@ -217,6 +217,10 @@ static int mmu_translate_asce(CPUS390XState *env, 
> target_ulong vaddr,
>           offs = (vaddr >> 17) & 0x3ff8;
>           break;
>       case _ASCE_TYPE_SEGMENT:
> +        if (env && (env->cregs[0] & CR0_EDAT) && (asce & _SEGMENT_ENTRY_FC)) {
> +            *raddr = (asce & 0xfffffffffff00000ULL) | (vaddr & 0xfffff);
> +            return 0;
> +        }

This is missing the page flags. I also think we should rather align the 
code with the PTE handling somehow. This way it gets pretty confusing to 
follow. How about something like this (untested)?


Alex


diff --git a/target-s390x/helper.c b/target-s390x/helper.c
index aa628b8..96c1c66 100644
--- a/target-s390x/helper.c
+++ b/target-s390x/helper.c
@@ -170,6 +170,48 @@ static void trigger_page_fault(CPUS390XState *env, 
target_ulong vaddr,
     trigger_pgm_exception(env, type, ilen);
 }

+static int mmu_translate_pte(CPUS390XState *env, target_ulong vaddr,
+                             uint64_t asc, uint64_t asce,
+                              target_ulong *raddr, int *flags, int rw)
+{
+    if (asce & _PAGE_INVALID) {
+        DPRINTF("%s: PTE=0x%" PRIx64 " invalid\n", __func__, asce);
+        trigger_page_fault(env, vaddr, PGM_PAGE_TRANS, asc, rw);
+        return -1;
+    }
+
+    if (asce & _PAGE_RO) {
+        *flags &= ~PAGE_WRITE;
+    }
+
+    *raddr = asce & _ASCE_ORIGIN;
+
+    PTE_DPRINTF("%s: PTE=0x%" PRIx64 "\n", __func__, asce);
+
+    return 0;
+}
+
+static int mmu_translate_segpte(CPUS390XState *env, target_ulong vaddr,
+                                uint64_t asc, uint64_t asce,
+                                 target_ulong *raddr, int *flags, int rw)
+{
+    if (asce & _SEGMENT_ENTRY_INV) {
+        DPRINTF("%s: SEG=0x%" PRIx64 " invalid\n", __func__, asce);
+        trigger_page_fault(env, vaddr, PGM_SEGMENT_TRANS, asc, rw);
+        return -1;
+    }
+
+    if (asce & _PAGE_RO) { /* XXX is this correct? */
+        *flags &= ~PAGE_WRITE;
+    }
+
+    *raddr = (asce & 0xfffffffffff00000ULL) | (vaddr & 0xfffff);
+
+    PTE_DPRINTF("%s: SEG=0x%" PRIx64 "\n", __func__, asce);
+
+    return 0;
+}
+
 static int mmu_translate_asce(CPUS390XState *env, target_ulong vaddr,
                               uint64_t asc, uint64_t asce, int level,
                               target_ulong *raddr, int *flags, int rw)
@@ -229,28 +271,19 @@ static int mmu_translate_asce(CPUS390XState *env, 
target_ulong vaddr,
     PTE_DPRINTF("%s: 0x%" PRIx64 " + 0x%" PRIx64 " => 0x%016" PRIx64 "\n",
                 __func__, origin, offs, new_asce);

-    if (level != _ASCE_TYPE_SEGMENT) {
+    } if (level == _ASCE_TYPE_SEGMENT) {
+        /* 4KB page */
+        return mmu_translate_pte(env, vaddr, asc, new_asce, raddr, 
flags, rw);
+    } else if (((level - 4) == _ASCE_TYPE_SEGMENT) &&
+               (env->cregs[0] & CR0_EDAT) &&
+        (asce & _SEGMENT_ENTRY_FC)) {
+        /* 1MB page */
+        return mmu_translate_segpte(env, vaddr, asc, new_asce, raddr, 
flags, rw);
+    } else {
         /* yet another region */
         return mmu_translate_asce(env, vaddr, asc, new_asce, level - 
4, raddr,
                                   flags, rw);
     }
-
-    /* PTE */
-    if (new_asce & _PAGE_INVALID) {
-        DPRINTF("%s: PTE=0x%" PRIx64 " invalid\n", __func__, new_asce);
-        trigger_page_fault(env, vaddr, PGM_PAGE_TRANS, asc, rw);
-        return -1;
-    }
-
-    if (new_asce & _PAGE_RO) {
-        *flags &= ~PAGE_WRITE;
-    }
-
-    *raddr = new_asce & _ASCE_ORIGIN;
-
-    PTE_DPRINTF("%s: PTE=0x%" PRIx64 "\n", __func__, new_asce);
-
-    return 0;
 }

 static int mmu_translate_asc(CPUS390XState *env, target_ulong vaddr,