DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 2FF75C0467C2
From: Bandan Das <bsd@redhat.com>
To: David Hildenbrand <david@redhat.com>
Cc: 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
References: <20170710204936.4001-1-bsd@redhat.com>
        <20170710204936.4001-4-bsd@redhat.com>
        <2d50ebc4-9328-ce08-b55b-6a331ee13cc3@redhat.com>
Date: Tue, 11 Jul 2017 13:58:33 -0400
In-Reply-To: <2d50ebc4-9328-ce08-b55b-6a331ee13cc3@redhat.com> (David
        Hildenbrand's message of "Tue, 11 Jul 2017 09:51:39 +0200")
Message-ID: <jpgy3ruam9i.fsf@linux.bootlegged.copy>
User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/25.2 (gnu/linux)
MIME-Version: 1.0
Content-Type: text/plain
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 7343
Lines: 220

David Hildenbrand <david@redhat.com> writes:

> 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>
>> ---
>>  arch/x86/include/asm/vmx.h |  6 +++++
>>  arch/x86/kvm/vmx.c         | 58 +++++++++++++++++++++++++++++++++++++++++++---
>>  2 files changed, 61 insertions(+), 3 deletions(-)
>> 
>> diff --git a/arch/x86/include/asm/vmx.h b/arch/x86/include/asm/vmx.h
>> index da5375e..5f63a2e 100644
>> --- a/arch/x86/include/asm/vmx.h
>> +++ b/arch/x86/include/asm/vmx.h
>> @@ -115,6 +115,10 @@
>>  #define VMX_MISC_SAVE_EFER_LMA			0x00000020
>>  #define VMX_MISC_ACTIVITY_HLT			0x00000040
>>  
>> +/* VMFUNC functions */
>> +#define VMX_VMFUNC_EPTP_SWITCHING               0x00000001
>> +#define VMFUNC_EPTP_ENTRIES  512
>> +
>>  static inline u32 vmx_basic_vmcs_revision_id(u64 vmx_basic)
>>  {
>>  	return vmx_basic & GENMASK_ULL(30, 0);
>> @@ -200,6 +204,8 @@ enum vmcs_field {
>>  	EOI_EXIT_BITMAP2_HIGH           = 0x00002021,
>>  	EOI_EXIT_BITMAP3                = 0x00002022,
>>  	EOI_EXIT_BITMAP3_HIGH           = 0x00002023,
>> +	EPTP_LIST_ADDRESS               = 0x00002024,
>> +	EPTP_LIST_ADDRESS_HIGH          = 0x00002025,
>>  	VMREAD_BITMAP                   = 0x00002026,
>>  	VMWRITE_BITMAP                  = 0x00002028,
>>  	XSS_EXIT_BITMAP                 = 0x0000202C,
>> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
>> index fe8f5fc..0a969fb 100644
>> --- a/arch/x86/kvm/vmx.c
>> +++ b/arch/x86/kvm/vmx.c
>> @@ -246,6 +246,7 @@ struct __packed vmcs12 {
>>  	u64 eoi_exit_bitmap1;
>>  	u64 eoi_exit_bitmap2;
>>  	u64 eoi_exit_bitmap3;
>> +	u64 eptp_list_address;
>>  	u64 xss_exit_bitmap;
>>  	u64 guest_physical_address;
>>  	u64 vmcs_link_pointer;
>> @@ -771,6 +772,7 @@ static const unsigned short vmcs_field_to_offset_table[] = {
>>  	FIELD64(EOI_EXIT_BITMAP1, eoi_exit_bitmap1),
>>  	FIELD64(EOI_EXIT_BITMAP2, eoi_exit_bitmap2),
>>  	FIELD64(EOI_EXIT_BITMAP3, eoi_exit_bitmap3),
>> +	FIELD64(EPTP_LIST_ADDRESS, eptp_list_address),
>>  	FIELD64(XSS_EXIT_BITMAP, xss_exit_bitmap),
>>  	FIELD64(GUEST_PHYSICAL_ADDRESS, guest_physical_address),
>>  	FIELD64(VMCS_LINK_POINTER, vmcs_link_pointer),
>> @@ -1402,6 +1404,13 @@ static inline bool nested_cpu_has_vmfunc(struct vmcs12 *vmcs12)
>>  	return nested_cpu_has2(vmcs12, SECONDARY_EXEC_ENABLE_VMFUNC);
>>  }
>>  
>> +static inline bool nested_cpu_has_eptp_switching(struct vmcs12 *vmcs12)
>> +{
>> +	return nested_cpu_has_vmfunc(vmcs12) &&
>> +		(vmcs12->vm_function_control &
>
> I wonder if it makes sense to rename vm_function_control to
> - vmfunc_control
> - vmfunc_controls (so it matches nested_vmx_vmfunc_controls)
> - vmfunc_ctrl

I tend to follow the SDM names because it's easy to look for them.

>> +		 VMX_VMFUNC_EPTP_SWITCHING);
>> +}
>> +
>>  static inline bool is_nmi(u32 intr_info)
>>  {
>>  	return (intr_info & (INTR_INFO_INTR_TYPE_MASK | INTR_INFO_VALID_MASK))
>> @@ -2791,7 +2800,12 @@ static void nested_vmx_setup_ctls_msrs(struct vcpu_vmx *vmx)
>>  	if (cpu_has_vmx_vmfunc()) {
>>  		vmx->nested.nested_vmx_secondary_ctls_high |=
>>  			SECONDARY_EXEC_ENABLE_VMFUNC;
>> -		vmx->nested.nested_vmx_vmfunc_controls = 0;
>> +		/*
>> +		 * Advertise EPTP switching unconditionally
>> +		 * since we emulate it
>> +		 */
>> +		vmx->nested.nested_vmx_vmfunc_controls =
>> +			VMX_VMFUNC_EPTP_SWITCHING;>  	}
>>  
>>  	/*
>> @@ -7772,6 +7786,9 @@ static int handle_vmfunc(struct kvm_vcpu *vcpu)
>>  	struct vcpu_vmx *vmx = to_vmx(vcpu);
>>  	struct vmcs12 *vmcs12;
>>  	u32 function = vcpu->arch.regs[VCPU_REGS_RAX];
>> +	u32 index = vcpu->arch.regs[VCPU_REGS_RCX];
>> +	struct page *page = NULL;
>> +	u64 *l1_eptp_list, address;
>>  
>>  	/*
>>  	 * VMFUNC is only supported for nested guests, but we always enable the
>> @@ -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's a good hint to see if L2 misbehaved and WARN_ON_ONCE makes sure it's
not misused.

>> +		goto fail;
>> +
>> +	if (!nested_cpu_has_ept(vmcs12) ||
>> +	    !nested_cpu_has_eptp_switching(vmcs12))
>> +		goto fail;
>> +
>> +	if (!vmcs12->eptp_list_address || index >= VMFUNC_EPTP_ENTRIES)
>> +		goto fail;
>
> I can find the definition for an vmexit in case of index >=
> VMFUNC_EPTP_ENTRIES, but not for !vmcs12->eptp_list_address in the SDM.
>
> Can you give me a hint?

I don't think there is. Since, we are basically emulating eptp switching
for L2, this is a good check to have.

>> +
>> +	page = nested_get_page(vcpu, vmcs12->eptp_list_address);
>> +	if (!page)
>>  		goto fail;
>> -	WARN_ONCE(1, "VMCS12 VM function control should have been zero");
>> +
>> +	l1_eptp_list = kmap(page);
>> +	address = l1_eptp_list[index];
>> +	if (!address)
>> +		goto fail;
>
> Can you move that check to the other address checks below? (or rework if
> this make sense, see below)
>
>> +	/*
>> +	 * If the (L2) guest does a vmfunc to the currently
>> +	 * active ept pointer, we don't have to do anything else
>> +	 */
>> +	if (vmcs12->ept_pointer != address) {
>> +		if (address >> cpuid_maxphyaddr(vcpu) ||
>> +		    !IS_ALIGNED(address, 4096))
>
> Couldn't the pfn still be invalid and make kvm_mmu_reload() fail?
> (triggering a KVM_REQ_TRIPLE_FAULT)

If there's a triple fault, I think it's a good idea to inject it
back. Basically, there's no need to take care of damage control
that L1 is intentionally doing.

>> +			goto fail;
>> +		kvm_mmu_unload(vcpu);
>> +		vmcs12->ept_pointer = address;
>> +		kvm_mmu_reload(vcpu);
>
> I was thinking about something like this:
>
> kvm_mmu_unload(vcpu);
> old = vmcs12->ept_pointer;
> vmcs12->ept_pointer = address;
> if (kvm_mmu_reload(vcpu)) {
> 	/* pointer invalid, restore previous state */
> 	kvm_clear_request(KVM_REQ_TRIPLE_FAULT, vcpu);
> 	vmcs12->ept_pointer = old;
> 	kvm_mmu_reload(vcpu);
> 	goto fail;
> }
>
> The you can inherit the checks from mmu_check_root().
>
>
> Wonder why I can't spot checks for cpuid_maxphyaddr() /
> IS_ALIGNED(address, 4096) for ordinary use of vmcs12->ept_pointer. The
> checks should be identical.

I think the reason is vmcs12->ept_pointer is never used directly. It's
used to create a shadow table but nevertheless, the check should be there.

>
>> +		kunmap(page);
>> +		nested_release_page_clean(page);
>
> shouldn't the kunmap + nested_release_page_clean go outside the if clause?

:) Indeed, thanks for the catch.

Bandan

>> +	}
>> +	return kvm_skip_emulated_instruction(vcpu);
>>  
>>  fail:
>> +	if (page) {
>> +		kunmap(page);
>> +		nested_release_page_clean(page);
>> +	}
>>  	nested_vmx_vmexit(vcpu, vmx->exit_reason,
>>  			  vmcs_read32(VM_EXIT_INTR_INFO),
>>  			  vmcs_readl(EXIT_QUALIFICATION));
>> 
>
> David and mmu code are not yet best friends. So sorry if I am missing
> something.