[PATCH 0/1] Disable GUEST_INTR_STATE_STI flag before injecting NMI to VMX guest

[PATCH 0/1] Disable GUEST_INTR_STATE_STI flag before injecting NMI to VMX guest

[Jes.Sorensen]

From: Jes Sorensen <Jes.Sorensen@redhat.com>

In certain setups, especially with the RHEL5 kernel, the guest would
exit with an invalid state and die if one added nmi_watchdog=1 as a
kernel boot parameter.

Gleb gets the credit for the thinking, I just did the testing and
tracing on this one.

Jes Sorensen (1):
  Disable GUEST_INTR_STATE_STI flag before injecting NMI to guest on
    VMX

 arch/x86/kvm/vmx.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

[PATCH 1/1] Disable GUEST_INTR_STATE_STI flag before injecting NMI to guest on VMX

[Jes.Sorensen]

From: Jes Sorensen <Jes.Sorensen@redhat.com>

Injecting an NMI while GUEST_INTR_STATE_STI is set may fail,
which can cause an EXIT with invalid state, resulting in the
guest dieing.

Credit to Gleb for figuring out why it was failing and how to
fix it.

Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
Signed-off-by: Gleb Natapov <gleb@redhat.com>
---
 arch/x86/kvm/vmx.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index cf56462..8e95371 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -2888,6 +2888,8 @@ static void vmx_inject_nmi(struct kvm_vcpu *vcpu)
 		kvm_rip_write(vcpu, vmx->rmode.irq.rip - 1);
 		return;
 	}
+	vmcs_write32(GUEST_INTERRUPTIBILITY_INFO,
+			vmcs_read32(GUEST_INTERRUPTIBILITY_INFO) & ~GUEST_INTR_STATE_STI);
 	vmcs_write32(VM_ENTRY_INTR_INFO_FIELD,
 			INTR_TYPE_NMI_INTR | INTR_INFO_VALID_MASK | NMI_VECTOR);
 }
-- 
1.7.1

Re: [PATCH 1/1] Disable GUEST_INTR_STATE_STI flag before injecting NMI to guest on VMX

[Jan Kiszka]

Am 26.08.2010 22:06, Jes.Sorensen@redhat.com wrote:
> From: Jes Sorensen <Jes.Sorensen@redhat.com>
> 
> Injecting an NMI while GUEST_INTR_STATE_STI is set may fail,
> which can cause an EXIT with invalid state, resulting in the
> guest dieing.

Very interesting. Reality obviously doesn't bother about the statement
of the vendor [1].

Just curious: is this limited to specific CPU models or actually a
generic issue?

> 
> Credit to Gleb for figuring out why it was failing and how to
> fix it.
> 
> Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
> Signed-off-by: Gleb Natapov <gleb@redhat.com>
> ---
>  arch/x86/kvm/vmx.c |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index cf56462..8e95371 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -2888,6 +2888,8 @@ static void vmx_inject_nmi(struct kvm_vcpu *vcpu)
>  		kvm_rip_write(vcpu, vmx->rmode.irq.rip - 1);
>  		return;
>  	}
> +	vmcs_write32(GUEST_INTERRUPTIBILITY_INFO,
> +			vmcs_read32(GUEST_INTERRUPTIBILITY_INFO) & ~GUEST_INTR_STATE_STI);
>  	vmcs_write32(VM_ENTRY_INTR_INFO_FIELD,
>  			INTR_TYPE_NMI_INTR | INTR_INFO_VALID_MASK | NMI_VECTOR);
>  }

Thinking about the implications: Independent of virtualization, this
means that no code code can in any way rely on the STI shadow if there
are NMIs present that could "consume" it. Because after return from
those NMIs, interrupts could then be injected on the instruction that
was originally under the shadow.

Jan

[1] http://thread.gmane.org/gmane.comp.emulators.kvm.devel/52144

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [PATCH 1/1] Disable GUEST_INTR_STATE_STI flag before injecting NMI to guest on VMX

[Jes Sorensen]

