From: Phil Dennis-Jordan <phil@philjordan.eu>

This seems to be entirely superfluous and is costly enough to show up in
profiling. hv_vcpu_interrupt() has been demonstrated to very reliably
cause VM exits - even if the target vCPU isn't even running, it will
immediately exit on entry.

Signed-off-by: Phil Dennis-Jordan <phil@philjordan.eu>
---
 target/i386/hvf/hvf.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/i386/hvf/hvf.c b/target/i386/hvf/hvf.c
index 3b6ee79fb2..936c31dbdd 100644
--- a/target/i386/hvf/hvf.c
+++ b/target/i386/hvf/hvf.c
@@ -214,7 +214,7 @@ static inline bool apic_bus_freq_is_known(CPUX86State *env)
 
 void hvf_kick_vcpu_thread(CPUState *cpu)
 {
-    cpus_kick_thread(cpu);
+    cpu->thread_kicked = true;
     hv_vcpu_interrupt(&cpu->accel->fd, 1);
 }
 
-- 
2.39.3 (Apple Git-146)

On 9/12/24 21:36, phil@philjordan.eu wrote:
> From: Phil Dennis-Jordan <phil@philjordan.eu>
> 
> This seems to be entirely superfluous and is costly enough to show up in

So the pthread_kill(cpu->thread, SIG_IPI) is entirely superfluous?

> profiling. hv_vcpu_interrupt() has been demonstrated to very reliably
> cause VM exits - even if the target vCPU isn't even running, it will
> immediately exit on entry.
> 
> Signed-off-by: Phil Dennis-Jordan <phil@philjordan.eu>
> ---
>   target/i386/hvf/hvf.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/target/i386/hvf/hvf.c b/target/i386/hvf/hvf.c
> index 3b6ee79fb2..936c31dbdd 100644
> --- a/target/i386/hvf/hvf.c
> +++ b/target/i386/hvf/hvf.c
> @@ -214,7 +214,7 @@ static inline bool apic_bus_freq_is_known(CPUX86State *env)
>   
>   void hvf_kick_vcpu_thread(CPUState *cpu)
>   {
> -    cpus_kick_thread(cpu);
> +    cpu->thread_kicked = true;
>       hv_vcpu_interrupt(&cpu->accel->fd, 1);
>   }
>

On 10.12.2024 04:22, Philippe Mathieu-Daudé wrote:
> On 9/12/24 21:36, phil@philjordan.eu wrote:
>> From: Phil Dennis-Jordan <phil@philjordan.eu>
>>
>> This seems to be entirely superfluous and is costly enough to show up in
>
> So the pthread_kill(cpu->thread, SIG_IPI) is entirely superfluous?
>
>> profiling. hv_vcpu_interrupt() has been demonstrated to very reliably
>> cause VM exits - even if the target vCPU isn't even running, it will
>> immediately exit on entry.
>>
>> Signed-off-by: Phil Dennis-Jordan <phil@philjordan.eu>
>> ---
>>   target/i386/hvf/hvf.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/target/i386/hvf/hvf.c b/target/i386/hvf/hvf.c
>> index 3b6ee79fb2..936c31dbdd 100644
>> --- a/target/i386/hvf/hvf.c
>> +++ b/target/i386/hvf/hvf.c
>> @@ -214,7 +214,7 @@ static inline bool 
>> apic_bus_freq_is_known(CPUX86State *env)
>>     void hvf_kick_vcpu_thread(CPUState *cpu)
>>   {
>> -    cpus_kick_thread(cpu);
>> +    cpu->thread_kicked = true;
>>       hv_vcpu_interrupt(&cpu->accel->fd, 1);
>>   }
>
SIG_IPI is macOS crutch handled in XNU kernel that was essential until 
Phil submitted proper kick support with hv_vcpu_interrupt().

On Tue 10. Dec 2024 at 10:21, Roman Bolshakov <rbolshakov@ddn.com> wrote:

> On 10.12.2024 04:22, Philippe Mathieu-Daudé wrote:
> > On 9/12/24 21:36, phil@philjordan.eu wrote:
> >> From: Phil Dennis-Jordan <phil@philjordan.eu>
> >>
> >> This seems to be entirely superfluous and is costly enough to show up in
> >
> > So the pthread_kill(cpu->thread, SIG_IPI) is entirely superfluous?
> >
> >> profiling. hv_vcpu_interrupt() has been demonstrated to very reliably
> >> cause VM exits - even if the target vCPU isn't even running, it will
> >> immediately exit on entry.
> >>
> >> Signed-off-by: Phil Dennis-Jordan <phil@philjordan.eu>
> >> ---
> >>   target/i386/hvf/hvf.c | 2 +-
> >>   1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/target/i386/hvf/hvf.c b/target/i386/hvf/hvf.c
> >> index 3b6ee79fb2..936c31dbdd 100644
> >> --- a/target/i386/hvf/hvf.c
> >> +++ b/target/i386/hvf/hvf.c
> >> @@ -214,7 +214,7 @@ static inline bool
> >> apic_bus_freq_is_known(CPUX86State *env)
> >>     void hvf_kick_vcpu_thread(CPUState *cpu)
> >>   {
> >> -    cpus_kick_thread(cpu);
> >> +    cpu->thread_kicked = true;
> >>       hv_vcpu_interrupt(&cpu->accel->fd, 1);
> >>   }
> >
> SIG_IPI is macOS crutch handled in XNU kernel that was essential until
> Phil submitted proper kick support with hv_vcpu_interrupt().
>
> Ah yes, perhaps it allowed exit from hv_vcpu_run(). hv_vcpu_run_until()
definitely does not exit early upon receiving SIG_IPI (USR1).

