KVM: MMU: fix huge page adapted on non-PAE host

The huge page size is 4M on non-PAE host, but 2M page size is used in transparent_hugepage_adjust(), so the page we get after adjust the mapping level is not the head page, the BUG_ON() will be triggered

arch/x86/kvm/mmu.c +0 -0

index 72102e0..be3cea4 100644

Quoted 5 lines · Xiao Guangrong

The huge page size is 4M on non-PAE host, but 2M page size is used in transparent_hugepage_adjust(), so the page we get after adjust the mapping level is not the head page, the BUG_ON() will be triggered

Quoted 5 lines

arch/x86/kvm/mmu.c +0 -0

index 72102e0..be3cea4 100644

arch/x86/kvm/mmu.c static void transparent_hugepage_adjust(struct kvm_vcpu *vcpu, · line 2601

Quoted 9 lines

arch/x86/kvm/mmu.c +1 -2

@@ -2595,8 +2595,7 @@ static void transparent_hugepage_adjust(struct kvm_vcpu *vcpu,
 			*gfnp = gfn;
 			kvm_release_pfn_clean(pfn);
 			pfn &= ~mask;
-			if (!get_page_unless_zero(pfn_to_page(pfn)))
-				BUG();
+			kvm_get_pfn(pfn);
 			*pfnp = pfn;
 		}
 	}

Shouldn't we adjust mask instead?

Quoted 6 lines · Avi Kivity

Quoted 5 lines · Xiao Guangrong

The huge page size is 4M on non-PAE host, but 2M page size is used in transparent_hugepage_adjust(), so the page we get after adjust the mapping level is not the head page, the BUG_ON() will be triggered

Quoted 17 lines

Quoted 5 lines

arch/x86/kvm/mmu.c +0 -0

index 72102e0..be3cea4 100644

arch/x86/kvm/mmu.c static void transparent_hugepage_adjust(struct kvm_vcpu *vcpu, · line 2601

Quoted 9 lines

arch/x86/kvm/mmu.c +1 -2

@@ -2595,8 +2595,7 @@ static void transparent_hugepage_adjust(struct kvm_vcpu *vcpu,
 			*gfnp = gfn;
 			kvm_release_pfn_clean(pfn);
 			pfn &= ~mask;
-			if (!get_page_unless_zero(pfn_to_page(pfn)))
-				BUG();
+			kvm_get_pfn(pfn);
 			*pfnp = pfn;
 		}
 	}

Shouldn't we adjust mask instead?

Adjusting mask to map the whole 4M huge page to KVM guest?

But it seams 4M page size is not supported on VMX/SVM.

Quoted 27 lines · Xiao Guangrong

Quoted 23 lines · Avi Kivity

Quoted 10 lines · Xiao Guangrong

The huge page size is 4M on non-PAE host, but 2M page size is used in transparent_hugepage_adjust(), so the page we get after adjust the mapping level is not the head page, the BUG_ON() will be triggered

arch/x86/kvm/mmu.c +0 -0

index 72102e0..be3cea4 100644

arch/x86/kvm/mmu.c static void transparent_hugepage_adjust(struct kvm_vcpu *vcpu, · line 2601

Quoted 9 lines

arch/x86/kvm/mmu.c +1 -2

@@ -2595,8 +2595,7 @@ static void transparent_hugepage_adjust(struct kvm_vcpu *vcpu,
 			*gfnp = gfn;
 			kvm_release_pfn_clean(pfn);
 			pfn &= ~mask;
-			if (!get_page_unless_zero(pfn_to_page(pfn)))
-				BUG();
+			kvm_get_pfn(pfn);
 			*pfnp = pfn;
 		}
 	}

Shouldn't we adjust mask instead?

Quoted 1 line

Adjusting mask to map the whole 4M huge page to KVM guest?

The code moves the refcount from the small page to the huge page. i.e. from pfn 0x1312 to pfn 0x1200. But if the huge page frame contains 0x400 pages, it should move the refcount to pfn 0x1000.

Quoted 1 line

But it seams 4M page size is not supported on VMX/SVM.

We always use 64-bit PTEs in the lowest level, whether using shadow, EPT, or NPT. Note NPT supports 32-bit PTEs in the lowest level, but we don't support that configuration. But that doesn't mean we can't use host 4M pages to back guest 2M pages (or direct maps).

Quoted 28 lines · Avi Kivity

Quoted 27 lines · Xiao Guangrong

Quoted 23 lines · Avi Kivity

Quoted 10 lines · Xiao Guangrong

