Re: [PATCH 4/8] i386: hvf: Implement CPU kick

[Roman Bolshakov <r.bolshakov@yadro.com>]

On Thu, Jun 25, 2020 at 09:07:04AM +0200, Claudio Fontana wrote:
> Hi Roman,
> 
> On 6/25/20 12:58 AM, Roman Bolshakov wrote:
> > HVF doesn't have a CPU kick and without it it's not possible to perform
> > an action on CPU thread until a VMEXIT happens. The kick is also needed
> > for timely interrupt delivery.
> > 
> > Existing implementation of CPU kick sends SIG_IPI (aka SIGUSR1) to vCPU
> > thread, but it's different from what hv_vcpu_interrupt does. The latter
> > one results in invocation of mp_cpus_kick() in XNU kernel [1].
> > 
> > While at it, correct type of hvf_fd to the type of hv_vcpuid_t to avoid
> > compilation warnings.
> > 
> > 1. https://opensource.apple.com/source/xnu/xnu-6153.81.5/osfmk/i386/mp.c
> > 
> > Cc: Cameron Esfahani <dirty@apple.com>
> > Signed-off-by: Roman Bolshakov <r.bolshakov@yadro.com>
> > ---
> >  cpus.c                | 13 +++++++++----
> >  include/hw/core/cpu.h |  2 +-
> >  include/sysemu/hvf.h  |  1 +
> >  target/i386/hvf/hvf.c | 11 +++++++++++
> >  4 files changed, 22 insertions(+), 5 deletions(-)
> > 
> > diff --git a/cpus.c b/cpus.c
> > index 26709677d3..36f38ce5c8 100644
> > --- a/cpus.c
> > +++ b/cpus.c
> > @@ -1783,10 +1783,15 @@ static void qemu_cpu_kick_thread(CPUState *cpu)
> >          return;
> >      }
> >      cpu->thread_kicked = true;
> > -    err = pthread_kill(cpu->thread->thread, SIG_IPI);
> > -    if (err && err != ESRCH) {
> > -        fprintf(stderr, "qemu:%s: %s", __func__, strerror(err));
> > -        exit(1);
> > +
> > +    if (hvf_enabled()) {
> > +        hvf_vcpu_kick(cpu);
> 
> could this be moved to qemu_cpu_kick, where we have already the ifs for accelerator types tcg and hax?
> 

Hi Claudio,

I did this because of cpu->thread_kicked which is not set or tested in
qemu_cpu_kick(). It's not used for tcg and mttcg but hax does seem to
use the qemu_cpu_kick_thread() and additionally sets cpu->exit_request
in qemu_cpu_kick(). There's a difference between hax/kvm and hvf, they
use different ways of siginalling the kick. hax/kvm use POSIX signals
while HVF sends an IPI from the host LAPIC to deliver the kick. The
patch highlights the difference.

As far as I understand if thread_kicked is set, multiple cpu kicks are
coalesced until thread_kicked is cleared. So, the answer to your
question: It could be moved to qemu_cpu_kick but then kick debouncing
should be duplicated inside hvf_vcpu_kick().

Regards,
Roman

> Not terribly important if then my cpus-refactoring goes forward, but on its own that should be the proper place for if (hvf_enabled()) I think.
> 
> 
> 
> > +    } else {
> > +        err = pthread_kill(cpu->thread->thread, SIG_IPI);
> > +        if (err && err != ESRCH) {
> > +            fprintf(stderr, "qemu:%s: %s", __func__, strerror(err));
> > +            exit(1);
> > +        }
> >      }
> >  #else /* _WIN32 */
> >      if (!qemu_cpu_is_self(cpu)) {