[Qemu-devel] [PATCH] cputlb: Don't assume do_unassigned_access() never returns

[Qemu-devel] [PATCH] cputlb: Don't assume do_unassigned_access() never returns

[Peter Maydell]

In get_page_addr_code(), if the guest PC doesn't correspond to RAM
then we currently run the CPU's do_unassigned_access() hook if it has
one, and otherwise we give up and exit QEMU with a more-or-less
useful message.  This code assumes that the do_unassigned_access hook
will never return, because if it does then we'll plough on attempting
to use a non-RAM TLB entry to get a RAM address and will abort() in
qemu_ram_addr_from_host_nofail().  Unfortunately some CPU
implementations of this hook do return: Microblaze, SPARC and the ARM
v7M.

Change the code to call report_bad_exec() if the hook returns, as
well as if it didn't have one.  This means we can tidy it up to use
the cpu_unassigned_access() function which wraps the "get the CPU
class and call the hook if it has one" work, since we aren't trying
to distinguish "no hook" from "hook existed and returned" any more.

This brings the handling of this hook into line with the handling
used for data accesses, where "hook returned" is treated the
same as "no hook existed" and gets you the default behaviour.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
Review from somebody who understands how the unassigned_access
hooks are supposed to work appreciated.

 cputlb.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/cputlb.c b/cputlb.c
index 6c39927..e665193 100644
--- a/cputlb.c
+++ b/cputlb.c
@@ -475,14 +475,13 @@ tb_page_addr_t get_page_addr_code(CPUArchState *env1, target_ulong addr)
     pd = iotlbentry->addr & ~TARGET_PAGE_MASK;
     mr = iotlb_to_region(cpu, pd, iotlbentry->attrs);
     if (memory_region_is_unassigned(mr)) {
-        CPUClass *cc = CPU_GET_CLASS(cpu);
-
-        if (cc->do_unassigned_access) {
-            cc->do_unassigned_access(cpu, addr, false, true, 0, 4);
-        } else {
-            report_bad_exec(cpu, addr);
-            exit(1);
-        }
+        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);
     }
     p = (void *)((uintptr_t)addr + env1->tlb_table[mmu_idx][page_index].addend);
     return qemu_ram_addr_from_host_nofail(p);
-- 
2.7.4

Re: [Qemu-devel] [PATCH] cputlb: Don't assume do_unassigned_access() never returns

[Richard Henderson]

On 02/09/2017 11:31 AM, Peter Maydell wrote:
> In get_page_addr_code(), if the guest PC doesn't correspond to RAM
> then we currently run the CPU's do_unassigned_access() hook if it has
> one, and otherwise we give up and exit QEMU with a more-or-less
> useful message.  This code assumes that the do_unassigned_access hook
> will never return, because if it does then we'll plough on attempting
> to use a non-RAM TLB entry to get a RAM address and will abort() in
> qemu_ram_addr_from_host_nofail().  Unfortunately some CPU
> implementations of this hook do return: Microblaze, SPARC and the ARM
> v7M.
>
> Change the code to call report_bad_exec() if the hook returns, as
> well as if it didn't have one.  This means we can tidy it up to use
> the cpu_unassigned_access() function which wraps the "get the CPU
> class and call the hook if it has one" work, since we aren't trying
> to distinguish "no hook" from "hook existed and returned" any more.
>
> This brings the handling of this hook into line with the handling
> used for data accesses, where "hook returned" is treated the
> same as "no hook existed" and gets you the default behaviour.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> Review from somebody who understands how the unassigned_access
> hooks are supposed to work appreciated.
>
>  cputlb.c | 15 +++++++--------
>  1 file changed, 7 insertions(+), 8 deletions(-)

The unassigned_access hook was (unfortunately) designed after the 
unaligned_access hook.  The later *can* usefully return to indicate that the 
cpu bit that enforces alignment traps is off.

Any unassigned_access hook that doesn't longjmp could be considered buggy.

Reviewed-by: Richard Henderson <rth@twiddle.net>


r~

Re: [Qemu-devel] [PATCH] cputlb: Don't assume do_unassigned_access() never returns

[Peter Maydell]

On 9 February 2017 at 20:03, Richard Henderson <rth@twiddle.net> wrote:
> On 02/09/2017 11:31 AM, Peter Maydell wrote:
>> Review from somebody who understands how the unassigned_access
>> hooks are supposed to work appreciated.

> The unassigned_access hook was (unfortunately) designed after the
> unaligned_access hook.  The later *can* usefully return to indicate that the
> cpu bit that enforces alignment traps is off.
>
> Any unassigned_access hook that doesn't longjmp could be
> considered buggy.

