Re: [Qemu-devel] [PATCH] tcg: handle EXCP_ATOMIC exception properly

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

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 10/02/2017 02:45, Pranith Kumar wrote:
>> The current method of executing atomic code in a guest uses
>> cpu_exec_step_atomic() from the outermost loop. This causes an abort()
>> when single stepping over atomic code since debug exception longjmp
>> will point to the the setlongjmp in cpu_exec(). Another issue with
>> this mechanism is that the flags which were set in atomic execution
>> will be lost since we do not call cpu_exec_enter().
>>
>> The following patch moves atomic exception handling to the exception
>> handler where all these issues are taken care of. The change in
>> start_exclusive() is necessary since now the cpu in atomic execution
>> will have its running flag set, but we do not want to count it as
>> pending.
>>
>> Thanks to Alex for helping me debug the issue.
>>
>> CC: Alex Bennée <alex.bennee@linaro.org>
>> CC: Richard Henderson <rth@twiddle.net>
>> CC: Paolo Bonzini <pbonzini@redhat.com>
>> Signed-off-by: Pranith Kumar <bobby.prani@gmail.com>
>> ---
>>  cpu-exec.c    | 2 ++
>>  cpus-common.c | 2 +-
>>  cpus.c        | 4 ----
>>  3 files changed, 3 insertions(+), 5 deletions(-)
>>
>> diff --git a/cpu-exec.c b/cpu-exec.c
>> index b0ddada8c1..dceacfc5dd 100644
>> --- a/cpu-exec.c
>> +++ b/cpu-exec.c
>> @@ -427,6 +427,8 @@ static inline bool cpu_handle_exception(CPUState *cpu, int *ret)
>>              *ret = cpu->exception_index;
>>              if (*ret == EXCP_DEBUG) {
>>                  cpu_handle_debug_exception(cpu);
>> +            } else if (*ret == EXCP_ATOMIC) {
>> +                cpu_exec_step_atomic(cpu);
>
> I think you can unlock/lock the iothread here, and also call

The iothread is already unlocked by this point (see tcg_cpu_exec).

> cpu_exec_end/start to work around the limitation in start_exclusive.

While that seems right it also seems very messy as it inverts the calls
so far. I fear we may end up very confused in special casing. Is there a
cleaner way we can unwind this?

>
> Paolo
>
>>              }
>>              cpu->exception_index = -1;
>>              return true;
>> diff --git a/cpus-common.c b/cpus-common.c
>> index 59f751ecf9..7b859752ea 100644
>> --- a/cpus-common.c
>> +++ b/cpus-common.c
>> @@ -192,7 +192,7 @@ void start_exclusive(void)
>>      smp_mb();
>>      running_cpus = 0;
>>      CPU_FOREACH(other_cpu) {
>> -        if (atomic_read(&other_cpu->running)) {
>> +        if (atomic_read(&other_cpu->running) && !qemu_cpu_is_self(other_cpu)) {
>>              other_cpu->has_waiter = true;
>>              running_cpus++;
>>              qemu_cpu_kick(other_cpu);
>> diff --git a/cpus.c b/cpus.c
>> index e1b82bcd49..981f23d52b 100644
>> --- a/cpus.c
>> +++ b/cpus.c
>> @@ -1461,10 +1461,6 @@ static void *qemu_tcg_cpu_thread_fn(void *arg)
>>                   */
>>                  g_assert(cpu->halted);
>>                  break;
>> -            case EXCP_ATOMIC:
>> -                qemu_mutex_unlock_iothread();
>> -                cpu_exec_step_atomic(cpu);
>> -                qemu_mutex_lock_iothread();
>>              default:
>>                  /* Ignore everything else? */
>>                  break;
>>


--
Alex Bennée