| Message ID | cover.1656039275.git.houwenlong.hwl@antgroup.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Fix wrong gfn range of tlb flushing with range | expand |
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 >
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
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
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
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 >
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 > >
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