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

[Peter Maydell]

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".

> 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