On 08/27/10 10:27, Jan Kiszka wrote:
> Am 26.08.2010 22:06, Jes.Sorensen@redhat.com wrote:
>> From: Jes Sorensen <Jes.Sorensen@redhat.com>
>>
>> Injecting an NMI while GUEST_INTR_STATE_STI is set may fail,
>> which can cause an EXIT with invalid state, resulting in the
>> guest dieing.
> 
> Very interesting. Reality obviously doesn't bother about the statement
> of the vendor [1].
> 
> Just curious: is this limited to specific CPU models or actually a
> generic issue?

I have been able to reproduce this on a core2 and a nehalem box, so I
think it is at least somewhat generic on Intel. Not sure if it can be
reproduced on AMD.

It also seems to be specific to which guest kernel I run, so timing may
play a part as well.

Cheers,
Jes

Re: [PATCH 1/1] Disable GUEST_INTR_STATE_STI flag before injecting NMI to guest on VMX

[Jan Kiszka]

Jes Sorensen wrote:
> On 08/27/10 10:27, Jan Kiszka wrote:
>> Am 26.08.2010 22:06, Jes.Sorensen@redhat.com wrote:
>>> From: Jes Sorensen <Jes.Sorensen@redhat.com>
>>>
>>> Injecting an NMI while GUEST_INTR_STATE_STI is set may fail,
>>> which can cause an EXIT with invalid state, resulting in the
>>> guest dieing.
>> Very interesting. Reality obviously doesn't bother about the statement
>> of the vendor [1].
>>
>> Just curious: is this limited to specific CPU models or actually a
>> generic issue?
> 
> I have been able to reproduce this on a core2 and a nehalem box, so I
> think it is at least somewhat generic on Intel. Not sure if it can be
> reproduced on AMD.

AMD does not differentiate between MOV-SS and STI interrupt shadows.

But AMD has its own NMI problems as it does not allow to trap after
IRET-from-NMI and requires magic dances which are partly broken in KVM.
I'm leaning towards NMI window emulation via the workaround we use for
older Intel CPUs without NMI window trapping as well.

> 
> It also seems to be specific to which guest kernel I run, so timing may
> play a part as well.

For sure, the critical window is one instruction wide...

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [PATCH 1/1] Disable GUEST_INTR_STATE_STI flag before injecting NMI to guest on VMX

[Avi Kivity]

  On 08/27/2010 11:39 AM, Jan Kiszka wrote:
>
> AMD does not differentiate between MOV-SS and STI interrupt shadows.
>
> But AMD has its own NMI problems as it does not allow to trap after
> IRET-from-NMI and requires magic dances which are partly broken in KVM.
> I'm leaning towards NMI window emulation via the workaround we use for
> older Intel CPUs without NMI window trapping as well.

Can you elaborate?

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

Re: [PATCH 1/1] Disable GUEST_INTR_STATE_STI flag before injecting NMI to guest on VMX

[Jan Kiszka]

Avi Kivity wrote:
>   On 08/27/2010 11:39 AM, Jan Kiszka wrote:
>> AMD does not differentiate between MOV-SS and STI interrupt shadows.
>>
>> But AMD has its own NMI problems as it does not allow to trap after
>> IRET-from-NMI and requires magic dances which are partly broken in KVM.
>> I'm leaning towards NMI window emulation via the workaround we use for
>> older Intel CPUs without NMI window trapping as well.
> 
> Can you elaborate?

Basically the issues you found regarding our single-step-based
workaround. Moreover, we cannot easily prevent that TF set by the NMI
code leaks onto the guest's stack.

Jörg and I stuck heads together about this during LinuxCon. We came to
the conclusion that we either have to emulate the instruction that
delays NMIs (ie. _every_ possible instruction for the interrupt shadow
case) or fall back to the VMX workaround based on interrupt window
trapping and an emergency timeout (much simpler, but not really correct).

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [PATCH 1/1] Disable GUEST_INTR_STATE_STI flag before injecting NMI to guest on VMX

[Avi Kivity]

  On 08/27/2010 02:06 PM, Jan Kiszka wrote:
> Avi Kivity wrote:
>>    On 08/27/2010 11:39 AM, Jan Kiszka wrote:
>>> AMD does not differentiate between MOV-SS and STI interrupt shadows.
>>>
>>> But AMD has its own NMI problems as it does not allow to trap after
>>> IRET-from-NMI and requires magic dances which are partly broken in KVM.
>>> I'm leaning towards NMI window emulation via the workaround we use for
>>> older Intel CPUs without NMI window trapping as well.
>> Can you elaborate?
> Basically the issues you found regarding our single-step-based
> workaround.

