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@xxxxxxxxxx>
> ---
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
