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

David Hildenbrand
2017-08-03T15:58:35Z

On 03.08.2017 17:36, Radim Krčmář wrote:
> 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:
>

Very good point. I'll just clear the respective field whenever we
release a page, this way we are on the safe side.

Good catch! Thanks!

--

Thanks,

David