I forgot them already.  What was that, exception during IRET?

> Moreover, we cannot easily prevent that TF set by the NMI
> code leaks onto the guest's stack.
>
> Jörg and I stuck heads together about this during LinuxCon. We came to
> the conclusion that we either have to emulate the instruction that
> delays NMIs (ie. _every_ possible instruction for the interrupt shadow
> case) or fall back to the VMX workaround based on interrupt window
> trapping and an emergency timeout (much simpler, but not really correct).

I'd really like to avoid the timer.  But I forgot all the details around 
this, I'll have to re-learn them so I can actually compare the two options.

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

Re: [PATCH 1/1] Disable GUEST_INTR_STATE_STI flag before injecting NMI to guest on VMX

[Jan Kiszka]

Avi Kivity wrote:
>   On 08/27/2010 02:06 PM, Jan Kiszka wrote:
>> Avi Kivity wrote:
>>>    On 08/27/2010 11:39 AM, Jan Kiszka wrote:
>>>> AMD does not differentiate between MOV-SS and STI interrupt shadows.
>>>>
>>>> But AMD has its own NMI problems as it does not allow to trap after
>>>> IRET-from-NMI and requires magic dances which are partly broken in KVM.
>>>> I'm leaning towards NMI window emulation via the workaround we use for
>>>> older Intel CPUs without NMI window trapping as well.
>>> Can you elaborate?
>> Basically the issues you found regarding our single-step-based
>> workaround.
> 
> I forgot them already.  What was that, exception during IRET?

Exception during IRET or any instruction under the interrupt shadow will
push the TF we set to step over this issue on the guest stack. We do not
intercept all the possible exceptions, so we can leak TF. Moreover,
multiplexing TF users is currently imperfect on AMD but, before fixing
that, we have to think about the approach in general.

> 
>> Moreover, we cannot easily prevent that TF set by the NMI
>> code leaks onto the guest's stack.
>>
>> Jörg and I stuck heads together about this during LinuxCon. We came to
>> the conclusion that we either have to emulate the instruction that
>> delays NMIs (ie. _every_ possible instruction for the interrupt shadow
>> case) or fall back to the VMX workaround based on interrupt window
>> trapping and an emergency timeout (much simpler, but not really correct).
> 
> I'd really like to avoid the timer.  But I forgot all the details around 
> this, I'll have to re-learn them so I can actually compare the two options.
> 

Hope the above helps you to get on track, otherwise drop more questions.
Also putting Joerg on CC (in the futile hope that the longer the CC list
is, the lesser the pain becomes for each individual).

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [PATCH 1/1] Disable GUEST_INTR_STATE_STI flag before injecting NMI to guest on VMX

[Avi Kivity]

  On 08/27/2010 05:13 PM, Jan Kiszka wrote:
>
>> I forgot them already.  What was that, exception during IRET?
> Exception during IRET or any instruction under the interrupt shadow will
> push the TF we set to step over this issue on the guest stack. We do not
> intercept all the possible exceptions, so we can leak TF. Moreover,
> multiplexing TF users is currently imperfect on AMD but, before fixing
> that, we have to think about the approach in general.

Thanks.  I think those are all solvable.  The key IMO is to take a state 
based approach to host bits - instead of setting or clearing a bit in 
response to an event, use the event as a trigger for recalculation of 
the bit's value.  This works for bits which have multiple uses, and for 
recovery from KVM_SET_*.  For guest bits which are needed by the host we 
also have a working approach - when the bit is overloaded, trap all 
instructions that can see it, as in CR0.TS.

It may take some work but I think we can achieve 100% accuracy without 
making the code unmaintainable.

>> I'd really like to avoid the timer.  But I forgot all the details around
>> this, I'll have to re-learn them so I can actually compare the two options.
>>
> Hope the above helps you to get on track, otherwise drop more questions.
> Also putting Joerg on CC (in the futile hope that the longer the CC list
> is, the lesser the pain becomes for each individual).

I think I got it.  And I also think we need to start documenting these 
invariants, to make it easier for people to see the method in all that 
chaos.

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

