On 10 October 2017 at 11:41, Paolo Bonzini <pbonz...@redhat.com> wrote:
> I've seen the same on x86. Using the program counter from translate.c
> here looks very fishy:
>
> /* Now we have a real cpu fault. Since this is the exact location of
> * the exception, we must undo the adjustment done by cpu_restore_state
> * for handling call return addresses. */
> cpu_restore_state(cpu, pc + GETPC_ADJ);
This is the right thing if the signal happened directly from
translated code (as it will for guest reads/writes). This
bit of code expects that cpu_restore_state() will just do nothing
if the pc value isn't actually in a TB.
> and cpu_restore_state would just return false because tb_find_pc fails. Maybe
> something like this?
>
> diff --git a/accel/tcg/user-exec.c b/accel/tcg/user-exec.c
> index 492ea0826c..66a4351b96 100644
> --- a/accel/tcg/user-exec.c
> +++ b/accel/tcg/user-exec.c
> @@ -119,10 +119,13 @@ static inline int handle_cpu_signal(uintptr_t pc,
> unsigned long address,
> return 1; /* the MMU fault was handled without causing real CPU
> fault */
> }
>
> - /* Now we have a real cpu fault. Since this is the exact location of
> - * the exception, we must undo the adjustment done by cpu_restore_state
> - * for handling call return addresses. */
> - cpu_restore_state(cpu, pc + GETPC_ADJ);
> + if (pc >= (uintptr_t)tcg_ctx.code_gen_buffer &&
> + pc < (uintptr_t)tcg_ctx.code_gen_ptr) {
> + /* Now we have a real cpu fault. Since this is the exact location of
> + * the exception, we must undo the adjustment done by
> cpu_restore_state
> + * for handling call return addresses. */
> + cpu_restore_state(cpu, pc + GETPC_ADJ);
> + }
I think that would be better inside cpu_restore_state(), because it's
an internal detail of the TCG accelerator that it happens to put
all its code inside those bounds.
thanks
-- PMM