The huge page size is 4M on non-PAE host, but 2M page size is used in transparent_hugepage_adjust(), so the page we get after adjust the mapping level is not the head page, the BUG_ON() will be triggered

arch/x86/kvm/mmu.c +0 -0

index 72102e0..be3cea4 100644

arch/x86/kvm/mmu.c static void transparent_hugepage_adjust(struct kvm_vcpu *vcpu, · line 2601

Quoted 9 lines

arch/x86/kvm/mmu.c +1 -2

@@ -2595,8 +2595,7 @@ static void transparent_hugepage_adjust(struct kvm_vcpu *vcpu,
 			*gfnp = gfn;
 			kvm_release_pfn_clean(pfn);
 			pfn &= ~mask;
-			if (!get_page_unless_zero(pfn_to_page(pfn)))
-				BUG();
+			kvm_get_pfn(pfn);
 			*pfnp = pfn;
 		}
 	}

Shouldn't we adjust mask instead?

Quoted 6 lines

Quoted 1 line

Adjusting mask to map the whole 4M huge page to KVM guest?

The code moves the refcount from the small page to the huge page. i.e. from pfn 0x1312 to pfn 0x1200. But if the huge page frame contains 0x400 pages, it should move the refcount to pfn 0x1000.

We need not move the refcount to the huge page (the head of pages), moving the refcount to the any middle small page is also ok, get_page() will properly handle it:

get_page() -> __get_page_tail():

|	struct page *page_head = compound_trans_head(page);
|
|	if (likely(page != page_head && get_page_unless_zero(page_head))) {
|		/*
|		 * page_head wasn't a dangling pointer but it
|		 * may not be a head page anymore by the time
|		 * we obtain the lock. That is ok as long as it
|		 * can't be freed from under us.
|		 */
|		flags = compound_lock_irqsave(page_head);
|		/* here __split_huge_page_refcount won't run anymore */
|		if (likely(PageTail(page))) {
|			__get_page_tail_foll(page, false);
|			got = true;
|		}
|		compound_unlock_irqrestore(page_head, flags);
|		if (unlikely(!got))
|			put_page(page_head);
|	}

The refcount of page_head is increased.

Quoted 64 lines · Xiao Guangrong

Quoted 28 lines · Avi Kivity

Quoted 27 lines · Xiao Guangrong

Quoted 23 lines · Avi Kivity

Quoted 10 lines · Xiao Guangrong

The huge page size is 4M on non-PAE host, but 2M page size is used in transparent_hugepage_adjust(), so the page we get after adjust the mapping level is not the head page, the BUG_ON() will be triggered

arch/x86/kvm/mmu.c +0 -0

index 72102e0..be3cea4 100644

arch/x86/kvm/mmu.c static void transparent_hugepage_adjust(struct kvm_vcpu *vcpu, · line 2601

Quoted 9 lines

arch/x86/kvm/mmu.c +1 -2

@@ -2595,8 +2595,7 @@ static void transparent_hugepage_adjust(struct kvm_vcpu *vcpu,
 			*gfnp = gfn;
 			kvm_release_pfn_clean(pfn);
 			pfn &= ~mask;
-			if (!get_page_unless_zero(pfn_to_page(pfn)))
-				BUG();
+			kvm_get_pfn(pfn);
 			*pfnp = pfn;
 		}
 	}

Shouldn't we adjust mask instead?

Quoted 6 lines

Quoted 1 line

Adjusting mask to map the whole 4M huge page to KVM guest?

The code moves the refcount from the small page to the huge page. i.e. from pfn 0x1312 to pfn 0x1200. But if the huge page frame contains 0x400 pages, it should move the refcount to pfn 0x1000.

We need not move the refcount to the huge page (the head of pages), moving the refcount to the any middle small page is also ok, get_page() will properly handle it:

get_page() -> __get_page_tail():

|	struct page *page_head = compound_trans_head(page);
|
|	if (likely(page != page_head && get_page_unless_zero(page_head))) {
|		/*
|		 * page_head wasn't a dangling pointer but it
|		 * may not be a head page anymore by the time
|		 * we obtain the lock. That is ok as long as it
|		 * can't be freed from under us.
|		 */
|		flags = compound_lock_irqsave(page_head);
|		/* here __split_huge_page_refcount won't run anymore */
|		if (likely(PageTail(page))) {
|			__get_page_tail_foll(page, false);
|			got = true;
|		}
|		compound_unlock_irqrestore(page_head, flags);
|		if (unlikely(!got))
|			put_page(page_head);
|	}