Re: [PATCH 1/1] Disable GUEST_INTR_STATE_STI flag before injecting NMI to guest on VMX

[Jan Kiszka]

Avi Kivity wrote:
>   On 08/27/2010 05:13 PM, Jan Kiszka wrote:
>>> I forgot them already.  What was that, exception during IRET?
>> Exception during IRET or any instruction under the interrupt shadow will
>> push the TF we set to step over this issue on the guest stack. We do not
>> intercept all the possible exceptions, so we can leak TF. Moreover,
>> multiplexing TF users is currently imperfect on AMD but, before fixing
>> that, we have to think about the approach in general.
> 
> Thanks.  I think those are all solvable.  The key IMO is to take a state 
> based approach to host bits - instead of setting or clearing a bit in 
> response to an event, use the event as a trigger for recalculation of 
> the bit's value.  This works for bits which have multiple uses, and for 
> recovery from KVM_SET_*.  For guest bits which are needed by the host we 
> also have a working approach - when the bit is overloaded, trap all 
> instructions that can see it, as in CR0.TS.
> 
> It may take some work but I think we can achieve 100% accuracy without 
> making the code unmaintainable.

Besides making TF usage robust against multiple users, including the
guest itself, my complexity concern is first of all about preventing its
leakage. We will have to trap _all_ exceptions and properly remove TF
from the guest state.

And then there is a potential performance price to pay (yes, accuracy
should come first): If the guest uses NMIs for profiles, thus at a
significant rate, AMD processors force us to exit twice per NMI return -
independent of the fact if there is another NMI pending or not.

> 
>>> I'd really like to avoid the timer.  But I forgot all the details around
>>> this, I'll have to re-learn them so I can actually compare the two options.
>>>
>> Hope the above helps you to get on track, otherwise drop more questions.
>> Also putting Joerg on CC (in the futile hope that the longer the CC list
>> is, the lesser the pain becomes for each individual).
> 
> I think I got it.  And I also think we need to start documenting these 
> invariants, to make it easier for people to see the method in all that 
> chaos.

Agreed.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [PATCH 1/1] Disable GUEST_INTR_STATE_STI flag before injecting NMI to guest on VMX

[Avi Kivity]

  On 08/27/2010 07:43 PM, Jan Kiszka wrote:
> Avi Kivity wrote:
>>    On 08/27/2010 05:13 PM, Jan Kiszka wrote:
>>>> I forgot them already.  What was that, exception during IRET?
>>> Exception during IRET or any instruction under the interrupt shadow will
>>> push the TF we set to step over this issue on the guest stack. We do not
>>> intercept all the possible exceptions, so we can leak TF. Moreover,
>>> multiplexing TF users is currently imperfect on AMD but, before fixing
>>> that, we have to think about the approach in general.
>> Thanks.  I think those are all solvable.  The key IMO is to take a state
>> based approach to host bits - instead of setting or clearing a bit in
>> response to an event, use the event as a trigger for recalculation of
>> the bit's value.  This works for bits which have multiple uses, and for
>> recovery from KVM_SET_*.  For guest bits which are needed by the host we
>> also have a working approach - when the bit is overloaded, trap all
>> instructions that can see it, as in CR0.TS.
>>
>> It may take some work but I think we can achieve 100% accuracy without
>> making the code unmaintainable.
> Besides making TF usage robust against multiple users, including the
> guest itself, my complexity concern is first of all about preventing its
> leakage. We will have to trap _all_ exceptions and properly remove TF
> from the guest state.

Note we already trap all exceptions on Intel when virtualizing real mode 
via vm86.

> And then there is a potential performance price to pay (yes, accuracy
> should come first): If the guest uses NMIs for profiles, thus at a
> significant rate, AMD processors force us to exit twice per NMI return -
> independent of the fact if there is another NMI pending or not.

Doesn't worry me too much.  NMI rate will be limited or 
program-under-test performance will suffer.  10K vs 20K exits/sec is 
substantial, but not worth worrying about for this fairly rare use case.

I'm more concerned that we don't push VMLOAD/VMSAVE to the heavyweight 
exit path, and that we don't use the svm interrupt queue (that could 
reduce 10% of the exits on normal interrupts).


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

