KVM: x86: vcpu state writeback should be aware of REQ_NMI

KVM: x86: vcpu state writeback should be aware of REQ_NMI

[Marcelo Tosatti]

Since "Fix race between nmi injection and enabling nmi window", pending NMI 
can be represented in KVM_REQ_NMI vcpu->requests bit. 

When setting vcpu state via SET_VCPU_EVENTS, for example during reset,
the REQ_NMI bit should be cleared otherwise pending NMI is transferred 
to nmi_pending upon vcpu entry.

Also should consider requests bit on runnable conditional.

BZ: http://bugzilla.redhat.com/show_bug.cgi?id=684719

Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index f1e4025..d7f4c4f 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -2677,8 +2677,11 @@ static int kvm_vcpu_ioctl_x86_set_vcpu_events(struct kvm_vcpu *vcpu,
 						  events->interrupt.shadow);
 
 	vcpu->arch.nmi_injected = events->nmi.injected;
-	if (events->flags & KVM_VCPUEVENT_VALID_NMI_PENDING)
+	if (events->flags & KVM_VCPUEVENT_VALID_NMI_PENDING) {
 		vcpu->arch.nmi_pending = events->nmi.pending;
+		if (!vcpu->arch.nmi_pending)
+			clear_bit(KVM_REQ_NMI, &vcpu->requests);
+	}
 	kvm_x86_ops->set_nmi_mask(vcpu, events->nmi.masked);
 
 	if (events->flags & KVM_VCPUEVENT_VALID_SIPI_VECTOR)
@@ -6149,7 +6152,8 @@ int kvm_arch_vcpu_runnable(struct kvm_vcpu *vcpu)
 		!vcpu->arch.apf.halted)
 		|| !list_empty_careful(&vcpu->async_pf.done)
 		|| vcpu->arch.mp_state == KVM_MP_STATE_SIPI_RECEIVED
-		|| vcpu->arch.nmi_pending ||
+		|| vcpu->arch.nmi_pending
+		|| test_bit(KVM_REQ_NMI, &vcpu->requests) ||
 		(kvm_arch_interrupt_allowed(vcpu) &&
 		 kvm_cpu_has_interrupt(vcpu));
 }

Re: KVM: x86: vcpu state writeback should be aware of REQ_NMI

[Avi Kivity]

On 03/24/2011 02:47 PM, Marcelo Tosatti wrote:
> Since "Fix race between nmi injection and enabling nmi window", pending NMI
> can be represented in KVM_REQ_NMI vcpu->requests bit.
>
> When setting vcpu state via SET_VCPU_EVENTS, for example during reset,
> the REQ_NMI bit should be cleared otherwise pending NMI is transferred
> to nmi_pending upon vcpu entry.
>
> Also should consider requests bit on runnable conditional.
>
> BZ: http://bugzilla.redhat.com/show_bug.cgi?id=684719
>
> Signed-off-by: Marcelo Tosatti<mtosatti@redhat.com>
>
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index f1e4025..d7f4c4f 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -2677,8 +2677,11 @@ static int kvm_vcpu_ioctl_x86_set_vcpu_events(struct kvm_vcpu *vcpu,
>   						  events->interrupt.shadow);
>
>   	vcpu->arch.nmi_injected = events->nmi.injected;
> -	if (events->flags&  KVM_VCPUEVENT_VALID_NMI_PENDING)
> +	if (events->flags&  KVM_VCPUEVENT_VALID_NMI_PENDING) {
>   		vcpu->arch.nmi_pending = events->nmi.pending;
> +		if (!vcpu->arch.nmi_pending)
> +			clear_bit(KVM_REQ_NMI,&vcpu->requests);
> +	}
>   	kvm_x86_ops->set_nmi_mask(vcpu, events->nmi.masked);
>
>   	if (events->flags&  KVM_VCPUEVENT_VALID_SIPI_VECTOR)
> @@ -6149,7 +6152,8 @@ int kvm_arch_vcpu_runnable(struct kvm_vcpu *vcpu)
>   		!vcpu->arch.apf.halted)
>   		|| !list_empty_careful(&vcpu->async_pf.done)
>   		|| vcpu->arch.mp_state == KVM_MP_STATE_SIPI_RECEIVED
> -		|| vcpu->arch.nmi_pending ||
> +		|| vcpu->arch.nmi_pending
> +		|| test_bit(KVM_REQ_NMI,&vcpu->requests) ||
>   		(kvm_arch_interrupt_allowed(vcpu)&&
>   		kvm_cpu_has_interrupt(vcpu));
>   }
>

