Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754238AbbKYQgT (ORCPT ); Wed, 25 Nov 2015 11:36:19 -0500 Received: from mx1.redhat.com ([209.132.183.28]:48496 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752950AbbKYQgR (ORCPT ); Wed, 25 Nov 2015 11:36:17 -0500 From: Bandan Das To: Cc: Gleb Natapov , Paolo Bonzini , Thomas Gleixner , Ingo Molnar , "H. Peter Anvin" , , Subject: Re: [PATCH] KVM: nVMX: remove incorrect vpid check in nested invvpid emulation References: <1448443299-9375-1-git-send-email-haozhong.zhang@intel.com> <20151125161834.GA22092@hzzhang-OptiPlex-9020.sh.intel.com> Date: Wed, 25 Nov 2015 11:36:15 -0500 In-Reply-To: <20151125161834.GA22092@hzzhang-OptiPlex-9020.sh.intel.com> (Haozhong Zhang's message of "Thu, 26 Nov 2015 00:18:34 +0800") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3642 Lines: 94 Haozhong Zhang writes: > On 11/25/15 10:45, Bandan Das wrote: >> Haozhong Zhang writes: >> >> > This patch removes the vpid check when emulating nested invvpid >> > instruction of type all-contexts invalidation. The existing code is >> > incorrect because: >> > (1) According to Intel SDM Vol 3, Section "INVVPID - Invalidate >> > Translations Based on VPID", invvpid instruction does not check >> > vpid in the invvpid descriptor when its type is all-contexts >> > invalidation. >> >> But iirc isn't vpid=0 reserved for root mode ? > Yes, > >> I think we don't want >> L1 hypervisor to be able do a invvpid(0). > > but the invvpid emulated here is doing the all-contexts invalidation that > does not use the given vpid and "invalidates all mappings tagged with all > VPIDs except VPID 0000H" (from Intel SDM). Actually, vpid_sync_context will always try single invalidation first and I was concerned that we will end up calling vpid_sync_context(0). But that will not happen since nested.vpid02 is always allocated a vpid. So... we are good :) >> >> > (2) According to the same document, invvpid of type all-contexts >> > invalidation does not require there is an active VMCS, so/and >> > get_vmcs12() in the existing code may result in a NULL-pointer >> > dereference. In practice, it can crash both KVM itself and L1 >> > hypervisors that use invvpid (e.g. Xen). >> >> If that is the case, then just check if it's null and return without >> doing anything. > > (according to Intel SDM) invvpid of type all-contexts invalidation > should not trigger a valid vmx fail if vpid in the current VMCS is 0. No, I meant just skip instruction and return but I doubt if there's any overhead of flushing mappings that don't exist in the first place. Anyway, better to do as the spec says. > However, this check and its following operation do change this semantics > in nested VMX, so it should be completely removed. > >> >> > Signed-off-by: Haozhong Zhang >> > --- >> > arch/x86/kvm/vmx.c | 5 ----- >> > 1 file changed, 5 deletions(-) >> > >> > diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c >> > index 87acc52..af823a3 100644 >> > --- a/arch/x86/kvm/vmx.c >> > +++ b/arch/x86/kvm/vmx.c >> > @@ -7394,11 +7394,6 @@ static int handle_invvpid(struct kvm_vcpu *vcpu) >> > >> > switch (type) { >> > case VMX_VPID_EXTENT_ALL_CONTEXT: >> > - if (get_vmcs12(vcpu)->virtual_processor_id == 0) { >> > - nested_vmx_failValid(vcpu, >> > - VMXERR_INVALID_OPERAND_TO_INVEPT_INVVPID); >> > - return 1; >> > - } >> > __vmx_flush_tlb(vcpu, to_vmx(vcpu)->nested.vpid02); >> > nested_vmx_succeed(vcpu); >> > break; >> >> I also noticed a BUG() here in the default. It might be a good idea to replace >> it with a WARN. > > Or, in nested_vmx_setup_ctls_msrs(): > > if (enable_vpid) > - vmx->nested.nested_vmx_vpid_caps = VMX_VPID_INVVPID_BIT | > - VMX_VPID_EXTENT_GLOBAL_CONTEXT_BIT; > + vmx->nested.nested_vmx_vpid_caps = VMX_VPID_EXTENT_GLOBAL_CONTEXT_BIT; > > because the current handle_invvpid() only handles all-contexts invalidation. > > Haozhong > > -- > To unsubscribe from this list: send the line "unsubscribe kvm" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/