On 10/10/2017 12:52, Peter Maydell wrote:
> 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.
Yes, it is right if it happens from translated code. But
cpu_restore_code currently expects either retaddr == 0 or retaddr from a
TB. And generalizing those expectations...
>> 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.
... makes sense too, and it can replace the existing check for !retaddr
(introduced in commit d8b2239bcd, "translate-all: exit cpu_restore_state
early if translating", 2017-03-09) which only works for softmmu.
Thanks,
Paolo
