[Qemu-devel] [PATCH 0/6] accel/tcg: Support execution from MMIO and small MMU regions

[Qemu-devel] [PATCH 0/6] accel/tcg: Support execution from MMIO and small MMU regions

[Peter Maydell]

This series adds support to TCG for executing from MMIO regions
and small MMU regions. The basic principle is that if get_page_addr_code()
finds that the region is not backed by a full page of RAM then it
returns -1, and tb_gen_code() then generates a non-cached TB
containing a single instruction. Execution from these regions
thus performs the instruction fetch every time, ensuring that we
get the read-from-MMIO and check-small-MMU-region permissions
checks right.

This means that the code path for "generate bus fault for failing
to load an instruction" no longer goes through get_page_addr_code(),
but instead via each target's translate code and its calls to
the cpu_ld*_code() or similar functions. Patch 1 makes sure we
can distinguish insn fetches from data loads when generating the
bus fault exceptions. (Aside: I have assumed that all cpu_ld*_code()
loads should trigger iside faults rather than dside. Hopefully this
is true...)

Patches 2 and 3 make trivial fixes to various callers of
get_page_addr_code(); patch 4 does the work of generating our
single-insn TBs. Patch 5 can then remove all the code that
(mis)handles MMIO regions from get_page_addr_code(). Finally
patch 6 drops the target/arm workarounds for not having support
for executing from small MPU regions.

Note for the Xilinx folks: this patchset makes the mmio-exec
testcase for running from the SPI flash pass. Cedric: you might
like to test the aspeed image you had that relies on execution
from an MMIO region too.

The diffstat is pretty satisfying for a patchset that adds
a feature, but it actually undersells it: this code renders the
hw/misc/mmio_interface.c and the mmio_ptr related code in memory.c
and the xilinx-spips device all obsolete, so there are another
couple of hundred lines of code to be deleted there. I opted not
to include that in this patchset, for ease of review.

NB: I tested this with icount, but there are potentially
some weird things that could happen with interactions between
icount's io-recompile and execution from an MMIO device
that returns different instructions each time it's read.

thanks
-- PMM


Peter Maydell (6):
  accel/tcg: Pass read access type through to io_readx()
  accel/tcg: Handle get_page_addr_code() returning -1 in hashtable
    lookups
  accel/tcg: Handle get_page_addr_code() returning -1 in
    tb_check_watchpoint()
  accel/tcg: tb_gen_code(): Create single-insn TB for execution from
    non-RAM
  accel/tcg: Return -1 for execution from MMIO regions in
    get_page_addr_code()
  target/arm: Allow execution from small regions

 accel/tcg/softmmu_template.h |  11 ++--
 include/qom/cpu.h            |   6 +++
 accel/tcg/cpu-exec.c         |   3 ++
 accel/tcg/cputlb.c           | 100 +++++------------------------------
 accel/tcg/translate-all.c    |  23 +++++++-
 memory.c                     |   3 +-
 target/arm/helper.c          |  23 --------
 7 files changed, 52 insertions(+), 117 deletions(-)

-- 
2.17.1

[Qemu-devel] [PATCH 1/6] accel/tcg: Pass read access type through to io_readx()

[Peter Maydell]

The io_readx() function needs to know whether the load it is
doing is an MMU_DATA_LOAD or an MMU_INST_FETCH, so that it
can pass the right value to the cpu_transaction_failed()
function. Plumb this information through from the softmmu
code.

This is currently not often going to give the wrong answer,
because usually instruction fetches go via get_page_addr_code().
However once we switch over to handling execution from non-RAM by
creating single-insn TBs, the path for an insn fetch to generate
a bus error will be through cpu_ld*_code() and io_readx(),
so without this change we will generate a d-side fault when we
should generate an i-side fault.

We also have to pass the access type via a CPU struct global
down to unassigned_mem_read(), for the benefit of the targets
which still use the cpu_unassigned_access() hook (m68k, mips,
sparc, xtensa).

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 accel/tcg/softmmu_template.h | 11 +++++++----
 include/qom/cpu.h            |  6 ++++++
 accel/tcg/cputlb.c           |  5 +++--
 memory.c                     |  3 ++-
 4 files changed, 18 insertions(+), 7 deletions(-)

diff --git a/accel/tcg/softmmu_template.h b/accel/tcg/softmmu_template.h
index badbf148803..f060a693d41 100644
--- a/accel/tcg/softmmu_template.h
+++ b/accel/tcg/softmmu_template.h
@@ -99,11 +99,12 @@ static inline DATA_TYPE glue(io_read, SUFFIX)(CPUArchState *env,
                                               size_t mmu_idx, size_t index,
                                               target_ulong addr,
                                               uintptr_t retaddr,
-                                              bool recheck)
+                                              bool recheck,
+                                              MMUAccessType access_type)
 {
     CPUIOTLBEntry *iotlbentry = &env->iotlb[mmu_idx][index];
     return io_readx(env, iotlbentry, mmu_idx, addr, retaddr, recheck,
-                    DATA_SIZE);
+                    access_type, DATA_SIZE);
 }
 #endif
 
@@ -140,7 +141,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,
-                                    tlb_addr & TLB_RECHECK);
+                                    tlb_addr & TLB_RECHECK,
+                                    READ_ACCESS_TYPE);
         res = TGT_LE(res);
         return res;
     }
@@ -207,7 +209,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,
-                                    tlb_addr & TLB_RECHECK);
+                                    tlb_addr & TLB_RECHECK,
+                                    READ_ACCESS_TYPE);
         res = TGT_BE(res);
         return res;
     }
diff --git a/include/qom/cpu.h b/include/qom/cpu.h
index bd796579ee4..ecf6ed556a9 100644
--- a/include/qom/cpu.h
+++ b/include/qom/cpu.h
@@ -386,6 +386,12 @@ struct CPUState {
      */
     uintptr_t mem_io_pc;
     vaddr mem_io_vaddr;
+    /*
+     * This is only needed for the legacy cpu_unassigned_access() hook;
+     * when all targets using it have been converted to use
+     * cpu_transaction_failed() instead it can be removed.
+     */
+    MMUAccessType mem_io_access_type;
 
     int kvm_fd;
     struct KVMState *kvm_state;
diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
index 20c147d6554..c491703f15f 100644
--- a/accel/tcg/cputlb.c
+++ b/accel/tcg/cputlb.c
@@ -789,7 +789,7 @@ 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,
-                         bool recheck, int size)
+                         bool recheck, MMUAccessType access_type, int size)
 {
     CPUState *cpu = ENV_GET_CPU(env);
     hwaddr mr_offset;
@@ -831,6 +831,7 @@ static uint64_t io_readx(CPUArchState *env, CPUIOTLBEntry *iotlbentry,
     }
 
     cpu->mem_io_vaddr = addr;
+    cpu->mem_io_access_type = access_type;
 
     if (mr->global_locking && !qemu_mutex_iothread_locked()) {
         qemu_mutex_lock_iothread();
@@ -843,7 +844,7 @@ static uint64_t io_readx(CPUArchState *env, CPUIOTLBEntry *iotlbentry,
             section->offset_within_address_space -
             section->offset_within_region;
 
-        cpu_transaction_failed(cpu, physaddr, addr, size, MMU_DATA_LOAD,
+        cpu_transaction_failed(cpu, physaddr, addr, size, access_type,
                                mmu_idx, iotlbentry->attrs, r, retaddr);
     }
     if (locked) {
diff --git a/memory.c b/memory.c
index e9cd4469688..2ea16e7bfb0 100644
--- a/memory.c
+++ b/memory.c
@@ -1249,7 +1249,8 @@ static uint64_t unassigned_mem_read(void *opaque, hwaddr addr,
     printf("Unassigned mem read " TARGET_FMT_plx "\n", addr);
 #endif
     if (current_cpu != NULL) {
-        cpu_unassigned_access(current_cpu, addr, false, false, 0, size);
+        bool is_exec = current_cpu->mem_io_access_type == MMU_INST_FETCH;
+        cpu_unassigned_access(current_cpu, addr, false, is_exec, 0, size);
     }
     return 0;
 }
-- 
2.17.1

[Qemu-devel] [PATCH 2/6] accel/tcg: Handle get_page_addr_code() returning -1 in hashtable lookups

[Peter Maydell]

When we support execution from non-RAM MMIO regions, get_page_addr_code()
will return -1 to indicate that there is no RAM at the requested address.
Handle this in the cpu-exec TB hashtable lookup code, treating it as
"no match found".

Note that the call to get_page_addr_code() in tb_lookup_cmp() needs
no changes -- a return of -1 will already correctly result in the
function returning false.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 accel/tcg/cpu-exec.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c
index c738b7f7d6e..6bcb6d99bd7 100644
--- a/accel/tcg/cpu-exec.c
+++ b/accel/tcg/cpu-exec.c
@@ -332,6 +332,9 @@ TranslationBlock *tb_htable_lookup(CPUState *cpu, target_ulong pc,
     desc.trace_vcpu_dstate = *cpu->trace_dstate;
     desc.pc = pc;
     phys_pc = get_page_addr_code(desc.env, pc);
+    if (phys_pc == -1) {
+        return NULL;
+    }
     desc.phys_page1 = phys_pc & TARGET_PAGE_MASK;
     h = tb_hash_func(phys_pc, pc, flags, cf_mask, *cpu->trace_dstate);
     return qht_lookup_custom(&tb_ctx.htable, &desc, h, tb_lookup_cmp);
-- 
2.17.1

[Qemu-devel] [PATCH 3/6] accel/tcg: Handle get_page_addr_code() returning -1 in tb_check_watchpoint()

[Peter Maydell]

When we support execution from non-RAM MMIO regions, get_page_addr_code()
will return -1 to indicate that there is no RAM at the requested address.
Handle this in tb_check_watchpoint() -- if the exception happened for a
PC which doesn't correspond to RAM then there is no need to invalidate
any TBs, because the one-instruction TB will not have been cached.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 accel/tcg/translate-all.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c
index 49d77fad44e..d18018fa99d 100644
--- a/accel/tcg/translate-all.c
+++ b/accel/tcg/translate-all.c
@@ -2121,7 +2121,9 @@ void tb_check_watchpoint(CPUState *cpu)
 
         cpu_get_tb_cpu_state(env, &pc, &cs_base, &flags);
         addr = get_page_addr_code(env, pc);
-        tb_invalidate_phys_range(addr, addr + 1);
+        if (addr != -1) {
+            tb_invalidate_phys_range(addr, addr + 1);
+        }
     }
 }
 
-- 
2.17.1

[Qemu-devel] [PATCH 4/6] accel/tcg: tb_gen_code(): Create single-insn TB for execution from non-RAM

[Peter Maydell]

If get_page_addr_code() returns -1, this indicates that there is no RAM
page we can read a full TB from. Instead we must create a TB which
contains a single instruction and which we do not cache, so it is
executed only once.

Since this means we can now have TBs which are not in any page list,
we also need to make tb_phys_invalidate() handle them (by not trying
to remove them from a nonexistent page list).

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 accel/tcg/translate-all.c | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c
index d18018fa99d..4c12d94dd13 100644
--- a/accel/tcg/translate-all.c
+++ b/accel/tcg/translate-all.c
@@ -1493,7 +1493,7 @@ static void tb_phys_invalidate__locked(TranslationBlock *tb)
  */
 void tb_phys_invalidate(TranslationBlock *tb, tb_page_addr_t page_addr)
 {
-    if (page_addr == -1) {
+    if (page_addr == -1 && tb->page_addr[0] != -1) {
         page_lock_tb(tb);
         do_tb_phys_invalidate(tb, true);
         page_unlock_tb(tb);
@@ -1608,6 +1608,17 @@ tb_link_page(TranslationBlock *tb, tb_page_addr_t phys_pc,
 
     assert_memory_lock();
 
+    if (phys_pc == -1) {
+        /*
+         * If the TB is not associated with a physical RAM page then
+         * it must be a temporary one-insn TB, and we have nothing to do
+         * except fill in the page_addr[] fields.
+         */
+        assert(tb->cflags & CF_NOCACHE);
+        tb->page_addr[0] = tb->page_addr[1] = -1;
+        return tb;
+    }
+
     /*
      * Add the TB to the page list, acquiring first the pages's locks.
      * We keep the locks held until after inserting the TB in the hash table,
@@ -1677,6 +1688,12 @@ TranslationBlock *tb_gen_code(CPUState *cpu,
 
     phys_pc = get_page_addr_code(env, pc);
 
+    if (phys_pc == -1) {
+        /* Generate a temporary TB with 1 insn in it */
+        cflags &= ~CF_COUNT_MASK;
+        cflags |= CF_NOCACHE | 1;
+    }
+
  buffer_overflow:
     tb = tb_alloc(pc);
     if (unlikely(!tb)) {
-- 
2.17.1

[Qemu-devel] [PATCH 5/6] accel/tcg: Return -1 for execution from MMIO regions in get_page_addr_code()

[Peter Maydell]

Now that all the callers can handle get_page_addr_code() returning -1,
remove all the code which tries to handle execution from MMIO regions
or small-MMU-region RAM areas. This will mean that we can correctly
execute from these areas, rather than ending up either aborting QEMU
or delivering an incorrect guest exception.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 accel/tcg/cputlb.c | 95 +++++-----------------------------------------
 1 file changed, 10 insertions(+), 85 deletions(-)

diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
index c491703f15f..abb0225dc79 100644
--- a/accel/tcg/cputlb.c
+++ b/accel/tcg/cputlb.c
@@ -741,39 +741,6 @@ void tlb_set_page(CPUState *cpu, target_ulong vaddr,
                             prot, mmu_idx, size);
 }
 
-static void report_bad_exec(CPUState *cpu, target_ulong addr)
-{
-    /* Accidentally executing outside RAM or ROM is quite common for
-     * several user-error situations, so report it in a way that
-     * makes it clear that this isn't a QEMU bug and provide suggestions
-     * about what a user could do to fix things.
-     */
-    error_report("Trying to execute code outside RAM or ROM at 0x"
-                 TARGET_FMT_lx, addr);
-    error_printf("This usually means one of the following happened:\n\n"
-                 "(1) You told QEMU to execute a kernel for the wrong machine "
-                 "type, and it crashed on startup (eg trying to run a "
-                 "raspberry pi kernel on a versatilepb QEMU machine)\n"
-                 "(2) You didn't give QEMU a kernel or BIOS filename at all, "
-                 "and QEMU executed a ROM full of no-op instructions until "
-                 "it fell off the end\n"
-                 "(3) Your guest kernel has a bug and crashed by jumping "
-                 "off into nowhere\n\n"
-                 "This is almost always one of the first two, so check your "
-                 "command line and that you are using the right type of kernel "
-                 "for this machine.\n"
-                 "If you think option (3) is likely then you can try debugging "
-                 "your guest with the -d debug options; in particular "
-                 "-d guest_errors will cause the log to include a dump of the "
-                 "guest register state at this point.\n\n"
-                 "Execution cannot continue; stopping here.\n\n");
-
-    /* Report also to the logs, with more detail including register dump */
-    qemu_log_mask(LOG_GUEST_ERROR, "qemu: fatal: Trying to execute code "
-                  "outside RAM or ROM at 0x" TARGET_FMT_lx "\n", addr);
-    log_cpu_state_mask(LOG_GUEST_ERROR, cpu, CPU_DUMP_FPU | CPU_DUMP_CCOP);
-}
-
 static inline ram_addr_t qemu_ram_addr_from_host_nofail(void *ptr)
 {
     ram_addr_t ram_addr;
@@ -963,7 +930,6 @@ tb_page_addr_t get_page_addr_code(CPUArchState *env, target_ulong addr)
     MemoryRegionSection *section;
     CPUState *cpu = ENV_GET_CPU(env);
     CPUIOTLBEntry *iotlbentry;
-    hwaddr physaddr, mr_offset;
 
     index = (addr >> TARGET_PAGE_BITS) & (CPU_TLB_SIZE - 1);
     mmu_idx = cpu_mmu_index(env, true);
@@ -977,65 +943,24 @@ tb_page_addr_t get_page_addr_code(CPUArchState *env, target_ulong addr)
                   (TLB_RECHECK | TLB_INVALID_MASK)) == 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)
+         * covers a smaller range than a target page. Return -1 to
+         * indicate that we cannot simply execute from RAM here;
+         * we will perform the necessary repeat of the MMU check
+         * when the "execute a single insn" code performs the
+         * load of the guest insn.
          */
+        return -1;
     }
 
     iotlbentry = &env->iotlb[mmu_idx][index];
     section = iotlb_to_section(cpu, iotlbentry->addr, iotlbentry->attrs);
     mr = section->mr;
     if (memory_region_is_unassigned(mr)) {
-        qemu_mutex_lock_iothread();
-        if (memory_region_request_mmio_ptr(mr, addr)) {
-            qemu_mutex_unlock_iothread();
-            /* A MemoryRegion is potentially added so re-run the
-             * get_page_addr_code.
-             */
-            return get_page_addr_code(env, addr);
-        }
-        qemu_mutex_unlock_iothread();
-
-        /* Give the new-style cpu_transaction_failed() hook first chance
-         * to handle this.
-         * This is not the ideal place to detect and generate CPU
-         * exceptions for instruction fetch failure (for instance
-         * we don't know the length of the access that the CPU would
-         * use, and it would be better to go ahead and try the access
-         * and use the MemTXResult it produced). However it is the
-         * simplest place we have currently available for the check.
+        /*
+         * Not guest RAM, so there is no ram_addr_t for it. Return -1,
+         * and we will execute a single insn from this device.
          */
-        mr_offset = (iotlbentry->addr & TARGET_PAGE_MASK) + addr;
-        physaddr = mr_offset +
-            section->offset_within_address_space -
-            section->offset_within_region;
-        cpu_transaction_failed(cpu, physaddr, addr, 0, MMU_INST_FETCH, mmu_idx,
-                               iotlbentry->attrs, MEMTX_DECODE_ERROR, 0);
-
-        cpu_unassigned_access(cpu, addr, false, true, 0, 4);
-        /* The CPU's unassigned access hook might have longjumped out
-         * with an exception. If it didn't (or there was no hook) then
-         * we can't proceed further.
-         */
-        report_bad_exec(cpu, addr);
-        exit(1);
+        return -1;
     }
     p = (void *)((uintptr_t)addr + env->tlb_table[mmu_idx][index].addend);
     return qemu_ram_addr_from_host_nofail(p);
-- 
2.17.1

[Qemu-devel] [PATCH 6/6] target/arm: Allow execution from small regions

[Peter Maydell]

Now that we have full support for small regions, including execution,
we can remove the workarounds where we marked all small regions as
non-executable for the M-profile MPU and SAU.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/arm/helper.c | 23 -----------------------
 1 file changed, 23 deletions(-)

diff --git a/target/arm/helper.c b/target/arm/helper.c
index a2ac96084e7..ed96e6c02fb 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -9784,17 +9784,6 @@ static bool get_phys_addr_pmsav7(CPUARMState *env, uint32_t address,
 
     fi->type = ARMFault_Permission;
     fi->level = 1;
-    /*
-     * Core QEMU code can't handle execution from small pages yet, so
-     * don't try it. This way we'll get an MPU exception, rather than
-     * eventually causing QEMU to exit in get_page_addr_code().
-     */
-    if (*page_size < TARGET_PAGE_SIZE && (*prot & PAGE_EXEC)) {
-        qemu_log_mask(LOG_UNIMP,
-                      "MPU: No support for execution from regions "
-                      "smaller than 1K\n");
-        *prot &= ~PAGE_EXEC;
-    }
     return !(*prot & (1 << access_type));
 }
 
@@ -10014,18 +10003,6 @@ static bool pmsav8_mpu_lookup(CPUARMState *env, uint32_t address,
 
     fi->type = ARMFault_Permission;
     fi->level = 1;
-    /*
-     * Core QEMU code can't handle execution from small pages yet, so
-     * don't try it. This means any attempted execution will generate
-     * an MPU exception, rather than eventually causing QEMU to exit in
-     * get_page_addr_code().
-     */
-    if (*is_subpage && (*prot & PAGE_EXEC)) {
-        qemu_log_mask(LOG_UNIMP,
-                      "MPU: No support for execution from regions "
-                      "smaller than 1K\n");
-        *prot &= ~PAGE_EXEC;
-    }
     return !(*prot & (1 << access_type));
 }
 
-- 
2.17.1

Re: [Qemu-devel] [PATCH 1/6] accel/tcg: Pass read access type through to io_readx()

[Richard Henderson]

On 07/10/2018 09:00 AM, Peter Maydell wrote:
> The io_readx() function needs to know whether the load it is
> doing is an MMU_DATA_LOAD or an MMU_INST_FETCH, so that it
> can pass the right value to the cpu_transaction_failed()
> function. Plumb this information through from the softmmu
> code.
> 
> This is currently not often going to give the wrong answer,
> because usually instruction fetches go via get_page_addr_code().
> However once we switch over to handling execution from non-RAM by
> creating single-insn TBs, the path for an insn fetch to generate
> a bus error will be through cpu_ld*_code() and io_readx(),
> so without this change we will generate a d-side fault when we
> should generate an i-side fault.
> 
> We also have to pass the access type via a CPU struct global
> down to unassigned_mem_read(), for the benefit of the targets
> which still use the cpu_unassigned_access() hook (m68k, mips,
> sparc, xtensa).
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  accel/tcg/softmmu_template.h | 11 +++++++----
>  include/qom/cpu.h            |  6 ++++++
>  accel/tcg/cputlb.c           |  5 +++--
>  memory.c                     |  3 ++-
>  4 files changed, 18 insertions(+), 7 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [Qemu-devel] [PATCH 2/6] accel/tcg: Handle get_page_addr_code() returning -1 in hashtable lookups

[Richard Henderson]

On 07/10/2018 09:00 AM, Peter Maydell wrote:
> When we support execution from non-RAM MMIO regions, get_page_addr_code()
> will return -1 to indicate that there is no RAM at the requested address.
> Handle this in the cpu-exec TB hashtable lookup code, treating it as
> "no match found".
> 
> Note that the call to get_page_addr_code() in tb_lookup_cmp() needs
> no changes -- a return of -1 will already correctly result in the
> function returning false.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  accel/tcg/cpu-exec.c | 3 +++
>  1 file changed, 3 insertions(+)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [Qemu-devel] [PATCH 3/6] accel/tcg: Handle get_page_addr_code() returning -1 in tb_check_watchpoint()

[Richard Henderson]

On 07/10/2018 09:00 AM, Peter Maydell wrote:
> When we support execution from non-RAM MMIO regions, get_page_addr_code()
> will return -1 to indicate that there is no RAM at the requested address.
> Handle this in tb_check_watchpoint() -- if the exception happened for a
> PC which doesn't correspond to RAM then there is no need to invalidate
> any TBs, because the one-instruction TB will not have been cached.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  accel/tcg/translate-all.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [Qemu-devel] [PATCH 4/6] accel/tcg: tb_gen_code(): Create single-insn TB for execution from non-RAM

[Richard Henderson]

On 07/10/2018 09:00 AM, Peter Maydell wrote:
> If get_page_addr_code() returns -1, this indicates that there is no RAM
> page we can read a full TB from. Instead we must create a TB which
> contains a single instruction and which we do not cache, so it is
> executed only once.
> 
> Since this means we can now have TBs which are not in any page list,
> we also need to make tb_phys_invalidate() handle them (by not trying
> to remove them from a nonexistent page list).
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  accel/tcg/translate-all.c | 19 ++++++++++++++++++-
>  1 file changed, 18 insertions(+), 1 deletion(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [Qemu-devel] [PATCH 5/6] accel/tcg: Return -1 for execution from MMIO regions in get_page_addr_code()

[Richard Henderson]

On 07/10/2018 09:00 AM, Peter Maydell wrote:
> Now that all the callers can handle get_page_addr_code() returning -1,
> remove all the code which tries to handle execution from MMIO regions
> or small-MMU-region RAM areas. This will mean that we can correctly
> execute from these areas, rather than ending up either aborting QEMU
> or delivering an incorrect guest exception.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  accel/tcg/cputlb.c | 95 +++++-----------------------------------------
>  1 file changed, 10 insertions(+), 85 deletions(-)

Yay!

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [Qemu-devel] [PATCH 6/6] target/arm: Allow execution from small regions

[Richard Henderson]

On 07/10/2018 09:00 AM, Peter Maydell wrote:
> Now that we have full support for small regions, including execution,
> we can remove the workarounds where we marked all small regions as
> non-executable for the M-profile MPU and SAU.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  target/arm/helper.c | 23 -----------------------
>  1 file changed, 23 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [Qemu-devel] [Qemu-arm] [PATCH 0/6] accel/tcg: Support execution from MMIO and small MMU regions

[Philippe Mathieu-Daudé]

[-- Attachment #1: Type: text/plain, Size: 4814 bytes --]

Hi Peter,

On 07/10/2018 01:00 PM, Peter Maydell wrote:
> This series adds support to TCG for executing from MMIO regions
> and small MMU regions. The basic principle is that if get_page_addr_code()
> finds that the region is not backed by a full page of RAM then it
> returns -1, and tb_gen_code() then generates a non-cached TB
> containing a single instruction. Execution from these regions
> thus performs the instruction fetch every time, ensuring that we
> get the read-from-MMIO and check-small-MMU-region permissions
> checks right.
> 
> This means that the code path for "generate bus fault for failing
> to load an instruction" no longer goes through get_page_addr_code(),
> but instead via each target's translate code and its calls to
> the cpu_ld*_code() or similar functions. Patch 1 makes sure we
> can distinguish insn fetches from data loads when generating the
> bus fault exceptions. (Aside: I have assumed that all cpu_ld*_code()
> loads should trigger iside faults rather than dside. Hopefully this
> is true...)
> 
> Patches 2 and 3 make trivial fixes to various callers of
> get_page_addr_code(); patch 4 does the work of generating our
> single-insn TBs. Patch 5 can then remove all the code that
> (mis)handles MMIO regions from get_page_addr_code(). Finally
> patch 6 drops the target/arm workarounds for not having support
> for executing from small MPU regions.
> 
> Note for the Xilinx folks: this patchset makes the mmio-exec
> testcase for running from the SPI flash pass. Cedric: you might
> like to test the aspeed image you had that relies on execution
> from an MMIO region too.

I applied and quickly tested your series on a MIPS SoC I'm working on
which has a tiny SRAM:

(qemu) info mtree
address-space: memory
  0000000000000000-ffffffffffffffff (prio 0, i/o): system
    0000000000000000-00000000000007ff (prio 0, ram): sram
    0000000010000000-00000000107fffff (prio 0, i/o): pflash
    0000000014000000-0000000014ffffff (prio 0, ram): dram
    000000001fc00000-000000001fc0ffff (prio 0, rom): srom

The firmware copies the ISR in this SRAM area, sadly it didn't work as
expected:

qemu-system-mips: Bad ram pointer 0x4a4
Aborted (core dumped)
(gdb) bt
#0  0x00007f5f34d84e7b in __GI_raise (sig=sig@entry=6) at
../sysdeps/unix/sysv/linux/raise.c:51
#1  0x00007f5f34d86231 in __GI_abort () at abort.c:79
#2  0x000055f4527a2074 in qemu_ram_addr_from_host_nofail (ptr=0x4a4) at
accel/tcg/cputlb.c:751
#3  0x000055f4527a2887 in get_page_addr_code (env=0x55f454d06728,
addr=2415932580) at accel/tcg/cputlb.c:966
#4  0x000055f4527bd206 in tb_htable_lookup (cpu=0x55f454cfe478,
pc=2415932580, cs_base=0, flags=268435472, cf_mask=0) at
accel/tcg/cpu-exec.c:334
#5  0x000055f4527b7b31 in tb_lookup__cpu_state (cpu=0x55f454cfe478,
pc=0x7f5f17cf7f98, cs_base=0x7f5f17cf7f9c, flags=0x7f5f17cf7f94,
cf_mask=0) at include/exec/tb-lookup.h:39
#6  0x000055f4527b7e29 in helper_lookup_tb_ptr (env=0x55f454d06728) at
accel/tcg/tcg-runtime.c:154
#7  0x00007f5f2494da2d in code_gen_buffer ()
#8  0x000055f4527bcd33 in cpu_tb_exec (cpu=0x55f454cfe478,
itb=0x7f5f2494d880 <code_gen_buffer+129107>) at accel/tcg/cpu-exec.c:171
#9  0x000055f4527bda6a in cpu_loop_exec_tb (cpu=0x55f454cfe478,
tb=0x7f5f2494d880 <code_gen_buffer+129107>, last_tb=0x7f5f17cf8538,
tb_exit=0x7f5f17cf8534) at accel/tcg/cpu-exec.c:615
#10 0x000055f4527bdd57 in cpu_exec (cpu=0x55f454cfe478) at
accel/tcg/cpu-exec.c:725
#11 0x000055f452770575 in tcg_cpu_exec (cpu=0x55f454cfe478) at cpus.c:1363
#12 0x000055f4527707cb in qemu_tcg_rr_cpu_thread_fn (arg=0x55f454cfe478)
at cpus.c:1463
#13 0x000055f452c58a4b in qemu_thread_start (args=0x55f454d2afc0) at
util/qemu-thread-posix.c:504
#14 0x00007f5f351115aa in start_thread (arg=0x7f5f17cfb700) at
pthread_create.c:463
#15 0x00007f5f34e46cbf in clone () at
../sysdeps/unix/sysv/linux/x86_64/clone.S:95

This firmware works using the "avoid subpages" trick, allocating 4K for
the SRAM (TARGET_PAGE_SIZE).

I didn't look further, maybe I need to figure out how to add a
"target/mips: Allow execution from small regions" too.

Regards,

Phil.

> The diffstat is pretty satisfying for a patchset that adds
> a feature, but it actually undersells it: this code renders the
> hw/misc/mmio_interface.c and the mmio_ptr related code in memory.c
> and the xilinx-spips device all obsolete, so there are another
> couple of hundred lines of code to be deleted there. I opted not
> to include that in this patchset, for ease of review.
> 
> NB: I tested this with icount, but there are potentially
> some weird things that could happen with interactions between
> icount's io-recompile and execution from an MMIO device
> that returns different instructions each time it's read.


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Qemu-devel] [Qemu-arm] [PATCH 1/6] accel/tcg: Pass read access type through to io_readx()