Quoted 2 lines

The refcount of page_head is increased.

So, the whole thing is unneeded? Andrea?

Quoted 68 lines · Avi Kivity

Quoted 66 lines · Xiao Guangrong

Quoted 28 lines · Avi Kivity

Quoted 27 lines · Xiao Guangrong

Quoted 23 lines · Avi Kivity

Quoted 10 lines · Xiao Guangrong

The huge page size is 4M on non-PAE host, but 2M page size is used in transparent_hugepage_adjust(), so the page we get after adjust the mapping level is not the head page, the BUG_ON() will be triggered

arch/x86/kvm/mmu.c +0 -0

index 72102e0..be3cea4 100644

arch/x86/kvm/mmu.c static void transparent_hugepage_adjust(struct kvm_vcpu *vcpu, · line 2601

Quoted 9 lines

arch/x86/kvm/mmu.c +1 -2

@@ -2595,8 +2595,7 @@ static void transparent_hugepage_adjust(struct kvm_vcpu *vcpu,
 			*gfnp = gfn;
 			kvm_release_pfn_clean(pfn);
 			pfn &= ~mask;
-			if (!get_page_unless_zero(pfn_to_page(pfn)))
-				BUG();
+			kvm_get_pfn(pfn);
 			*pfnp = pfn;
 		}
 	}

Shouldn't we adjust mask instead?

Quoted 6 lines

Quoted 1 line

Adjusting mask to map the whole 4M huge page to KVM guest?

The code moves the refcount from the small page to the huge page. i.e. from pfn 0x1312 to pfn 0x1200. But if the huge page frame contains 0x400 pages, it should move the refcount to pfn 0x1000.

We need not move the refcount to the huge page (the head of pages), moving the refcount to the any middle small page is also ok, get_page() will properly handle it:

get_page() -> __get_page_tail():

|	struct page *page_head = compound_trans_head(page);
|
|	if (likely(page != page_head && get_page_unless_zero(page_head))) {
|		/*
|		 * page_head wasn't a dangling pointer but it
|		 * may not be a head page anymore by the time
|		 * we obtain the lock. That is ok as long as it
|		 * can't be freed from under us.
|		 */
|		flags = compound_lock_irqsave(page_head);
|		/* here __split_huge_page_refcount won't run anymore */
|		if (likely(PageTail(page))) {
|			__get_page_tail_foll(page, false);
|			got = true;
|		}
|		compound_unlock_irqrestore(page_head, flags);
|		if (unlikely(!got))
|			put_page(page_head);
|	}

The refcount of page_head is increased.

Quoted 2 lines

So, the whole thing is unneeded? Andrea?

I think the reason we move refcount in current code is, we should increase the refcount of the page we will mapped into shadow page table, since we always decrease its refcount after it is mapped. (That is this patch does.)

Quoted 74 lines · Xiao Guangrong

Quoted 70 lines · Avi Kivity

Quoted 64 lines · Xiao Guangrong

Quoted 28 lines · Avi Kivity

Quoted 27 lines · Xiao Guangrong

Quoted 23 lines · Avi Kivity

Quoted 10 lines · Xiao Guangrong

The huge page size is 4M on non-PAE host, but 2M page size is used in transparent_hugepage_adjust(), so the page we get after adjust the mapping level is not the head page, the BUG_ON() will be triggered

arch/x86/kvm/mmu.c +0 -0

index 72102e0..be3cea4 100644

arch/x86/kvm/mmu.c static void transparent_hugepage_adjust(struct kvm_vcpu *vcpu, · line 2601

Quoted 9 lines

arch/x86/kvm/mmu.c +1 -2

@@ -2595,8 +2595,7 @@ static void transparent_hugepage_adjust(struct kvm_vcpu *vcpu,
 			*gfnp = gfn;
 			kvm_release_pfn_clean(pfn);
 			pfn &= ~mask;
-			if (!get_page_unless_zero(pfn_to_page(pfn)))
-				BUG();
+			kvm_get_pfn(pfn);
 			*pfnp = pfn;
 		}
 	}

Shouldn't we adjust mask instead?

Quoted 6 lines

Quoted 1 line

Adjusting mask to map the whole 4M huge page to KVM guest?

The code moves the refcount from the small page to the huge page. i.e. from pfn 0x1312 to pfn 0x1200. But if the huge page frame contains 0x400 pages, it should move the refcount to pfn 0x1000.

We need not move the refcount to the huge page (the head of pages), moving the refcount to the any middle small page is also ok, get_page() will properly handle it:

