[PATCH 0/5] Fix wrong gfn range of tlb flushing with range

[PATCH 0/5] Fix wrong gfn range of tlb flushing with range

[Hou Wenlong]

Commit c3134ce240eed
("KVM: Replace old tlb flush function with new one to flush a specified range.")
replaces old tlb flush function with kvm_flush_remote_tlbs_with_address()
to do tlb flushing. However, the gfn range of tlb flushing is wrong in
some cases. E.g., when a spte is dropped, the start gfn of tlb flushing
should be the gfn of spte not the base gfn of SP which contains the spte.
So this patchset would fix them and do some cleanups.

Hou Wenlong (5):
  KVM: x86/mmu: Fix wrong gfn range of tlb flushing in
    validate_direct_spte()
  KVM: x86/mmu: Fix wrong gfn range of tlb flushing in
    kvm_set_pte_rmapp()
  KVM: x86/mmu: Reduce gfn range of tlb flushing in
    tdp_mmu_map_handle_target_level()
  KVM: x86/mmu: Fix wrong start gfn of tlb flushing with range
  KVM: x86/mmu: Use 1 as the size of gfn range for tlb flushing in
    FNAME(invlpg)()

 arch/x86/kvm/mmu/mmu.c         | 15 +++++++++------
 arch/x86/kvm/mmu/paging_tmpl.h |  2 +-
 arch/x86/kvm/mmu/tdp_mmu.c     |  4 ++--
 3 files changed, 12 insertions(+), 9 deletions(-)

--
2.31.1

[PATCH 1/5] KVM: x86/mmu: Fix wrong gfn range of tlb flushing in validate_direct_spte()

[Hou Wenlong]

The spte pointing to the children SP is dropped, so the whole gfn range
covered by the children SP should be flushed.

Fixes: c3134ce240eed ("KVM: Replace old tlb flush function with new one to flush a specified range.")
Signed-off-by: Hou Wenlong <houwenlong.hwl@antgroup.com>
---
 arch/x86/kvm/mmu/mmu.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
index 79c6a821ea0d..b8a1f5b46b9d 100644
--- a/arch/x86/kvm/mmu/mmu.c
+++ b/arch/x86/kvm/mmu/mmu.c
@@ -2333,7 +2333,8 @@ static void validate_direct_spte(struct kvm_vcpu *vcpu, u64 *sptep,
 			return;
 
 		drop_parent_pte(child, sptep);
-		kvm_flush_remote_tlbs_with_address(vcpu->kvm, child->gfn, 1);
+		kvm_flush_remote_tlbs_with_address(vcpu->kvm, child->gfn,
+				KVM_PAGES_PER_HPAGE(child->role.level + 1));
 	}
 }
 
-- 
2.31.1

[PATCH 2/5] KVM: x86/mmu: Fix wrong gfn range of tlb flushing in kvm_set_pte_rmapp()

[Hou Wenlong]

When the spte of hupe page is dropped in kvm_set_pte_rmapp(),
the whole gfn range covered by the spte should be flushed.
However, rmap_walk_init_level() doesn't align down the gfn
for new level like tdp iterator does, then the gfn used in
kvm_set_pte_rmapp() is not the base gfn of huge page. And
the size of gfn range is wrong too for huge page. Since
the base gfn of huge page is more meaningful during the
rmap walking, so align down the gfn for new level and use
the correct size of huge page for tlb flushing in
kvm_set_pte_rmapp().

Fixes: c3134ce240eed ("KVM: Replace old tlb flush function with new one to flush a specified range.")
Signed-off-by: Hou Wenlong <houwenlong.hwl@antgroup.com>
---
 arch/x86/kvm/mmu/mmu.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
index b8a1f5b46b9d..37bfc88ea212 100644
--- a/arch/x86/kvm/mmu/mmu.c
+++ b/arch/x86/kvm/mmu/mmu.c
@@ -1427,7 +1427,7 @@ static bool kvm_set_pte_rmapp(struct kvm *kvm, struct kvm_rmap_head *rmap_head,
 	}
 
 	if (need_flush && kvm_available_flush_tlb_with_range()) {
-		kvm_flush_remote_tlbs_with_address(kvm, gfn, 1);
+		kvm_flush_remote_tlbs_with_address(kvm, gfn, KVM_PAGES_PER_HPAGE(level));
 		return false;
 	}
 