Re: [PATCH 1/1] Disable GUEST_INTR_STATE_STI flag before injecting NMI to guest on VMX

[Avi Kivity]

  On 08/27/2010 11:27 AM, Jan Kiszka wrote:
> Am 26.08.2010 22:06, Jes.Sorensen@redhat.com wrote:
>> From: Jes Sorensen<Jes.Sorensen@redhat.com>
>>
>> Injecting an NMI while GUEST_INTR_STATE_STI is set may fail,
>> which can cause an EXIT with invalid state, resulting in the
>> guest dieing.
> Very interesting. Reality obviously doesn't bother about the statement
> of the vendor [1].
>
> Just curious: is this limited to specific CPU models or actually a
> generic issue?
>

The manual states that whether a processor accepts NMIs when 
blocked-by-STI or not is processor dependent.

> Thinking about the implications: Independent of virtualization, this
> means that no code code can in any way rely on the STI shadow if there
> are NMIs present that could "consume" it. Because after return from
> those NMIs, interrupts could then be injected on the instruction that
> was originally under the shadow.
>

Wow.  Maybe we should request an interrupt window instead when 
blocked-by-STI is active instead of clearing it.

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

Re: [PATCH 1/1] Disable GUEST_INTR_STATE_STI flag before injecting NMI to guest on VMX

[Jan Kiszka]

Avi Kivity wrote:
>   On 08/27/2010 11:27 AM, Jan Kiszka wrote:
>> Am 26.08.2010 22:06, Jes.Sorensen@redhat.com wrote:
>>> From: Jes Sorensen<Jes.Sorensen@redhat.com>
>>>
>>> Injecting an NMI while GUEST_INTR_STATE_STI is set may fail,
>>> which can cause an EXIT with invalid state, resulting in the
>>> guest dieing.
>> Very interesting. Reality obviously doesn't bother about the statement
>> of the vendor [1].
>>
>> Just curious: is this limited to specific CPU models or actually a
>> generic issue?
>>
> 
> The manual states that whether a processor accepts NMIs when 
> blocked-by-STI or not is processor dependent.

Yes, but this is fairly new, and when Gleb asked Intel, the answer was a
clear "no, there is no such requirement". Maybe someone found the
related processor code in the meantime...

> 
>> Thinking about the implications: Independent of virtualization, this
>> means that no code code can in any way rely on the STI shadow if there
>> are NMIs present that could "consume" it. Because after return from
>> those NMIs, interrupts could then be injected on the instruction that
>> was originally under the shadow.
>>
> 
> Wow.  Maybe we should request an interrupt window instead when 
> blocked-by-STI is active instead of clearing it.
> 

Then we are (almost) back in pre-NMI-window times when the guest happens
to spin with IRQs disabled.

What a mess.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [PATCH 1/1] Disable GUEST_INTR_STATE_STI flag before injecting NMI to guest on VMX

[Avi Kivity]

  On 08/27/2010 02:06 PM, Jan Kiszka wrote:
>
>> Wow.  Maybe we should request an interrupt window instead when
>> blocked-by-STI is active instead of clearing it.
>>
> Then we are (almost) back in pre-NMI-window times when the guest happens
> to spin with IRQs disabled.

No.  We only request an interrupt window if we're blocked by STI.  That 
implies that interrupts will be enabled by the next instruction.

(except if the code is sti; cli?)

Is there anything in x86 that doesn't suck?

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

Re: [PATCH 1/1] Disable GUEST_INTR_STATE_STI flag before injecting NMI to guest on VMX

[Jan Kiszka]

Avi Kivity wrote:
>   On 08/27/2010 02:06 PM, Jan Kiszka wrote:
>>> Wow.  Maybe we should request an interrupt window instead when
>>> blocked-by-STI is active instead of clearing it.
>>>
>> Then we are (almost) back in pre-NMI-window times when the guest happens
>> to spin with IRQs disabled.
> 
> No.  We only request an interrupt window if we're blocked by STI.  That 
> implies that interrupts will be enabled by the next instruction.
> 
> (except if the code is sti; cli?)

Yes, we are only talking about weird use cases like the above or if the
guest decides to leave IRQs off on NMI return. So it is not as bad as
without any VNMI support.

> 
> Is there anything in x86 that doesn't suck?
> 

