Re: [PULL 01/39] accel/tcg: mttcg remove false-negative halted assertion

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

"Nicholas Piggin" <npiggin@gmail.com> writes:

> On Sat Sep 16, 2023 at 1:29 PM AEST, Richard Henderson wrote:
>> From: Nicholas Piggin <npiggin@gmail.com>
>>
>> mttcg asserts that an execution ending with EXCP_HALTED must have
>> cpu->halted. However between the event or instruction that sets
>> cpu->halted and requests exit and the assertion here, an
>> asynchronous event could clear cpu->halted.
>>
>> This leads to crashes running AIX on ppc/pseries because it uses
>> H_CEDE/H_PROD hcalls, where H_CEDE sets self->halted = 1 and
>> H_PROD sets other cpu->halted = 0 and kicks it.
>>
>> H_PROD could be turned into an interrupt to wake, but several other
>> places in ppc, sparc, and semihosting follow what looks like a similar
>> pattern setting halted = 0 directly. So remove this assertion.
>>
>> Reported-by: Ivan Warren <ivan@vmfacility.fr>
>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>> Message-Id: <20230829010658.8252-1-npiggin@gmail.com>
>> [rth: Keep the case label and adjust the comment.]
>
> Hey Richard,
>
> Thanks for picking this up.
>
> I think EXCP_HLT and EXCP_HALTED are effectively the same, so they could
> be merged after this.
>
> I couldn't quite decipher the intended difference between them, HLT is
> "hlt instruction reached", but it does tend to go into a mode where it
> is halted waiting for external event. Is there some useful difference in
> semantics we should retain (and at least try to find a way to assert)?

I always thought HALTED was where the system was halted (e.g. during a
shutdown) but I agree its less than clear.

Do both effectively end up in wait_for_io for some event to start the
loop again?

>
> I did look at how to avoid the halted race and keep the assert, e.g.,
> have the CPU only modify its own halted, and external events would have
> a wakeup field to set. In the end it wasn't clear that that was any
> simpler and you still have races to reason about, now between the two
> fields. So unless someone wants to keep both, should we merge?
>
> Thanks,
> Nick
>
>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
>> ---
>>  accel/tcg/tcg-accel-ops-mttcg.c | 9 ++-------
>>  1 file changed, 2 insertions(+), 7 deletions(-)
>>
>> diff --git a/accel/tcg/tcg-accel-ops-mttcg.c b/accel/tcg/tcg-accel-ops-mttcg.c
>> index b276262007..4b0dfb4be7 100644
>> --- a/accel/tcg/tcg-accel-ops-mttcg.c
>> +++ b/accel/tcg/tcg-accel-ops-mttcg.c
>> @@ -100,14 +100,9 @@ static void *mttcg_cpu_thread_fn(void *arg)
>>                  break;
>>              case EXCP_HALTED:
>>                  /*
>> -                 * during start-up the vCPU is reset and the thread is
>> -                 * kicked several times. If we don't ensure we go back
>> -                 * to sleep in the halted state we won't cleanly
>> -                 * start-up when the vCPU is enabled.
>> -                 *
>> -                 * cpu->halted should ensure we sleep in wait_io_event
>> +                 * Usually cpu->halted is set, but may have already been
>> +                 * reset by another thread by the time we arrive here.
>>                   */
>> -                g_assert(cpu->halted);
>>                  break;
>>              case EXCP_ATOMIC:
>>                  qemu_mutex_unlock_iothread();


-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro