Add support for MMU protection regions that are smaller than
TARGET_PAGE_SIZE. We do this by marking the TLB entry for those
pages with a flag TLB_RECHECK. This flag causes us to always
take the slow-path for accesses. In the slow path we can then
special case them to always call tlb_fill() again, so we have
the correct information for the exact address being accessed.

This change allows us to handle reading and writing from small
regions; we cannot deal with execution from the small region.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Message-id: 20180620130619.11362-2-peter.maydell@linaro.org
---
 accel/tcg/softmmu_template.h |  24 ++++---
 include/exec/cpu-all.h       |   5 +-
 accel/tcg/cputlb.c           | 131 +++++++++++++++++++++++++++++------
 3 files changed, 130 insertions(+), 30 deletions(-)

diff --git a/accel/tcg/softmmu_template.h b/accel/tcg/softmmu_template.h
index 239ea6692b4..c47591c9709 100644
--- a/accel/tcg/softmmu_template.h
+++ b/accel/tcg/softmmu_template.h
@@ -98,10 +98,12 @@
 static inline DATA_TYPE glue(io_read, SUFFIX)(CPUArchState *env,
                                               size_t mmu_idx, size_t index,
                                               target_ulong addr,
-                                              uintptr_t retaddr)
+                                              uintptr_t retaddr,
+                                              bool recheck)
 {
     CPUIOTLBEntry *iotlbentry = &env->iotlb[mmu_idx][index];
-    return io_readx(env, iotlbentry, mmu_idx, addr, retaddr, DATA_SIZE);
+    return io_readx(env, iotlbentry, mmu_idx, addr, retaddr, recheck,
+                    DATA_SIZE);
 }
 #endif
 
@@ -138,7 +140,8 @@ WORD_TYPE helper_le_ld_name(CPUArchState *env, target_ulong addr,
 
         /* ??? Note that the io helpers always read data in the target
            byte ordering.  We should push the LE/BE request down into io.  */
-        res = glue(io_read, SUFFIX)(env, mmu_idx, index, addr, retaddr);
+        res = glue(io_read, SUFFIX)(env, mmu_idx, index, addr, retaddr,
+                                    tlb_addr & TLB_RECHECK);
         res = TGT_LE(res);
         return res;
     }
@@ -205,7 +208,8 @@ WORD_TYPE helper_be_ld_name(CPUArchState *env, target_ulong addr,
 
         /* ??? Note that the io helpers always read data in the target
            byte ordering.  We should push the LE/BE request down into io.  */
-        res = glue(io_read, SUFFIX)(env, mmu_idx, index, addr, retaddr);
+        res = glue(io_read, SUFFIX)(env, mmu_idx, index, addr, retaddr,
+                                    tlb_addr & TLB_RECHECK);
         res = TGT_BE(res);
         return res;
     }
@@ -259,10 +263,12 @@ static inline void glue(io_write, SUFFIX)(CPUArchState *env,
                                           size_t mmu_idx, size_t index,
                                           DATA_TYPE val,
                                           target_ulong addr,
-                                          uintptr_t retaddr)
+                                          uintptr_t retaddr,
+                                          bool recheck)
 {
     CPUIOTLBEntry *iotlbentry = &env->iotlb[mmu_idx][index];
-    return io_writex(env, iotlbentry, mmu_idx, val, addr, retaddr, DATA_SIZE);
+    return io_writex(env, iotlbentry, mmu_idx, val, addr, retaddr,
+                     recheck, DATA_SIZE);
 }
 
 void helper_le_st_name(CPUArchState *env, target_ulong addr, DATA_TYPE val,
@@ -298,7 +304,8 @@ void helper_le_st_name(CPUArchState *env, target_ulong addr, DATA_TYPE val,
         /* ??? Note that the io helpers always read data in the target
            byte ordering.  We should push the LE/BE request down into io.  */
         val = TGT_LE(val);
-        glue(io_write, SUFFIX)(env, mmu_idx, index, val, addr, retaddr);
+        glue(io_write, SUFFIX)(env, mmu_idx, index, val, addr,
+                               retaddr, tlb_addr & TLB_RECHECK);
         return;
     }
 
@@ -375,7 +382,8 @@ void helper_be_st_name(CPUArchState *env, target_ulong addr, DATA_TYPE val,
         /* ??? Note that the io helpers always read data in the target
            byte ordering.  We should push the LE/BE request down into io.  */
         val = TGT_BE(val);
-        glue(io_write, SUFFIX)(env, mmu_idx, index, val, addr, retaddr);
+        glue(io_write, SUFFIX)(env, mmu_idx, index, val, addr, retaddr,
+                               tlb_addr & TLB_RECHECK);
         return;
     }
 
diff --git a/include/exec/cpu-all.h b/include/exec/cpu-all.h
index 7fa726b8e36..7338f57062f 100644
--- a/include/exec/cpu-all.h
+++ b/include/exec/cpu-all.h
@@ -330,11 +330,14 @@ CPUArchState *cpu_copy(CPUArchState *env);
 #define TLB_NOTDIRTY        (1 << (TARGET_PAGE_BITS - 2))
 /* Set if TLB entry is an IO callback.  */
 #define TLB_MMIO            (1 << (TARGET_PAGE_BITS - 3))
+/* Set if TLB entry must have MMU lookup repeated for every access */
+#define TLB_RECHECK         (1 << (TARGET_PAGE_BITS - 4))
 
 /* Use this mask to check interception with an alignment mask
  * in a TCG backend.
  */
-#define TLB_FLAGS_MASK  (TLB_INVALID_MASK | TLB_NOTDIRTY | TLB_MMIO)
+#define TLB_FLAGS_MASK  (TLB_INVALID_MASK | TLB_NOTDIRTY | TLB_MMIO \
+                         | TLB_RECHECK)
 
 void dump_exec_info(FILE *f, fprintf_function cpu_fprintf);
 void dump_opcount_info(FILE *f, fprintf_function cpu_fprintf);
diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
index 719cca2268b..eebe97dabb7 100644
--- a/accel/tcg/cputlb.c
+++ b/accel/tcg/cputlb.c
@@ -613,27 +613,42 @@ void tlb_set_page_with_attrs(CPUState *cpu, target_ulong vaddr,
     target_ulong code_address;
     uintptr_t addend;
     CPUTLBEntry *te, *tv, tn;
-    hwaddr iotlb, xlat, sz;
+    hwaddr iotlb, xlat, sz, paddr_page;
+    target_ulong vaddr_page;
     unsigned vidx = env->vtlb_index++ % CPU_VTLB_SIZE;
     int asidx = cpu_asidx_from_attrs(cpu, attrs);
 
     assert_cpu_is_self(cpu);
-    assert(size >= TARGET_PAGE_SIZE);
-    if (size != TARGET_PAGE_SIZE) {
-        tlb_add_large_page(env, vaddr, size);
-    }
 
-    sz = size;
-    section = address_space_translate_for_iotlb(cpu, asidx, paddr, &xlat, &sz,
-                                                attrs, &prot);
+    if (size < TARGET_PAGE_SIZE) {
+        sz = TARGET_PAGE_SIZE;
+    } else {
+        if (size > TARGET_PAGE_SIZE) {
+            tlb_add_large_page(env, vaddr, size);
+        }
+        sz = size;
+    }
+    vaddr_page = vaddr & TARGET_PAGE_MASK;
+    paddr_page = paddr & TARGET_PAGE_MASK;
+
+    section = address_space_translate_for_iotlb(cpu, asidx, paddr_page,
+                                                &xlat, &sz, attrs, &prot);
     assert(sz >= TARGET_PAGE_SIZE);
 
     tlb_debug("vaddr=" TARGET_FMT_lx " paddr=0x" TARGET_FMT_plx
               " prot=%x idx=%d\n",
               vaddr, paddr, prot, mmu_idx);
 
-    address = vaddr;
-    if (!memory_region_is_ram(section->mr) && !memory_region_is_romd(section->mr)) {
+    address = vaddr_page;
+    if (size < TARGET_PAGE_SIZE) {
+        /*
+         * Slow-path the TLB entries; we will repeat the MMU check and TLB
+         * fill on every access.
+         */
+        address |= TLB_RECHECK;
+    }
+    if (!memory_region_is_ram(section->mr) &&
+        !memory_region_is_romd(section->mr)) {
         /* IO memory case */
         address |= TLB_MMIO;
         addend = 0;
@@ -643,10 +658,10 @@ void tlb_set_page_with_attrs(CPUState *cpu, target_ulong vaddr,
     }
 
     code_address = address;
-    iotlb = memory_region_section_get_iotlb(cpu, section, vaddr, paddr, xlat,
-                                            prot, &address);
+    iotlb = memory_region_section_get_iotlb(cpu, section, vaddr_page,
+                                            paddr_page, xlat, prot, &address);
 
-    index = (vaddr >> TARGET_PAGE_BITS) & (CPU_TLB_SIZE - 1);
+    index = (vaddr_page >> TARGET_PAGE_BITS) & (CPU_TLB_SIZE - 1);
     te = &env->tlb_table[mmu_idx][index];
     /* do not discard the translation in te, evict it into a victim tlb */
     tv = &env->tlb_v_table[mmu_idx][vidx];
@@ -662,18 +677,18 @@ void tlb_set_page_with_attrs(CPUState *cpu, target_ulong vaddr,
      * TARGET_PAGE_BITS, and either
      *  + the ram_addr_t of the page base of the target RAM (if NOTDIRTY or ROM)
      *  + the offset within section->mr of the page base (otherwise)
-     * We subtract the vaddr (which is page aligned and thus won't
+     * We subtract the vaddr_page (which is page aligned and thus won't
      * disturb the low bits) to give an offset which can be added to the
      * (non-page-aligned) vaddr of the eventual memory access to get
      * the MemoryRegion offset for the access. Note that the vaddr we
      * subtract here is that of the page base, and not the same as the
      * vaddr we add back in io_readx()/io_writex()/get_page_addr_code().
      */
-    env->iotlb[mmu_idx][index].addr = iotlb - vaddr;
+    env->iotlb[mmu_idx][index].addr = iotlb - vaddr_page;
     env->iotlb[mmu_idx][index].attrs = attrs;
 
     /* Now calculate the new entry */
-    tn.addend = addend - vaddr;
+    tn.addend = addend - vaddr_page;
     if (prot & PAGE_READ) {
         tn.addr_read = address;
     } else {
@@ -694,7 +709,7 @@ void tlb_set_page_with_attrs(CPUState *cpu, target_ulong vaddr,
             tn.addr_write = address | TLB_MMIO;
         } else if (memory_region_is_ram(section->mr)
                    && cpu_physical_memory_is_clean(
-                        memory_region_get_ram_addr(section->mr) + xlat)) {
+                       memory_region_get_ram_addr(section->mr) + xlat)) {
             tn.addr_write = address | TLB_NOTDIRTY;
         } else {
             tn.addr_write = address;
@@ -767,7 +782,8 @@ static inline ram_addr_t qemu_ram_addr_from_host_nofail(void *ptr)
 
 static uint64_t io_readx(CPUArchState *env, CPUIOTLBEntry *iotlbentry,
                          int mmu_idx,
-                         target_ulong addr, uintptr_t retaddr, int size)
+                         target_ulong addr, uintptr_t retaddr,
+                         bool recheck, int size)
 {
     CPUState *cpu = ENV_GET_CPU(env);
     hwaddr mr_offset;
@@ -777,6 +793,29 @@ static uint64_t io_readx(CPUArchState *env, CPUIOTLBEntry *iotlbentry,
     bool locked = false;
     MemTxResult r;
 
+    if (recheck) {
+        /*
+         * This is a TLB_RECHECK access, where the MMU protection
+         * covers a smaller range than a target page, and we must
+         * repeat the MMU check here. This tlb_fill() call might
+         * longjump out if this access should cause a guest exception.
+         */
+        int index;
+        target_ulong tlb_addr;
+
+        tlb_fill(cpu, addr, size, MMU_DATA_LOAD, mmu_idx, retaddr);
+
+        index = (addr >> TARGET_PAGE_BITS) & (CPU_TLB_SIZE - 1);
+        tlb_addr = env->tlb_table[mmu_idx][index].addr_read;
+        if (!(tlb_addr & ~(TARGET_PAGE_MASK | TLB_RECHECK))) {
+            /* RAM access */
+            uintptr_t haddr = addr + env->tlb_table[mmu_idx][index].addend;
+
+            return ldn_p((void *)haddr, size);
+        }
+        /* Fall through for handling IO accesses */
+    }
+
     section = iotlb_to_section(cpu, iotlbentry->addr, iotlbentry->attrs);
     mr = section->mr;
     mr_offset = (iotlbentry->addr & TARGET_PAGE_MASK) + addr;
@@ -811,7 +850,7 @@ static uint64_t io_readx(CPUArchState *env, CPUIOTLBEntry *iotlbentry,
 static void io_writex(CPUArchState *env, CPUIOTLBEntry *iotlbentry,
                       int mmu_idx,
                       uint64_t val, target_ulong addr,
-                      uintptr_t retaddr, int size)
+                      uintptr_t retaddr, bool recheck, int size)
 {
     CPUState *cpu = ENV_GET_CPU(env);
     hwaddr mr_offset;
@@ -820,6 +859,30 @@ static void io_writex(CPUArchState *env, CPUIOTLBEntry *iotlbentry,
     bool locked = false;
     MemTxResult r;
 
+    if (recheck) {
+        /*
+         * This is a TLB_RECHECK access, where the MMU protection
+         * covers a smaller range than a target page, and we must
+         * repeat the MMU check here. This tlb_fill() call might
+         * longjump out if this access should cause a guest exception.
+         */
+        int index;
+        target_ulong tlb_addr;
+
+        tlb_fill(cpu, addr, size, MMU_DATA_STORE, mmu_idx, retaddr);
+
+        index = (addr >> TARGET_PAGE_BITS) & (CPU_TLB_SIZE - 1);
+        tlb_addr = env->tlb_table[mmu_idx][index].addr_write;
+        if (!(tlb_addr & ~(TARGET_PAGE_MASK | TLB_RECHECK))) {
+            /* RAM access */
+            uintptr_t haddr = addr + env->tlb_table[mmu_idx][index].addend;
+
+            stn_p((void *)haddr, size, val);
+            return;
+        }
+        /* Fall through for handling IO accesses */
+    }
+
     section = iotlb_to_section(cpu, iotlbentry->addr, iotlbentry->attrs);
     mr = section->mr;
     mr_offset = (iotlbentry->addr & TARGET_PAGE_MASK) + addr;
@@ -903,6 +966,32 @@ tb_page_addr_t get_page_addr_code(CPUArchState *env, target_ulong addr)
             tlb_fill(ENV_GET_CPU(env), addr, 0, MMU_INST_FETCH, mmu_idx, 0);
         }
     }
+
+    if (unlikely(env->tlb_table[mmu_idx][index].addr_code & TLB_RECHECK)) {
+        /*
+         * This is a TLB_RECHECK access, where the MMU protection
+         * covers a smaller range than a target page, and we must
+         * repeat the MMU check here. This tlb_fill() call might
+         * longjump out if this access should cause a guest exception.
+         */
+        int index;
+        target_ulong tlb_addr;
+
+        tlb_fill(cpu, addr, 0, MMU_INST_FETCH, mmu_idx, 0);
+
+        index = (addr >> TARGET_PAGE_BITS) & (CPU_TLB_SIZE - 1);
+        tlb_addr = env->tlb_table[mmu_idx][index].addr_code;
+        if (!(tlb_addr & ~(TARGET_PAGE_MASK | TLB_RECHECK))) {
+            /* RAM access. We can't handle this, so for now just stop */
+            cpu_abort(cpu, "Unable to handle guest executing from RAM within "
+                      "a small MPU region at 0x" TARGET_FMT_lx, addr);
+        }
+        /*
+         * Fall through to handle IO accesses (which will almost certainly
+         * also result in failure)
+         */
+    }
+
     iotlbentry = &env->iotlb[mmu_idx][index];
     section = iotlb_to_section(cpu, iotlbentry->addr, iotlbentry->attrs);
     mr = section->mr;
@@ -1011,8 +1100,8 @@ static void *atomic_mmu_lookup(CPUArchState *env, target_ulong addr,
         tlb_addr = tlbe->addr_write & ~TLB_INVALID_MASK;
     }
 
-    /* Notice an IO access  */
-    if (unlikely(tlb_addr & TLB_MMIO)) {
+    /* Notice an IO access or a needs-MMU-lookup access */
+    if (unlikely(tlb_addr & (TLB_MMIO | TLB_RECHECK))) {
         /* There's really nothing that can be done to
            support this apart from stop-the-world.  */
         goto stop_the_world;
-- 
2.17.1

Le 26/06/2018 à 18:56, Peter Maydell a écrit :
> Add support for MMU protection regions that are smaller than
> TARGET_PAGE_SIZE. We do this by marking the TLB entry for those
> pages with a flag TLB_RECHECK. This flag causes us to always
> take the slow-path for accesses. In the slow path we can then
> special case them to always call tlb_fill() again, so we have
> the correct information for the exact address being accessed.
> 
> This change allows us to handle reading and writing from small
> regions; we cannot deal with execution from the small region.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> Message-id: 20180620130619.11362-2-peter.maydell@linaro.org
> ---
>  accel/tcg/softmmu_template.h |  24 ++++---
>  include/exec/cpu-all.h       |   5 +-
>  accel/tcg/cputlb.c           | 131 +++++++++++++++++++++++++++++------
>  3 files changed, 130 insertions(+), 30 deletions(-)

This patch breaks Quadra 800 emulation, any idea why?

ABCFGHIJK
qemu: fatal: Unable to handle guest executing from RAM within a small
MPU region at 0x0014cb5a
D0 = 0000006a   A0 = 002d8a19   F0 = 7fff ffffffffffffffff  (         nan)
D1 = 00000010   A1 = 002d8a19   F1 = 7fff ffffffffffffffff  (         nan)
D2 = 000003e0   A2 = 00332310   F2 = 7fff ffffffffffffffff  (         nan)
D3 = 00000000   A3 = 00331f98   F3 = 7fff ffffffffffffffff  (         nan)
D4 = 0036da87   A4 = 0036daa3   F4 = 7fff ffffffffffffffff  (         nan)
D5 = 000003e0   A5 = 0036de67   F5 = 7fff ffffffffffffffff  (         nan)
D6 = 002d8a18   A6 = 002d8a1a   F6 = 7fff ffffffffffffffff  (         nan)
D7 = 0014ac46   A7 = 00331ed8   F7 = 7fff ffffffffffffffff  (         nan)
PC = 0014cb5a   SR = 2700 T:0 I:7 SI -----
FPSR = 00000000 ---- -------- -----  FPCR = 0000 X RN --------
  A7(MSP) = 00000000   A7(USP) = 00000000 ->A7(ISP) = 00331f38
VBR = 0x00364528
SFC = 0 DFC 0
SSW 00000000 TCR 00008000 URP 00000000 SRP 00001000
DTTR0/1: 00000000/f807a040 ITTR0/1: 00000000/f807a040
MMUSR 00000000, fault at 00000000
Aborted (core dumped)

Laurent

On 28 June 2018 at 14:03, Laurent Vivier <laurent@vivier.eu> wrote:
> Le 26/06/2018 à 18:56, Peter Maydell a écrit :
>> Add support for MMU protection regions that are smaller than
>> TARGET_PAGE_SIZE. We do this by marking the TLB entry for those
>> pages with a flag TLB_RECHECK. This flag causes us to always
>> take the slow-path for accesses. In the slow path we can then
>> special case them to always call tlb_fill() again, so we have
>> the correct information for the exact address being accessed.
>>
>> This change allows us to handle reading and writing from small
>> regions; we cannot deal with execution from the small region.
>>
>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
>> Message-id: 20180620130619.11362-2-peter.maydell@linaro.org
>> ---
>>  accel/tcg/softmmu_template.h |  24 ++++---
>>  include/exec/cpu-all.h       |   5 +-
>>  accel/tcg/cputlb.c           | 131 +++++++++++++++++++++++++++++------
>>  3 files changed, 130 insertions(+), 30 deletions(-)
>
> This patch breaks Quadra 800 emulation, any idea why?
>
> ABCFGHIJK
> qemu: fatal: Unable to handle guest executing from RAM within a small
> MPU region at 0x0014cb5a

Hmm, that shouldn't happen unless your target code was
incorrectly returning a too-small page size. (I say
"incorrectly" because before this patchseries that was
unsupported and would have had weird effects depending on
exactly what the order of guest accesses to the page was.)

You could look at whether the m68k code is calling tlb_set_page()
with a wrong page_size value and why that happens. You can
get back the old behaviour by having your code do
   if (page_size < TARGET_PAGE_SIZE) {
       page_size = TARGET_PAGE_SIZE;
   }

but that is definitely a bit of a hack.

Does the m68k MMU let you specify permissions and mappings
for sub-page sizes ?

I do notice an oddity:
in m68k_cpu_handle_mmu_fault() we call get_physical_address()
but then ignore the page_size it returns when we call tlb_set_page()
and instead use TARGET_PAGE_SIZE. But in the ptest helper function
we use the page_size from get_physical_address() directly.
Are these bits of code deliberately different?

In fact it's not clear to me at all that PTEST should be
updating the QEMU TLB: it only needs to update the MMU
status registers. (The 68030 manual I have says that in
hardware PTEST doesn't update the ATC, which is the h/w
equivalent to doing a TLB update.)

thanks
-- PMM

Le 28/06/2018 à 15:23, Peter Maydell a écrit :
> On 28 June 2018 at 14:03, Laurent Vivier <laurent@vivier.eu> wrote:
>> Le 26/06/2018 à 18:56, Peter Maydell a écrit :
>>> Add support for MMU protection regions that are smaller than
>>> TARGET_PAGE_SIZE. We do this by marking the TLB entry for those
>>> pages with a flag TLB_RECHECK. This flag causes us to always
>>> take the slow-path for accesses. In the slow path we can then
>>> special case them to always call tlb_fill() again, so we have
>>> the correct information for the exact address being accessed.
>>>
>>> This change allows us to handle reading and writing from small
>>> regions; we cannot deal with execution from the small region.
>>>
>>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>>> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
>>> Message-id: 20180620130619.11362-2-peter.maydell@linaro.org
>>> ---
>>>  accel/tcg/softmmu_template.h |  24 ++++---
>>>  include/exec/cpu-all.h       |   5 +-
>>>  accel/tcg/cputlb.c           | 131 +++++++++++++++++++++++++++++------
>>>  3 files changed, 130 insertions(+), 30 deletions(-)
>>
>> This patch breaks Quadra 800 emulation, any idea why?
>>
>> ABCFGHIJK
>> qemu: fatal: Unable to handle guest executing from RAM within a small
>> MPU region at 0x0014cb5a
> 
> Hmm, that shouldn't happen unless your target code was
> incorrectly returning a too-small page size. (I say
> "incorrectly" because before this patchseries that was
> unsupported and would have had weird effects depending on
> exactly what the order of guest accesses to the page was.)
> 
> You could look at whether the m68k code is calling tlb_set_page()
> with a wrong page_size value and why that happens. You can
> get back the old behaviour by having your code do
>    if (page_size < TARGET_PAGE_SIZE) {
>        page_size = TARGET_PAGE_SIZE;
>    }
> 
> but that is definitely a bit of a hack.

Thank you to have had a look at this.

I've added traces and tlb_set_page() is always called with page_size ==
TARGET_PAGE_SIZE.

m68k linux kernel always use 4 kB page that is the value of
TARGET_PAGE_SIZE.
68040 MMU can also use 8 kB page, but in our case it doesn't (and of
course 8 kB > TARGET_PAGE_SIZE).

> Does the m68k MMU let you specify permissions and mappings
> for sub-page sizes ?

I'm not aware of subpage in m68k MMU. but we have TLB entries that are
separated between code and data: does it change something in your code?
Accessing an address as a data access and then as an instruction access
could appear like a TLB_RECHECK?

> I do notice an oddity:
> in m68k_cpu_handle_mmu_fault() we call get_physical_address()
> but then ignore the page_size it returns when we call tlb_set_page()
> and instead use TARGET_PAGE_SIZE. But in the ptest helper function
> we use the page_size from get_physical_address() directly.
> Are these bits of code deliberately different?

I remember I had problem to make this to work. But I think  you're
right, it should be page_size everywhere. But I guess it's not the cause
of my problem (I tried :) )...

> In fact it's not clear to me at all that PTEST should be
> updating the QEMU TLB: it only needs to update the MMU
> status registers. (The 68030 manual I have says that in
> hardware PTEST doesn't update the ATC, which is the h/w
> equivalent to doing a TLB update.)

In QEMU, we emulate for the moment the 68040 MMU, and PTEST for 68040 is
not defined as the one for 68030.

For 68040, we have:

"A matching entry in the address translation cache (data or instruction)
specified by the function code will be flushed by PTEST. Completion of
PTEST results in the creation of a new address translation cache entry"

Thanks,
Laurent

On 28 June 2018 at 20:23, Laurent Vivier <laurent@vivier.eu> wrote:
> Le 28/06/2018 à 15:23, Peter Maydell a écrit :
>> On 28 June 2018 at 14:03, Laurent Vivier <laurent@vivier.eu> wrote:
>>> Le 26/06/2018 à 18:56, Peter Maydell a écrit :
>>>> Add support for MMU protection regions that are smaller than
>>>> TARGET_PAGE_SIZE. We do this by marking the TLB entry for those
>>>> pages with a flag TLB_RECHECK. This flag causes us to always
>>>> take the slow-path for accesses. In the slow path we can then
>>>> special case them to always call tlb_fill() again, so we have
>>>> the correct information for the exact address being accessed.
>>>>
>>>> This change allows us to handle reading and writing from small
>>>> regions; we cannot deal with execution from the small region.
>>>>
>>>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>>>> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
>>>> Message-id: 20180620130619.11362-2-peter.maydell@linaro.org
>>>> ---
>>>>  accel/tcg/softmmu_template.h |  24 ++++---
>>>>  include/exec/cpu-all.h       |   5 +-
>>>>  accel/tcg/cputlb.c           | 131 +++++++++++++++++++++++++++++------
>>>>  3 files changed, 130 insertions(+), 30 deletions(-)
>>>
>>> This patch breaks Quadra 800 emulation, any idea why?
>>>
>>> ABCFGHIJK
>>> qemu: fatal: Unable to handle guest executing from RAM within a small
>>> MPU region at 0x0014cb5a
>>
>> Hmm, that shouldn't happen unless your target code was
>> incorrectly returning a too-small page size. (I say
>> "incorrectly" because before this patchseries that was
>> unsupported and would have had weird effects depending on
>> exactly what the order of guest accesses to the page was.)
>>
>> You could look at whether the m68k code is calling tlb_set_page()
>> with a wrong page_size value and why that happens. You can
>> get back the old behaviour by having your code do
>>    if (page_size < TARGET_PAGE_SIZE) {
>>        page_size = TARGET_PAGE_SIZE;
>>    }
>>
>> but that is definitely a bit of a hack.
>
> Thank you to have had a look at this.
>
> I've added traces and tlb_set_page() is always called with page_size ==
> TARGET_PAGE_SIZE.
>
> m68k linux kernel always use 4 kB page that is the value of
> TARGET_PAGE_SIZE.
> 68040 MMU can also use 8 kB page, but in our case it doesn't (and of
> course 8 kB > TARGET_PAGE_SIZE).

> I'm not aware of subpage in m68k MMU. but we have TLB entries that are
> separated between code and data: does it change something in your code?
> Accessing an address as a data access and then as an instruction access
> could appear like a TLB_RECHECK?

If you never pass a page_size < TARGET_PAGE_SIZE to
tlb_set_page() then we should never mark anything as TLB_RECHECK:
the theory was no behaviour change for the currently-being-used case.

Do you have a repro case (images, command line) that I can
use to investigate ?

>> In fact it's not clear to me at all that PTEST should be
>> updating the QEMU TLB: it only needs to update the MMU
>> status registers. (The 68030 manual I have says that in
>> hardware PTEST doesn't update the ATC, which is the h/w
>> equivalent to doing a TLB update.)
>
> In QEMU, we emulate for the moment the 68040 MMU, and PTEST for 68040 is
> not defined as the one for 68030.
>
> For 68040, we have:
>
> "A matching entry in the address translation cache (data or instruction)
> specified by the function code will be flushed by PTEST. Completion of
> PTEST results in the creation of a new address translation cache entry"

Oh, OK. Since the QEMU TLB isn't really the same as the hardware
TLB then it isn't strictly required to update our TLB here, but
if the hardware does that then it doesn't hurt.

thanks
-- PMM

Le 28/06/2018 à 22:05, Peter Maydell a écrit :
> On 28 June 2018 at 20:23, Laurent Vivier <laurent@vivier.eu> wrote:
>> Le 28/06/2018 à 15:23, Peter Maydell a écrit :
>>> On 28 June 2018 at 14:03, Laurent Vivier <laurent@vivier.eu> wrote:
>>>> Le 26/06/2018 à 18:56, Peter Maydell a écrit :
>>>>> Add support for MMU protection regions that are smaller than
>>>>> TARGET_PAGE_SIZE. We do this by marking the TLB entry for those
>>>>> pages with a flag TLB_RECHECK. This flag causes us to always
>>>>> take the slow-path for accesses. In the slow path we can then
>>>>> special case them to always call tlb_fill() again, so we have
>>>>> the correct information for the exact address being accessed.
>>>>>
>>>>> This change allows us to handle reading and writing from small
>>>>> regions; we cannot deal with execution from the small region.
>>>>>
>>>>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>>>>> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
>>>>> Message-id: 20180620130619.11362-2-peter.maydell@linaro.org
>>>>> ---
>>>>>  accel/tcg/softmmu_template.h |  24 ++++---
>>>>>  include/exec/cpu-all.h       |   5 +-
>>>>>  accel/tcg/cputlb.c           | 131 +++++++++++++++++++++++++++++------
>>>>>  3 files changed, 130 insertions(+), 30 deletions(-)
>>>>
>>>> This patch breaks Quadra 800 emulation, any idea why?
>>>>
>>>> ABCFGHIJK
>>>> qemu: fatal: Unable to handle guest executing from RAM within a small
>>>> MPU region at 0x0014cb5a
>>>
>>> Hmm, that shouldn't happen unless your target code was
>>> incorrectly returning a too-small page size. (I say
>>> "incorrectly" because before this patchseries that was
>>> unsupported and would have had weird effects depending on
>>> exactly what the order of guest accesses to the page was.)
>>>
>>> You could look at whether the m68k code is calling tlb_set_page()
>>> with a wrong page_size value and why that happens. You can
>>> get back the old behaviour by having your code do
>>>    if (page_size < TARGET_PAGE_SIZE) {
>>>        page_size = TARGET_PAGE_SIZE;
>>>    }
>>>
>>> but that is definitely a bit of a hack.
>>
>> Thank you to have had a look at this.
>>
>> I've added traces and tlb_set_page() is always called with page_size ==
>> TARGET_PAGE_SIZE.
>>
>> m68k linux kernel always use 4 kB page that is the value of
>> TARGET_PAGE_SIZE.
>> 68040 MMU can also use 8 kB page, but in our case it doesn't (and of
>> course 8 kB > TARGET_PAGE_SIZE).
> 
>> I'm not aware of subpage in m68k MMU. but we have TLB entries that are
>> separated between code and data: does it change something in your code?
>> Accessing an address as a data access and then as an instruction access
>> could appear like a TLB_RECHECK?
> 
> If you never pass a page_size < TARGET_PAGE_SIZE to
> tlb_set_page() then we should never mark anything as TLB_RECHECK:
> the theory was no behaviour change for the currently-being-used case.
> 
> Do you have a repro case (images, command line) that I can
> use to investigate ?
- checkout the branch q800-dev-part1 from
  git://github.com/vivier/qemu-m68k.git

- configure and build

 './configure' '--target-list=m68k-softmmu' '--enable-debug' \
               '--enable-debug-tcg' '--enable-debug-info'

  my gcc is from Fedora 27, version 7.3.1 20180303 (Red Hat 7.3.1-5)

- get the kernel from the debian installer:

wget
https://cdimage.debian.org/mirror/cdimage/ports/9.0/m68k/iso-cd/debian-9.0-m68k-NETINST-1.iso

guestfish --add debian-9.0-m68k-NETINST-1.iso --ro \
          --mount /dev/sda:/ <<_EOF_
copy-out /install/kernels/vmlinux-4.15.0-2-m68k .
_EOF_

- and run

./m68k-softmmu/qemu-system-m68k -M q800 \
    -serial none -serial mon:stdio \
    -kernel vmlinux-4.15.0-2-m68k \
    -nographic

ABCFGHIJK
qemu: fatal: Unable to handle guest executing from RAM within a small
MPU region at 0x0029bb2c
D0 = 003ca111   A0 = 003ca111   F0 = 7fff ffffffffffffffff  (         nan)
D1 = 00000000   A1 = 0000000a   F1 = 7fff ffffffffffffffff  (         nan)
D2 = 00000000   A2 = 00395314   F2 = 7fff ffffffffffffffff  (         nan)
D3 = 00000001   A3 = 003ca110   F3 = 7fff ffffffffffffffff  (         nan)
D4 = 000003e0   A4 = 003ca4e8   F4 = 7fff ffffffffffffffff  (         nan)
D5 = 00393fc8   A5 = 0033d77b   F5 = 7fff ffffffffffffffff  (         nan)
D6 = 003ca108   A6 = 00393fc4   F6 = 7fff ffffffffffffffff  (         nan)
D7 = 00000002   A7 = 00393ef8   F7 = 7fff ffffffffffffffff  (         nan)
PC = 0029bb2c   SR = 2700 T:0 I:7 SI -----
FPSR = 00000000 ---- -------- -----  FPCR = 0000 X RN --------
  A7(MSP) = 00000000   A7(USP) = 00000000 ->A7(ISP) = 00393f68
VBR = 0x003bfce8
SFC = 5 DFC 5
SSW 00000000 TCR 00008000 URP 00000000 SRP 00001000
DTTR0/1: 00000000/f807a040 ITTR0/1: 00000000/f807a040
MMUSR 00000000, fault at 00000000
Aborted (core dumped)

Thanks,
Laurent

On 28 June 2018 at 23:26, Laurent Vivier <laurent@vivier.eu> wrote:
> ./m68k-softmmu/qemu-system-m68k -M q800 \
>     -serial none -serial mon:stdio \
>     -kernel vmlinux-4.15.0-2-m68k \
>     -nographic

Thanks for the test case. I'm still investigating, but there
are a couple of things happening here.

First, there's a bug in get_page_addr_code()'s "is this a
TLB miss?" condition which was introduced in commit 71b9a45330fe22:

    if (unlikely(env->tlb_table[mmu_idx][index].addr_code !=
                 (addr & (TARGET_PAGE_MASK | TLB_INVALID_MASK)))) {

takes a (not necessarily page aligned) address, and masks out
everything but the page-aligned top half (good) and the
TLB_INVALID bit (not good, because that could be either 0 or 1
depending on the address). This means sometimes we'll incorrectly
decide we got a miss in the TLB and do an unnecessary refill.

The second thing that's going on here is that the m68k target
code writes TLB entries for the same address with different
prot bits without doing a flush in between:

tlb_set_page_with_attrs: vaddr=0029b000 paddr=0x000000000029b000 prot=3 idx=0
tlb_set_page_with_attrs: vaddr=0029b000 paddr=0x000000000029b000 prot=7 idx=0

The tlb_set_page_with_attrs() code isn't expecting this, so
we end up with two TLB entries for the same address, one in
the main TLB and one in the victim cache TLB. The bug above
means that we get this sequence of events:
 * fill main TLB entry with prot=3 entry
 * later, fill main TLB with prot=7 entry, and evict prot=3
   entry to victim cache
 * hit on the prot=7 entry in the main TLB
 * refill condition incorrectly fails, but we hit in the victim cache
 * so we pull the prot=3 entry from victim to main TLB
 * prot=3 means "addr_code == -1", so the check of the TLB_RECHECK
   bit succeeds
 * in the TLB_RECHECK code we do a tlb_fill()
 * that fills in the main TLB with a prot=7 entry again, bouncing
   the prot=3 entry back out to the victim cache
 * prot=7 means the addr_code is correct, so we find ourselves in
   the "TLB_RECHECK but this is RAM" abort code path

I'm not sure whether it's supposed to be the responsibility
of the target code or the common accel/tcg code to ensure
that we don't have multiple TLB entries for the same address.

thanks
-- PMM

Peter Maydell <peter.maydell@linaro.org> writes:

> On 28 June 2018 at 23:26, Laurent Vivier <laurent@vivier.eu> wrote:
>> ./m68k-softmmu/qemu-system-m68k -M q800 \
>>     -serial none -serial mon:stdio \
>>     -kernel vmlinux-4.15.0-2-m68k \
>>     -nographic
>
> Thanks for the test case. I'm still investigating, but there
> are a couple of things happening here.
>
> First, there's a bug in get_page_addr_code()'s "is this a
> TLB miss?" condition which was introduced in commit 71b9a45330fe22:
>
>     if (unlikely(env->tlb_table[mmu_idx][index].addr_code !=
>                  (addr & (TARGET_PAGE_MASK | TLB_INVALID_MASK)))) {
>
> takes a (not necessarily page aligned) address, and masks out
> everything but the page-aligned top half (good) and the
> TLB_INVALID bit (not good, because that could be either 0 or 1
> depending on the address). This means sometimes we'll incorrectly
> decide we got a miss in the TLB and do an unnecessary refill.
>
> The second thing that's going on here is that the m68k target
> code writes TLB entries for the same address with different
> prot bits without doing a flush in between:
>
> tlb_set_page_with_attrs: vaddr=0029b000 paddr=0x000000000029b000 prot=3 idx=0
> tlb_set_page_with_attrs: vaddr=0029b000 paddr=0x000000000029b000 prot=7 idx=0
>
> The tlb_set_page_with_attrs() code isn't expecting this, so
> we end up with two TLB entries for the same address, one in
> the main TLB and one in the victim cache TLB. The bug above
> means that we get this sequence of events:
>  * fill main TLB entry with prot=3 entry
>  * later, fill main TLB with prot=7 entry, and evict prot=3
>    entry to victim cache
>  * hit on the prot=7 entry in the main TLB
>  * refill condition incorrectly fails, but we hit in the victim cache
>  * so we pull the prot=3 entry from victim to main TLB
>  * prot=3 means "addr_code == -1", so the check of the TLB_RECHECK
>    bit succeeds
>  * in the TLB_RECHECK code we do a tlb_fill()
>  * that fills in the main TLB with a prot=7 entry again, bouncing
>    the prot=3 entry back out to the victim cache
>  * prot=7 means the addr_code is correct, so we find ourselves in
>    the "TLB_RECHECK but this is RAM" abort code path
>
> I'm not sure whether it's supposed to be the responsibility
> of the target code or the common accel/tcg code to ensure
> that we don't have multiple TLB entries for the same address.

My gut feeling is we should fail safely in the case of the guest writing
two mostly identical page entries in a row. We can check for aliasing
when we update and either evict to the victim cache or reset the vtlb
entry.

>
> thanks
> -- PMM


--
Alex Bennée

On 28 June 2018 at 23:26, Laurent Vivier <laurent@vivier.eu> wrote:
> Le 28/06/2018 à 22:05, Peter Maydell a écrit :
>> Do you have a repro case (images, command line) that I can
>> use to investigate ?
> - checkout the branch q800-dev-part1 from
>   git://github.com/vivier/qemu-m68k.git
>
> - configure and build
>
>  './configure' '--target-list=m68k-softmmu' '--enable-debug' \
>                '--enable-debug-tcg' '--enable-debug-info'
>
>   my gcc is from Fedora 27, version 7.3.1 20180303 (Red Hat 7.3.1-5)
>
> - get the kernel from the debian installer:
>
> wget
> https://cdimage.debian.org/mirror/cdimage/ports/9.0/m68k/iso-cd/debian-9.0-m68k-NETINST-1.iso
>
> guestfish --add debian-9.0-m68k-NETINST-1.iso --ro \
>           --mount /dev/sda:/ <<_EOF_
> copy-out /install/kernels/vmlinux-4.15.0-2-m68k .
> _EOF_
>
> - and run
>
> ./m68k-softmmu/qemu-system-m68k -M q800 \
>     -serial none -serial mon:stdio \
>     -kernel vmlinux-4.15.0-2-m68k \
>     -nographic

What is this testcase supposed to print when it works?
I tried reverting 55df6fcf5476b44bc1b9, but that just prints
"ABCFGHIJK" and then nothing else.

thanks
-- PMM

Le 29/06/2018 à 17:28, Peter Maydell a écrit :
> On 28 June 2018 at 23:26, Laurent Vivier <laurent@vivier.eu> wrote:
>> Le 28/06/2018 à 22:05, Peter Maydell a écrit :
>>> Do you have a repro case (images, command line) that I can
>>> use to investigate ?
>> - checkout the branch q800-dev-part1 from
>>   git://github.com/vivier/qemu-m68k.git
>>
>> - configure and build
>>
>>  './configure' '--target-list=m68k-softmmu' '--enable-debug' \
>>                '--enable-debug-tcg' '--enable-debug-info'
>>
>>   my gcc is from Fedora 27, version 7.3.1 20180303 (Red Hat 7.3.1-5)
>>
>> - get the kernel from the debian installer:
>>
>> wget
>> https://cdimage.debian.org/mirror/cdimage/ports/9.0/m68k/iso-cd/debian-9.0-m68k-NETINST-1.iso
>>
>> guestfish --add debian-9.0-m68k-NETINST-1.iso --ro \
>>           --mount /dev/sda:/ <<_EOF_
>> copy-out /install/kernels/vmlinux-4.15.0-2-m68k .
>> _EOF_
>>
>> - and run
>>
>> ./m68k-softmmu/qemu-system-m68k -M q800 \
>>     -serial none -serial mon:stdio \
>>     -kernel vmlinux-4.15.0-2-m68k \
>>     -nographic
> 
> What is this testcase supposed to print when it works?
> I tried reverting 55df6fcf5476b44bc1b9, but that just prints
> "ABCFGHIJK" and then nothing else.

At this point, you can either remove the -nographic or add -append
"console=ttyS0 vga=off" to have the kernel boot logs.

If you want to have started an userspace command, add the initrd from
the CD:

guestfish --add debian-9.0-m68k-NETINST-1.iso --ro \
          --mount /dev/sda:/ <<_EOF_
copy-out /install/cdrom/initrd.gz .
_EOF_

Thanks,
Laurent