Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932728AbdGKNxF (ORCPT ); Tue, 11 Jul 2017 09:53:05 -0400 Received: from mx1.redhat.com ([209.132.183.28]:34034 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932505AbdGKNxD (ORCPT ); Tue, 11 Jul 2017 09:53:03 -0400 DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 6D7CAC04B31C Authentication-Results: ext-mx07.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx07.extmail.prod.ext.phx2.redhat.com; spf=pass smtp.mailfrom=rkrcmar@redhat.com DKIM-Filter: OpenDKIM Filter v2.11.0 mx1.redhat.com 6D7CAC04B31C Date: Tue, 11 Jul 2017 15:52:52 +0200 From: Radim =?utf-8?B?S3LEjW3DocWZ?= To: David Hildenbrand Cc: Bandan Das , kvm@vger.kernel.org, pbonzini@redhat.com, linux-kernel@vger.kernel.org Subject: Re: [PATCH v4 3/3] KVM: nVMX: Emulate EPTP switching for the L1 hypervisor Message-ID: <20170711135251.GA3326@potion> References: <20170710204936.4001-1-bsd@redhat.com> <20170710204936.4001-4-bsd@redhat.com> <2d50ebc4-9328-ce08-b55b-6a331ee13cc3@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <2d50ebc4-9328-ce08-b55b-6a331ee13cc3@redhat.com> X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.31]); Tue, 11 Jul 2017 13:53:03 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2128 Lines: 63 [David did a great review, so I'll just point out things I noticed.] 2017-07-11 09:51+0200, David Hildenbrand: > On 10.07.2017 22:49, Bandan Das wrote: > > When L2 uses vmfunc, L0 utilizes the associated vmexit to > > emulate a switching of the ept pointer by reloading the > > guest MMU. > > > > Signed-off-by: Paolo Bonzini > > Signed-off-by: Bandan Das > > --- > > diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c > > @@ -7784,11 +7801,46 @@ static int handle_vmfunc(struct kvm_vcpu *vcpu) > > } > > > > vmcs12 = get_vmcs12(vcpu); > > - if ((vmcs12->vm_function_control & (1 << function)) == 0) > > + if (((vmcs12->vm_function_control & (1 << function)) == 0) || > > + WARN_ON_ONCE(function)) > > "... instruction causes a VM exit if the bit at position EAX is 0 in the > VM-function controls (the selected VM function is > not enabled)." > > So g2 can trigger this WARN_ON_ONCE, no? I think we should drop it then > completely. It assumes that vm_function_control is not > 1, which is (should be) guaranteed by VM entry check, because the nested_vmx_vmfunc_controls MSR is 1. > > + goto fail; The rest of the code assumes that the function is VMX_VMFUNC_EPTP_SWITCHING, so some WARN_ON_ONCE is reasonable. Writing it as WARN_ON_ONCE(function != VMX_VMFUNC_EPTP_SWITCHING) would be cleared and I'd prefer to move the part that handles VMX_VMFUNC_EPTP_SWITCHING into a new function. (Imagine that Intel is going to add more than one VM FUNC. :]) > > + if (!nested_cpu_has_ept(vmcs12) || > > + !nested_cpu_has_eptp_switching(vmcs12)) > > + goto fail; This brings me to a missing vm-entry check: If “EPTP switching” VM-function control is 1, the “enable EPT” VM-execution control must also be 1. In addition, the EPTP-list address must satisfy the following checks: • Bits 11:0 of the address must be 0. • The address must not set any bits beyond the processor’s physical-address width. so this one could be if (!nested_cpu_has_eptp_switching(vmcs12) || WARN_ON_ONCE(!nested_cpu_has_ept(vmcs12))) after adding the check.