Re: [PATCH] tcg: drop qemu_cpu_is_self() in tlb_flush_by_mmuidx[_async_work]

[Igor Mammedov <imammedo@redhat.com>]

On Mon, 27 Jan 2025 14:24:56 +0100
Philippe Mathieu-Daudé <philmd@linaro.org> wrote:

> +Peter
> 
> On 23/1/25 14:38, Igor Mammedov wrote:
> > On Thu, 23 Jan 2025 12:09:59 +0000
> > Alex Bennée <alex.bennee@linaro.org> wrote:
> >   
> >> Igor Mammedov <imammedo@redhat.com> writes:
> >>  
> >>> On Thu, 23 Jan 2025 10:52:15 +0000
> >>> Alex Bennée <alex.bennee@linaro.org> wrote:
> >>>     
> >>>> Igor Mammedov <imammedo@redhat.com> writes:
> >>>>      
> >>>>> QEMU will crash with following debug enabled
> >>>>>    # define DEBUG_TLB_GATE 1
> >>>>>    # define DEBUG_TLB_LOG_GATE 1
> >>>>> due to [1] introduced assert and as it happenstlb_flush_by_mmuidx[_async_work]
> >>>>> functions are called not only from vcpu thread but also from reset handler
> >>>>> that is called from main thread at cpu realize time when vcpu is already
> >>>>> created
> >>>>>    x86_cpu_new -> ... ->
> >>>>>        x86_cpu_realizefn -> cpu_reset -> ... ->
> >>>>>            tcg_cpu_reset_hold
> >>>>>
> >>>>> drop assert to fix crash.  
> >>>>
> >>>> Hmm the assert is there for a good reason because we do not want to be
> >>>> flushing another CPUs state. However the assert itself:
> >>>>
> >>>>    g_assert(!(cpu)->created || qemu_cpu_is_self(cpu));
> >>>>
> >>>> was trying to account for pre-initialised vCPUs. What has changed?
> >>>>
> >>>> cpu_thread_signal_created(cpu) is called just before we start running
> >>>> the main loop in mttcg_cpu_thread_fn. So any other thread messing with
> >>>> the CPUs TLB can potentially mess things up.  
> >>>
> >>> it reproduces on current master, so yes it likely has changed over time.
> >>> I've just stumbled on it when attempting to get rid of cpu->created
> >>> usage.  
> >>
> >> Why the drive to get rid of cpu->created?  
> > 
> > During review of Philippe's cpu cleanups,
> > I've noticed that  cpu->created is mostly used for signaling
> > main loop thread we've started vcpu thread with a couple of
> > odd cases in tcg and kvm.
> >    - 1st silently bit-rotted being behind ifdefs
> >    - 2nd is work around CPU being prematurely visible to others  
> 
> 2nd is:
> 
> commit 56adee407fc564da19e49cfe18e20e3da92320be
> Author: Peter Xu <peterx@redhat.com>
> Date:   Fri Feb 17 00:18:32 2023 +0800
> 
>      kvm: dirty-ring: Fix race with vcpu creation
> 
>      It's possible that we want to reap a dirty ring on a vcpu that is 
> during
>      creation, because the vcpu is put onto list (CPU_FOREACH visible) 
> before
>      initialization of the structures.  In this case:
> 
>      qemu_init_vcpu
>          x86_cpu_realizefn
>              cpu_exec_realizefn
>                  cpu_list_add      <---- can be probed by CPU_FOREACH
>              qemu_init_vcpu
>                  cpus_accel->create_vcpu_thread(cpu);
>                      kvm_init_vcpu
>                          map kvm_dirty_gfns  <--- kvm_dirty_gfns valid
> 
>      Don't try to reap dirty ring on vcpus during creation or it'll crash.
> 
> Looking at cpu_list_add() in cpu_exec_realizefn():
> 
> hw/core/cpu-common.c-190-    /* Wait until cpu initialization complete 
> before exposing cpu. */
> hw/core/cpu-common.c:191:    cpu_list_add(cpu);
> 
> IMO the problem is with cpu_list_add(), we shouldn't expose the vCPU
> until it is realized.

that was my reasoning as well

> 
> cpu_list_add() seems to be doing 2 things, auto-assign CPU index if
> UNASSIGNED_CPU_INDEX, then insert to global cpus_queue.
> 
> IIRC cpu_list_add() is called early because various cpu init code
> expects cpu->index to be assigned.
> 
> Maybe we could extract the 'safely assign an unique cpu index' part
> (guarding by qemu_cpu_list_lock), having cpu_list_add() just add to
> the global queue. I'll give it a try...

ok,
lets see if we can postpone cpu_list_add() till the end of realize time.

> 
> >   
> >> I guess we could assert:
> >>
> >>    g_assert(!current_cpu || qemu_cpu_is_self(cpu);
> >>
> >> as current_cpu should only be set as we go into the main thread. However
> >> there is a sketchy setting of current_cpu in cpu_exec() that I'm not
> >> sure should be there. Also do_run_on_cpu() messes with current_cpu in a
> >> way I don't fully understand either.  
> > 
> > I'd rather not rely on that, even if it works it would be subject to
> > to the same kind of breakage.
> > 
> > How about instead of workaround check we would have 2 variants
> > of tlb_flush_by_mmuidx[_async_work], one that have self check
> > and other for usage externally (i.e. from reset handler).
> > That won't have to rely on sketchy globals (which becomes more
> > sketchy in context of Philippes's multi accel work),
> > and it would clearly document what can be used externally.
> >   
> >>  
> >>>
> >>>     
> >>>>> 1)
> >>>>> Fixes: f0aff0f124028 ("cputlb: add assert_cpu_is_self checks")
> >>>>> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> >>>>> ---
> >>>>>   accel/tcg/cputlb.c | 4 ----
> >>>>>   1 file changed, 4 deletions(-)
> >>>>>
> >>>>> diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
> >>>>> index b26c0e088f..2da803103c 100644
> >>>>> --- a/accel/tcg/cputlb.c
> >>>>> +++ b/accel/tcg/cputlb.c
> >>>>> @@ -381,8 +381,6 @@ static void tlb_flush_by_mmuidx_async_work(CPUState *cpu, run_on_cpu_data data)
> >>>>>       uint16_t all_dirty, work, to_clean;
> >>>>>       int64_t now = get_clock_realtime();
> >>>>>   
> >>>>> -    assert_cpu_is_self(cpu);
> >>>>> -
> >>>>>       tlb_debug("mmu_idx:0x%04" PRIx16 "\n", asked);
> >>>>>   
> >>>>>       qemu_spin_lock(&cpu->neg.tlb.c.lock);
> >>>>> @@ -419,8 +417,6 @@ void tlb_flush_by_mmuidx(CPUState *cpu, uint16_t idxmap)
> >>>>>   {
> >>>>>       tlb_debug("mmu_idx: 0x%" PRIx16 "\n", idxmap);
> >>>>>   
> >>>>> -    assert_cpu_is_self(cpu);
> >>>>> -
> >>>>>       tlb_flush_by_mmuidx_async_work(cpu, RUN_ON_CPU_HOST_INT(idxmap));
> >>>>>   }  
> >>>>      
> >>  
> > 
> >   
>