| Message ID | 20250225184628.3590671-5-alex.bennee@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | cputlb: add tlb_flush_other_cpu | expand |
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~
On 2/25/25 12:02, Richard Henderson wrote: > 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. (4) Running in round-robin mode, so there is *never* a race between cpus. Anything else I've forgotten? r~
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)) /* run_on_cpu_data.target_ptr should always be big enough for a * vaddr even on 32 bit builds