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

["Alex Bennée" <alex.bennee@linaro.org>]

Peter Maydell <peter.maydell@linaro.org> writes:

> On 7 November 2017 at 16:52, Alex Bennée <alex.bennee@linaro.org> 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 <alex.bennee@linaro.org>
>> Reported-by: Peter Maydell <peter.maydell@linaro.org>
>> Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
>> Cc: Richard Henderson <rth@twiddle.net>
>> ---
>>  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.

>
>> If it is not in the translated code we could be
>> +     * faulting during translation itself.
>> +     *
>> +     * Either way we need return early to avoid blowing up on a
>> +     * recursive tb_lock() as we can't resolve it here.
>>       */
>>
>> -    if (!retaddr) {
>> +    if (!retaddr ||
>> +        (retaddr < (uintptr_t) tcg_init_ctx.code_gen_buffer) ||
>> +        (retaddr > (uintptr_t) (tcg_init_ctx.code_gen_buffer +
>> +                                tcg_init_ctx.code_gen_buffer_size))) {
>>          return r;
>>      }
>
> thanks
> -- PMM


--
Alex Bennée