Ouch, right.  But shouldn't we have similar processing when getting vcpu 
events?  Otherwise a pending nmi can be lost during live migration.

-- 
error compiling committee.c: too many arguments to function

Re: KVM: x86: vcpu state writeback should be aware of REQ_NMI

[Gleb Natapov]

On Thu, Mar 24, 2011 at 09:47:00AM -0300, Marcelo Tosatti wrote:
> 
> Since "Fix race between nmi injection and enabling nmi window", pending NMI 
> can be represented in KVM_REQ_NMI vcpu->requests bit. 
> 
> When setting vcpu state via SET_VCPU_EVENTS, for example during reset,
> the REQ_NMI bit should be cleared otherwise pending NMI is transferred 
> to nmi_pending upon vcpu entry.
> 
> Also should consider requests bit on runnable conditional.
> 
> BZ: http://bugzilla.redhat.com/show_bug.cgi?id=684719
> 
Looks like we need to clear request bit on cpu reset too. KVM_REQ_NMI
start to become more complicated that it was initially. May be replaced
it with something like this:


diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 1b8b16a..6a66d19 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -5151,6 +5151,7 @@ static void kvm_put_guest_xcr0(struct kvm_vcpu *vcpu)
 static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
 {
 	int r;
+	int nmi_pending;
 	bool req_int_win = !irqchip_in_kernel(vcpu->kvm) &&
 		vcpu->run->request_interrupt_window;
 
@@ -5188,19 +5189,19 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
 			r = 1;
 			goto out;
 		}
-		if (kvm_check_request(KVM_REQ_NMI, vcpu))
-			vcpu->arch.nmi_pending = true;
 	}
 
 	r = kvm_mmu_reload(vcpu);
 	if (unlikely(r))
 		goto out;
 
+	nmi_pending = vcpu->arch.nmi_pending;
+
 	if (kvm_check_request(KVM_REQ_EVENT, vcpu) || req_int_win) {
 		inject_pending_event(vcpu);
 
 		/* enable NMI/IRQ window open exits if needed */
-		if (vcpu->arch.nmi_pending)
+		if (nmi_pending)
 			kvm_x86_ops->enable_nmi_window(vcpu);
 		else if (kvm_cpu_has_interrupt(vcpu) || req_int_win)
 			kvm_x86_ops->enable_irq_window(vcpu);
> Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>
> 
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index f1e4025..d7f4c4f 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -2677,8 +2677,11 @@ static int kvm_vcpu_ioctl_x86_set_vcpu_events(struct kvm_vcpu *vcpu,
>  						  events->interrupt.shadow);
>  
>  	vcpu->arch.nmi_injected = events->nmi.injected;
> -	if (events->flags & KVM_VCPUEVENT_VALID_NMI_PENDING)
> +	if (events->flags & KVM_VCPUEVENT_VALID_NMI_PENDING) {
>  		vcpu->arch.nmi_pending = events->nmi.pending;
> +		if (!vcpu->arch.nmi_pending)
> +			clear_bit(KVM_REQ_NMI, &vcpu->requests);
> +	}
>  	kvm_x86_ops->set_nmi_mask(vcpu, events->nmi.masked);
>  
>  	if (events->flags & KVM_VCPUEVENT_VALID_SIPI_VECTOR)
> @@ -6149,7 +6152,8 @@ int kvm_arch_vcpu_runnable(struct kvm_vcpu *vcpu)
>  		!vcpu->arch.apf.halted)
>  		|| !list_empty_careful(&vcpu->async_pf.done)
>  		|| vcpu->arch.mp_state == KVM_MP_STATE_SIPI_RECEIVED
> -		|| vcpu->arch.nmi_pending ||
> +		|| vcpu->arch.nmi_pending
> +		|| test_bit(KVM_REQ_NMI, &vcpu->requests) ||
>  		(kvm_arch_interrupt_allowed(vcpu) &&
>  		 kvm_cpu_has_interrupt(vcpu));
>  }
> 

