All of lore.kernel.org
 help / color / mirror / Atom feed
From: "Radim Krčmář" <rkrcmar@redhat.com>
To: David Hildenbrand <david@redhat.com>
Cc: kvm@vger.kernel.org, Paolo Bonzini <pbonzini@redhat.com>
Subject: Re: [PATCH v1 1/2] KVM: nVMX: get rid of nested_get_page()
Date: Thu, 3 Aug 2017 17:36:02 +0200	[thread overview]
Message-ID: <20170803153601.GH32403@flask> (raw)
In-Reply-To: <20170803140907.23681-2-david@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

  reply	other threads:[~2017-08-03 15:36 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-08-03 14:09 [PATCH v1 0/2] KVM: nVMX: remove nested_get_page() David Hildenbrand
2017-08-03 14:09 ` [PATCH v1 1/2] KVM: nVMX: get rid of nested_get_page() David Hildenbrand
2017-08-03 15:36   ` Radim Krčmář [this message]
2017-08-03 15:58     ` David Hildenbrand
2017-08-03 16:05     ` Jim Mattson
2017-08-03 17:41       ` Radim Krčmář
2017-08-03 18:42         ` Jim Mattson
2017-08-03 18:40   ` Konrad Rzeszutek Wilk
2017-08-03 19:42     ` David Hildenbrand
2017-08-07 10:48   ` Paolo Bonzini
2017-08-03 14:09 ` [PATCH v1 2/2] KVM: nVMX: get rid of nested_release_page* David Hildenbrand
2017-08-03 15:14 ` [PATCH v1 0/2] KVM: nVMX: remove nested_get_page() Paolo Bonzini

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20170803153601.GH32403@flask \
    --to=rkrcmar@redhat.com \
    --cc=david@redhat.com \
    --cc=kvm@vger.kernel.org \
    --cc=pbonzini@redhat.com \
    --subject='Re: [PATCH v1 1/2] KVM: nVMX: get rid of nested_get_page()' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.