Re: [PATCH v3] i386: hvf: Implement CPU kick

[Roman Bolshakov]

On Thu, Jul 02, 2020 at 02:42:45PM +0200, Paolo Bonzini wrote:
> On 02/07/20 12:57, Roman Bolshakov wrote:
> > There's still a small chance of kick loss, on user-to-kernel border
> > between atomic_mb_set's just before the entry to hv_vcpu_run and just
> > after it.
> 
> Good point, but we can fix it.
> 
> > -static void dummy_signal(int sig)
> > +static void hvf_handle_ipi(int sig)
> >  {
> > +    CPUState *cpu = pthread_getspecific(hvf_cpu);
> 
> You can use current_cpu here.  If it's NULL, just return (it's a
> per-thread variable).
> 
> > +    X86CPU *x86_cpu = X86_CPU(cpu);
> > +    CPUX86State *env = &x86_cpu->env;
> > +
> > +    if (!atomic_xchg(&env->hvf_in_guest, false)) {
> 
> Here, thinking more about it, we need not write hvf_in_guest, so:
> 
>       /* Write cpu->exit_request before reading env->hvf_in_guest.  */
>       smp_mb();
>       if (!atomic_read(&env->hvf_in_guest)) {
>               ...
>       }
> 
> > +        wvmcs(cpu->hvf_fd, VMCS_PIN_BASED_CTLS,
> > +              rvmcs(cpu->hvf_fd, VMCS_PIN_BASED_CTLS)
> > +                | VMCS_PIN_BASED_CTLS_VMX_PREEMPT_TIMER);
> > +    }
> >  }
> >  
> >  int hvf_init_vcpu(CPUState *cpu)
> > @@ -631,7 +650,9 @@ int hvf_vcpu_exec(CPUState *cpu)
> >              return EXCP_HLT;
> >          }
> >  
> > +        atomic_mb_set(&env->hvf_in_guest, true);
> >          hv_return_t r  = hv_vcpu_run(cpu->hvf_fd);
> > +        atomic_mb_set(&env->hvf_in_guest, false);
> 
> 
> And here you can do instead:
> 
>       atomic_set(&env->hvf_in_guest, true);
>       /* Read cpu->exit_request after writing env->hvf_in_guest.  */
>       smp_mb();
>       if (atomic_read(&cpu->exit_request)) {
>           qemu_mutex_lock_iothread();
>           atomic_set(&env->hvf_in_guest, false);
>           return EXCP_INTERRUPT;
>       }
>       hv_return_t r  = hv_vcpu_run(cpu->hvf_fd);
>       atomic_store_release(&env->hvf_in_guest, false);
> 
> This matching "write A/smp_mb()/read B" and "write B/smp_mb()/read A" is
> a very common idiom for lock-free signaling between threads.
> 

Hi Paolo,

Thanks for the feedback and the guidelines. I think I've got the idea:

exit_request is the way to record the fact of kick request even if it
was sent outside of hv_vcpu_run().

Best regards,
Roman