On 10/12/24 10:52, Phil Dennis-Jordan wrote:
> 
> 
> On Tue 10. Dec 2024 at 10:21, Roman Bolshakov <rbolshakov@ddn.com 
> <mailto:rbolshakov@ddn.com>> wrote:
> 
>     On 10.12.2024 04:22, Philippe Mathieu-Daudé wrote:
>      > On 9/12/24 21:36, phil@philjordan.eu <mailto:phil@philjordan.eu>
>     wrote:
>      >> From: Phil Dennis-Jordan <phil@philjordan.eu
>     <mailto:phil@philjordan.eu>>
>      >>
>      >> This seems to be entirely superfluous and is costly enough to
>     show up in
>      >
>      > So the pthread_kill(cpu->thread, SIG_IPI) is entirely superfluous?
>      >
>      >> profiling. hv_vcpu_interrupt() has been demonstrated to very
>     reliably
>      >> cause VM exits - even if the target vCPU isn't even running, it will
>      >> immediately exit on entry.
>      >>
>      >> Signed-off-by: Phil Dennis-Jordan <phil@philjordan.eu
>     <mailto:phil@philjordan.eu>>
>      >> ---
>      >>   target/i386/hvf/hvf.c | 2 +-
>      >>   1 file changed, 1 insertion(+), 1 deletion(-)
>      >>
>      >> diff --git a/target/i386/hvf/hvf.c b/target/i386/hvf/hvf.c
>      >> index 3b6ee79fb2..936c31dbdd 100644
>      >> --- a/target/i386/hvf/hvf.c
>      >> +++ b/target/i386/hvf/hvf.c
>      >> @@ -214,7 +214,7 @@ static inline bool
>      >> apic_bus_freq_is_known(CPUX86State *env)
>      >>     void hvf_kick_vcpu_thread(CPUState *cpu)
>      >>   {
>      >> -    cpus_kick_thread(cpu);
>      >> +    cpu->thread_kicked = true;
>      >>       hv_vcpu_interrupt(&cpu->accel->fd, 1);
>      >>   }
>      >
>     SIG_IPI is macOS crutch handled in XNU kernel that was essential until
>     Phil submitted proper kick support with hv_vcpu_interrupt().
> 
> Ah yes, perhaps it allowed exit from hv_vcpu_run(). hv_vcpu_run_until() 
> definitely does not exit early upon receiving SIG_IPI (USR1).

Maybe worth explaining and mentioning commit bf9bf2306cc ("i386/hvf:
In kick_vcpu use hv_vcpu_interrupt to force exit") in this patch
description?

On Mon 9. Dec 2024 at 22:22, Philippe Mathieu-Daudé <philmd@linaro.org>
wrote:

> On 9/12/24 21:36, phil@philjordan.eu wrote:
> > From: Phil Dennis-Jordan <phil@philjordan.eu>
> >
> > This seems to be entirely superfluous and is costly enough to show up in
>
> So the pthread_kill(cpu->thread, SIG_IPI) is entirely superfluous?
>

Yes, that is my conclusion after looking at the code and testing without
the signal sending. (I am only talking about HVF/x86 here!)

By my understanding, the HVF vCPU thread is always in one of 3 states:
- running hv_vcpu_run_until (hv_vcpu_interrupt() forces the transition out
of this, even in edge cases)
- waiting in qemu_wait_io_event In the main  hvf_cpu_thread_fn loop
(signalling the condition variable will bring it out of this)
- actively processing a VM exit, I/O request, etc (whatever it’s doing
can’t be interrupted, but it will make progress and check its job queue on
completion)

So I can’t see any purpose the signal is supposed to be serving. I mean,
maybe I’ve missed something important - always happy to be corrected and
learn something new. :-)

I’ll still need to investigate if the arm64 version is also doing this
unnecessarily and whether we can remove the signal handler entirely.


> > profiling. hv_vcpu_interrupt() has been demonstrated to very reliably
> > cause VM exits - even if the target vCPU isn't even running, it will
> > immediately exit on entry.
> >
> > Signed-off-by: Phil Dennis-Jordan <phil@philjordan.eu>
> > ---
> >   target/i386/hvf/hvf.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/target/i386/hvf/hvf.c b/target/i386/hvf/hvf.c
> > index 3b6ee79fb2..936c31dbdd 100644
> > --- a/target/i386/hvf/hvf.c
> > +++ b/target/i386/hvf/hvf.c
> > @@ -214,7 +214,7 @@ static inline bool
> apic_bus_freq_is_known(CPUX86State *env)
> >
> >   void hvf_kick_vcpu_thread(CPUState *cpu)
> >   {
> > -    cpus_kick_thread(cpu);
> > +    cpu->thread_kicked = true;
> >       hv_vcpu_interrupt(&cpu->accel->fd, 1);
> >   }
> >
>
>