On Mon, 4 Dec 2023 at 14:40, Philippe Mathieu-Daudé <phi...@linaro.org> wrote:
>
> Unplugging vCPU triggers the following assertion in
Unplugging leaks the tcg context refcount but does not trigger the
assertion directly. Maybe clarify that by changing the wording:
"Plugging a vCPU after it has been unplugged triggers..."
> tcg_register_thread():
>
> 796 void tcg_register_thread(void)
> 797 {
> ...
> 812 /* Claim an entry in tcg_ctxs */
> 813 n = qatomic_fetch_inc(&tcg_cur_ctxs);
> 814 g_assert(n < tcg_max_ctxs);
>
> Implement and use tcg_unregister_thread() so when a
> vCPU is unplugged, the tcg_cur_ctxs refcount is
> decremented.
>
> Reported-by: Michal Suchánek <msucha...@suse.de>
> Suggested-by: Stefan Hajnoczi <stefa...@gmail.com>
> Signed-off-by: Philippe Mathieu-Daudé <phi...@linaro.org>
> ---
> RFC: untested
> Report:
> https://lore.kernel.org/qemu-devel/20231204183638.gz9...@kitsune.suse.cz/
> ---
> include/tcg/startup.h | 5 +++++
> accel/tcg/tcg-accel-ops-mttcg.c | 1 +
> accel/tcg/tcg-accel-ops-rr.c | 1 +
> tcg/tcg.c | 17 +++++++++++++++++
> 4 files changed, 24 insertions(+)
>
> diff --git a/include/tcg/startup.h b/include/tcg/startup.h
> index f71305765c..520942a4a1 100644
> --- a/include/tcg/startup.h
> +++ b/include/tcg/startup.h
> @@ -45,6 +45,11 @@ void tcg_init(size_t tb_size, int splitwx, unsigned
> max_cpus);
> */
> void tcg_register_thread(void);
>
> +/**
> + * tcg_unregister_thread: Unregister this thread with the TCG runtime
> + */
> +void tcg_unregister_thread(void);
> +
> /**
> * tcg_prologue_init(): Generate the code for the TCG prologue
> *
> diff --git a/accel/tcg/tcg-accel-ops-mttcg.c b/accel/tcg/tcg-accel-ops-mttcg.c
> index fac80095bb..88d7427aad 100644
> --- a/accel/tcg/tcg-accel-ops-mttcg.c
> +++ b/accel/tcg/tcg-accel-ops-mttcg.c
> @@ -120,6 +120,7 @@ static void *mttcg_cpu_thread_fn(void *arg)
>
> tcg_cpus_destroy(cpu);
> qemu_mutex_unlock_iothread();
> + tcg_unregister_thread();
> rcu_remove_force_rcu_notifier(&force_rcu.notifier);
> rcu_unregister_thread();
> return NULL;
> diff --git a/accel/tcg/tcg-accel-ops-rr.c b/accel/tcg/tcg-accel-ops-rr.c
> index 611932f3c3..c2af3aad21 100644
> --- a/accel/tcg/tcg-accel-ops-rr.c
> +++ b/accel/tcg/tcg-accel-ops-rr.c
> @@ -302,6 +302,7 @@ static void *rr_cpu_thread_fn(void *arg)
> rr_deal_with_unplugged_cpus();
> }
>
> + tcg_unregister_thread();
> rcu_remove_force_rcu_notifier(&force_rcu);
> rcu_unregister_thread();
> return NULL;
> diff --git a/tcg/tcg.c b/tcg/tcg.c
> index d2ea22b397..5125342d70 100644
> --- a/tcg/tcg.c
> +++ b/tcg/tcg.c
> @@ -781,11 +781,18 @@ static void alloc_tcg_plugin_context(TCGContext *s)
> * modes.
> */
> #ifdef CONFIG_USER_ONLY
> +
> void tcg_register_thread(void)
> {
> tcg_ctx = &tcg_init_ctx;
> }
> +
> +void tcg_unregister_thread(void)
> +{
> +}
> +
> #else
> +
> void tcg_register_thread(void)
> {
> TCGContext *s = g_malloc(sizeof(*s));
> @@ -814,6 +821,16 @@ void tcg_register_thread(void)
>
> tcg_ctx = s;
> }
> +
> +void tcg_unregister_thread(void)
> +{
> + unsigned int n;
> +
> + n = qatomic_fetch_dec(&tcg_cur_ctxs);
> + g_free(tcg_ctxs[n]);
> + qatomic_set(&tcg_ctxs[n], NULL);
> +}
tcg_ctxs[n] may not be our context, so this looks like it could free
another thread's context and lead to undefined behavior.
I haven't read the code so I can't suggest an alternative myself.
Stefan
> +
> #endif /* !CONFIG_USER_ONLY */
>
> /* pool based memory allocation */
> --
> 2.41.0
>