[Philippe Mathieu-Daudé]

On 07/10/2018 01:00 PM, Peter Maydell wrote:
> The io_readx() function needs to know whether the load it is
> doing is an MMU_DATA_LOAD or an MMU_INST_FETCH, so that it
> can pass the right value to the cpu_transaction_failed()
> function. Plumb this information through from the softmmu
> code.
> 
> This is currently not often going to give the wrong answer,
> because usually instruction fetches go via get_page_addr_code().
> However once we switch over to handling execution from non-RAM by
> creating single-insn TBs, the path for an insn fetch to generate
> a bus error will be through cpu_ld*_code() and io_readx(),
> so without this change we will generate a d-side fault when we
> should generate an i-side fault.
> 
> We also have to pass the access type via a CPU struct global
> down to unassigned_mem_read(), for the benefit of the targets
> which still use the cpu_unassigned_access() hook (m68k, mips,
> sparc, xtensa).
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> ---
>  accel/tcg/softmmu_template.h | 11 +++++++----
>  include/qom/cpu.h            |  6 ++++++
>  accel/tcg/cputlb.c           |  5 +++--
>  memory.c                     |  3 ++-
>  4 files changed, 18 insertions(+), 7 deletions(-)
> 
> diff --git a/accel/tcg/softmmu_template.h b/accel/tcg/softmmu_template.h
> index badbf148803..f060a693d41 100644
> --- a/accel/tcg/softmmu_template.h
> +++ b/accel/tcg/softmmu_template.h
> @@ -99,11 +99,12 @@ static inline DATA_TYPE glue(io_read, SUFFIX)(CPUArchState *env,
>                                                size_t mmu_idx, size_t index,
>                                                target_ulong addr,
>                                                uintptr_t retaddr,
> -                                              bool recheck)
> +                                              bool recheck,
> +                                              MMUAccessType access_type)
>  {
>      CPUIOTLBEntry *iotlbentry = &env->iotlb[mmu_idx][index];
>      return io_readx(env, iotlbentry, mmu_idx, addr, retaddr, recheck,
> -                    DATA_SIZE);
> +                    access_type, DATA_SIZE);
>  }
>  #endif
>  
> @@ -140,7 +141,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,
> -                                    tlb_addr & TLB_RECHECK);
> +                                    tlb_addr & TLB_RECHECK,
> +                                    READ_ACCESS_TYPE);
>          res = TGT_LE(res);
>          return res;
>      }
> @@ -207,7 +209,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,
> -                                    tlb_addr & TLB_RECHECK);
> +                                    tlb_addr & TLB_RECHECK,
> +                                    READ_ACCESS_TYPE);
>          res = TGT_BE(res);
>          return res;
>      }
> diff --git a/include/qom/cpu.h b/include/qom/cpu.h
> index bd796579ee4..ecf6ed556a9 100644
> --- a/include/qom/cpu.h
> +++ b/include/qom/cpu.h
> @@ -386,6 +386,12 @@ struct CPUState {
>       */
>      uintptr_t mem_io_pc;
>      vaddr mem_io_vaddr;
> +    /*
> +     * This is only needed for the legacy cpu_unassigned_access() hook;
> +     * when all targets using it have been converted to use
> +     * cpu_transaction_failed() instead it can be removed.
> +     */
> +    MMUAccessType mem_io_access_type;
>  
>      int kvm_fd;
>      struct KVMState *kvm_state;
> diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
> index 20c147d6554..c491703f15f 100644
> --- a/accel/tcg/cputlb.c
> +++ b/accel/tcg/cputlb.c
> @@ -789,7 +789,7 @@ 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,
> -                         bool recheck, int size)
> +                         bool recheck, MMUAccessType access_type, int size)
>  {
>      CPUState *cpu = ENV_GET_CPU(env);
>      hwaddr mr_offset;
> @@ -831,6 +831,7 @@ static uint64_t io_readx(CPUArchState *env, CPUIOTLBEntry *iotlbentry,
>      }
>  
>      cpu->mem_io_vaddr = addr;
> +    cpu->mem_io_access_type = access_type;
>  
>      if (mr->global_locking && !qemu_mutex_iothread_locked()) {
>          qemu_mutex_lock_iothread();
> @@ -843,7 +844,7 @@ static uint64_t io_readx(CPUArchState *env, CPUIOTLBEntry *iotlbentry,
>              section->offset_within_address_space -
>              section->offset_within_region;
>  
> -        cpu_transaction_failed(cpu, physaddr, addr, size, MMU_DATA_LOAD,
> +        cpu_transaction_failed(cpu, physaddr, addr, size, access_type,
>                                 mmu_idx, iotlbentry->attrs, r, retaddr);
>      }
>      if (locked) {
> diff --git a/memory.c b/memory.c
> index e9cd4469688..2ea16e7bfb0 100644
> --- a/memory.c
> +++ b/memory.c
> @@ -1249,7 +1249,8 @@ static uint64_t unassigned_mem_read(void *opaque, hwaddr addr,
>      printf("Unassigned mem read " TARGET_FMT_plx "\n", addr);
>  #endif
>      if (current_cpu != NULL) {
> -        cpu_unassigned_access(current_cpu, addr, false, false, 0, size);
> +        bool is_exec = current_cpu->mem_io_access_type == MMU_INST_FETCH;
> +        cpu_unassigned_access(current_cpu, addr, false, is_exec, 0, size);
>      }
>      return 0;
>  }
> 

Re: [Qemu-devel] [Qemu-arm] [PATCH 5/6] accel/tcg: Return -1 for execution from MMIO regions in get_page_addr_code()

[Philippe Mathieu-Daudé]

On 07/10/2018 01:00 PM, Peter Maydell wrote:
> Now that all the callers can handle get_page_addr_code() returning -1,
> remove all the code which tries to handle execution from MMIO regions
> or small-MMU-region RAM areas. This will mean that we can correctly
> execute from these areas, rather than ending up either aborting QEMU
> or delivering an incorrect guest exception.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Tested-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> ---
>  accel/tcg/cputlb.c | 95 +++++-----------------------------------------
>  1 file changed, 10 insertions(+), 85 deletions(-)
> 
> diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
> index c491703f15f..abb0225dc79 100644
> --- a/accel/tcg/cputlb.c
> +++ b/accel/tcg/cputlb.c
> @@ -741,39 +741,6 @@ void tlb_set_page(CPUState *cpu, target_ulong vaddr,
>                              prot, mmu_idx, size);
>  }
>  
> -static void report_bad_exec(CPUState *cpu, target_ulong addr)
> -{
> -    /* Accidentally executing outside RAM or ROM is quite common for
> -     * several user-error situations, so report it in a way that
> -     * makes it clear that this isn't a QEMU bug and provide suggestions
> -     * about what a user could do to fix things.
> -     */
> -    error_report("Trying to execute code outside RAM or ROM at 0x"
> -                 TARGET_FMT_lx, addr);
> -    error_printf("This usually means one of the following happened:\n\n"
> -                 "(1) You told QEMU to execute a kernel for the wrong machine "
> -                 "type, and it crashed on startup (eg trying to run a "
> -                 "raspberry pi kernel on a versatilepb QEMU machine)\n"
> -                 "(2) You didn't give QEMU a kernel or BIOS filename at all, "
> -                 "and QEMU executed a ROM full of no-op instructions until "
> -                 "it fell off the end\n"
> -                 "(3) Your guest kernel has a bug and crashed by jumping "
> -                 "off into nowhere\n\n"
> -                 "This is almost always one of the first two, so check your "
> -                 "command line and that you are using the right type of kernel "
> -                 "for this machine.\n"
> -                 "If you think option (3) is likely then you can try debugging "
> -                 "your guest with the -d debug options; in particular "
> -                 "-d guest_errors will cause the log to include a dump of the "
> -                 "guest register state at this point.\n\n"
> -                 "Execution cannot continue; stopping here.\n\n");
> -
> -    /* Report also to the logs, with more detail including register dump */
> -    qemu_log_mask(LOG_GUEST_ERROR, "qemu: fatal: Trying to execute code "
> -                  "outside RAM or ROM at 0x" TARGET_FMT_lx "\n", addr);
> -    log_cpu_state_mask(LOG_GUEST_ERROR, cpu, CPU_DUMP_FPU | CPU_DUMP_CCOP);
> -}
> -
>  static inline ram_addr_t qemu_ram_addr_from_host_nofail(void *ptr)
>  {
>      ram_addr_t ram_addr;
> @@ -963,7 +930,6 @@ tb_page_addr_t get_page_addr_code(CPUArchState *env, target_ulong addr)
>      MemoryRegionSection *section;
>      CPUState *cpu = ENV_GET_CPU(env);
>      CPUIOTLBEntry *iotlbentry;
> -    hwaddr physaddr, mr_offset;
>  
>      index = (addr >> TARGET_PAGE_BITS) & (CPU_TLB_SIZE - 1);
>      mmu_idx = cpu_mmu_index(env, true);
> @@ -977,65 +943,24 @@ tb_page_addr_t get_page_addr_code(CPUArchState *env, target_ulong addr)
>                    (TLB_RECHECK | TLB_INVALID_MASK)) == 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)
> +         * covers a smaller range than a target page. Return -1 to
> +         * indicate that we cannot simply execute from RAM here;
> +         * we will perform the necessary repeat of the MMU check
> +         * when the "execute a single insn" code performs the
> +         * load of the guest insn.
>           */
> +        return -1;
>      }
>  
>      iotlbentry = &env->iotlb[mmu_idx][index];
>      section = iotlb_to_section(cpu, iotlbentry->addr, iotlbentry->attrs);
>      mr = section->mr;
>      if (memory_region_is_unassigned(mr)) {
> -        qemu_mutex_lock_iothread();
> -        if (memory_region_request_mmio_ptr(mr, addr)) {
> -            qemu_mutex_unlock_iothread();
> -            /* A MemoryRegion is potentially added so re-run the
> -             * get_page_addr_code.
> -             */
> -            return get_page_addr_code(env, addr);
> -        }
> -        qemu_mutex_unlock_iothread();
> -
> -        /* Give the new-style cpu_transaction_failed() hook first chance
> -         * to handle this.
> -         * This is not the ideal place to detect and generate CPU
> -         * exceptions for instruction fetch failure (for instance
> -         * we don't know the length of the access that the CPU would
> -         * use, and it would be better to go ahead and try the access
> -         * and use the MemTXResult it produced). However it is the
> -         * simplest place we have currently available for the check.
> +        /*
> +         * Not guest RAM, so there is no ram_addr_t for it. Return -1,
> +         * and we will execute a single insn from this device.
>           */
> -        mr_offset = (iotlbentry->addr & TARGET_PAGE_MASK) + addr;
> -        physaddr = mr_offset +
> -            section->offset_within_address_space -
> -            section->offset_within_region;
> -        cpu_transaction_failed(cpu, physaddr, addr, 0, MMU_INST_FETCH, mmu_idx,
> -                               iotlbentry->attrs, MEMTX_DECODE_ERROR, 0);
> -
> -        cpu_unassigned_access(cpu, addr, false, true, 0, 4);
> -        /* The CPU's unassigned access hook might have longjumped out
> -         * with an exception. If it didn't (or there was no hook) then
> -         * we can't proceed further.
> -         */
> -        report_bad_exec(cpu, addr);
> -        exit(1);
> +        return -1;
>      }
>      p = (void *)((uintptr_t)addr + env->tlb_table[mmu_idx][index].addend);
>      return qemu_ram_addr_from_host_nofail(p);
> 

Re: [Qemu-devel] [Qemu-arm] [PATCH 6/6] target/arm: Allow execution from small regions

[Philippe Mathieu-Daudé]

On 07/10/2018 01:00 PM, Peter Maydell wrote:
> Now that we have full support for small regions, including execution,
> we can remove the workarounds where we marked all small regions as
> non-executable for the M-profile MPU and SAU.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> ---
>  target/arm/helper.c | 23 -----------------------
>  1 file changed, 23 deletions(-)
> 
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index a2ac96084e7..ed96e6c02fb 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -9784,17 +9784,6 @@ static bool get_phys_addr_pmsav7(CPUARMState *env, uint32_t address,
>  
>      fi->type = ARMFault_Permission;
>      fi->level = 1;
> -    /*
> -     * Core QEMU code can't handle execution from small pages yet, so
> -     * don't try it. This way we'll get an MPU exception, rather than
> -     * eventually causing QEMU to exit in get_page_addr_code().
> -     */
> -    if (*page_size < TARGET_PAGE_SIZE && (*prot & PAGE_EXEC)) {
> -        qemu_log_mask(LOG_UNIMP,
> -                      "MPU: No support for execution from regions "
> -                      "smaller than 1K\n");
> -        *prot &= ~PAGE_EXEC;
> -    }
>      return !(*prot & (1 << access_type));
>  }
>  
> @@ -10014,18 +10003,6 @@ static bool pmsav8_mpu_lookup(CPUARMState *env, uint32_t address,
>  
>      fi->type = ARMFault_Permission;
>      fi->level = 1;
> -    /*
> -     * Core QEMU code can't handle execution from small pages yet, so
> -     * don't try it. This means any attempted execution will generate
> -     * an MPU exception, rather than eventually causing QEMU to exit in
> -     * get_page_addr_code().
> -     */
> -    if (*is_subpage && (*prot & PAGE_EXEC)) {
> -        qemu_log_mask(LOG_UNIMP,
> -                      "MPU: No support for execution from regions "
> -                      "smaller than 1K\n");
> -        *prot &= ~PAGE_EXEC;
> -    }
>      return !(*prot & (1 << access_type));
>  }
>  
> 

Re: [Qemu-devel] [Qemu-arm] [PATCH 0/6] accel/tcg: Support execution from MMIO and small MMU regions

[Peter Maydell]

On 11 July 2018 at 05:21, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> Hi Peter,
>
> On 07/10/2018 01:00 PM, Peter Maydell wrote:
>> This series adds support to TCG for executing from MMIO regions
>> and small MMU regions. The basic principle is that if get_page_addr_code()
>> finds that the region is not backed by a full page of RAM then it
>> returns -1, and tb_gen_code() then generates a non-cached TB
>> containing a single instruction. Execution from these regions
>> thus performs the instruction fetch every time, ensuring that we
>> get the read-from-MMIO and check-small-MMU-region permissions
>> checks right.
>>
>> This means that the code path for "generate bus fault for failing
>> to load an instruction" no longer goes through get_page_addr_code(),
>> but instead via each target's translate code and its calls to
>> the cpu_ld*_code() or similar functions. Patch 1 makes sure we
>> can distinguish insn fetches from data loads when generating the
>> bus fault exceptions. (Aside: I have assumed that all cpu_ld*_code()
>> loads should trigger iside faults rather than dside. Hopefully this
>> is true...)
>>
>> Patches 2 and 3 make trivial fixes to various callers of
>> get_page_addr_code(); patch 4 does the work of generating our
>> single-insn TBs. Patch 5 can then remove all the code that
>> (mis)handles MMIO regions from get_page_addr_code(). Finally
>> patch 6 drops the target/arm workarounds for not having support
>> for executing from small MPU regions.
>>
>> Note for the Xilinx folks: this patchset makes the mmio-exec
>> testcase for running from the SPI flash pass. Cedric: you might
>> like to test the aspeed image you had that relies on execution
>> from an MMIO region too.
>
> I applied and quickly tested your series on a MIPS SoC I'm working on
> which has a tiny SRAM:
>
> (qemu) info mtree
> address-space: memory
>   0000000000000000-ffffffffffffffff (prio 0, i/o): system
>     0000000000000000-00000000000007ff (prio 0, ram): sram
>     0000000010000000-00000000107fffff (prio 0, i/o): pflash
>     0000000014000000-0000000014ffffff (prio 0, ram): dram
>     000000001fc00000-000000001fc0ffff (prio 0, rom): srom
>
> The firmware copies the ISR in this SRAM area, sadly it didn't work as
> expected:
>
> qemu-system-mips: Bad ram pointer 0x4a4
> Aborted (core dumped)
> (gdb) bt
> #0  0x00007f5f34d84e7b in __GI_raise (sig=sig@entry=6) at
> ../sysdeps/unix/sysv/linux/raise.c:51
> #1  0x00007f5f34d86231 in __GI_abort () at abort.c:79
> #2  0x000055f4527a2074 in qemu_ram_addr_from_host_nofail (ptr=0x4a4) at
> accel/tcg/cputlb.c:751
> #3  0x000055f4527a2887 in get_page_addr_code (env=0x55f454d06728,
> addr=2415932580) at accel/tcg/cputlb.c:966
> #4  0x000055f4527bd206 in tb_htable_lookup (cpu=0x55f454cfe478,
> pc=2415932580, cs_base=0, flags=268435472, cf_mask=0) at
> accel/tcg/cpu-exec.c:334
> #5  0x000055f4527b7b31 in tb_lookup__cpu_state (cpu=0x55f454cfe478,
> pc=0x7f5f17cf7f98, cs_base=0x7f5f17cf7f9c, flags=0x7f5f17cf7f94,
> cf_mask=0) at include/exec/tb-lookup.h:39
> #6  0x000055f4527b7e29 in helper_lookup_tb_ptr (env=0x55f454d06728) at
> accel/tcg/tcg-runtime.c:154
> #7  0x00007f5f2494da2d in code_gen_buffer ()
> #8  0x000055f4527bcd33 in cpu_tb_exec (cpu=0x55f454cfe478,
> itb=0x7f5f2494d880 <code_gen_buffer+129107>) at accel/tcg/cpu-exec.c:171
> #9  0x000055f4527bda6a in cpu_loop_exec_tb (cpu=0x55f454cfe478,
> tb=0x7f5f2494d880 <code_gen_buffer+129107>, last_tb=0x7f5f17cf8538,
> tb_exit=0x7f5f17cf8534) at accel/tcg/cpu-exec.c:615
> #10 0x000055f4527bdd57 in cpu_exec (cpu=0x55f454cfe478) at
> accel/tcg/cpu-exec.c:725
> #11 0x000055f452770575 in tcg_cpu_exec (cpu=0x55f454cfe478) at cpus.c:1363
> #12 0x000055f4527707cb in qemu_tcg_rr_cpu_thread_fn (arg=0x55f454cfe478)
> at cpus.c:1463
> #13 0x000055f452c58a4b in qemu_thread_start (args=0x55f454d2afc0) at
> util/qemu-thread-posix.c:504
> #14 0x00007f5f351115aa in start_thread (arg=0x7f5f17cfb700) at
> pthread_create.c:463
> #15 0x00007f5f34e46cbf in clone () at
> ../sysdeps/unix/sysv/linux/x86_64/clone.S:95
>
> This firmware works using the "avoid subpages" trick, allocating 4K for
> the SRAM (TARGET_PAGE_SIZE).

