Re: [PATCH v4 5/6] KVM: nVMX: Fix conditions for NMI injection

[Jan Kiszka <jan.kiszka@web.de>]

[-- Attachment #1: Type: text/plain, Size: 3050 bytes --]

On 2013-04-14 18:18, Gleb Natapov wrote:
> On Sun, Apr 14, 2013 at 05:53:05PM +0200, Jan Kiszka wrote:
>> On 2013-04-14 17:23, Gleb Natapov wrote:
>>> On Sun, Apr 14, 2013 at 12:12:49PM +0200, Jan Kiszka wrote:
>>>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>>>
>>>> The logic for checking if interrupts can be injected has to be applied
>>>> also on NMIs. The difference is that if NMI interception is on these
>>>> events are consumed and blocked by the VM exit.
>>>>
>>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>>>> ---
>>>>  arch/x86/kvm/vmx.c |   28 ++++++++++++++++++++++++++++
>>>>  1 files changed, 28 insertions(+), 0 deletions(-)
>>>>
>>>> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
>>>> index 56e7519..ad9b4bc 100644
>>>> --- a/arch/x86/kvm/vmx.c
>>>> +++ b/arch/x86/kvm/vmx.c
>>>> @@ -4190,6 +4190,12 @@ static bool nested_exit_on_intr(struct kvm_vcpu *vcpu)
>>>>  		PIN_BASED_EXT_INTR_MASK;
>>>>  }
>>>>  
>>>> +static bool nested_exit_on_nmi(struct kvm_vcpu *vcpu)
>>>> +{
>>>> +	return get_vmcs12(vcpu)->pin_based_vm_exec_control &
>>>> +		PIN_BASED_NMI_EXITING;
>>>> +}
>>>> +
>>>>  static void enable_irq_window(struct kvm_vcpu *vcpu)
>>>>  {
>>>>  	u32 cpu_based_vm_exec_control;
>>>> @@ -4315,6 +4321,28 @@ static void vmx_set_nmi_mask(struct kvm_vcpu *vcpu, bool masked)
>>>>  
>>>>  static int vmx_nmi_allowed(struct kvm_vcpu *vcpu)
>>>>  {
>>>> +	if (is_guest_mode(vcpu)) {
>>>> +		struct vmcs12 *vmcs12 = get_vmcs12(vcpu);
>>>> +
>>>> +		if (to_vmx(vcpu)->nested.nested_run_pending ||
>>>> +		    vmcs_read32(GUEST_ACTIVITY_STATE) ==
>>>> +			   GUEST_ACTIVITY_WAIT_SIPI)
>>> The same is true for interrupt too,
>>
>> Yes, but aren't we already waiting with interrupts disabled in that state?
>>
> Why? L1 can do vmcs_write(GUEST_ACTIVITY_STATE,
> GUEST_ACTIVITY_WAIT_SIPI) at any point.

Hmm, ok.

> 
>>> but I do not think that we should allow
>>> nested guest directly enter GUEST_ACTIVITY_WAIT_SIPI state or any other
>>> state except ACTIVE. They should be emulated instead.
>>
>> Well, they aren't emulated yet but directly applied. So I think the
>> patch is correct in the current context at least.
>>
> If my understanding is correct the facts that it is directly applied is
> a serious bug and should be fixed.

Yeah, L1 could put the vCPU in wait-for-SIPI, and only that physical
signal will wake it up again. But L0 will not send it...

> 
>> What negative effects do you expect from entering those states with L2?
>>
> As I wrote below (and you misunderstood me :)) it looks like L0 external
> interrupts do not generate vmexit while active VMCS is in Wait-For-SIPI
> state. In any case we should carefully examine what are the implications
> of this state since KVM never uses it and does not know how to handle it.

OK, but this should be conceptually unrelated to this patch. So, given
that you applied patch 4, should I simply remove the activity state
check from this one in order to proceed with it?

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 263 bytes --]