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

["Nicholas Piggin" <npiggin@gmail.com>]

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 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();