Hard to imagine.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [PATCH 1/1] Disable GUEST_INTR_STATE_STI flag before injecting NMI to guest on VMX

[Gleb Natapov]

On Fri, Aug 27, 2010 at 12:44:41PM +0300, Avi Kivity wrote:
> >Thinking about the implications: Independent of virtualization, this
> >means that no code code can in any way rely on the STI shadow if there
> >are NMIs present that could "consume" it. Because after return from
> >those NMIs, interrupts could then be injected on the instruction that
> >was originally under the shadow.
> >
> 
> Wow.  Maybe we should request an interrupt window instead when
> blocked-by-STI is active instead of clearing it.
> 
Wow indeed. We can remember blocked by sti state before injecting NMI
and request nmi window open exit. When we get nmi window open exit we
can restore blocked by sti flag.

--
			Gleb.

Re: [PATCH 1/1] Disable GUEST_INTR_STATE_STI flag before injecting NMI to guest on VMX

[Jan Kiszka]

Gleb Natapov wrote:
> On Fri, Aug 27, 2010 at 12:44:41PM +0300, Avi Kivity wrote:
>>> Thinking about the implications: Independent of virtualization, this
>>> means that no code code can in any way rely on the STI shadow if there
>>> are NMIs present that could "consume" it. Because after return from
>>> those NMIs, interrupts could then be injected on the instruction that
>>> was originally under the shadow.
>>>
>> Wow.  Maybe we should request an interrupt window instead when
>> blocked-by-STI is active instead of clearing it.
>>
> Wow indeed. We can remember blocked by sti state before injecting NMI
> and request nmi window open exit. When we get nmi window open exit we
> can restore blocked by sti flag.

For sure we could. But I still wonder what happens to the shadow in such
a scenario on real HW.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [PATCH 1/1] Disable GUEST_INTR_STATE_STI flag before injecting NMI to guest on VMX

[Gleb Natapov]

On Fri, Aug 27, 2010 at 01:23:06PM +0200, Jan Kiszka wrote:
> Gleb Natapov wrote:
> > On Fri, Aug 27, 2010 at 12:44:41PM +0300, Avi Kivity wrote:
> >>> Thinking about the implications: Independent of virtualization, this
> >>> means that no code code can in any way rely on the STI shadow if there
> >>> are NMIs present that could "consume" it. Because after return from
> >>> those NMIs, interrupts could then be injected on the instruction that
> >>> was originally under the shadow.
> >>>
> >> Wow.  Maybe we should request an interrupt window instead when
> >> blocked-by-STI is active instead of clearing it.
> >>
> > Wow indeed. We can remember blocked by sti state before injecting NMI
> > and request nmi window open exit. When we get nmi window open exit we
> > can restore blocked by sti flag.
> 
> For sure we could. But I still wonder what happens to the shadow in such
> a scenario on real HW.
> 
Me too, so lets wait for vendor answer.

--
			Gleb.

Re: [PATCH 1/1] Disable GUEST_INTR_STATE_STI flag before injecting NMI to guest on VMX

[Gleb Natapov]

On Fri, Aug 27, 2010 at 10:27:37AM +0200, Jan Kiszka wrote:
> Am 26.08.2010 22:06, Jes.Sorensen@redhat.com wrote:
> > From: Jes Sorensen <Jes.Sorensen@redhat.com>
> > 
> > Injecting an NMI while GUEST_INTR_STATE_STI is set may fail,
> > which can cause an EXIT with invalid state, resulting in the
> > guest dieing.
> 
> Very interesting. Reality obviously doesn't bother about the statement
> of the vendor [1].
> 
I re-read my mail thread with vendor and to be fair vendor said that we should
clear blocked by STI before injecting NMI. It's my fault I missed it.

