Re: [PATCH 2/3] KVM: nVMX: Fix conditions for NMI and interrupt injection

[Jan Kiszka <jan.kiszka@siemens.com>]

On 2013-03-14 16:12, Gleb Natapov wrote:
> On Wed, Mar 13, 2013 at 05:53:45PM +0100, Jan Kiszka wrote:
>> If we are in guest mode, L0 can only inject events into L2 if L1 has
>> nothing pending. Otherwise, L0 would overwrite L1's events and they
>> would get lost. This check is conceptually independent of
>> nested_exit_on_intr.
>>
>> If L1 traps external interrupts, then we also need to look at L1's
>> idt_vectoring_info_field. If it is empty, we can kick the guest from L2
>> to L1, just like the previous code worked.
>>
>> Finally, the logic for checking interrupt has to be applied also on NMIs
>> in an analogous way. This enables NMI interception for nested guests.
>>
>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>> ---
>>  arch/x86/kvm/vmx.c |   59 ++++++++++++++++++++++++++++++++++++++++++++-------
>>  1 files changed, 51 insertions(+), 8 deletions(-)
>>
>> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
>> index b50174d..10de336 100644
>> --- a/arch/x86/kvm/vmx.c
>> +++ b/arch/x86/kvm/vmx.c
>> @@ -4211,6 +4211,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;
>> @@ -4307,6 +4313,30 @@ static void vmx_inject_nmi(struct kvm_vcpu *vcpu)
>>  
>>  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 &&
>> +		    (vmcs12->vm_entry_intr_info_field & INTR_INFO_VALID_MASK))
>> +			return 0;
>> +		if (nested_exit_on_nmi(vcpu)) {
>> +			/*
>> +			 * Check if the idt_vectoring_info_field is free. We
>> +			 * cannot raise EXIT_REASON_EXCEPTION_NMI if it isn't.
>> +			 */
>> +			if (vmcs12->idt_vectoring_info_field &
>> +			    VECTORING_INFO_VALID_MASK)
>> +				return 0;
>> +			nested_vmx_vmexit(vcpu);
>> +			vmcs12->vm_exit_reason = EXIT_REASON_EXCEPTION_NMI;
>> +			vmcs12->vm_exit_intr_info = NMI_VECTOR |
>> +				INTR_TYPE_NMI_INTR | INTR_INFO_VALID_MASK;
>> +			/*
>> +			 * fall through to normal code, but now in L1, not L2
>> +			 */
>> +		}
>> +	}
>> +
>>  	if (!cpu_has_virtual_nmis() && to_vmx(vcpu)->soft_vnmi_blocked)
>>  		return 0;
>>  
>> @@ -4346,16 +4376,29 @@ static void vmx_set_nmi_mask(struct kvm_vcpu *vcpu, bool masked)
>>  
>>  static int vmx_interrupt_allowed(struct kvm_vcpu *vcpu)
>>  {
>> -	if (is_guest_mode(vcpu) && nested_exit_on_intr(vcpu)) {
>> +	if (is_guest_mode(vcpu)) {
>>  		struct vmcs12 *vmcs12 = get_vmcs12(vcpu);
>> -		if (to_vmx(vcpu)->nested.nested_run_pending ||
>> -		    (vmcs12->idt_vectoring_info_field &
>> -		     VECTORING_INFO_VALID_MASK))
>> +
>> +		if (to_vmx(vcpu)->nested.nested_run_pending &&
>> +		    (vmcs12->vm_entry_intr_info_field & INTR_INFO_VALID_MASK))
>>  			return 0;
> I do not understand this. As far as I remember Nadav's explanation we
> have to enter guest if nested_run_pending is set because VMX does not
> expect vmexit to happen without running guest at all. May be
> idt_vectoring_info_field processing is the only reason, may be not. I
> wouldn't gamble on it.

What are the problems? Specifically if we emulate the effects of an
immediate vmexit properly. I'm not categorically excluding that some
case is missing. If we do not allow soft-vmexit here, we will set the
interrupt window later and will get such an immediate exit as well
(provided the L2 was interruptible).

> 
>> -		nested_vmx_vmexit(vcpu);
>> -		vmcs12->vm_exit_reason = EXIT_REASON_EXTERNAL_INTERRUPT;
>> -		vmcs12->vm_exit_intr_info = 0;
>> -		/* fall through to normal code, but now in L1, not L2 */
>> +		if (nested_exit_on_intr(vcpu)) {
>> +			/*
>> +			 * Check if the idt_vectoring_info_field is free. We
>> +			 * cannot raise EXIT_REASON_EXTERNAL_INTERRUPT if it
>> +			 * isn't.
>> +			 */
>> +			if (vmcs12->idt_vectoring_info_field &
>> +			    VECTORING_INFO_VALID_MASK)
>> +				return 0;
> I think we actually need to return 0 if idt_vectoring_info_field is
> valid even if !nested_exit_on_intr(). If we do not we let L0 inject
> interrupt into L2 and then overwrite it on entry from
> vmcs12->idt_vectoring_info_field.

Sorry, don't understand the last sentence. This check is about the
software idt_vectoring_info_field, the one we keep for L1, not the real
field.

Jan

-- 
Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
Corporate Competence Center Embedded Linux