@@ -1455,7 +1455,7 @@ static void
 rmap_walk_init_level(struct slot_rmap_walk_iterator *iterator, int level)
 {
 	iterator->level = level;
-	iterator->gfn = iterator->start_gfn;
+	iterator->gfn = iterator->start_gfn & -KVM_PAGES_PER_HPAGE(level);
 	iterator->rmap = gfn_to_rmap(iterator->gfn, level, iterator->slot);
 	iterator->end_rmap = gfn_to_rmap(iterator->end_gfn, level, iterator->slot);
 }
-- 
2.31.1

Re: [PATCH 2/5] KVM: x86/mmu: Fix wrong gfn range of tlb flushing in kvm_set_pte_rmapp()

[Sean Christopherson]

On Fri, Jun 24, 2022, Hou Wenlong wrote:
> When the spte of hupe page is dropped in kvm_set_pte_rmapp(),
> the whole gfn range covered by the spte should be flushed.
> However, rmap_walk_init_level() doesn't align down the gfn
> for new level like tdp iterator does, then the gfn used in
> kvm_set_pte_rmapp() is not the base gfn of huge page. And
> the size of gfn range is wrong too for huge page. Since
> the base gfn of huge page is more meaningful during the
> rmap walking, so align down the gfn for new level and use
> the correct size of huge page for tlb flushing in
> kvm_set_pte_rmapp().

It's also worth noting that kvm_set_pte_rmapp() is the other user of the rmap
iterators that consumes @gfn, i.e. modifying iterator->gfn is safe-ish.

> Fixes: c3134ce240eed ("KVM: Replace old tlb flush function with new one to flush a specified range.")
> Signed-off-by: Hou Wenlong <houwenlong.hwl@antgroup.com>
> ---
>  arch/x86/kvm/mmu/mmu.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
> index b8a1f5b46b9d..37bfc88ea212 100644
> --- a/arch/x86/kvm/mmu/mmu.c
> +++ b/arch/x86/kvm/mmu/mmu.c
> @@ -1427,7 +1427,7 @@ static bool kvm_set_pte_rmapp(struct kvm *kvm, struct kvm_rmap_head *rmap_head,
>  	}
>  
>  	if (need_flush && kvm_available_flush_tlb_with_range()) {
> -		kvm_flush_remote_tlbs_with_address(kvm, gfn, 1);
> +		kvm_flush_remote_tlbs_with_address(kvm, gfn, KVM_PAGES_PER_HPAGE(level));
>  		return false;
>  	}
>  
> @@ -1455,7 +1455,7 @@ static void
>  rmap_walk_init_level(struct slot_rmap_walk_iterator *iterator, int level)
>  {
>  	iterator->level = level;
> -	iterator->gfn = iterator->start_gfn;
> +	iterator->gfn = iterator->start_gfn & -KVM_PAGES_PER_HPAGE(level);

Hrm, arguably this be done on start_gfn in slot_rmap_walk_init().  Having iter->gfn
be less than iter->start_gfn will be odd.

>  	iterator->rmap = gfn_to_rmap(iterator->gfn, level, iterator->slot);
>  	iterator->end_rmap = gfn_to_rmap(iterator->end_gfn, level, iterator->slot);
>  }
> -- 
> 2.31.1
> 

Re: [PATCH 2/5] KVM: x86/mmu: Fix wrong gfn range of tlb flushing in kvm_set_pte_rmapp()

[Hou Wenlong]

