Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933569AbdGKTMQ (ORCPT ); Tue, 11 Jul 2017 15:12:16 -0400 Received: from mx1.redhat.com ([209.132.183.28]:40718 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932483AbdGKTMP (ORCPT ); Tue, 11 Jul 2017 15:12:15 -0400 DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com BA9643D940 Authentication-Results: ext-mx06.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx06.extmail.prod.ext.phx2.redhat.com; spf=pass smtp.mailfrom=rkrcmar@redhat.com DKIM-Filter: OpenDKIM Filter v2.11.0 mx1.redhat.com BA9643D940 Date: Tue, 11 Jul 2017 21:12:07 +0200 From: Radim =?utf-8?B?S3LEjW3DocWZ?= To: Bandan Das Cc: David Hildenbrand , 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: <20170711191207.GD3326@potion> References: <20170710204936.4001-1-bsd@redhat.com> <20170710204936.4001-4-bsd@redhat.com> <2d50ebc4-9328-ce08-b55b-6a331ee13cc3@redhat.com> <20170711135251.GA3326@potion> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.30]); Tue, 11 Jul 2017 19:12:14 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3371 Lines: 87 2017-07-11 14:05-0400, Bandan Das: > Radim Krčmář writes: > > > [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. :]) > > IMO, for now, this should be fine because we are not even passing through the > hardware's eptp switching. Even if there are other vm functions, they > won't be available for the nested case and cause any conflict. Yeah, it is fine function-wise, I was just pointing out that it looks ugly to me. Btw. have you looked what we'd need to do for the hardware pass-through? I'd expect big changes to MMU. :) > >> > + 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))) > > I will reverse the order here but the vm entry check is unnecessary because > the check on the list address is already being done in this function. Here is too late, the nested VM-entry should have failed, never letting this situation happen. We want an equivalent of if (nested_cpu_has_eptp_switching(vmcs12) && !nested_cpu_has_ept(vmcs12)) return VMXERR_ENTRY_INVALID_CONTROL_FIELD; in nested controls checks, right next to the reserved fields check. And then also the check EPTP-list check. All of them only checked when nested_cpu_has_vmfunc(vmcs12).