DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 6D7CAC04B31C
Date: Tue, 11 Jul 2017 15:52:52 +0200
From: Radim =?utf-8?B?S3LEjW3DocWZ?= <rkrcmar@redhat.com>
To: David Hildenbrand <david@redhat.com>
Cc: Bandan Das <bsd@redhat.com>, 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>
Sender: linux-kernel-owner@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 <pbonzini@redhat.com>
> > Signed-off-by: Bandan Das <bsd@redhat.com>
> > ---
> > 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.