qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Alexander Graf
Subject: Re: [Qemu-devel] [PATCH 3/3] s390x/helper: Added format control bit to MMU translation
Date: Fri, 25 Apr 2014 14:58:35 +0200
User-agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:24.0) Gecko/20100101 Thunderbird/24.4.0


On 25.04.14 14:56, Thomas Huth wrote:
On Fri, 25 Apr 2014 14:36:11 +0200
Alexander Graf <address@hidden> wrote:
On 25.04.14 14:15, Thomas Huth wrote:
On Thu, 24 Apr 2014 18:36:18 +0200
Alexander Graf <address@hidden> wrote:
[...]
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)?
I gave it a try ... works fine with two small modifications...

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? */
You can remove the XXX comment, the protection bit is the same for
both, page table entries and segment table entries.

+        *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) {
I had to remove the "}" at the beginning of above line.

+        /* 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)) {
That's got to be "(new_asce & _SEGMENT_ENTRY_FC)" instead.

+        /* 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,
I'm fine with these changes, too. So how shall we continue? Could you
assemble a complete patch or shall I prepare one?
You go ahead and do one. If you can think of a good way to combine
mmu_translate_pte and mmu_translate_seg into a single function I'd be
happy to see that too :).

With the RO flag identical the only differences left are the page mask
(4k is implicit for QEMU's TLB, that's why it's omitted for normal PTEs)
and the page fault injection. Do 1MB PTEs fault with a page fault or a
segment fault? If they fault with a page fault, it's probably the best
to just have one mmu_translate_pte() function with a mask parameter.
No, you always get segment translation exceptions instead. And there is
another difference: The entry-is-invalid bit is at a different location
(_PAGE_INVALID vs. _SEGMENT_ENTRY_INV). So I think it's nicer to keep
the functions separated instead of adding too much if-statements or
functions parameters there, ok?

Yes, I agree.


Alex




reply via email to

[Prev in Thread] Current Thread [Next in Thread]