Are you sure this is executing from the SRAM at this point?
The PC value in the backtrace is 2415932580 == 900034A4. I don't
know how the MMU is configured at this point, but my guess is that
that's actually in the pflash area, and you're running into a
different bug:
https://lists.nongnu.org/archive/html/qemu-devel/2017-01/msg00674.html
where when we attempt to execute from a romd device that's in romd
mode we incorrectly think it's RAM in get_page_addr_code() and crash.

Previously my view on that had been "yes, it's a bug, but if we
fixed it the only change would be that we'd fall over with
'can't execute from a device' rather than crashing". However
now we can execute from devices fixing the bug is potentially
more useful...

thanks
-- PMM

Re: [Qemu-devel] [Qemu-arm] [PATCH 0/6] accel/tcg: Support execution from MMIO and small MMU regions

[Peter Maydell]

On 12 July 2018 at 17:37, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 11 July 2018 at 05:21, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>> I applied and quickly tested your series on a MIPS SoC I'm working on
>> which has a tiny SRAM:
>>
>> (qemu) info mtree
>> address-space: memory
>>   0000000000000000-ffffffffffffffff (prio 0, i/o): system
>>     0000000000000000-00000000000007ff (prio 0, ram): sram
>>     0000000010000000-00000000107fffff (prio 0, i/o): pflash
>>     0000000014000000-0000000014ffffff (prio 0, ram): dram
>>     000000001fc00000-000000001fc0ffff (prio 0, rom): srom
>>
>> The firmware copies the ISR in this SRAM area, sadly it didn't work as
>> expected:
>>
>> qemu-system-mips: Bad ram pointer 0x4a4

