KVM: nVMX: Don't return error on nested bitmap memory allocation failure

If get_free_page() fails for nested bitmap area, it's evident that we are gonna get screwed anyway but returning failure because we failed allocating memory for a nested structure seems like an unnecessary big hammer. Also, save the call for later; after we are done with other non-nested allocations.

Am 2015-04-28 um 21:55 schrieb Bandan Das:

Quoted 6 lines

If get_free_page() fails for nested bitmap area, it's evident that we are gonna get screwed anyway but returning failure because we failed allocating memory for a nested structure seems like an unnecessary big hammer. Also, save the call for later; after we are done with other non-nested allocations.

Frankly, I prefer failures over automatic degradations. And, as you noted, the whole system will probably explode anyway if allocation of a single page already fails. So what does this buy us?

What could makes sense is making the allocation of the vmread/write bitmap depend on enable_shadow_vmcs, and that again depend on nested.

Jan

Quoted 11 lines · Jan Kiszka

Am 2015-04-28 um 21:55 schrieb Bandan Das:

Quoted 6 lines

If get_free_page() fails for nested bitmap area, it's evident that we are gonna get screwed anyway but returning failure because we failed allocating memory for a nested structure seems like an unnecessary big hammer. Also, save the call for later; after we are done with other non-nested allocations.

Frankly, I prefer failures over automatic degradations. And, as you noted, the whole system will probably explode anyway if allocation of a single page already fails. So what does this buy us?

Yeah... I hear you. Ok, let me put it this way - Assume that we can defer this allocation up until the point that the nested subsystem is actually used i.e L1 tries running a guest and we try to allocate this area. If get_free_page() failed in that case, would we still want to kill L1 too ? I guess no.

Quoted 5 lines

Yeah... I hear you. Ok, let me put it this way - Assume that we can defer this allocation up until the point that the nested subsystem is actually used i.e L1 tries running a guest and we try to allocate this area. If get_free_page() failed in that case, would we still want to kill L1 too ? I guess no.

We could block the hypervisor thread on the allocation, just like it would block on faults for swapped out pages or new ones that have to be reclaimed from the page cache first.

Quoted 9 lines · Jan Kiszka

Quoted 5 lines

Yeah... I hear you. Ok, let me put it this way - Assume that we can defer this allocation up until the point that the nested subsystem is actually used i.e L1 tries running a guest and we try to allocate this area. If get_free_page() failed in that case, would we still want to kill L1 too ? I guess no.

We could block the hypervisor thread on the allocation, just like it would block on faults for swapped out pages or new ones that have to be reclaimed from the page cache first.

In that case we should avoid making the allocation GFP_ATOMIC to begin with.

If a GFP_KERNEL allocation failed, returning -ENOMEM from KVM_RUN (which practically means killing the guest) would actually be a very real possibility.

Paolo

Quoted 9 lines

Quoted 9 lines

Quoted 9 lines

Quoted 9 lines

Quoted 5 lines

Quoted 5 lines

Yeah... I hear you. Ok, let me put it this way - Assume that we can defer this allocation up until the point that the nested subsystem is actually used i.e L1 tries running a guest and we try to allocate this area. If get_free_page() failed in that case, would we still want to kill L1 too ? I guess no.

We could block the hypervisor thread on the allocation, just like it would block on faults for swapped out pages or new ones that have to be reclaimed from the page cache first.

So, block on a failure hoping that eventually it will succeed ?

Quoted 5 lines

Quoted 5 lines

In that case we should avoid making the allocation GFP_ATOMIC to begin with.

If a GFP_KERNEL allocation failed, returning -ENOMEM from KVM_RUN (which practically means killing the guest) would actually be a very real possibility.

Sorry Paolo, I missed your point. Isn't the allocation already GFP_KERNEL ?

If get_free_page() fails for nested bitmap area, it's evident that we are gonna get screwed anyway but returning failure because we failed allocating memory for a nested structure seems like an unnecessary big hammer. Also, save the call for later; after we are done with other non-nested allocations.

Signed-off-by Bandan Das <[email protected]>

---
arch/x86/kvm/vmx.c | 29 +++++++++++++++--------------

1 file changed, 15 insertions(+), 14 deletions(-)

arch/x86/kvm/vmx.c +12 -12

index f7b6168..200bc5c 100644
@@ -6039,20 +6039,22 @@ static __init int hardware_setup(void)
 	if (!vmx_msr_bitmap_longmode_x2apic)
 		goto out4;
 
-	if (nested) {
-		vmx_msr_bitmap_nested =
-			(unsigned long *)__get_free_page(GFP_KERNEL);
-		if (!vmx_msr_bitmap_nested)
-			goto out5;
-	}
-
 	vmx_vmread_bitmap = (unsigned long *)__get_free_page(GFP_KERNEL);
 	if (!vmx_vmread_bitmap)
-		goto out6;
+		goto out5;
 
 	vmx_vmwrite_bitmap = (unsigned long *)__get_free_page(GFP_KERNEL);
 	if (!vmx_vmwrite_bitmap)
-		goto out7;
+		goto out6;
+
+	if (nested) {
+		vmx_msr_bitmap_nested =
+			(unsigned long *)__get_free_page(GFP_KERNEL);
+		if (!vmx_msr_bitmap_nested) {
+			printk(KERN_WARNING
+			       "vmx: Failed getting memory for nested bitmap\n");
+			nested = 0;
+	}
 
 	memset(vmx_vmread_bitmap, 0xff, PAGE_SIZE);
 	memset(vmx_vmwrite_bitmap, 0xff, PAGE_SIZE);

@@ -6073,7 +6075,7 @@ static __init int hardware_setup(void)
 
 	if (setup_vmcs_config(&vmcs_config) < 0) {
 		r = -EIO;
-		goto out8;
+		goto out7;
 	}
 
 	if (boot_cpu_has(X86_FEATURE_NX))

@@ -6190,13 +6192,12 @@ static __init int hardware_setup(void)
 
 	return alloc_kvm_area();
 
-out8:
-	free_page((unsigned long)vmx_vmwrite_bitmap);

out7:

arch/x86/kvm/vmx.c +0 -2

-	free_page((unsigned long)vmx_vmread_bitmap);
-out6:

	if (nested)
		free_page((unsigned long)vmx_msr_bitmap_nested);

arch/x86/kvm/vmx.c +3 -0

+	free_page((unsigned long)vmx_vmwrite_bitmap);
+out6:
+	free_page((unsigned long)vmx_vmread_bitmap);

out5:

	free_page((unsigned long)vmx_msr_bitmap_longmode_x2apic);

out4:

free_page appears to check whether the address is zero before it actually frees the page. Perhaps it is better to leverage this behaviour to remove all the outX and simplify the code.