KVM: x86: better fix for race between nmi injection and enabling nmi window (v2)
From: Gleb Natapov
Fix race between nmi injection and enabling nmi window in a simpler way.
Signed-off-by: Marcelo Tosatti
v2: document benign race.
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index bfd7763..e6bdc14 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -5152,6 +5152,7 @@ static void kvm_put_guest_xcr0(struct kvm_vcpu *vcpu)
static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
{
int r;
+ bool nmi_pending;
bool req_int_win = !irqchip_in_kernel(vcpu->kvm) &&
vcpu->run->request_interrupt_window;
@@ -5197,11 +5198,19 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
if (unlikely(r))
goto out;
+ /*
+* An NMI can be injected between local nmi_pending read and
+* vcpu->arch.nmi_pending read inside inject_pending_event().
+* But in that case, KVM_REQ_EVENT will be set, which makes
+* the race described above benign.
+*/
+ nmi_pending = ACCESS_ONCE(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);
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: KVM: x86: better fix for race between nmi injection and enabling nmi window
On Thu, Mar 31, 2011 at 11:25:46AM +0200, Avi Kivity wrote:
> On 03/31/2011 11:24 AM, Gleb Natapov wrote:
> >On Thu, Mar 31, 2011 at 11:23:28AM +0200, Avi Kivity wrote:
> >> On 03/30/2011 08:47 PM, Gleb Natapov wrote:
> >> >On Wed, Mar 30, 2011 at 07:16:34PM +0200, Avi Kivity wrote:
> >> >> On 03/30/2011 06:30 PM, Marcelo Tosatti wrote:
> >> >> >Based on Gleb's idea, fix race between nmi injection and enabling
> >> >> >nmi window in a simpler way.
> >> >> >
> >> >> >Signed-off-by: Marcelo Tosatti
> >> >> >
> >> >> >diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> >> >> >index a6a129f..9a7cc1be 100644
> >> >> >--- a/arch/x86/kvm/x86.c
> >> >> >+++ b/arch/x86/kvm/x86.c
> >> >> >@@ -5152,6 +5152,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;
> >> >> >
> >> >> >@@ -5195,11 +5196,13 @@ static int vcpu_enter_guest(struct kvm_vcpu
> >> *vcpu)
> >> >> >if (unlikely(r))
> >> >> >goto out;
> >> >> >
> >> >> >+ nmi_pending = ACCESS_ONCE(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);
> >> >> >
> >> >>
> >> >> What about the check in inject_pending_events()?
> >> >>
> >> >Didn't we decide that this check is not a problem? Worst that can happen
> >> >is NMI injection will be delayed till next exit.
> >>
> >> Could be very far in the future.
> >>
> >Next host interrupt. But with tickles host and guest yeah.
> >
>
> esp. important with NMI, which may be used in a situation where your
> tick (and everything else) are dead.
>
If host is alive eventually cpu should receive reschedule IPI.
--
Gleb.
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: KVM: x86: better fix for race between nmi injection and enabling nmi window
On Wed, Mar 30, 2011 at 08:47:03PM +0200, Gleb Natapov wrote:
> On Wed, Mar 30, 2011 at 07:16:34PM +0200, Avi Kivity wrote:
> > On 03/30/2011 06:30 PM, Marcelo Tosatti wrote:
> > >Based on Gleb's idea, fix race between nmi injection and enabling
> > >nmi window in a simpler way.
> > >
> > >Signed-off-by: Marcelo Tosatti
> > >
> > >diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> > >index a6a129f..9a7cc1be 100644
> > >--- a/arch/x86/kvm/x86.c
> > >+++ b/arch/x86/kvm/x86.c
> > >@@ -5152,6 +5152,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;
> > >
> > >@@ -5195,11 +5196,13 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
> > > if (unlikely(r))
> > > goto out;
> > >
> > >+ nmi_pending = ACCESS_ONCE(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);
> > >
> >
> > What about the check in inject_pending_events()?
> >
> Didn't we decide that this check is not a problem? Worst that can happen
> is NMI injection will be delayed till next exit.
Yes.
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: KVM: x86: better fix for race between nmi injection and enabling nmi window
On Thu, Mar 31, 2011 at 11:25:46AM +0200, Avi Kivity wrote: > >> >> >else if (kvm_cpu_has_interrupt(vcpu) || > >> req_int_win) > >> >> >kvm_x86_ops->enable_irq_window(vcpu); > >> >> > > >> >> > >> >> What about the check in inject_pending_events()? > >> >> > >> >Didn't we decide that this check is not a problem? Worst that can happen > >> >is NMI injection will be delayed till next exit. > >> > >> Could be very far in the future. > >> > >Next host interrupt. But with tickles host and guest yeah. > > > > esp. important with NMI, which may be used in a situation where your > tick (and everything else) are dead. Well the host must be alive. So eventually NMI will be delivered to the guest, but not closely matching guest visible clocks. What is the problem with that? The race should be rare enough to not interfere with NMI watchdog-like things? -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: KVM: x86: better fix for race between nmi injection and enabling nmi window
On 03/31/2011 11:24 AM, Gleb Natapov wrote:
On Thu, Mar 31, 2011 at 11:23:28AM +0200, Avi Kivity wrote:
> On 03/30/2011 08:47 PM, Gleb Natapov wrote:
> >On Wed, Mar 30, 2011 at 07:16:34PM +0200, Avi Kivity wrote:
> >> On 03/30/2011 06:30 PM, Marcelo Tosatti wrote:
> >> >Based on Gleb's idea, fix race between nmi injection and enabling
> >> >nmi window in a simpler way.
> >> >
> >> >Signed-off-by: Marcelo Tosatti
> >> >
> >> >diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> >> >index a6a129f..9a7cc1be 100644
> >> >--- a/arch/x86/kvm/x86.c
> >> >+++ b/arch/x86/kvm/x86.c
> >> >@@ -5152,6 +5152,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;
> >> >
> >> >@@ -5195,11 +5196,13 @@ static int vcpu_enter_guest(struct kvm_vcpu
*vcpu)
> >> > if (unlikely(r))
> >> > goto out;
> >> >
> >> >+ nmi_pending = ACCESS_ONCE(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);
> >> >
> >>
> >> What about the check in inject_pending_events()?
> >>
> >Didn't we decide that this check is not a problem? Worst that can happen
> >is NMI injection will be delayed till next exit.
>
> Could be very far in the future.
>
Next host interrupt. But with tickles host and guest yeah.
esp. important with NMI, which may be used in a situation where your
tick (and everything else) are dead.
--
error compiling committee.c: too many arguments to function
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: KVM: x86: better fix for race between nmi injection and enabling nmi window
On Thu, Mar 31, 2011 at 11:23:28AM +0200, Avi Kivity wrote:
> On 03/30/2011 08:47 PM, Gleb Natapov wrote:
> >On Wed, Mar 30, 2011 at 07:16:34PM +0200, Avi Kivity wrote:
> >> On 03/30/2011 06:30 PM, Marcelo Tosatti wrote:
> >> >Based on Gleb's idea, fix race between nmi injection and enabling
> >> >nmi window in a simpler way.
> >> >
> >> >Signed-off-by: Marcelo Tosatti
> >> >
> >> >diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> >> >index a6a129f..9a7cc1be 100644
> >> >--- a/arch/x86/kvm/x86.c
> >> >+++ b/arch/x86/kvm/x86.c
> >> >@@ -5152,6 +5152,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;
> >> >
> >> >@@ -5195,11 +5196,13 @@ static int vcpu_enter_guest(struct kvm_vcpu
> >> *vcpu)
> >> > if (unlikely(r))
> >> > goto out;
> >> >
> >> >+nmi_pending = ACCESS_ONCE(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);
> >> >
> >>
> >> What about the check in inject_pending_events()?
> >>
> >Didn't we decide that this check is not a problem? Worst that can happen
> >is NMI injection will be delayed till next exit.
>
> Could be very far in the future.
>
Next host interrupt. But with tickles host and guest yeah.
--
Gleb.
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: KVM: x86: better fix for race between nmi injection and enabling nmi window
On 03/30/2011 08:47 PM, Gleb Natapov wrote:
On Wed, Mar 30, 2011 at 07:16:34PM +0200, Avi Kivity wrote:
> On 03/30/2011 06:30 PM, Marcelo Tosatti wrote:
> >Based on Gleb's idea, fix race between nmi injection and enabling
> >nmi window in a simpler way.
> >
> >Signed-off-by: Marcelo Tosatti
> >
> >diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> >index a6a129f..9a7cc1be 100644
> >--- a/arch/x86/kvm/x86.c
> >+++ b/arch/x86/kvm/x86.c
> >@@ -5152,6 +5152,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;
> >
> >@@ -5195,11 +5196,13 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
> > if (unlikely(r))
> > goto out;
> >
> >+ nmi_pending = ACCESS_ONCE(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);
> >
>
> What about the check in inject_pending_events()?
>
Didn't we decide that this check is not a problem? Worst that can happen
is NMI injection will be delayed till next exit.
Could be very far in the future.
--
error compiling committee.c: too many arguments to function
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: KVM: x86: better fix for race between nmi injection and enabling nmi window
On Wed, Mar 30, 2011 at 07:16:34PM +0200, Avi Kivity wrote:
> On 03/30/2011 06:30 PM, Marcelo Tosatti wrote:
> >Based on Gleb's idea, fix race between nmi injection and enabling
> >nmi window in a simpler way.
> >
> >Signed-off-by: Marcelo Tosatti
> >
> >diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> >index a6a129f..9a7cc1be 100644
> >--- a/arch/x86/kvm/x86.c
> >+++ b/arch/x86/kvm/x86.c
> >@@ -5152,6 +5152,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;
> >
> >@@ -5195,11 +5196,13 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
> > if (unlikely(r))
> > goto out;
> >
> >+nmi_pending = ACCESS_ONCE(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);
> >
>
> What about the check in inject_pending_events()?
>
Didn't we decide that this check is not a problem? Worst that can happen
is NMI injection will be delayed till next exit.
--
Gleb.
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: KVM: x86: better fix for race between nmi injection and enabling nmi window
On 03/30/2011 06:30 PM, Marcelo Tosatti wrote:
Based on Gleb's idea, fix race between nmi injection and enabling
nmi window in a simpler way.
Signed-off-by: Marcelo Tosatti
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index a6a129f..9a7cc1be 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -5152,6 +5152,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;
@@ -5195,11 +5196,13 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
if (unlikely(r))
goto out;
+ nmi_pending = ACCESS_ONCE(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);
What about the check in inject_pending_events()?
--
error compiling committee.c: too many arguments to function
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: KVM: x86: better fix for race between nmi injection and enabling nmi window
On Wed, Mar 30, 2011 at 06:33:22PM +0200, Gleb Natapov wrote: > On Wed, Mar 30, 2011 at 01:30:28PM -0300, Marcelo Tosatti wrote: > > > > Based on Gleb's idea, fix race between nmi injection and enabling > > nmi window in a simpler way. > > > > Signed-off-by: Marcelo Tosatti > > > But we need to revert the patch that introduced use of request for NMI > first. Otherwise NMI will not be delivered, no? Yes, pushed, can you verify please? -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: KVM: x86: better fix for race between nmi injection and enabling nmi window
On Wed, Mar 30, 2011 at 01:30:28PM -0300, Marcelo Tosatti wrote:
>
> Based on Gleb's idea, fix race between nmi injection and enabling
> nmi window in a simpler way.
>
> Signed-off-by: Marcelo Tosatti
>
But we need to revert the patch that introduced use of request for NMI
first. Otherwise NMI will not be delivered, no?
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index a6a129f..9a7cc1be 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -5152,6 +5152,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;
>
> @@ -5195,11 +5196,13 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
> if (unlikely(r))
> goto out;
>
> + nmi_pending = ACCESS_ONCE(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);
--
Gleb.
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
KVM: x86: better fix for race between nmi injection and enabling nmi window
Based on Gleb's idea, fix race between nmi injection and enabling
nmi window in a simpler way.
Signed-off-by: Marcelo Tosatti
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index a6a129f..9a7cc1be 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -5152,6 +5152,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;
@@ -5195,11 +5196,13 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
if (unlikely(r))
goto out;
+ nmi_pending = ACCESS_ONCE(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);
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
