Re: [PATCH v1 1/2] KVM: nVMX: get rid of nested_get_page()

["Radim Krčmář" <rkrcmar@redhat.com>]

2017-08-03 16:09+0200, David Hildenbrand:
> nested_get_page() just sounds confusing. All we want is a page from G1.
> This is even unrelated to nested.
> 
> Let's introduce kvm_vcpu_gpa_to_page() so we don't get too lengthy
> lines.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---

I like the cleanup, but a subtle change in behavior that makes me wary:

> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> @@ -9535,15 +9528,15 @@ static void nested_get_vmcs12_pages(struct kvm_vcpu *vcpu,
>  		 */
>  		if (vmx->nested.apic_access_page) /* shouldn't happen */
>  			nested_release_page(vmx->nested.apic_access_page);
> -		vmx->nested.apic_access_page =
> -			nested_get_page(vcpu, vmcs12->apic_access_addr);
> +		page = kvm_vcpu_gpa_to_page(vcpu, vmcs12->apic_access_addr);

If what shouldn't happen happened and then kvm_vcpu_gpa_to_page()
failed, we'd be calling put_page() twice on the same page.  I think the
situation currently can happen if VM entry fails after this point.

Assigning 'vmx->nested.apic_access_page = NULL' when releasing the page
sounds safer.

Unless I'm reading something wrong, the "shouldn't happen" really
shouldn't happen if we did something like this:

diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index e34373838b31..d26e6693f748 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -10462,8 +10462,6 @@ static int enter_vmx_non_root_mode(struct kvm_vcpu *vcpu, bool from_vmentry)
 		return 1;
 	}
 
-	nested_get_vmcs12_pages(vcpu, vmcs12);
-
 	msr_entry_idx = nested_vmx_load_msr(vcpu,
 					    vmcs12->vm_entry_msr_load_addr,
 					    vmcs12->vm_entry_msr_load_count);
@@ -10475,6 +10473,8 @@ static int enter_vmx_non_root_mode(struct kvm_vcpu *vcpu, bool from_vmentry)
 		return 1;
 	}
 
+	nested_get_vmcs12_pages(vcpu, vmcs12);
+
 	/*
 	 * Note no nested_vmx_succeed or nested_vmx_fail here. At this point
 	 * we are no longer running L1, and VMLAUNCH/VMRESUME has not yet

>  		/*
>  		 * If translation failed, no matter: This feature asks
>  		 * to exit when accessing the given address, and if it
>  		 * can never be accessed, this feature won't do
>  		 * anything anyway.
>  		 */
> -		if (vmx->nested.apic_access_page) {
> +		if (!is_error_page(page)) {
> +			vmx->nested.apic_access_page = page;
>  			hpa = page_to_phys(vmx->nested.apic_access_page);
>  			vmcs_write64(APIC_ACCESS_ADDR, hpa);
>  		} else {
> @@ -9560,8 +9553,7 @@ static void nested_get_vmcs12_pages(struct kvm_vcpu *vcpu,
>  	if (nested_cpu_has(vmcs12, CPU_BASED_TPR_SHADOW)) {
>  		if (vmx->nested.virtual_apic_page) /* shouldn't happen */
>  			nested_release_page(vmx->nested.virtual_apic_page);

Ditto,

thanks.

> -		vmx->nested.virtual_apic_page =
> -			nested_get_page(vcpu, vmcs12->virtual_apic_page_addr);
> +		page = kvm_vcpu_gpa_to_page(vcpu, vmcs12->apic_access_addr);
>  
>  		/*
>  		 * If translation failed, VM entry will fail because