[v3] i386: hvf: Implement CPU kick

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].

mp_cpus_kick() sends an IPI through the host LAPIC to the HVF vCPU.
And the kick interrupt leads to VM exit because "external-interrupt
exiting” VM-execution control is enabled for HVF.

hv_vcpu_interrupt() would have no effect if it's delivered when vCPU is
outside of a guest, therefore to avoid kick loss it's complemented with
a SIG_IPI handler and zero VMX-preemption timer. If the kick happens
outside of hv_vcpu_run(), the signal handler will re-queue the kick by
enabling zero VMX-preemption timer for the next hv_vcpu_run().

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.

Also 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>
---

This is a rework of a kick patch in v2 of the series
(https://lists.gnu.org/archive/html/qemu-devel/2020-06/msg09899.html)
based on the comments from Paolo. Unfortunately the race is still there,
may be the race window is just a bit smaller.

Changes since v2:
 - Reworked workaround to minimize kick loss race. Use signals to
   interrupt vCPU thread outside of hv_vcpu_run() and turn-on/turn-off
   VMX-preemeption timer, while timer value is always zero. v3 also
   assumes that VMX-preemption timer control is always available on the
   hardware that supports HVF.

Changes since v1:
 - Reduced kick loss race (Paolo) and removed SIG_IPI blocking

 cpus.c                 | 13 +++++++----
 include/hw/core/cpu.h  |  2 +-
 include/sysemu/hvf.h   |  1 +
 target/i386/cpu.h      |  1 +
 target/i386/hvf/hvf.c  | 53 ++++++++++++++++++++++++++++++++++++++++--
 target/i386/hvf/vmcs.h |  1 +
 6 files changed, 64 insertions(+), 7 deletions(-)

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.

Paolo

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