On Sat, Jun 25, 2022 at 06:53:25AM +0800, Sean Christopherson wrote:
> On Fri, Jun 24, 2022, Hou Wenlong wrote:
> > When the spte of hupe page is dropped in kvm_set_pte_rmapp(),
> > the whole gfn range covered by the spte should be flushed.
> > However, rmap_walk_init_level() doesn't align down the gfn
> > for new level like tdp iterator does, then the gfn used in
> > kvm_set_pte_rmapp() is not the base gfn of huge page. And
> > the size of gfn range is wrong too for huge page. Since
> > the base gfn of huge page is more meaningful during the
> > rmap walking, so align down the gfn for new level and use
> > the correct size of huge page for tlb flushing in
> > kvm_set_pte_rmapp().
> 
> It's also worth noting that kvm_set_pte_rmapp() is the other user of the rmap
> iterators that consumes @gfn, i.e. modifying iterator->gfn is safe-ish.
>
> > Fixes: c3134ce240eed ("KVM: Replace old tlb flush function with new one to flush a specified range.")
> > Signed-off-by: Hou Wenlong <houwenlong.hwl@antgroup.com>
> > ---
> >  arch/x86/kvm/mmu/mmu.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
> > index b8a1f5b46b9d..37bfc88ea212 100644
> > --- a/arch/x86/kvm/mmu/mmu.c
> > +++ b/arch/x86/kvm/mmu/mmu.c
> > @@ -1427,7 +1427,7 @@ static bool kvm_set_pte_rmapp(struct kvm *kvm, struct kvm_rmap_head *rmap_head,
> >  	}
> >  
> >  	if (need_flush && kvm_available_flush_tlb_with_range()) {
> > -		kvm_flush_remote_tlbs_with_address(kvm, gfn, 1);
> > +		kvm_flush_remote_tlbs_with_address(kvm, gfn, KVM_PAGES_PER_HPAGE(level));
> >  		return false;
> >  	}
> >  
> > @@ -1455,7 +1455,7 @@ static void
> >  rmap_walk_init_level(struct slot_rmap_walk_iterator *iterator, int level)
> >  {
> >  	iterator->level = level;
> > -	iterator->gfn = iterator->start_gfn;
> > +	iterator->gfn = iterator->start_gfn & -KVM_PAGES_PER_HPAGE(level);
> 
> Hrm, arguably this be done on start_gfn in slot_rmap_walk_init().  Having iter->gfn
> be less than iter->start_gfn will be odd.
>
Hrm, iter->gfn may be bigger than iter->end_gfn in slot_rmap_walk_next(), which would
also be odd. I just think it may be better to pass the base gfn of huge page. However,
as you said, kvm_set_pte_rmapp() is the only user of the rmap iterator that consumes @gfn,
only modifying it in that function is enough.

> >  	iterator->rmap = gfn_to_rmap(iterator->gfn, level, iterator->slot);
> >  	iterator->end_rmap = gfn_to_rmap(iterator->end_gfn, level, iterator->slot);
> >  }
> > -- 
> > 2.31.1
> > 

[PATCH 3/5] KVM: x86/mmu: Reduce gfn range of tlb flushing in tdp_mmu_map_handle_target_level()

[Hou Wenlong]

Since the children SP is zapped, the gfn range of tlb flushing should be
the range covered by children SP not parent SP. Replace sp->gfn which is
the base gfn of parent SP with iter->gfn and use the correct size of
gfn range for children SP to reduce tlb flushing range.

Signed-off-by: Hou Wenlong <houwenlong.hwl@antgroup.com>
---
 arch/x86/kvm/mmu/tdp_mmu.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kvm/mmu/tdp_mmu.c b/arch/x86/kvm/mmu/tdp_mmu.c
