Re: [Qemu-arm] [PATCH] accel/tcg/translate-all: expand cpu_restore_state retaddr check

[Peter Maydell]

On 7 November 2017 at 18:45, Alex Bennée <address@hidden> wrote:
>
> Peter Maydell <address@hidden> writes:
>
>> On 7 November 2017 at 16:52, Alex Bennée <address@hidden> wrote:
>>> We are still seeing signals during translation time when we walk over
>>> a page protection boundary. This expands the check to ensure the
>>> retaddr is inside the code generation buffer. The original suggestion
>>> was to check versus tcg_ctx.code_gen_ptr but as we now segment the
>>> translation buffer we have to settle for just a general check for
>>> being inside.
>>>
>>> Signed-off-by: Alex Bennée <address@hidden>
>>> Reported-by: Peter Maydell <address@hidden>
>>> Suggested-by: Paolo Bonzini <address@hidden>
>>> Cc: Richard Henderson <address@hidden>
>>> ---
>>>  accel/tcg/translate-all.c | 20 ++++++++++++--------
>>>  1 file changed, 12 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c
>>> index 34c5e28d07..eb255af402 100644
>>> --- a/accel/tcg/translate-all.c
>>> +++ b/accel/tcg/translate-all.c
>>> @@ -357,16 +357,20 @@ bool cpu_restore_state(CPUState *cpu, uintptr_t 
>>> retaddr)
>>>      TranslationBlock *tb;
>>>      bool r = false;
>>>
>>> -    /* A retaddr of zero is invalid so we really shouldn't have ended
>>> -     * up here. The target code has likely forgotten to check retaddr
>>> -     * != 0 before attempting to restore state. We return early to
>>> -     * avoid blowing up on a recursive tb_lock(). The target must have
>>> -     * previously survived a failed cpu_restore_state because
>>> -     * tb_find_pc(0) would have failed anyway. It still should be
>>> -     * fixed though.
>>> +    /* The retaddr has to be in the region of current code buffer. If
>>> +     * it's not we will not be able to resolve it here. If it is zero
>>> +     * the calling code has likely forgotten to check retaddr before
>>> +     * calling here.
>>
>> This part of the comment isn't correct -- it's entirely expected
>> that we will get here with a zero retaddr, because that is
>> how the rest of the code tells this function "no state restoration
>> required".
>
> Then why call cpu_restore_state at all? We should be consistent as there
> are plenty of places that do things like:
>
>     if (pc) {
>         /* now we have a real cpu fault */
>         cpu_restore_state(cs, pc);
>     }
>
> I'm happy to make a 0 retaddr officially valid and actually document it
> in exec-all.h. It's not like most callers even bother checking the
> return code.

Hmm, there's more places than I expected that do that "don't call
if 0" check than I thought. Overall it seems better to me to officially
allow the zero, rather than having lots of callsites that all have
to remember to check.

Incidentally if retaddr is zero then
(retaddr < (uintptr_t) tcg_init_ctx.code_gen_buffer)
is always true and you don't need to explicitly check for zero, though
it might be clearer to do so if we think we might change the rest
of the condition in future.

thanks
-- PMM