> Are you sure this is executing from the SRAM at this point?
> The PC value in the backtrace is 2415932580 == 900034A4. I don't
> know how the MMU is configured at this point, but my guess is that
> that's actually in the pflash area, and you're running into a
> different bug:
> https://lists.nongnu.org/archive/html/qemu-devel/2017-01/msg00674.html
> where when we attempt to execute from a romd device that's in romd
> mode we incorrectly think it's RAM in get_page_addr_code() and crash.

I just sent a follow-on patch which should fix the 'bad ram pointer'
assert. If your guest really is trying to execute from the pflash
here then it will just misbehave in a different way, though,
since pflash status response bytes are probably not valid MIPS
instructions...

thanks
-- PMM

Re: [Qemu-devel] [PATCH 4/6] accel/tcg: tb_gen_code(): Create single-insn TB for execution from non-RAM

[Emilio G. Cota]

On Tue, Jul 10, 2018 at 17:00:11 +0100, Peter Maydell wrote:
> If get_page_addr_code() returns -1, this indicates that there is no RAM
> page we can read a full TB from. Instead we must create a TB which
> contains a single instruction and which we do not cache, so it is
> executed only once.
> 
> Since this means we can now have TBs which are not in any page list,
> we also need to make tb_phys_invalidate() handle them (by not trying
> to remove them from a nonexistent page list).
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Emilio G. Cota <cota@braap.org>

		Emilio

