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