Changeset
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(-)
Git apply log
Switched to a new branch '20180710160013.26559-1-peter.maydell@linaro.org'
Applying: accel/tcg: Pass read access type through to io_readx()
Applying: accel/tcg: Handle get_page_addr_code() returning -1 in hashtable lookups
Applying: accel/tcg: Handle get_page_addr_code() returning -1 in tb_check_watchpoint()
Applying: accel/tcg: tb_gen_code(): Create single-insn TB for execution from non-RAM
Applying: accel/tcg: Return -1 for execution from MMIO regions in get_page_addr_code()
Applying: target/arm: Allow execution from small regions
To https://github.com/patchew-project/qemu
 + 64c6d84...2b40a8f patchew/20180710160013.26559-1-peter.maydell@linaro.org -> patchew/20180710160013.26559-1-peter.maydell@linaro.org (forced update)
Test passed: checkpatch

loading

Test passed: docker-mingw@fedora

loading

Test passed: docker-quick@centos7

loading

[Qemu-devel] [PATCH 0/6] accel/tcg: Support execution from MMIO and small MMU regions
Posted by Peter Maydell, 1 week ago
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


Re: [Qemu-devel] [Qemu-arm] [PATCH 0/6] accel/tcg: Support execution from MMIO and small MMU regions
Posted by Philippe Mathieu-Daudé, 1 week ago
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.

Re: [Qemu-devel] [Qemu-arm] [PATCH 0/6] accel/tcg: Support execution from MMIO and small MMU regions
Posted by Peter Maydell, 1 week ago
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
Posted by Peter Maydell, 1 week ago
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 0/6] accel/tcg: Support execution from MMIO and small MMU regions
Posted by KONRAD Frederic, 4 days ago
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
Posted by Peter Maydell, 4 days ago
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

[Qemu-devel] [PATCH 1/6] accel/tcg: Pass read access type through to io_readx()
Posted by Peter Maydell, 1 week ago
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


Re: [Qemu-devel] [PATCH 1/6] accel/tcg: Pass read access type through to io_readx()
Posted by Richard Henderson, 1 week ago
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] [Qemu-arm] [PATCH 1/6] accel/tcg: Pass read access type through to io_readx()
Posted by Philippe Mathieu-Daudé, 1 week ago
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;
>  }
> 

[Qemu-devel] [PATCH 2/6] accel/tcg: Handle get_page_addr_code() returning -1 in hashtable lookups
Posted by Peter Maydell, 1 week ago
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


Re: [Qemu-devel] [PATCH 2/6] accel/tcg: Handle get_page_addr_code() returning -1 in hashtable lookups
Posted by Richard Henderson, 1 week ago
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 2/6] accel/tcg: Handle get_page_addr_code() returning -1 in hashtable lookups
Posted by Emilio G. Cota, 1 week ago
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.

[Qemu-devel] [PATCH 3/6] accel/tcg: Handle get_page_addr_code() returning -1 in tb_check_watchpoint()
Posted by Peter Maydell, 1 week ago
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


Re: [Qemu-devel] [PATCH 3/6] accel/tcg: Handle get_page_addr_code() returning -1 in tb_check_watchpoint()
Posted by Richard Henderson, 1 week ago
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~


[Qemu-devel] [PATCH 4/6] accel/tcg: tb_gen_code(): Create single-insn TB for execution from non-RAM
Posted by Peter Maydell, 1 week ago
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


Re: [Qemu-devel] [PATCH 4/6] accel/tcg: tb_gen_code(): Create single-insn TB for execution from non-RAM
Posted by Richard Henderson, 1 week ago
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 4/6] accel/tcg: tb_gen_code(): Create single-insn TB for execution from non-RAM
Posted by Emilio G. Cota, 1 week ago
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

[Qemu-devel] [PATCH 5/6] accel/tcg: Return -1 for execution from MMIO regions in get_page_addr_code()
Posted by Peter Maydell, 1 week ago
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


Re: [Qemu-devel] [PATCH 5/6] accel/tcg: Return -1 for execution from MMIO regions in get_page_addr_code()
Posted by Richard Henderson, 1 week ago
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] [Qemu-arm] [PATCH 5/6] accel/tcg: Return -1 for execution from MMIO regions in get_page_addr_code()
Posted by Philippe Mathieu-Daudé, 1 week ago
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);
> 

[Qemu-devel] [PATCH 6/6] target/arm: Allow execution from small regions
Posted by Peter Maydell, 1 week ago
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 6/6] target/arm: Allow execution from small regions
Posted by Richard Henderson, 1 week ago
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 6/6] target/arm: Allow execution from small regions
Posted by Philippe Mathieu-Daudé, 1 week ago
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));
>  }
>  
>