Re: [Qemu-devel] [PATCH v2 05/11] translate-all: exit cpu_restore_state early if translating

[Richard Henderson]

On 03/03/2017 09:03 PM, Alex Bennée wrote:
> > We *should* have retaddr == 0 for this case, which indicates that we
> > should not attempt to restore state.  Are you seeing a non-zero value?
>
> Actually looking at xtensa I see:
>
>   Attempt to resolve CPU state @ 0x0 while translating
>
> So maybe I should check just that - but I don't see where we ensure we
> always pass zero.

cpu_ld*_cmmu, in include/exec/cpu_ldst_template.h, has

    return glue(glue(glue(cpu_ld, USUFFIX), MEMSUFFIX), _ra)(env, ptr, 0);

so there's your zero.

> #0  0x00005555555e3712 in cpu_restore_state (address@hidden, address@hidden) at 
> /home/alex/lsrc/qemu/qemu.git/translate-all.c:338
> #1  0x000055555564cb38 in tlb_fill (address@hidden, address@hidden, 
> address@hidden, address@hidden, address@hidden) at 
> /home/alex/lsrc/qemu/qemu.git/target/xtensa/op_helper.c:73
> #2  0x000055555562d604 in helper_ret_ldb_cmmu (address@hidden, addr=537034752, 
> oi=<optimised out>, address@hidden) at 
> /home/alex/lsrc/qemu/qemu.git/softmmu_template.h:127

Confirmed.  This is a simple bug in xtensa -- failure to check retaddr == 0 
before calling cpu_restore_state.

That said, I do wonder if it wouldn't be better to move that check inside 
cpu_restore_state instead.  Put the check there now, but leave the follow-on 
cleanup for the next devel cycle.

It would also save auditing the other usages of cpu_restore_state in the tree.

> > Is this in fact linux-user, not softmmu, as you imply from tlb_fill?
> > Because handle_cpu_signal will in fact pass a genuine non-zero retaddr
> > for the SIGSEGV resulting from a cpu_ld*_code from a non-mapped
> > address.
>
> I think that is another call chain that might trip us up. Peter
> mentioned he'd hit it. This one is definitely softmmu.

This is really easy to reproduce on any guest with a call to a null function 
pointer.


r~