--
			Gleb.

Re: KVM: x86: vcpu state writeback should be aware of REQ_NMI

[Avi Kivity]

On 03/24/2011 03:27 PM, Gleb Natapov wrote:
> On Thu, Mar 24, 2011 at 09:47:00AM -0300, Marcelo Tosatti wrote:
> >
> >  Since "Fix race between nmi injection and enabling nmi window", pending NMI
> >  can be represented in KVM_REQ_NMI vcpu->requests bit.
> >
> >  When setting vcpu state via SET_VCPU_EVENTS, for example during reset,
> >  the REQ_NMI bit should be cleared otherwise pending NMI is transferred
> >  to nmi_pending upon vcpu entry.
> >
> >  Also should consider requests bit on runnable conditional.
> >
> >  BZ: http://bugzilla.redhat.com/show_bug.cgi?id=684719
> >
> Looks like we need to clear request bit on cpu reset too. KVM_REQ_NMI
> start to become more complicated that it was initially. May be replaced
> it with something like this:
>
>
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 1b8b16a..6a66d19 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -5151,6 +5151,7 @@ static void kvm_put_guest_xcr0(struct kvm_vcpu *vcpu)
>   static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
>   {
>   	int r;
> +	int nmi_pending;
>   	bool req_int_win = !irqchip_in_kernel(vcpu->kvm)&&
>   		vcpu->run->request_interrupt_window;
>
> @@ -5188,19 +5189,19 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
>   			r = 1;
>   			goto out;
>   		}
> -		if (kvm_check_request(KVM_REQ_NMI, vcpu))
> -			vcpu->arch.nmi_pending = true;
>   	}
>
>   	r = kvm_mmu_reload(vcpu);
>   	if (unlikely(r))
>   		goto out;
>
> +	nmi_pending = vcpu->arch.nmi_pending;
> +

ACCESS_ONCE() to prevent compiler cleverness