Re: [Qemu-devel] [PATCH 2/6] accel/tcg: Handle get_page_addr_code() returning -1 in hashtable lookups

[Emilio G. Cota]

On Tue, Jul 10, 2018 at 17:00:09 +0100, Peter Maydell wrote:
> When we support execution from non-RAM MMIO regions, get_page_addr_code()
> will return -1 to indicate that there is no RAM at the requested address.
> Handle this in the cpu-exec TB hashtable lookup code, treating it as
> "no match found".
> 
> Note that the call to get_page_addr_code() in tb_lookup_cmp() needs
> no changes -- a return of -1 will already correctly result in the
> function returning false.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Emilio G. Cota <cota@braap.org>

		E.

Re: [Qemu-devel] [PATCH 0/6] accel/tcg: Support execution from MMIO and small MMU regions

[KONRAD Frederic]

Hi Peter,

Nice! Thanks for that.

A little question though.. What will happen in the case where the
CPU start executing code at random place because of eg: a badly
configured kernel?

Seeing the patch 5 I guess it will really execute stuff.. So
maybe this is less user-friendly?

Cheers,
Fred

On 07/10/2018 06:00 PM, Peter Maydell wrote:
> This series adds support to TCG for executing from MMIO regions
> and small MMU regions. The basic principle is that if get_page_addr_code()
> finds that the region is not backed by a full page of RAM then it
> returns -1, and tb_gen_code() then generates a non-cached TB
> containing a single instruction. Execution from these regions
> thus performs the instruction fetch every time, ensuring that we
> get the read-from-MMIO and check-small-MMU-region permissions
> checks right.
> 
> This means that the code path for "generate bus fault for failing
> to load an instruction" no longer goes through get_page_addr_code(),
> but instead via each target's translate code and its calls to
> the cpu_ld*_code() or similar functions. Patch 1 makes sure we
> can distinguish insn fetches from data loads when generating the
> bus fault exceptions. (Aside: I have assumed that all cpu_ld*_code()
> loads should trigger iside faults rather than dside. Hopefully this
> is true...)
> 
> Patches 2 and 3 make trivial fixes to various callers of
> get_page_addr_code(); patch 4 does the work of generating our
> single-insn TBs. Patch 5 can then remove all the code that
> (mis)handles MMIO regions from get_page_addr_code(). Finally
> patch 6 drops the target/arm workarounds for not having support
> for executing from small MPU regions.
> 
> Note for the Xilinx folks: this patchset makes the mmio-exec
> testcase for running from the SPI flash pass. Cedric: you might
> like to test the aspeed image you had that relies on execution
> from an MMIO region too.
> 
> The diffstat is pretty satisfying for a patchset that adds
> a feature, but it actually undersells it: this code renders the
> hw/misc/mmio_interface.c and the mmio_ptr related code in memory.c
> and the xilinx-spips device all obsolete, so there are another
> couple of hundred lines of code to be deleted there. I opted not
> to include that in this patchset, for ease of review.
> 
> NB: I tested this with icount, but there are potentially
> some weird things that could happen with interactions between
> icount's io-recompile and execution from an MMIO device
> that returns different instructions each time it's read.
> 
> thanks
> -- PMM
> 
> 
> Peter Maydell (6):
>    accel/tcg: Pass read access type through to io_readx()
>    accel/tcg: Handle get_page_addr_code() returning -1 in hashtable
>      lookups
>    accel/tcg: Handle get_page_addr_code() returning -1 in
>      tb_check_watchpoint()
>    accel/tcg: tb_gen_code(): Create single-insn TB for execution from
>      non-RAM
>    accel/tcg: Return -1 for execution from MMIO regions in
>      get_page_addr_code()
>    target/arm: Allow execution from small regions
> 
>   accel/tcg/softmmu_template.h |  11 ++--
>   include/qom/cpu.h            |   6 +++
>   accel/tcg/cpu-exec.c         |   3 ++
>   accel/tcg/cputlb.c           | 100 +++++------------------------------
>   accel/tcg/translate-all.c    |  23 +++++++-
>   memory.c                     |   3 +-
>   target/arm/helper.c          |  23 --------
>   7 files changed, 52 insertions(+), 117 deletions(-)
> 

Re: [Qemu-devel] [PATCH 0/6] accel/tcg: Support execution from MMIO and small MMU regions

[Peter Maydell]

On 16 July 2018 at 13:30, KONRAD Frederic <frederic.konrad@adacore.com> wrote:
> Hi Peter,
>
> Nice! Thanks for that.
>
> A little question though.. What will happen in the case where the
> CPU start executing code at random place because of eg: a badly
> configured kernel?
>
> Seeing the patch 5 I guess it will really execute stuff.. So
> maybe this is less user-friendly?

Yes, it's true that we will now happily execute from anything
(device, unassigned memory, etc), and do what the real hardware
would do in that situation (random unhelpful things, infinite
loop of taking exceptions). That's a bit unavoidable though I
think, and there are already lots of cases where QEMU will just
sit there with a black screen because the user has loaded in
a bad guest image that goes off into the weeds without printing
anything to a UART.