> Just curious: is this limited to specific CPU models or actually a
> generic issue?
> 
> > 
> > Credit to Gleb for figuring out why it was failing and how to
> > fix it.
> > 
> > Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
> > Signed-off-by: Gleb Natapov <gleb@redhat.com>
> > ---
> >  arch/x86/kvm/vmx.c |    2 ++
> >  1 files changed, 2 insertions(+), 0 deletions(-)
> > 
> > diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> > index cf56462..8e95371 100644
> > --- a/arch/x86/kvm/vmx.c
> > +++ b/arch/x86/kvm/vmx.c
> > @@ -2888,6 +2888,8 @@ static void vmx_inject_nmi(struct kvm_vcpu *vcpu)
> >  		kvm_rip_write(vcpu, vmx->rmode.irq.rip - 1);
> >  		return;
> >  	}
> > +	vmcs_write32(GUEST_INTERRUPTIBILITY_INFO,
> > +			vmcs_read32(GUEST_INTERRUPTIBILITY_INFO) & ~GUEST_INTR_STATE_STI);
> >  	vmcs_write32(VM_ENTRY_INTR_INFO_FIELD,
> >  			INTR_TYPE_NMI_INTR | INTR_INFO_VALID_MASK | NMI_VECTOR);
> >  }
> 
> Thinking about the implications: Independent of virtualization, this
> means that no code code can in any way rely on the STI shadow if there
> are NMIs present that could "consume" it. Because after return from
> those NMIs, interrupts could then be injected on the instruction that
> was originally under the shadow.
> 
> Jan
> 
> [1] http://thread.gmane.org/gmane.comp.emulators.kvm.devel/52144
> 
> -- 
> Siemens AG, Corporate Technology, CT T DE IT 1
> Corporate Competence Center Embedded Linux

--
			Gleb.

Re: [PATCH 1/1] Disable GUEST_INTR_STATE_STI flag before injecting NMI to guest on VMX

[Jan Kiszka]

Gleb Natapov wrote:
> On Fri, Aug 27, 2010 at 10:27:37AM +0200, Jan Kiszka wrote:
>> Am 26.08.2010 22:06, Jes.Sorensen@redhat.com wrote:
>>> From: Jes Sorensen <Jes.Sorensen@redhat.com>
>>>
>>> Injecting an NMI while GUEST_INTR_STATE_STI is set may fail,
>>> which can cause an EXIT with invalid state, resulting in the
>>> guest dieing.
>> Very interesting. Reality obviously doesn't bother about the statement
>> of the vendor [1].
>>
> I re-read my mail thread with vendor and to be fair vendor said that we should
> clear blocked by STI before injecting NMI. It's my fault I missed it.

Ah, ok, then I take this back.

Still, this leaves blocked-by-STI useless when NMIs are present. We are
somehow in the same troubles as on AMD.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [PATCH 1/1] Disable GUEST_INTR_STATE_STI flag before injecting NMI to guest on VMX

[Avi Kivity]

  On 08/26/2010 11:06 PM, Jes.Sorensen@redhat.com wrote:
> From: Jes Sorensen<Jes.Sorensen@redhat.com>
>
> Injecting an NMI while GUEST_INTR_STATE_STI is set may fail,
> which can cause an EXIT with invalid state, resulting in the
> guest dieing.
>
> Credit to Gleb for figuring out why it was failing and how to
> fix it.
>
> Signed-off-by: Jes Sorensen<Jes.Sorensen@redhat.com>
> Signed-off-by: Gleb Natapov<gleb@redhat.com>
> ---
>   arch/x86/kvm/vmx.c |    2 ++
>   1 files changed, 2 insertions(+), 0 deletions(-)
>
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index cf56462..8e95371 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -2888,6 +2888,8 @@ static void vmx_inject_nmi(struct kvm_vcpu *vcpu)
>   		kvm_rip_write(vcpu, vmx->rmode.irq.rip - 1);
>   		return;
>   	}
> +	vmcs_write32(GUEST_INTERRUPTIBILITY_INFO,
> +			vmcs_read32(GUEST_INTERRUPTIBILITY_INFO)&  ~GUEST_INTR_STATE_STI);

vmcs_clear_bits() is a nicer way of doing this.

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

Re: [PATCH 1/1] Disable GUEST_INTR_STATE_STI flag before injecting NMI to guest on VMX

[Jes Sorensen]