>   	if (kvm_check_request(KVM_REQ_EVENT, vcpu) || req_int_win) {
>   		inject_pending_event(vcpu);

This can use a newer vcpu->arch.nmi_pending.

>
>   		/* enable NMI/IRQ window open exits if needed */
> -		if (vcpu->arch.nmi_pending)
> +		if (nmi_pending)
>   			kvm_x86_ops->enable_nmi_window(vcpu);
>   		else if (kvm_cpu_has_interrupt(vcpu) || req_int_win)
>   			kvm_x86_ops->enable_irq_window(vcpu);

-- 
error compiling committee.c: too many arguments to function

Re: KVM: x86: vcpu state writeback should be aware of REQ_NMI

[Gleb Natapov]

On Thu, Mar 24, 2011 at 03:33:35PM +0200, Avi Kivity wrote:
> On 03/24/2011 03:27 PM, Gleb Natapov wrote:
> >On Thu, Mar 24, 2011 at 09:47:00AM -0300, Marcelo Tosatti wrote:
> >>
> >>  Since "Fix race between nmi injection and enabling nmi window", pending NMI
> >>  can be represented in KVM_REQ_NMI vcpu->requests bit.
> >>
> >>  When setting vcpu state via SET_VCPU_EVENTS, for example during reset,
> >>  the REQ_NMI bit should be cleared otherwise pending NMI is transferred
> >>  to nmi_pending upon vcpu entry.
> >>
> >>  Also should consider requests bit on runnable conditional.
> >>
> >>  BZ: http://bugzilla.redhat.com/show_bug.cgi?id=684719
> >>
> >Looks like we need to clear request bit on cpu reset too. KVM_REQ_NMI
> >start to become more complicated that it was initially. May be replaced
> >it with something like this:
> >
> >
> >diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> >index 1b8b16a..6a66d19 100644
> >--- a/arch/x86/kvm/x86.c
> >+++ b/arch/x86/kvm/x86.c
> >@@ -5151,6 +5151,7 @@ static void kvm_put_guest_xcr0(struct kvm_vcpu *vcpu)
> >  static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
> >  {
> >  	int r;
> >+	int nmi_pending;
> >  	bool req_int_win = !irqchip_in_kernel(vcpu->kvm)&&
> >  		vcpu->run->request_interrupt_window;
> >
> >@@ -5188,19 +5189,19 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
> >  			r = 1;
> >  			goto out;
> >  		}
> >-		if (kvm_check_request(KVM_REQ_NMI, vcpu))
> >-			vcpu->arch.nmi_pending = true;
> >  	}
> >
> >  	r = kvm_mmu_reload(vcpu);
> >  	if (unlikely(r))
> >  		goto out;
> >
> >+	nmi_pending = vcpu->arch.nmi_pending;
> >+
> 
> ACCESS_ONCE() to prevent compiler cleverness
> 
> >  	if (kvm_check_request(KVM_REQ_EVENT, vcpu) || req_int_win) {
> >  		inject_pending_event(vcpu);
> 
> This can use a newer vcpu->arch.nmi_pending.
> 
We can do inject_pending_event(vcpu, nmi_pending); but is this a problem
that newer version of vcpu->arch.nmi_pending will be used? If NMI can be
injected it will - no problem. It it can't it will be delayed till the
next guest entry - no problem NMI is async anyway.

> >
> >  		/* enable NMI/IRQ window open exits if needed */
> >-		if (vcpu->arch.nmi_pending)
> >+		if (nmi_pending)
> >  			kvm_x86_ops->enable_nmi_window(vcpu);
> >  		else if (kvm_cpu_has_interrupt(vcpu) || req_int_win)
> >  			kvm_x86_ops->enable_irq_window(vcpu);
> 
> -- 
> error compiling committee.c: too many arguments to function

--
			Gleb.

Re: KVM: x86: vcpu state writeback should be aware of REQ_NMI

[Marcelo Tosatti]

On Thu, Mar 24, 2011 at 03:27:16PM +0200, Gleb Natapov wrote:
> On Thu, Mar 24, 2011 at 09:47:00AM -0300, Marcelo Tosatti wrote:
> > 
> > Since "Fix race between nmi injection and enabling nmi window", pending NMI 
> > can be represented in KVM_REQ_NMI vcpu->requests bit. 
> > 
> > When setting vcpu state via SET_VCPU_EVENTS, for example during reset,
> > the REQ_NMI bit should be cleared otherwise pending NMI is transferred 
> > to nmi_pending upon vcpu entry.
> > 
> > Also should consider requests bit on runnable conditional.
> > 
> > BZ: http://bugzilla.redhat.com/show_bug.cgi?id=684719
> > 
> Looks like we need to clear request bit on cpu reset too. KVM_REQ_NMI
> start to become more complicated that it was initially. May be replaced
> it with something like this:
> 
> 
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 1b8b16a..6a66d19 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -5151,6 +5151,7 @@ static void kvm_put_guest_xcr0(struct kvm_vcpu *vcpu)
>  static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
>  {
>  	int r;
> +	int nmi_pending;
>  	bool req_int_win = !irqchip_in_kernel(vcpu->kvm) &&
>  		vcpu->run->request_interrupt_window;
>  
> @@ -5188,19 +5189,19 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
>  			r = 1;
>  			goto out;
>  		}
> -		if (kvm_check_request(KVM_REQ_NMI, vcpu))
> -			vcpu->arch.nmi_pending = true;
>  	}
>
>  	r = kvm_mmu_reload(vcpu);
>  	if (unlikely(r))
>  		goto out;
> +	nmi_pending = vcpu->arch.nmi_pending;		
> +
>  	if (kvm_check_request(KVM_REQ_EVENT, vcpu) || req_int_win) {

Yep thats nicer.

Race: remote CPU sets nmi_pending = true here, NMI injection not
allowed, fails to open NMI window when it should. Unless i'm mistaken
this should be rare enough to be irrelevant.

Two patches one to revert REQ_NMI then another to fix the original problem
makes backporting easier.

Re: KVM: x86: vcpu state writeback should be aware of REQ_NMI

[Gleb Natapov]

On Thu, Mar 24, 2011 at 11:59:11AM -0300, Marcelo Tosatti wrote:
> On Thu, Mar 24, 2011 at 03:27:16PM +0200, Gleb Natapov wrote:
> > On Thu, Mar 24, 2011 at 09:47:00AM -0300, Marcelo Tosatti wrote:
> > > 
> > > Since "Fix race between nmi injection and enabling nmi window", pending NMI 
> > > can be represented in KVM_REQ_NMI vcpu->requests bit. 
> > > 
> > > When setting vcpu state via SET_VCPU_EVENTS, for example during reset,
> > > the REQ_NMI bit should be cleared otherwise pending NMI is transferred 
> > > to nmi_pending upon vcpu entry.
> > > 
> > > Also should consider requests bit on runnable conditional.
> > > 
> > > BZ: http://bugzilla.redhat.com/show_bug.cgi?id=684719
> > > 
> > Looks like we need to clear request bit on cpu reset too. KVM_REQ_NMI
> > start to become more complicated that it was initially. May be replaced
> > it with something like this:
> > 
> > 
> > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> > index 1b8b16a..6a66d19 100644
> > --- a/arch/x86/kvm/x86.c
> > +++ b/arch/x86/kvm/x86.c
> > @@ -5151,6 +5151,7 @@ static void kvm_put_guest_xcr0(struct kvm_vcpu *vcpu)
> >  static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
> >  {
> >  	int r;
> > +	int nmi_pending;
> >  	bool req_int_win = !irqchip_in_kernel(vcpu->kvm) &&
> >  		vcpu->run->request_interrupt_window;
> >  
> > @@ -5188,19 +5189,19 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
> >  			r = 1;
> >  			goto out;
> >  		}
> > -		if (kvm_check_request(KVM_REQ_NMI, vcpu))
> > -			vcpu->arch.nmi_pending = true;
> >  	}
> >
> >  	r = kvm_mmu_reload(vcpu);
> >  	if (unlikely(r))
> >  		goto out;
> > +	nmi_pending = vcpu->arch.nmi_pending;		
> > +
> >  	if (kvm_check_request(KVM_REQ_EVENT, vcpu) || req_int_win) {
> 
> Yep thats nicer.
> 
> Race: remote CPU sets nmi_pending = true here, NMI injection not
> allowed, fails to open NMI window when it should. Unless i'm mistaken
> this should be rare enough to be irrelevant.
> 
Yes, that what Avi pointed too. I think the worse thing that could happen
is delaying NMI till next vcpu entry.

> Two patches one to revert REQ_NMI then another to fix the original problem
> makes backporting easier.
If we agree this is the way to go will do that.

--
			Gleb.

Re: KVM: x86: vcpu state writeback should be aware of REQ_NMI

[Avi Kivity]

On 03/24/2011 05:19 PM, Gleb Natapov wrote:
> >  Two patches one to revert REQ_NMI then another to fix the original problem
> >  makes backporting easier.
> If we agree this is the way to go will do that.
>

Not sure, want to think about it for a bit.

-- 
error compiling committee.c: too many arguments to function

Re: KVM: x86: vcpu state writeback should be aware of REQ_NMI

[Marcelo Tosatti]

On Thu, Mar 24, 2011 at 05:35:27PM +0200, Avi Kivity wrote:
> On 03/24/2011 05:19 PM, Gleb Natapov wrote:
> >>  Two patches one to revert REQ_NMI then another to fix the original problem
> >>  makes backporting easier.
> >If we agree this is the way to go will do that.
> >
> 
> Not sure, want to think about it for a bit.

Ping?

Re: KVM: x86: vcpu state writeback should be aware of REQ_NMI

[Avi Kivity]

On 03/29/2011 03:12 PM, Marcelo Tosatti wrote:
> On Thu, Mar 24, 2011 at 05:35:27PM +0200, Avi Kivity wrote:
> >  On 03/24/2011 05:19 PM, Gleb Natapov wrote:
> >  >>   Two patches one to revert REQ_NMI then another to fix the original problem
> >  >>   makes backporting easier.
> >  >If we agree this is the way to go will do that.
> >  >
> >
> >  Not sure, want to think about it for a bit.
>
> Ping?
>

I think Gleb's approach is better. ->requests is fine for notifications 
but not for state.

I'd like to see ACCESS_ONCE() semantics for the vcpu variable, so we 
don't introduce races, even if they are benign.  A problem with benign 
races is that you have to re-think about them later to be sure they are 
still benign.

We also need to document which vcpu variables are asynchronous.

-- 
error compiling committee.c: too many arguments to function