get_page() -> __get_page_tail():

|	struct page *page_head = compound_trans_head(page);
|
|	if (likely(page != page_head && get_page_unless_zero(page_head))) {
|		/*
|		 * page_head wasn't a dangling pointer but it
|		 * may not be a head page anymore by the time
|		 * we obtain the lock. That is ok as long as it
|		 * can't be freed from under us.
|		 */
|		flags = compound_lock_irqsave(page_head);
|		/* here __split_huge_page_refcount won't run anymore */
|		if (likely(PageTail(page))) {
|			__get_page_tail_foll(page, false);
|			got = true;
|		}
|		compound_unlock_irqrestore(page_head, flags);
|		if (unlikely(!got))
|			put_page(page_head);
|	}

Quoted 2 lines

The refcount of page_head is increased.

So, the whole thing is unneeded? Andrea?

Quoted 4 lines

I think the reason we move refcount in current code is, we should increase the refcount of the page we will mapped into shadow page table, since we always decrease its refcount after it is mapped. (That is this patch does.)

As far as I can tell __get_user_pages_fast() will take the reference count in the page head in the first place.

Quoted 10 lines

Quoted 6 lines

I think the reason we move refcount in current code is, we should increase the refcount of the page we will mapped into shadow page table, since we always decrease its refcount after it is mapped. (That is this patch does.)

As far as I can tell __get_user_pages_fast() will take the reference count in the page head in the first place.

IIUC, the refcount used in the Compound Page is like this:

get_user_pages / get_page(page):

	head_page = page->first_page;
	if (page is not the head page)
		page->__mapcount++
	head_page->_count++

put_page(page):

	head_page = page->first_page;
	if (page is not the head page)
		page->__mapcount--
	head_page->_count--

The "right thing" we should be doing is running get_page() on every small page within the frame (we asked for a small page but are opportunistrically using the pages around it, without a proper ref). That's a bit slow though, so we cheat.

But I guess we can start with your fix. But what about shifting mask by one bit? Isn't it sufficient?

(quoted diff) +3 -1

-		mask = KVM_PAGES_PER_HPAGE(level) - 1;
+		mask = KVM_PAGES_PER_HPAGE(level);
+		mask *= KVM_HOST_HPAGES_PER_HPAGE;
+		mask -= 1;

This should move the reference to the right place.

As far as I can tell __get_user_pages_fast() will take the reference count in the page head in the first place.

Hi,

Quoted 2 lines · Avi Kivity

As far as I can tell __get_user_pages_fast() will take the reference count in the page head in the first place.

    mask = KVM_PAGES_PER_HPAGE(level) - 1;

The BUG would trigger if the above KVM mask is 2M (that is the NPT/EPT pmd size), but the hugepage size in the host is 4M (noPAE 32bit).

The refcount is taken only in the head page for heads, and in both for tails.

Because we've mmu notifier, we never keep the pages mapped by sptes refcounted, we drop them all. So all we need to do is just to move the refcount on the same exact pfn that is then freed by mmu_set_spte (kvm_release_pfn_clean at the end).

The adjustement is not done for the refcounting, the issue here is, we want to adjust the "pfn" passed to mmu_set_spte, and in turn we've to move the refcounting too, because the kvm_release_pfn_clean will run on that "pfn" (not on the pfn returned by gup-fast anymore).

So it looks fine to just do get_page and the patch looks correct (not sure if the mmio the mmio check is needed or if we can just do get_page) as long as the "pfn" that is returned through &pfn parameter and then passssed to mmu_set_sptes is the same one were we do get_page.

The reason it was a get_page_unless_zero() is that it wanted to check that there was no THP split and the head page was still there. Problem is that with a 4M host page size and 2M NTP/EPT pmd size, we need to get_page a tail page half of the time, and get_page_unless_zero() won't be a correct refcount for tail pages, not equivalent to a full get_page.

Overall the most important thing is that the pfn returned is the correct one that matches the alignment of the NPT/EPT hugepmd size, the refcounting just closely follows that aligned "pfn".

Yes, I see it now. Adjusting mask is incorrect since we won't have the same adjustment on release. I'll apply the patch for 3.5.

Quoted 2 lines · Avi Kivity

Yes, I see it now. Adjusting mask is incorrect since we won't have the same adjustment on release. I'll apply the patch for 3.5.

Sounds great to me. One thing I'm not sure about is about the real need of the mmio check vs a stright get_page (we shouldn't ever get to a gup-fast succeeding if this was a mmio region?) but I don't see chances that it could hurt either.