Re: [PATCH] KVM: VMX: simplify NMI mask management

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

On 2011-03-23 17:28, Avi Kivity wrote:
> On 03/23/2011 05:10 PM, Jan Kiszka wrote:
>> On 2011-03-23 15:58, Avi Kivity wrote:
>>>  On 03/23/2011 04:54 PM, Jan Kiszka wrote:
>>>>>   @@ -3991,9 +3987,7 @@ static void __vmx_complete_interrupts(struct vcpu_vmx *vmx,
>>>>>    		* Clear bit "block by NMI" before VM entry if a NMI
>>>>>    		* delivery faulted.
>>>>>    		*/
>>>>>   -		vmcs_clear_bits(GUEST_INTERRUPTIBILITY_INFO,
>>>>>   -				GUEST_INTR_STATE_NMI);
>>>>>   -		vmx->nmi_known_unmasked = true;
>>>>>   +		vmx_set_nmi_mask(&vmx->vcpu, false);
>>>>>    		break;
>>>>>    	case INTR_TYPE_SOFT_EXCEPTION:
>>>>>    		vmx->vcpu.arch.event_exit_inst_len =
>>>>
>>>>  Looks consistent to me, but maybe you should update the comment above
>>>>  that's now also applied to the emulated mask.
>>>
>>>  In what way?
>>>
>>>           /*
>>>            * SDM 3: 27.7.1.2 (September 2008)
>>>            * Clear bit "block by NMI" before VM entry if a NMI
>>>            * delivery faulted.
>>>            */
>>>
>>>  Seems pretty descriptive.
>>
>> IIRC, the above only applies to CPUs that support VNMI and is
>> meaningless for older ones. But vmx_set_nmi_mask is not void for those
>> old CPUs.
> 
> That's what I pointed out in the second paragraph of the commit log - 
> that it's desired for non-vnmi capable cpus:
> 
> static void vmx_inject_nmi(struct kvm_vcpu *vcpu)
> {
>      struct vcpu_vmx *vmx = to_vmx(vcpu);
> 
>      if (!cpu_has_virtual_nmis()) {
>          /*
>           * Tracking the NMI-blocked state in software is built upon
>           * finding the next open IRQ window. This, in turn, depends on
>           * well-behaving guests: They have to keep IRQs disabled at
>           * least as long as the NMI handler runs. Otherwise we may
>           * cause NMI nesting, maybe breaking the guest. But as this is
>           * highly unlikely, we can live with the residual risk.
>           */
>          vmx->soft_vnmi_blocked = 1;
>          vmx->vnmi_blocked_time = 0;
>      }
> 
> We block nmi here, and if we later take a fault, nmi remains both 
> undelivered and blocked.
> 
> The SDM refers to the fact the the cpu blocks NMI first and checks for 
> exceptions during delivery later.  That happens to match the soft-vnmi 
> implementation, so the comment remains valid.
> 

Makes sense now.

Jan

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