On 08/27/10 11:21, Avi Kivity wrote:
>> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
>> index cf56462..8e95371 100644
>> --- a/arch/x86/kvm/vmx.c
>> +++ b/arch/x86/kvm/vmx.c
>> @@ -2888,6 +2888,8 @@ static void vmx_inject_nmi(struct kvm_vcpu *vcpu)
>>           kvm_rip_write(vcpu, vmx->rmode.irq.rip - 1);
>>           return;
>>       }
>> +    vmcs_write32(GUEST_INTERRUPTIBILITY_INFO,
>> +            vmcs_read32(GUEST_INTERRUPTIBILITY_INFO)& 
>> ~GUEST_INTR_STATE_STI);
> 
> vmcs_clear_bits() is a nicer way of doing this.
> 

Ok, try v2 that I just posted - and forgot to add v2 in the Subject line
to - sorry.

Cheers,
Jes

Re: [PATCH 1/1] Disable GUEST_INTR_STATE_STI flag before injecting NMI to guest on VMX

[Avi Kivity]

  On 08/27/2010 12:41 PM, Jes Sorensen wrote:
> On 08/27/10 11:21, Avi Kivity wrote:
>>> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
>>> index cf56462..8e95371 100644
>>> --- a/arch/x86/kvm/vmx.c
>>> +++ b/arch/x86/kvm/vmx.c
>>> @@ -2888,6 +2888,8 @@ static void vmx_inject_nmi(struct kvm_vcpu *vcpu)
>>>            kvm_rip_write(vcpu, vmx->rmode.irq.rip - 1);
>>>            return;
>>>        }
>>> +    vmcs_write32(GUEST_INTERRUPTIBILITY_INFO,
>>> +            vmcs_read32(GUEST_INTERRUPTIBILITY_INFO)&
>>> ~GUEST_INTR_STATE_STI);
>> vmcs_clear_bits() is a nicer way of doing this.
>>
> Ok, try v2 that I just posted - and forgot to add v2 in the Subject line
> to - sorry.
>

Well, in light of Jan's comment re sti; hlt being clobbered by this, we 
should think about requesting an interrupt window instead...

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

Re: [PATCH 1/1] Disable GUEST_INTR_STATE_STI flag before injecting NMI to guest on VMX

[Jes Sorensen]

On 08/27/10 11:47, Avi Kivity wrote:
>  On 08/27/2010 12:41 PM, Jes Sorensen wrote:
>> Ok, try v2 that I just posted - and forgot to add v2 in the Subject line
>> to - sorry.
>>
> 
> Well, in light of Jan's comment re sti; hlt being clobbered by this, we
> should think about requesting an interrupt window instead...
> 

Ok, I heading onto thin ice here :)

How does one do that, just a call to
    kvm_x86_ops->enable_nmi_window(vcpu);
or is something else needed, like setting vcpu->arch.nmi_pending ?

Cheers,
Jes

Re: [PATCH 1/1] Disable GUEST_INTR_STATE_STI flag before injecting NMI to guest on VMX

[Avi Kivity]

  On 08/27/2010 12:56 PM, Jes Sorensen wrote:
>
>> Well, in light of Jan's comment re sti; hlt being clobbered by this, we
>> should think about requesting an interrupt window instead...
>>
> Ok, I heading onto thin ice here :)
>
> How does one do that, just a call to
>      kvm_x86_ops->enable_nmi_window(vcpu);
> or is something else needed, like setting vcpu->arch.nmi_pending ?

->enable_irq_window()

And need to update ->nmi_allowed() to disallow nmi if blocked-by-sti.

I've asked the vendor about this, so let's wait for their reply first.

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

Re: [PATCH 1/1] Disable GUEST_INTR_STATE_STI flag before injecting NMI to guest on VMX

[Jes Sorensen]

On 08/27/10 11:59, Avi Kivity wrote:
>  On 08/27/2010 12:56 PM, Jes Sorensen wrote:
>>
>>> Well, in light of Jan's comment re sti; hlt being clobbered by this, we
>>> should think about requesting an interrupt window instead...
>>>
>> Ok, I heading onto thin ice here :)
>>
>> How does one do that, just a call to
>>      kvm_x86_ops->enable_nmi_window(vcpu);
>> or is something else needed, like setting vcpu->arch.nmi_pending ?
> 
> ->enable_irq_window()
> 
> And need to update ->nmi_allowed() to disallow nmi if blocked-by-sti.
> 
> I've asked the vendor about this, so let's wait for their reply first.
> 

Ok, let me know when you hear back.

Cheers,
Jes