index f3a430d64975..85838ae169b8 100644
--- a/arch/x86/kvm/mmu/tdp_mmu.c
+++ b/arch/x86/kvm/mmu/tdp_mmu.c
@@ -1075,8 +1075,8 @@ static int tdp_mmu_map_handle_target_level(struct kvm_vcpu *vcpu,
 		return RET_PF_RETRY;
 	else if (is_shadow_present_pte(iter->old_spte) &&
 		 !is_last_spte(iter->old_spte, iter->level))
-		kvm_flush_remote_tlbs_with_address(vcpu->kvm, sp->gfn,
-						   KVM_PAGES_PER_HPAGE(iter->level + 1));
+		kvm_flush_remote_tlbs_with_address(vcpu->kvm, iter->gfn,
+						   KVM_PAGES_PER_HPAGE(iter->level));
 
 	/*
 	 * If the page fault was caused by a write but the page is write
-- 
2.31.1

[PATCH 4/5] KVM: x86/mmu: Fix wrong start gfn of tlb flushing with range

[Hou Wenlong]

When a spte is dropped, the start gfn of tlb flushing should
be the gfn of spte not the base gfn of SP which contains the
spte.

Fixes: c3134ce240eed ("KVM: Replace old tlb flush function with new one to flush a specified range.")
Signed-off-by: Hou Wenlong <houwenlong.hwl@antgroup.com>
---
 arch/x86/kvm/mmu/mmu.c         | 8 +++++---
 arch/x86/kvm/mmu/paging_tmpl.h | 3 ++-
 2 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
index 37bfc88ea212..577b85860891 100644
--- a/arch/x86/kvm/mmu/mmu.c
+++ b/arch/x86/kvm/mmu/mmu.c
@@ -1145,7 +1145,8 @@ static void drop_large_spte(struct kvm *kvm, u64 *sptep, bool flush)
 	drop_spte(kvm, sptep);
 
 	if (flush)
-		kvm_flush_remote_tlbs_with_address(kvm, sp->gfn,
+		kvm_flush_remote_tlbs_with_address(kvm,
+			kvm_mmu_page_get_gfn(sp, sptep - sp->spt),
 			KVM_PAGES_PER_HPAGE(sp->role.level));
 }
 
@@ -1596,7 +1597,7 @@ static void __rmap_add(struct kvm *kvm,
 	if (rmap_count > RMAP_RECYCLE_THRESHOLD) {
 		kvm_unmap_rmapp(kvm, rmap_head, NULL, gfn, sp->role.level, __pte(0));
 		kvm_flush_remote_tlbs_with_address(
-				kvm, sp->gfn, KVM_PAGES_PER_HPAGE(sp->role.level));
+				kvm, gfn, KVM_PAGES_PER_HPAGE(sp->role.level));
 	}
 }
 
@@ -6397,7 +6398,8 @@ static bool kvm_mmu_zap_collapsible_spte(struct kvm *kvm,
 			pte_list_remove(kvm, rmap_head, sptep);
 
 			if (kvm_available_flush_tlb_with_range())
-				kvm_flush_remote_tlbs_with_address(kvm, sp->gfn,
+				kvm_flush_remote_tlbs_with_address(kvm,
+					kvm_mmu_page_get_gfn(sp, sptep - sp->spt),
 					KVM_PAGES_PER_HPAGE(sp->role.level));
 			else
 				need_tlb_flush = 1;
diff --git a/arch/x86/kvm/mmu/paging_tmpl.h b/arch/x86/kvm/mmu/paging_tmpl.h
index 2448fa8d8438..fa78ee0caffd 100644
--- a/arch/x86/kvm/mmu/paging_tmpl.h
+++ b/arch/x86/kvm/mmu/paging_tmpl.h
@@ -938,7 +938,8 @@ static void FNAME(invlpg)(struct kvm_vcpu *vcpu, gva_t gva, hpa_t root_hpa)
 			mmu_page_zap_pte(vcpu->kvm, sp, sptep, NULL);
 			if (is_shadow_present_pte(old_spte))
 				kvm_flush_remote_tlbs_with_address(vcpu->kvm,
-					sp->gfn, KVM_PAGES_PER_HPAGE(sp->role.level));
+					kvm_mmu_page_get_gfn(sp, sptep - sp->spt),
+					KVM_PAGES_PER_HPAGE(sp->role.level));
 
 			if (!rmap_can_add(vcpu))
 				break;
-- 
2.31.1

[PATCH 5/5] KVM: x86/mmu: Use 1 as the size of gfn range for tlb flushing in FNAME(invlpg)()

[Hou Wenlong]

Only SP with PG_LEVLE_4K level could be unsync, so the size of gfn range
must be 1.

Signed-off-by: Hou Wenlong <houwenlong.hwl@antgroup.com>
---
 arch/x86/kvm/mmu/paging_tmpl.h | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/arch/x86/kvm/mmu/paging_tmpl.h b/arch/x86/kvm/mmu/paging_tmpl.h
index fa78ee0caffd..fc6d8dcff019 100644
--- a/arch/x86/kvm/mmu/paging_tmpl.h
+++ b/arch/x86/kvm/mmu/paging_tmpl.h
@@ -938,8 +938,7 @@ static void FNAME(invlpg)(struct kvm_vcpu *vcpu, gva_t gva, hpa_t root_hpa)
 			mmu_page_zap_pte(vcpu->kvm, sp, sptep, NULL);
 			if (is_shadow_present_pte(old_spte))
 				kvm_flush_remote_tlbs_with_address(vcpu->kvm,
-					kvm_mmu_page_get_gfn(sp, sptep - sp->spt),
-					KVM_PAGES_PER_HPAGE(sp->role.level));
+					kvm_mmu_page_get_gfn(sp, sptep - sp->spt), 1);
 
 			if (!rmap_can_add(vcpu))
 				break;
-- 
2.31.1

Re: [PATCH 0/5] Fix wrong gfn range of tlb flushing with range

[Sean Christopherson]

On Fri, Jun 24, 2022, Hou Wenlong wrote:
> Commit c3134ce240eed
> ("KVM: Replace old tlb flush function with new one to flush a specified range.")
> replaces old tlb flush function with kvm_flush_remote_tlbs_with_address()
> to do tlb flushing. However, the gfn range of tlb flushing is wrong in
> some cases. E.g., when a spte is dropped, the start gfn of tlb flushing

Heh, "some" cases.  Looks like KVM is wrong on 7 of 15 cases.  And IIRC, there
were already several rounds of fixes due to passing "end" instead of "nr_pages".

Patches look ok on a quick read through, but I'd have to stare a bunch more to
be confident.

Part of me wonders if we should just revert the whole thing and then only reintroduce
range-based flushing with proper testing and maybe even require performance numbers
to justify the benefits.  Give that almost 50% of the users are broken, it's pretty
obvious that no one running KVM actually tests the behavior.

> should be the gfn of spte not the base gfn of SP which contains the spte.
> So this patchset would fix them and do some cleanups.
> 
> Hou Wenlong (5):
>   KVM: x86/mmu: Fix wrong gfn range of tlb flushing in
>     validate_direct_spte()
>   KVM: x86/mmu: Fix wrong gfn range of tlb flushing in
>     kvm_set_pte_rmapp()
>   KVM: x86/mmu: Reduce gfn range of tlb flushing in
>     tdp_mmu_map_handle_target_level()
>   KVM: x86/mmu: Fix wrong start gfn of tlb flushing with range
>   KVM: x86/mmu: Use 1 as the size of gfn range for tlb flushing in
>     FNAME(invlpg)()
> 
>  arch/x86/kvm/mmu/mmu.c         | 15 +++++++++------
>  arch/x86/kvm/mmu/paging_tmpl.h |  2 +-
>  arch/x86/kvm/mmu/tdp_mmu.c     |  4 ++--
>  3 files changed, 12 insertions(+), 9 deletions(-)
> 
> --
> 2.31.1
> 

Re: [PATCH 0/5] Fix wrong gfn range of tlb flushing with range

[Paolo Bonzini]

On 6/25/22 01:06, Sean Christopherson wrote:
>> ("KVM: Replace old tlb flush function with new one to flush a specified range.")
>> replaces old tlb flush function with kvm_flush_remote_tlbs_with_address()
>> to do tlb flushing. However, the gfn range of tlb flushing is wrong in
>> some cases. E.g., when a spte is dropped, the start gfn of tlb flushing
> Heh, "some" cases.  Looks like KVM is wrong on 7 of 15 cases.  And IIRC, there
> were already several rounds of fixes due to passing "end" instead of "nr_pages".
> 
> Patches look ok on a quick read through, but I'd have to stare a bunch more to
> be confident.
> 
> Part of me wonders if we should just revert the whole thing and then only reintroduce
> range-based flushing with proper testing and maybe even require performance numbers
> to justify the benefits.  Give that almost 50% of the users are broken, it's pretty
> obvious that no one running KVM actually tests the behavior.
> 

I'm pretty sure it's in use on Azure.  Some of the changes are flushing 
less, for the others it's more than likely that Hyper-V treats a 1-page 
flush the same if the address points to a huge page.

Paolo

Re: [PATCH 0/5] Fix wrong gfn range of tlb flushing with range

[Hou Wenlong]

On Sat, Jun 25, 2022 at 05:13:22PM +0800, Paolo Bonzini wrote:
> On 6/25/22 01:06, Sean Christopherson wrote:
> >>("KVM: Replace old tlb flush function with new one to flush a specified range.")
> >>replaces old tlb flush function with kvm_flush_remote_tlbs_with_address()
> >>to do tlb flushing. However, the gfn range of tlb flushing is wrong in
> >>some cases. E.g., when a spte is dropped, the start gfn of tlb flushing
> >Heh, "some" cases.  Looks like KVM is wrong on 7 of 15 cases.  And IIRC, there
> >were already several rounds of fixes due to passing "end" instead of "nr_pages".
> >
> >Patches look ok on a quick read through, but I'd have to stare a bunch more to
> >be confident.
> >
> >Part of me wonders if we should just revert the whole thing and then only reintroduce
> >range-based flushing with proper testing and maybe even require performance numbers
> >to justify the benefits.  Give that almost 50% of the users are broken, it's pretty
> >obvious that no one running KVM actually tests the behavior.
> >
> 
> I'm pretty sure it's in use on Azure.  Some of the changes are
> flushing less, for the others it's more than likely that Hyper-V
> treats a 1-page flush the same if the address points to a huge page.
> 
I lookup hyperv_fill_flush_guest_mapping_list(), gpa_list.page.largepage
is always false. Or the behaviour you said is implemented in Hyper-V not
in KVM ? 

> Paolo

Re: [PATCH 0/5] Fix wrong gfn range of tlb flushing with range

[Paolo Bonzini]

On 6/27/22 06:15, Hou Wenlong wrote:
>> I'm pretty sure it's in use on Azure.  Some of the changes are
>> flushing less, for the others it's more than likely that Hyper-V
>> treats a 1-page flush the same if the address points to a huge page.
>>
> I lookup hyperv_fill_flush_guest_mapping_list(), gpa_list.page.largepage
> is always false. Or the behaviour you said is implemented in Hyper-V not
> in KVM ?
> 

Yes, I mean in Hyper-V (and/or in the processor).

Paolo

Re: [PATCH 0/5] Fix wrong gfn range of tlb flushing with range

[David Matlack]

On Fri, Jun 24, 2022 at 11:36:56AM +0800, Hou Wenlong wrote:
> Commit c3134ce240eed
> ("KVM: Replace old tlb flush function with new one to flush a specified range.")
> replaces old tlb flush function with kvm_flush_remote_tlbs_with_address()
> to do tlb flushing. However, the gfn range of tlb flushing is wrong in
> some cases. E.g., when a spte is dropped, the start gfn of tlb flushing
> should be the gfn of spte not the base gfn of SP which contains the spte.
> So this patchset would fix them and do some cleanups.

One thing that would help prevent future buggy use of
kvm_flush_remote_tlbs_with_address(), and clean up this series, would be to
introduce some helper functions for common operations. In fact, even if
there is only one caller, I still think it would be useful to have helper
functions because it makes it clear the author's intent.

For example, I think the following helpers would be useful in this series:

/* Flush the given page (huge or not) of guest memory. */
static void kvm_flush_remote_tlbs_gfn(struct kvm *kvm, gfn_t gfn, int level)
{
        u64 pages = KVM_PAGES_PER_HPAGE(level);

        kvm_flush_remote_tlbs_with_address(kvm, gfn, pages);
}

/* Flush the range of guest memory mapped by the given SPTE. */
static void kvm_flush_remote_tlbs_sptep(struct kvm *kvm, u64 *sptep)
{
        struct kvm_mmu_page *sp = sptep_to_sp(sptep);
        gfn_t gfn = kvm_mmu_page_get_gfn(sp, spte_index(sptep));

        kvm_flush_remote_tlbs_gfn(kvm, gfn, sp->role.level);
}

/* Flush all memory mapped by the given direct SP. */
static void kvm_flush_remote_tlbs_direct_sp(struct kvm *kvm, struct kvm_mmu_page *sp)
{
        WARN_ON_ONCE(!sp->role.direct);
        kvm_flush_remote_tlbs_gfn(kvm, sp->gfn, sp->role.level + 1);
}

> 
> Hou Wenlong (5):
>   KVM: x86/mmu: Fix wrong gfn range of tlb flushing in
>     validate_direct_spte()
>   KVM: x86/mmu: Fix wrong gfn range of tlb flushing in
>     kvm_set_pte_rmapp()
>   KVM: x86/mmu: Reduce gfn range of tlb flushing in
>     tdp_mmu_map_handle_target_level()
>   KVM: x86/mmu: Fix wrong start gfn of tlb flushing with range
>   KVM: x86/mmu: Use 1 as the size of gfn range for tlb flushing in
>     FNAME(invlpg)()
> 
>  arch/x86/kvm/mmu/mmu.c         | 15 +++++++++------
>  arch/x86/kvm/mmu/paging_tmpl.h |  2 +-
>  arch/x86/kvm/mmu/tdp_mmu.c     |  4 ++--
>  3 files changed, 12 insertions(+), 9 deletions(-)
> 
> --
> 2.31.1
> 

Re: [PATCH 0/5] Fix wrong gfn range of tlb flushing with range

[Hou Wenlong]

On Wed, Jul 27, 2022 at 06:59:20AM +0800, David Matlack wrote:
> On Fri, Jun 24, 2022 at 11:36:56AM +0800, Hou Wenlong wrote:
> > Commit c3134ce240eed
> > ("KVM: Replace old tlb flush function with new one to flush a specified range.")
> > replaces old tlb flush function with kvm_flush_remote_tlbs_with_address()
> > to do tlb flushing. However, the gfn range of tlb flushing is wrong in
> > some cases. E.g., when a spte is dropped, the start gfn of tlb flushing
> > should be the gfn of spte not the base gfn of SP which contains the spte.
> > So this patchset would fix them and do some cleanups.
> 
> One thing that would help prevent future buggy use of
> kvm_flush_remote_tlbs_with_address(), and clean up this series, would be to
> introduce some helper functions for common operations. In fact, even if
> there is only one caller, I still think it would be useful to have helper
> functions because it makes it clear the author's intent.
> 
> For example, I think the following helpers would be useful in this series:
> 
> /* Flush the given page (huge or not) of guest memory. */
> static void kvm_flush_remote_tlbs_gfn(struct kvm *kvm, gfn_t gfn, int level)
> {
>         u64 pages = KVM_PAGES_PER_HPAGE(level);
> 
>         kvm_flush_remote_tlbs_with_address(kvm, gfn, pages);
> }
> 
> /* Flush the range of guest memory mapped by the given SPTE. */
> static void kvm_flush_remote_tlbs_sptep(struct kvm *kvm, u64 *sptep)
> {
>         struct kvm_mmu_page *sp = sptep_to_sp(sptep);
>         gfn_t gfn = kvm_mmu_page_get_gfn(sp, spte_index(sptep));
> 
>         kvm_flush_remote_tlbs_gfn(kvm, gfn, sp->role.level);
> }
> 
> /* Flush all memory mapped by the given direct SP. */
> static void kvm_flush_remote_tlbs_direct_sp(struct kvm *kvm, struct kvm_mmu_page *sp)
> {
>         WARN_ON_ONCE(!sp->role.direct);
>         kvm_flush_remote_tlbs_gfn(kvm, sp->gfn, sp->role.level + 1);
> }
>
Thanks for your good suggestions, I'll add those helpers in the next version.

> > 
> > Hou Wenlong (5):
> >   KVM: x86/mmu: Fix wrong gfn range of tlb flushing in
> >     validate_direct_spte()
> >   KVM: x86/mmu: Fix wrong gfn range of tlb flushing in
> >     kvm_set_pte_rmapp()
> >   KVM: x86/mmu: Reduce gfn range of tlb flushing in
> >     tdp_mmu_map_handle_target_level()
> >   KVM: x86/mmu: Fix wrong start gfn of tlb flushing with range
> >   KVM: x86/mmu: Use 1 as the size of gfn range for tlb flushing in
> >     FNAME(invlpg)()
> > 
> >  arch/x86/kvm/mmu/mmu.c         | 15 +++++++++------
> >  arch/x86/kvm/mmu/paging_tmpl.h |  2 +-
> >  arch/x86/kvm/mmu/tdp_mmu.c     |  4 ++--
> >  3 files changed, 12 insertions(+), 9 deletions(-)
> > 
> > --
> > 2.31.1
> >