Re: [PATCH 4/4] tcg:tlb: use tcg_debug_assert() in assert_cpu_is_self()

[Richard Henderson <richard.henderson@linaro.org>]

On 2/25/25 10:46, Alex Bennée wrote:
> From: Igor Mammedov <imammedo@redhat.com>
> 
> that will enable assert_cpu_is_self when QEMU is configured with
>     --enable-debug
> without need for manual patching DEBUG_TLB_GATE define.
> 
> Need to manually path DEBUG_TLB_GATE define to enable assert,
> let regression caused by [1] creep in unnoticed.
> 
> 1) 30933c4fb4f3d ("tcg/cputlb: remove other-cpu capability from TLB flushing")
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> Suggested-by: Alex Bennée <alex.bennee@linaro.org>
> Message-Id: <20250207162048.1890669-5-imammedo@redhat.com>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>   accel/tcg/cputlb.c | 7 ++-----
>   1 file changed, 2 insertions(+), 5 deletions(-)
> 
> diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
> index fc16a576f0..65b04b1055 100644
> --- a/accel/tcg/cputlb.c
> +++ b/accel/tcg/cputlb.c
> @@ -73,11 +73,8 @@
>       } \
>   } while (0)
>   
> -#define assert_cpu_is_self(cpu) do {                              \
> -        if (DEBUG_TLB_GATE) {                                     \
> -            g_assert(!(cpu)->created || qemu_cpu_is_self(cpu));   \
> -        }                                                         \
> -    } while (0)
> +#define assert_cpu_is_self(cpu)                             \
> +    tcg_debug_assert(!(cpu)->created || qemu_cpu_is_self(cpu))

I think this check is just wrong or incomplete.

The intent here is to check that we're not attempting to modify the softmmu tlb 
asynchronously while a cpu is running.

(0) A synchronous flush to the current cpu is (obviously?) ok.
(1) A flush to a cpu that is not yet created is (or should be) a no-op.

Not checked here are any of the other reasons a flush might be ok:

(2) The system as a whole is stopped, on the way in from migration/vmload.
(3) The cpu is offline, on the way in from poweroff/reset.

If we decide that {1, 2, 3} are too complicated to check, then perhaps the solution to 
queue flushes to the cpu's workqueue is the appropriate solution.  But so far all I see is 
that we have an incomplete check, and no ready explanation for why that check can't be 
improved.


r~