Peter Maydell <peter.mayd...@linaro.org> writes:
> On 7 November 2017 at 16:52, Alex Bennée <alex.ben...@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.ben...@linaro.org>
>> Reported-by: Peter Maydell <peter.mayd...@linaro.org>
>> Suggested-by: Paolo Bonzini <pbonz...@redhat.com>
>> Cc: Richard Henderson <r...@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
