From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jan Kiszka Subject: Re: [PATCH] KVM: VMX: simplify NMI mask management Date: Wed, 23 Mar 2011 18:14:03 +0100 Message-ID: <4D8A2A5B.7070609@siemens.com> References: <1300885976-8755-1-git-send-email-avi@redhat.com> <4D8A098D.5070902@siemens.com> <4D8A0A92.7030105@redhat.com> <4D8A0D51.10204@siemens.com> <4D8A1FBB.3010809@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Cc: Marcelo Tosatti , "kvm@vger.kernel.org" To: Avi Kivity Return-path: Received: from david.siemens.de ([192.35.17.14]:33215 "EHLO david.siemens.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752320Ab1CWROL (ORCPT ); Wed, 23 Mar 2011 13:14:11 -0400 In-Reply-To: <4D8A1FBB.3010809@redhat.com> Sender: kvm-owner@vger.kernel.org List-ID: 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