I use the M profile unassigned_access hook to implement
the "high addresses are magic, but only if you're in
Handler mode" behaviour. So for Thread mode I want the
default handling. You could argue that this is kind
of abusing the hook, though.

Microblaze returns from the hook in the case where the
processor version register bit says "this CPU doesn't
generate exceptions for instruction/data aborts".
Since it looks like you can have a CPU config that
chooses to generate an exception for insn abort
but not data abort (or vice-versa), this looks like
a valid case for allowing the hook to return (you
need the hook for the case which you want to have
throw exceptions, but it gets called for the other
case too and needs to be able to say "nope" there).

The sparc code is too complicated for me to be able
to judge whether its non-longjmping codepaths are
valid or bugs.

thanks
-- PMM

Re: [Qemu-devel] [PATCH] cputlb: Don't assume do_unassigned_access() never returns

[Richard Henderson]

On 02/10/2017 07:58 AM, Peter Maydell wrote:
> On 9 February 2017 at 20:03, Richard Henderson <rth@twiddle.net> wrote:
>> On 02/09/2017 11:31 AM, Peter Maydell wrote:
>>> Review from somebody who understands how the unassigned_access
>>> hooks are supposed to work appreciated.
>
>> The unassigned_access hook was (unfortunately) designed after the
>> unaligned_access hook.  The later *can* usefully return to indicate that the
>> cpu bit that enforces alignment traps is off.
>>
>> Any unassigned_access hook that doesn't longjmp could be
>> considered buggy.
>
> I use the M profile unassigned_access hook to implement
> the "high addresses are magic, but only if you're in
> Handler mode" behaviour. So for Thread mode I want the
> default handling. You could argue that this is kind
> of abusing the hook, though.

I'm surprised you don't assign those high addresses to a magic device to handle 
that sort of magic.  Then they wouldn't be "unassigned".

> Microblaze returns from the hook in the case where the
> processor version register bit says "this CPU doesn't
> generate exceptions for instruction/data aborts".
> Since it looks like you can have a CPU config that
> chooses to generate an exception for insn abort
> but not data abort (or vice-versa), this looks like
> a valid case for allowing the hook to return (you
> need the hook for the case which you want to have
> throw exceptions, but it gets called for the other
> case too and needs to be able to say "nope" there).

If the cpu doesn't implement aborts, then the processor must provide some 
default value for the memory return.  But the code path along which a return 
happens does not allow for data to be read.  We would need to expand the hook 
definition for this.

> The sparc code is too complicated for me to be able
> to judge whether its non-longjmping codepaths are
> valid or bugs.

I argued with Artyom that (at least) the sparc64 return code path is wrong. 
But I have no idea what the real cpu does for that case.


r~

Re: [Qemu-devel] [PATCH] cputlb: Don't assume do_unassigned_access() never returns

[Peter Maydell]

On 9 February 2017 at 23:32, Richard Henderson <rth@twiddle.net> wrote:
> On 02/10/2017 07:58 AM, Peter Maydell wrote:
>> I use the M profile unassigned_access hook to implement
>> the "high addresses are magic, but only if you're in
>> Handler mode" behaviour. So for Thread mode I want the
>> default handling. You could argue that this is kind
>> of abusing the hook, though.
>
>
> I'm surprised you don't assign those high addresses to a magic device to
> handle that sort of magic.  Then they wouldn't be "unassigned".

We used to do that, but removed it in commit 542b3478a0 recently,
because it's a nasty hack. There really isn't a device there
(let alone a block of RAM). Among other things, reads and
writes aren't supposed to be magic, only executes and only
if the CPU is in a particular state. At least using the
unassigned_access hook keeps weird CPU behaviour in the
CPU and doesn't let it leak out into the system model.

(The proper non-hacky approach to this would be to do what
the spec says we should and make the branch instruction
itself behave differently if the register being branched to
has the magic value. But that would slow down branches
generally.)

>> Microblaze returns from the hook in the case where the
>> processor version register bit says "this CPU doesn't
>> generate exceptions for instruction/data aborts".
>> Since it looks like you can have a CPU config that
>> chooses to generate an exception for insn abort
>> but not data abort (or vice-versa), this looks like
>> a valid case for allowing the hook to return (you
>> need the hook for the case which you want to have
>> throw exceptions, but it gets called for the other
>> case too and needs to be able to say "nope" there).
>
>
> If the cpu doesn't implement aborts, then the processor must provide some
> default value for the memory return.  But the code path along which a return
> happens does not allow for data to be read.  We would need to expand the
> hook definition for this.

Yeah; at the moment the assumption is the cpu is ok with
qemu's generic default of RAZ/WI.

thanks
-- PMM