It's possible we could devise a user-friendliness option that
tried to pick up symptoms of guests being stuck (eg tracking
whether we're continuously taking exceptions) but that gets
into heuristics a bit.

thanks
-- PMM

Re: [Qemu-devel] [PATCH 0/6] accel/tcg: Support execution from MMIO and small MMU regions

[Cédric Le Goater]

On 07/10/2018 06:00 PM, Peter Maydell wrote:
> This series adds support to TCG for executing from MMIO regions
> and small MMU regions. The basic principle is that if get_page_addr_code()
> finds that the region is not backed by a full page of RAM then it
> returns -1, and tb_gen_code() then generates a non-cached TB
> containing a single instruction. Execution from these regions
> thus performs the instruction fetch every time, ensuring that we
> get the read-from-MMIO and check-small-MMU-region permissions
> checks right.
> 
> This means that the code path for "generate bus fault for failing
> to load an instruction" no longer goes through get_page_addr_code(),
> but instead via each target's translate code and its calls to
> the cpu_ld*_code() or similar functions. Patch 1 makes sure we
> can distinguish insn fetches from data loads when generating the
> bus fault exceptions. (Aside: I have assumed that all cpu_ld*_code()
> loads should trigger iside faults rather than dside. Hopefully this
> is true...)
> 
> Patches 2 and 3 make trivial fixes to various callers of
> get_page_addr_code(); patch 4 does the work of generating our
> single-insn TBs. Patch 5 can then remove all the code that
> (mis)handles MMIO regions from get_page_addr_code(). Finally
> patch 6 drops the target/arm workarounds for not having support
> for executing from small MPU regions.
> 
> Note for the Xilinx folks: this patchset makes the mmio-exec
> testcase for running from the SPI flash pass. Cedric: you might
> like to test the aspeed image you had that relies on execution
> from an MMIO region too.

I have added a memory region alias at 0x0 on the memory region where
the first flash device is mapped and all aspeed machines, palmetto, 
romulus, witherspoon booted fine. 

More or less 4MB of data access is generated and the slowdown is hardly 
noticeable, around one second on a laptop.

I wonder if I should add a bool option to the machine to activate
or not the feature ?

Thanks,

C. 

> The diffstat is pretty satisfying for a patchset that adds
> a feature, but it actually undersells it: this code renders the
> hw/misc/mmio_interface.c and the mmio_ptr related code in memory.c
> and the xilinx-spips device all obsolete, so there are another
> couple of hundred lines of code to be deleted there. I opted not
> to include that in this patchset, for ease of review.
> 
> NB: I tested this with icount, but there are potentially
> some weird things that could happen with interactions between
> icount's io-recompile and execution from an MMIO device
> that returns different instructions each time it's read.
> 
> thanks
> -- PMM
> 
> 
> Peter Maydell (6):
>   accel/tcg: Pass read access type through to io_readx()
>   accel/tcg: Handle get_page_addr_code() returning -1 in hashtable
>     lookups
>   accel/tcg: Handle get_page_addr_code() returning -1 in
>     tb_check_watchpoint()
>   accel/tcg: tb_gen_code(): Create single-insn TB for execution from
>     non-RAM
>   accel/tcg: Return -1 for execution from MMIO regions in
>     get_page_addr_code()
>   target/arm: Allow execution from small regions
> 
>  accel/tcg/softmmu_template.h |  11 ++--
>  include/qom/cpu.h            |   6 +++
>  accel/tcg/cpu-exec.c         |   3 ++
>  accel/tcg/cputlb.c           | 100 +++++------------------------------
>  accel/tcg/translate-all.c    |  23 +++++++-
>  memory.c                     |   3 +-
>  target/arm/helper.c          |  23 --------
>  7 files changed, 52 insertions(+), 117 deletions(-)
> 

Re: [Qemu-devel] [PATCH 0/6] accel/tcg: Support execution from MMIO and small MMU regions

[Cédric Le Goater]

On 07/10/2018 06:00 PM, Peter Maydell wrote:
> This series adds support to TCG for executing from MMIO regions
> and small MMU regions. The basic principle is that if get_page_addr_code()
> finds that the region is not backed by a full page of RAM then it
> returns -1, and tb_gen_code() then generates a non-cached TB
> containing a single instruction. Execution from these regions
> thus performs the instruction fetch every time, ensuring that we
> get the read-from-MMIO and check-small-MMU-region permissions
> checks right.
> 
> This means that the code path for "generate bus fault for failing
> to load an instruction" no longer goes through get_page_addr_code(),
> but instead via each target's translate code and its calls to
> the cpu_ld*_code() or similar functions. Patch 1 makes sure we
> can distinguish insn fetches from data loads when generating the
> bus fault exceptions. (Aside: I have assumed that all cpu_ld*_code()
> loads should trigger iside faults rather than dside. Hopefully this
> is true...)
> 
> Patches 2 and 3 make trivial fixes to various callers of
> get_page_addr_code(); patch 4 does the work of generating our
> single-insn TBs. Patch 5 can then remove all the code that
> (mis)handles MMIO regions from get_page_addr_code(). Finally
> patch 6 drops the target/arm workarounds for not having support
> for executing from small MPU regions.
> 
> Note for the Xilinx folks: this patchset makes the mmio-exec
> testcase for running from the SPI flash pass. Cedric: you might
> like to test the aspeed image you had that relies on execution
> from an MMIO region too.


For the series,

Tested-by: Cédric Le Goater <clg@kaod.org>

Thanks,

C.

> 
> The diffstat is pretty satisfying for a patchset that adds
> a feature, but it actually undersells it: this code renders the
> hw/misc/mmio_interface.c and the mmio_ptr related code in memory.c
> and the xilinx-spips device all obsolete, so there are another
> couple of hundred lines of code to be deleted there. I opted not
> to include that in this patchset, for ease of review.
> 
> NB: I tested this with icount, but there are potentially
> some weird things that could happen with interactions between
> icount's io-recompile and execution from an MMIO device
> that returns different instructions each time it's read.
> 
> thanks
> -- PMM
> 
> 
> Peter Maydell (6):
>   accel/tcg: Pass read access type through to io_readx()
>   accel/tcg: Handle get_page_addr_code() returning -1 in hashtable
>     lookups
>   accel/tcg: Handle get_page_addr_code() returning -1 in
>     tb_check_watchpoint()
>   accel/tcg: tb_gen_code(): Create single-insn TB for execution from
>     non-RAM
>   accel/tcg: Return -1 for execution from MMIO regions in
>     get_page_addr_code()
>   target/arm: Allow execution from small regions
> 
>  accel/tcg/softmmu_template.h |  11 ++--
>  include/qom/cpu.h            |   6 +++
>  accel/tcg/cpu-exec.c         |   3 ++
>  accel/tcg/cputlb.c           | 100 +++++------------------------------
>  accel/tcg/translate-all.c    |  23 +++++++-
>  memory.c                     |   3 +-
>  target/arm/helper.c          |  23 --------
>  7 files changed, 52 insertions(+), 117 deletions(-)
> 

Re: [Qemu-devel] [PATCH 0/6] accel/tcg: Support execution from MMIO and small MMU regions

[Peter Maydell]

On 23 July 2018 at 15:57, Cédric Le Goater <clg@kaod.org> wrote:
> On 07/10/2018 06:00 PM, Peter Maydell wrote:
>> Note for the Xilinx folks: this patchset makes the mmio-exec
>> testcase for running from the SPI flash pass. Cedric: you might
>> like to test the aspeed image you had that relies on execution
>> from an MMIO region too.
>
> I have added a memory region alias at 0x0 on the memory region where
> the first flash device is mapped and all aspeed machines, palmetto,
> romulus, witherspoon booted fine.
>
> More or less 4MB of data access is generated and the slowdown is hardly
> noticeable, around one second on a laptop.
>
> I wonder if I should add a bool option to the machine to activate
> or not the feature ?

I would tend to go with "not" -- if we need the feature then
it's better to run slowly but correctly, and if we don't need
the feature then it doesn't cost us any speed I think.

thanks
-- PMM

Re: [Qemu-devel] [PATCH 0/6] accel/tcg: Support execution from MMIO and small MMU regions

[Cédric Le Goater]

On 07/23/2018 05:17 PM, Peter Maydell wrote:
> On 23 July 2018 at 15:57, Cédric Le Goater <clg@kaod.org> wrote:
>> On 07/10/2018 06:00 PM, Peter Maydell wrote:
>>> Note for the Xilinx folks: this patchset makes the mmio-exec
>>> testcase for running from the SPI flash pass. Cedric: you might
>>> like to test the aspeed image you had that relies on execution
>>> from an MMIO region too.
>>
>> I have added a memory region alias at 0x0 on the memory region where
>> the first flash device is mapped and all aspeed machines, palmetto,
>> romulus, witherspoon booted fine.
>>
>> More or less 4MB of data access is generated and the slowdown is hardly
>> noticeable, around one second on a laptop.
>>
>> I wonder if I should add a bool option to the machine to activate
>> or not the feature ?
> 
> I would tend to go with "not" -- if we need the feature then
> it's better to run slowly but correctly, and if we don't need
> the feature then it doesn't cost us any speed I think.


I have chosen a brutal activation (for the moment) :

  https://github.com/legoater/qemu/commit/10b08c5e8df385bba342f02fa0440916203b6fa8

I will send when your patchset is committed.

Images used for tests are here : 

  https://openpower.xyz/job/openbmc-build/distro=ubuntu,target=romulus/lastSuccessfulBuild/artifact/deploy/images/romulus/flash-romulus

  https://openpower.xyz/job/openbmc-build/distro=ubuntu,target=palmetto/lastSuccessfulBuild/artifact/deploy/images/palmetto/flash-palmetto

  https://openpower.xyz/job/openbmc-build/distro=ubuntu,target=witherspoon/lastSuccessfulBuild/artifact/deploy/images/witherspoon/obmc-phosphor-image-witherspoon.ubi.mtd

Thanks,

C. 

Re: [Qemu-devel] [Qemu-arm] [PATCH 0/6] accel/tcg: Support execution from MMIO and small MMU regions

[Peter Maydell]

On 10 July 2018 at 17:00, Peter Maydell <peter.maydell@linaro.org> wrote:
> This series adds support to TCG for executing from MMIO regions
> and small MMU regions. The basic principle is that if get_page_addr_code()
> finds that the region is not backed by a full page of RAM then it
> returns -1, and tb_gen_code() then generates a non-cached TB
> containing a single instruction. Execution from these regions
> thus performs the instruction fetch every time, ensuring that we
> get the read-from-MMIO and check-small-MMU-region permissions
> checks right.

Hi; this series has been reviewed, and I propose to put it into
my target-arm.for-3.1 queue, unless anybody has a preference for
it going into master via some other route.

thanks
-- PMM

Re: [Qemu-devel] [PATCH 5/6] accel/tcg: Return -1 for execution from MMIO regions in get_page_addr_code()

[Thomas Huth]

On 2018-07-10 18:00, Peter Maydell wrote:
> Now that all the callers can handle get_page_addr_code() returning -1,
> remove all the code which tries to handle execution from MMIO regions
> or small-MMU-region RAM areas. This will mean that we can correctly
> execute from these areas, rather than ending up either aborting QEMU
> or delivering an incorrect guest exception.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  accel/tcg/cputlb.c | 95 +++++-----------------------------------------
>  1 file changed, 10 insertions(+), 85 deletions(-)
> 
> diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
> index c491703f15f..abb0225dc79 100644
> --- a/accel/tcg/cputlb.c
> +++ b/accel/tcg/cputlb.c
> @@ -741,39 +741,6 @@ void tlb_set_page(CPUState *cpu, target_ulong vaddr,
>                              prot, mmu_idx, size);
>  }
>  
> -static void report_bad_exec(CPUState *cpu, target_ulong addr)
> -{
> -    /* Accidentally executing outside RAM or ROM is quite common for
> -     * several user-error situations, so report it in a way that
> -     * makes it clear that this isn't a QEMU bug and provide suggestions
> -     * about what a user could do to fix things.
> -     */
> -    error_report("Trying to execute code outside RAM or ROM at 0x"
> -                 TARGET_FMT_lx, addr);
> -    error_printf("This usually means one of the following happened:\n\n"
> -                 "(1) You told QEMU to execute a kernel for the wrong machine "
> -                 "type, and it crashed on startup (eg trying to run a "
> -                 "raspberry pi kernel on a versatilepb QEMU machine)\n"
> -                 "(2) You didn't give QEMU a kernel or BIOS filename at all, "
> -                 "and QEMU executed a ROM full of no-op instructions until "
> -                 "it fell off the end\n"
> -                 "(3) Your guest kernel has a bug and crashed by jumping "
> -                 "off into nowhere\n\n"
> -                 "This is almost always one of the first two, so check your "
> -                 "command line and that you are using the right type of kernel "
> -                 "for this machine.\n"
> -                 "If you think option (3) is likely then you can try debugging "
> -                 "your guest with the -d debug options; in particular "
> -                 "-d guest_errors will cause the log to include a dump of the "
> -                 "guest register state at this point.\n\n"
> -                 "Execution cannot continue; stopping here.\n\n");

 Hi Peter!

Looks like this patch now causes QEMU to segfault instead of printing the
above error message in certain cases, e.g.:

$ gdb --args aarch64-softmmu/qemu-system-aarch64 -M n800
[...]
(gdb) r
Starting program: aarch64-softmmu/qemu-system-aarch64 -M n800
[...]
Program received signal SIGSEGV, Segmentation fault.
[...]
(gdb) bt
#0  0x0000555555addc68 in onenand_read (opaque=0x555557600600, addr=98304, size=4) at hw/block/onenand.c:612
#1  0x00005555558b175c in memory_region_read_accessor (mr=0x555557600b80, addr=98304, value=0x7fffdbffe360, size=4, shift=0, mask=4294967295, attrs=...)
    at memory.c:440
#2  0x00005555558ae669 in access_with_adjusted_size (addr=addr@entry=98304, value=value@entry=0x7fffdbffe360, size=size@entry=4, access_size_min=<optimized out>, access_size_max=<optimized out>, access_fn=access_fn@entry=0x5555558b1720 <memory_region_read_accessor>, mr=mr@entry=0x555557600b80, attrs=attrs@entry=...) at memory.c:570
#3  0x00005555558b3016 in memory_region_dispatch_read (attrs=..., size=4, pval=0x7fffdbffe360, addr=98304, mr=0x555557600b80) at memory.c:1375
#4  0x00005555558b3016 in memory_region_dispatch_read (mr=0x555557600b80, addr=addr@entry=98304, pval=pval@entry=0x7fffdbffe360, size=size@entry=4, attrs=...)
    at memory.c:1402
#5  0x000055555583cb23 in io_readx (env=env@entry=0x555556b58a30, iotlbentry=iotlbentry@entry=0x555556b6d6b0, mmu_idx=mmu_idx@entry=1, addr=addr@entry=98304, retaddr=retaddr@entry=0, recheck=<optimized out>, access_type=access_type@entry=MMU_INST_FETCH, size=size@entry=4) at accel/tcg/cputlb.c:729
#6  0x00005555558d79cd in helper_le_ldl_cmmu (access_type=MMU_INST_FETCH, recheck=<optimized out>, retaddr=0, addr=98304, index=96, mmu_idx=1, env=0x555556b58a30)
    at accel/tcg/softmmu_template.h:106
#7  0x00005555558d79cd in helper_le_ldl_cmmu (env=env@entry=0x555556b58a30, addr=addr@entry=98304, oi=33, retaddr=retaddr@entry=0)
    at accel/tcg/softmmu_template.h:144
#8  0x00005555559d2595 in arm_tr_translate_insn (retaddr=0, ptr=98304, env=0x555556b58a30) at include/exec/cpu_ldst_template.h:102

Any clue what's going on here?

 Thomas

Re: [Qemu-devel] [PATCH 5/6] accel/tcg: Return -1 for execution from MMIO regions in get_page_addr_code()

[Richard Henderson]

On 11/14/18 6:19 PM, Thomas Huth wrote:
> Program received signal SIGSEGV, Segmentation fault.
> [...]
> (gdb) bt
> #0  0x0000555555addc68 in onenand_read (opaque=0x555557600600, addr=98304, size=4) at hw/block/onenand.c:612

So the crash is an off-by-one on the line above:

--- a/hw/block/onenand.c
+++ b/hw/block/onenand.c
@@ -608,7 +608,7 @@ static uint64_t onenand_read(void *opaque, hwaddr addr,
     int offset = addr >> s->shift;

     switch (offset) {
-    case 0x0000 ... 0xc000:
+    case 0x0000 ... 0xbfff:
         return lduw_le_p(s->boot[0] + addr);

     case 0xf000:       /* Manufacturer ID */

as the memory segment has size 0xc000.

The guest will now eventually crash with

onenand_read: unknown OneNAND register c000
...
onenand_read: unknown OneNAND register fefe
qemu: hardware error: onenand_read: implement ECC

CPU #0:
R00=00000000 R01=00000000 R02=00000000 R03=00000000
R04=00000000 R05=00000000 R06=00000000 R07=00000000
R08=00000000 R09=00000000 R10=00000000 R11=00000000
R12=00000000 R13=00000000 R14=00000000 R15=0001fe04
PSR=400001d3 -Z-- A svc32
s00=00000000 s01=00000000 d00=0000000000000000
s02=00000000 s03=00000000 d01=0000000000000000
s04=00000000 s05=00000000 d02=0000000000000000
s06=00000000 s07=00000000 d03=0000000000000000
s08=00000000 s09=00000000 d04=0000000000000000
s10=00000000 s11=00000000 d05=0000000000000000
s12=00000000 s13=00000000 d06=0000000000000000
s14=00000000 s15=00000000 d07=0000000000000000
s16=00000000 s17=00000000 d08=0000000000000000
s18=00000000 s19=00000000 d09=0000000000000000
s20=00000000 s21=00000000 d10=0000000000000000
s22=00000000 s23=00000000 d11=0000000000000000
s24=00000000 s25=00000000 d12=0000000000000000
s26=00000000 s27=00000000 d13=0000000000000000
s28=00000000 s29=00000000 d14=0000000000000000
s30=00000000 s31=00000000 d15=0000000000000000
FPSCR: 00000000
Aborted (core dumped)

I'll note that fprintf at the end of onenand_read should be
qemu_log(LOG_GUEST_ERROR) instead.


r~

Re: [Qemu-devel] [PATCH 5/6] accel/tcg: Return -1 for execution from MMIO regions in get_page_addr_code()

[Peter Maydell]

On 15 November 2018 at 07:32, Richard Henderson <rth@twiddle.net> wrote:
> On 11/14/18 6:19 PM, Thomas Huth wrote:
>> Program received signal SIGSEGV, Segmentation fault.
>> [...]
>> (gdb) bt
>> #0  0x0000555555addc68 in onenand_read (opaque=0x555557600600, addr=98304, size=4) at hw/block/onenand.c:612
>
> So the crash is an off-by-one on the line above:
>
> --- a/hw/block/onenand.c
> +++ b/hw/block/onenand.c
> @@ -608,7 +608,7 @@ static uint64_t onenand_read(void *opaque, hwaddr addr,
>      int offset = addr >> s->shift;
>
>      switch (offset) {
> -    case 0x0000 ... 0xc000:
> +    case 0x0000 ... 0xbfff:
>          return lduw_le_p(s->boot[0] + addr);
>
>      case 0xf000:       /* Manufacturer ID */
>
> as the memory segment has size 0xc000.

Presumably it should be ... 0xbffe, since we are
doing a 16-bit load ?

> The guest will now eventually crash with
>
> onenand_read: unknown OneNAND register c000
> ...
> onenand_read: unknown OneNAND register fefe
> qemu: hardware error: onenand_read: implement ECC
>
> CPU #0:
> R00=00000000 R01=00000000 R02=00000000 R03=00000000
> R04=00000000 R05=00000000 R06=00000000 R07=00000000
> R08=00000000 R09=00000000 R10=00000000 R11=00000000
> R12=00000000 R13=00000000 R14=00000000 R15=0001fe04
> PSR=400001d3 -Z-- A svc32
> s00=00000000 s01=00000000 d00=0000000000000000
> s02=00000000 s03=00000000 d01=0000000000000000
> s04=00000000 s05=00000000 d02=0000000000000000
> s06=00000000 s07=00000000 d03=0000000000000000
> s08=00000000 s09=00000000 d04=0000000000000000
> s10=00000000 s11=00000000 d05=0000000000000000
> s12=00000000 s13=00000000 d06=0000000000000000
> s14=00000000 s15=00000000 d07=0000000000000000
> s16=00000000 s17=00000000 d08=0000000000000000
> s18=00000000 s19=00000000 d09=0000000000000000
> s20=00000000 s21=00000000 d10=0000000000000000
> s22=00000000 s23=00000000 d11=0000000000000000
> s24=00000000 s25=00000000 d12=0000000000000000
> s26=00000000 s27=00000000 d13=0000000000000000
> s28=00000000 s29=00000000 d14=0000000000000000
> s30=00000000 s31=00000000 d15=0000000000000000
> FPSCR: 00000000
> Aborted (core dumped)
>
> I'll note that fprintf at the end of onenand_read should be
> qemu_log(LOG_GUEST_ERROR) instead.

Yeah, I'll put together a patch which makes it use the qemu_log
facilities rather than fprintf() and hw_error(). With that
plus the case statement fix then QEMU correctly just sits there
as the guest execution races through memory...

thanks
-- PMM

Re: [Qemu-devel] [PATCH 5/6] accel/tcg: Return -1 for execution from MMIO regions in get_page_addr_code()

[Richard Henderson]

On 11/15/18 2:53 PM, Peter Maydell wrote:
>>      switch (offset) {
>> -    case 0x0000 ... 0xc000:
>> +    case 0x0000 ... 0xbfff:
>>          return lduw_le_p(s->boot[0] + addr);
>>
>>      case 0xf000:       /* Manufacturer ID */
>>
>> as the memory segment has size 0xc000.
> 
> Presumably it should be ... 0xbffe, since we are
> doing a 16-bit load ?

Ah, true.

> Yeah, I'll put together a patch which makes it use the qemu_log
> facilities rather than fprintf() and hw_error(). With that
> plus the case statement fix then QEMU correctly just sits there
> as the guest execution races through memory...

Excellent, thanks.


r~