Re: [PATCH 02/16] cpus: Improve qemu_cpu_kick_self() docstring

On 17/8/26 17:28, Paolo Bonzini wrote:
> On 8/13/26 20:16, Philippe Mathieu-Daudé wrote:
>> Be a bit more descriptive than "Unblock cpu" :)
>>
>> Signed-off-by: Philippe Mathieu-Daudé <[email protected]>
>> ---
>>   include/system/cpus.h | 10 +++++++++-
>>   1 file changed, 9 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/system/cpus.h b/include/system/cpus.h
>> index 508444ccf1c..ade13e068eb 100644
>> --- a/include/system/cpus.h
>> +++ b/include/system/cpus.h
>> @@ -30,7 +30,15 @@ void resume_all_vcpus(void);
>>   void pause_all_vcpus(void);
>>   void cpu_stop_current(void);
>> -/* Unblock cpu */
>> +/**
>> + * qemu_cpu_kick_self - Force vCPU to re-enter to its inner main loop
>> + *
>> + * Signal the current vCPU thread to exit any blocking operations and
>> + * re-enter its inner execution loop to process pending requests,
>> + * possibly returning to its outer execution loop.
>> + *
>> + * Must be called from within the vCPU thread itself.
>> + */
> 
> Neither is correct.

"Must be called from within the vCPU thread itself." is because of
the assertion:

system/cpus.c:452:void qemu_cpu_kick_self(void)
system/cpus.c-453-{
system/cpus.c-454-    assert(current_cpu);
system/cpus.c-455-    cpus_kick_thread(current_cpu);
system/cpus.c-456-}

>  What it does is force the vCPU not to enter the
> accelerator, and immediately exit again to process events.  This is only
> needed because KVM doesn't want you to process events because KVM allows
> you to do so.

I got confused by this comment in accel/mshv/mshv-all.c and interpreted
too much:

/*
  * The signal handler is triggered when QEMU's main thread receives a 
SIG_IPI
  * (SIGUSR1). This signal causes the current CPU thread to be kicked, 
forcing a
  * VM exit on the CPU. The VM exit generates an exit reason that breaks 
the loop
  * (see mshv_cpu_exec). If the exit is due to a Ctrl+A+x command, the 
system
  * will shut down. For other cases, the system will continue running.
  */
static void sa_ipi_handler(int sig)
{
     /* TODO: call IOCTL to set_immediate_exit, once implemented. */

     qemu_cpu_kick_self();
}
> 
> I'd rather have something like:
> 
> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
> index 83cbd120a84..e5d068e341f 100644
> --- a/accel/kvm/kvm-all.c
> +++ b/accel/kvm/kvm-all.c
> @@ -3292,7 +3292,7 @@ static void kvm_cpu_kick_self(void)
>       if (kvm_immediate_exit) {
>           kvm_cpu_kick(current_cpu);
>       } else {
> -        qemu_cpu_kick_self();
> +        cpus_kick_thread(current_cpu);
>       }
>   }
> 
> diff --git a/target/i386/nvmm/nvmm-all.c b/target/i386/nvmm/nvmm-all.c
> index 8a1af35ed32..b9b1ee39850 100644
> --- a/target/i386/nvmm/nvmm-all.c
> +++ b/target/i386/nvmm/nvmm-all.c
> @@ -753,7 +753,7 @@ nvmm_vcpu_loop(CPUState *cpu)
>   #if NVMM_USER_VERSION >= 2
>               nvmm_vcpu_stop(vcpu);
>   #else
> -            qemu_cpu_kick_self();
> +            cpus_kick_thread(current_cpu);
>   #endif
>           }
> 
> and get rid of qemu_cpu_kick_self() completely.

OK I'll take that route.