From mboxrd@z Thu Jan 1 00:00:00 1970 From: Avi Kivity Subject: Re: [PATCH] KVM: VMX: simplify NMI mask management Date: Wed, 23 Mar 2011 18:28:43 +0200 Message-ID: <4D8A1FBB.3010809@redhat.com> References: <1300885976-8755-1-git-send-email-avi@redhat.com> <4D8A098D.5070902@siemens.com> <4D8A0A92.7030105@redhat.com> <4D8A0D51.10204@siemens.com> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Cc: Marcelo Tosatti , "kvm@vger.kernel.org" To: Jan Kiszka Return-path: Received: from mx1.redhat.com ([209.132.183.28]:40233 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751198Ab1CWQ2s (ORCPT ); Wed, 23 Mar 2011 12:28:48 -0400 In-Reply-To: <4D8A0D51.10204@siemens.com> Sender: kvm-owner@vger.kernel.org List-ID: 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. -- error compiling committee.c: too many arguments to function