* Re: [PATCH 15/22] kvm: mmu: Support changed pte notifier in tdp MMU @ 2020-10-07 21:20 kernel test robot 0 siblings, 0 replies; 10+ messages in thread From: kernel test robot @ 2020-10-07 21:20 UTC (permalink / raw) To: kbuild [-- Attachment #1: Type: text/plain, Size: 5071 bytes --] CC: kbuild-all(a)lists.01.org In-Reply-To: <20200925212302.3979661-16-bgardon@google.com> References: <20200925212302.3979661-16-bgardon@google.com> TO: Ben Gardon <bgardon@google.com> Hi Ben, Thank you for the patch! Perhaps something to improve: [auto build test WARNING on linus/master] [also build test WARNING on v5.9-rc8 next-20201007] [cannot apply to kvm/linux-next linux/master vhost/linux-next] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch] url: https://github.com/0day-ci/linux/commits/Ben-Gardon/Introduce-the-TDP-MMU/20200926-052649 base: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 6d28cf7dfede6cfca5119a0d415a6a447c68f3a0 :::::: branch date: 12 days ago :::::: commit date: 12 days ago config: x86_64-randconfig-m001-20201008 (attached as .config) compiler: gcc-9 (Debian 9.3.0-15) 9.3.0 If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot <lkp@intel.com> Reported-by: Dan Carpenter <dan.carpenter@oracle.com> smatch warnings: arch/x86/kvm/mmu/tdp_mmu.c:756 set_tdp_spte() error: uninitialized symbol 'new_spte'. vim +/new_spte +756 arch/x86/kvm/mmu/tdp_mmu.c 8753659d94fc330 Ben Gardon 2020-09-25 725 8753659d94fc330 Ben Gardon 2020-09-25 726 /* 8753659d94fc330 Ben Gardon 2020-09-25 727 * Handle the changed_pte MMU notifier for the TDP MMU. 8753659d94fc330 Ben Gardon 2020-09-25 728 * data is a pointer to the new pte_t mapping the HVA specified by the MMU 8753659d94fc330 Ben Gardon 2020-09-25 729 * notifier. 8753659d94fc330 Ben Gardon 2020-09-25 730 * Returns non-zero if a flush is needed before releasing the MMU lock. 8753659d94fc330 Ben Gardon 2020-09-25 731 */ 8753659d94fc330 Ben Gardon 2020-09-25 732 static int set_tdp_spte(struct kvm *kvm, struct kvm_memory_slot *slot, 8753659d94fc330 Ben Gardon 2020-09-25 733 struct kvm_mmu_page *root, gfn_t gfn, gfn_t unused, 8753659d94fc330 Ben Gardon 2020-09-25 734 unsigned long data) 8753659d94fc330 Ben Gardon 2020-09-25 735 { 8753659d94fc330 Ben Gardon 2020-09-25 736 struct tdp_iter iter; 8753659d94fc330 Ben Gardon 2020-09-25 737 pte_t *ptep = (pte_t *)data; 8753659d94fc330 Ben Gardon 2020-09-25 738 kvm_pfn_t new_pfn; 8753659d94fc330 Ben Gardon 2020-09-25 739 u64 new_spte; 8753659d94fc330 Ben Gardon 2020-09-25 740 int need_flush = 0; 8753659d94fc330 Ben Gardon 2020-09-25 741 int as_id = kvm_mmu_page_as_id(root); 8753659d94fc330 Ben Gardon 2020-09-25 742 8753659d94fc330 Ben Gardon 2020-09-25 743 WARN_ON(pte_huge(*ptep)); 8753659d94fc330 Ben Gardon 2020-09-25 744 8753659d94fc330 Ben Gardon 2020-09-25 745 new_pfn = pte_pfn(*ptep); 8753659d94fc330 Ben Gardon 2020-09-25 746 8753659d94fc330 Ben Gardon 2020-09-25 747 for_each_tdp_pte_root(iter, root, gfn, gfn + 1) { 8753659d94fc330 Ben Gardon 2020-09-25 748 if (iter.level != PG_LEVEL_4K) 8753659d94fc330 Ben Gardon 2020-09-25 749 continue; 8753659d94fc330 Ben Gardon 2020-09-25 750 8753659d94fc330 Ben Gardon 2020-09-25 751 if (!is_shadow_present_pte(iter.old_spte)) 8753659d94fc330 Ben Gardon 2020-09-25 752 break; 8753659d94fc330 Ben Gardon 2020-09-25 753 8753659d94fc330 Ben Gardon 2020-09-25 754 *iter.sptep = 0; 8753659d94fc330 Ben Gardon 2020-09-25 755 handle_changed_spte(kvm, as_id, iter.gfn, iter.old_spte, 8753659d94fc330 Ben Gardon 2020-09-25 @756 new_spte, iter.level); 8753659d94fc330 Ben Gardon 2020-09-25 757 8753659d94fc330 Ben Gardon 2020-09-25 758 kvm_flush_remote_tlbs_with_address(kvm, iter.gfn, 1); 8753659d94fc330 Ben Gardon 2020-09-25 759 8753659d94fc330 Ben Gardon 2020-09-25 760 if (!pte_write(*ptep)) { 8753659d94fc330 Ben Gardon 2020-09-25 761 new_spte = kvm_mmu_changed_pte_notifier_make_spte( 8753659d94fc330 Ben Gardon 2020-09-25 762 iter.old_spte, new_pfn); 8753659d94fc330 Ben Gardon 2020-09-25 763 8753659d94fc330 Ben Gardon 2020-09-25 764 *iter.sptep = new_spte; 8753659d94fc330 Ben Gardon 2020-09-25 765 handle_changed_spte(kvm, as_id, iter.gfn, iter.old_spte, 8753659d94fc330 Ben Gardon 2020-09-25 766 new_spte, iter.level); 8753659d94fc330 Ben Gardon 2020-09-25 767 } 8753659d94fc330 Ben Gardon 2020-09-25 768 8753659d94fc330 Ben Gardon 2020-09-25 769 need_flush = 1; 8753659d94fc330 Ben Gardon 2020-09-25 770 } 8753659d94fc330 Ben Gardon 2020-09-25 771 8753659d94fc330 Ben Gardon 2020-09-25 772 if (need_flush) 8753659d94fc330 Ben Gardon 2020-09-25 773 kvm_flush_remote_tlbs_with_address(kvm, gfn, 1); 8753659d94fc330 Ben Gardon 2020-09-25 774 8753659d94fc330 Ben Gardon 2020-09-25 775 return 0; 8753659d94fc330 Ben Gardon 2020-09-25 776 } 8753659d94fc330 Ben Gardon 2020-09-25 777 --- 0-DAY CI Kernel Test Service, Intel Corporation https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org [-- Attachment #2: config.gz --] [-- Type: application/gzip, Size: 41414 bytes --] ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 00/22] Introduce the TDP MMU @ 2020-09-25 21:22 Ben Gardon 2020-09-25 21:22 ` [PATCH 15/22] kvm: mmu: Support changed pte notifier in tdp MMU Ben Gardon 0 siblings, 1 reply; 10+ messages in thread From: Ben Gardon @ 2020-09-25 21:22 UTC (permalink / raw) To: linux-kernel, kvm Cc: Cannon Matthews, Paolo Bonzini, Peter Xu, Sean Christopherson, Peter Shier, Peter Feiner, Junaid Shahid, Jim Mattson, Yulei Zhang, Wanpeng Li, Vitaly Kuznetsov, Xiao Guangrong, Ben Gardon Over the years, the needs for KVM's x86 MMU have grown from running small guests to live migrating multi-terabyte VMs with hundreds of vCPUs. Where we previously depended on shadow paging to run all guests, we now have two dimensional paging (TDP). This patch set introduces a new implementation of much of the KVM MMU, optimized for running guests with TDP. We have re-implemented many of the MMU functions to take advantage of the relative simplicity of TDP and eliminate the need for an rmap. Building on this simplified implementation, a future patch set will change the synchronization model for this "TDP MMU" to enable more parallelism than the monolithic MMU lock. A TDP MMU is currently in use at Google and has given us the performance necessary to live migrate our 416 vCPU, 12TiB m2-ultramem-416 VMs. This work was motivated by the need to handle page faults in parallel for very large VMs. When VMs have hundreds of vCPUs and terabytes of memory, KVM's MMU lock suffers extreme contention, resulting in soft-lockups and long latency on guest page faults. This contention can be easily seen running the KVM selftests demand_paging_test with a couple hundred vCPUs. Over a 1 second profile of the demand_paging_test, with 416 vCPUs and 4G per vCPU, 98% of the time was spent waiting for the MMU lock. At Google, the TDP MMU reduced the test duration by 89% and the execution was dominated by get_user_pages and the user fault FD ioctl instead of the MMU lock. This series is the first of two. In this series we add a basic implementation of the TDP MMU. In the next series we will improve the performance of the TDP MMU and allow it to execute MMU operations in parallel. The overall purpose of the KVM MMU is to program paging structures (CR3/EPT/NPT) to encode the mapping of guest addresses to host physical addresses (HPA), and to provide utilities for other KVM features, for example dirty logging. The definition of the L1 guest physical address (GPA) to HPA mapping comes in two parts: KVM's memslots map GPA to HVA, and the kernel MM/x86 host page tables map HVA -> HPA. Without TDP, the MMU must program the x86 page tables to encode the full translation of guest virtual addresses (GVA) to HPA. This requires "shadowing" the guest's page tables to create a composite x86 paging structure. This solution is complicated, requires separate paging structures for each guest CR3, and requires emulating guest page table changes. The TDP case is much simpler. In this case, KVM lets the guest control CR3 and programs the EPT/NPT paging structures with the GPA -> HPA mapping. The guest has no way to change this mapping and only one version of the paging structure is needed per L1 paging mode. In this case the paging mode is some combination of the number of levels in the paging structure, the address space (normal execution or system management mode, on x86), and other attributes. Most VMs only ever use 1 paging mode and so only ever need one TDP structure. This series implements a "TDP MMU" through alternative implementations of MMU functions for running L1 guests with TDP. The TDP MMU falls back to the existing shadow paging implementation when TDP is not available, and interoperates with the existing shadow paging implementation for nesting. The use of the TDP MMU can be controlled by a module parameter which is snapshot on VM creation and follows the life of the VM. This snapshot is used in many functions to decide whether or not to use TDP MMU handlers for a given operation. This series can also be viewed in Gerrit here: https://linux-review.googlesource.com/c/virt/kvm/kvm/+/2538 (Thanks to Dmitry Vyukov <dvyukov@google.com> for setting up the Gerrit instance) Ben Gardon (22): kvm: mmu: Separate making SPTEs from set_spte kvm: mmu: Introduce tdp_iter kvm: mmu: Init / Uninit the TDP MMU kvm: mmu: Allocate and free TDP MMU roots kvm: mmu: Add functions to handle changed TDP SPTEs kvm: mmu: Make address space ID a property of memslots kvm: mmu: Support zapping SPTEs in the TDP MMU kvm: mmu: Separate making non-leaf sptes from link_shadow_page kvm: mmu: Remove disallowed_hugepage_adjust shadow_walk_iterator arg kvm: mmu: Add TDP MMU PF handler kvm: mmu: Factor out allocating a new tdp_mmu_page kvm: mmu: Allocate struct kvm_mmu_pages for all pages in TDP MMU kvm: mmu: Support invalidate range MMU notifier for TDP MMU kvm: mmu: Add access tracking for tdp_mmu kvm: mmu: Support changed pte notifier in tdp MMU kvm: mmu: Add dirty logging handler for changed sptes kvm: mmu: Support dirty logging for the TDP MMU kvm: mmu: Support disabling dirty logging for the tdp MMU kvm: mmu: Support write protection for nesting in tdp MMU kvm: mmu: NX largepage recovery for TDP MMU kvm: mmu: Support MMIO in the TDP MMU kvm: mmu: Don't clear write flooding count for direct roots arch/x86/include/asm/kvm_host.h | 17 + arch/x86/kvm/Makefile | 3 +- arch/x86/kvm/mmu/mmu.c | 437 ++++++---- arch/x86/kvm/mmu/mmu_internal.h | 98 +++ arch/x86/kvm/mmu/paging_tmpl.h | 3 +- arch/x86/kvm/mmu/tdp_iter.c | 198 +++++ arch/x86/kvm/mmu/tdp_iter.h | 55 ++ arch/x86/kvm/mmu/tdp_mmu.c | 1315 +++++++++++++++++++++++++++++++ arch/x86/kvm/mmu/tdp_mmu.h | 52 ++ include/linux/kvm_host.h | 2 + virt/kvm/kvm_main.c | 7 +- 11 files changed, 2022 insertions(+), 165 deletions(-) create mode 100644 arch/x86/kvm/mmu/tdp_iter.c create mode 100644 arch/x86/kvm/mmu/tdp_iter.h create mode 100644 arch/x86/kvm/mmu/tdp_mmu.c create mode 100644 arch/x86/kvm/mmu/tdp_mmu.h -- 2.28.0.709.gb0816b6eb0-goog ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 15/22] kvm: mmu: Support changed pte notifier in tdp MMU 2020-09-25 21:22 [PATCH 00/22] Introduce the TDP MMU Ben Gardon @ 2020-09-25 21:22 ` Ben Gardon 2020-09-26 0:33 ` Paolo Bonzini ` (2 more replies) 0 siblings, 3 replies; 10+ messages in thread From: Ben Gardon @ 2020-09-25 21:22 UTC (permalink / raw) To: linux-kernel, kvm Cc: Cannon Matthews, Paolo Bonzini, Peter Xu, Sean Christopherson, Peter Shier, Peter Feiner, Junaid Shahid, Jim Mattson, Yulei Zhang, Wanpeng Li, Vitaly Kuznetsov, Xiao Guangrong, Ben Gardon In order to interoperate correctly with the rest of KVM and other Linux subsystems, the TDP MMU must correctly handle various MMU notifiers. Add a hook and handle the change_pte MMU notifier. Tested by running kvm-unit-tests and KVM selftests on an Intel Haswell machine. This series introduced no new failures. This series can be viewed in Gerrit at: https://linux-review.googlesource.com/c/virt/kvm/kvm/+/2538 Signed-off-by: Ben Gardon <bgardon@google.com> --- arch/x86/kvm/mmu/mmu.c | 46 +++++++++++++------------ arch/x86/kvm/mmu/mmu_internal.h | 13 +++++++ arch/x86/kvm/mmu/tdp_mmu.c | 61 +++++++++++++++++++++++++++++++++ arch/x86/kvm/mmu/tdp_mmu.h | 3 ++ 4 files changed, 102 insertions(+), 21 deletions(-) diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c index 8c1e806b3d53f..0d80abe82ca93 100644 --- a/arch/x86/kvm/mmu/mmu.c +++ b/arch/x86/kvm/mmu/mmu.c @@ -122,9 +122,6 @@ module_param(dbg, bool, 0644); #define PTE_PREFETCH_NUM 8 -#define PT_FIRST_AVAIL_BITS_SHIFT 10 -#define PT64_SECOND_AVAIL_BITS_SHIFT 54 - /* * The mask used to denote special SPTEs, which can be either MMIO SPTEs or * Access Tracking SPTEs. @@ -147,13 +144,6 @@ module_param(dbg, bool, 0644); #define PT32_INDEX(address, level)\ (((address) >> PT32_LEVEL_SHIFT(level)) & ((1 << PT32_LEVEL_BITS) - 1)) - -#ifdef CONFIG_DYNAMIC_PHYSICAL_MASK -#define PT64_BASE_ADDR_MASK (physical_mask & ~(u64)(PAGE_SIZE-1)) -#else -#define PT64_BASE_ADDR_MASK (((1ULL << 52) - 1) & ~(u64)(PAGE_SIZE-1)) -#endif - #define PT32_BASE_ADDR_MASK PAGE_MASK #define PT32_DIR_BASE_ADDR_MASK \ (PAGE_MASK & ~((1ULL << (PAGE_SHIFT + PT32_LEVEL_BITS)) - 1)) @@ -170,9 +160,6 @@ module_param(dbg, bool, 0644); #include <trace/events/kvm.h> -#define SPTE_HOST_WRITEABLE (1ULL << PT_FIRST_AVAIL_BITS_SHIFT) -#define SPTE_MMU_WRITEABLE (1ULL << (PT_FIRST_AVAIL_BITS_SHIFT + 1)) - /* make pte_list_desc fit well in cache line */ #define PTE_LIST_EXT 3 @@ -1708,6 +1695,21 @@ static int kvm_unmap_rmapp(struct kvm *kvm, struct kvm_rmap_head *rmap_head, return kvm_zap_rmapp(kvm, rmap_head); } +u64 kvm_mmu_changed_pte_notifier_make_spte(u64 old_spte, kvm_pfn_t new_pfn) +{ + u64 new_spte; + + new_spte = old_spte & ~PT64_BASE_ADDR_MASK; + new_spte |= (u64)new_pfn << PAGE_SHIFT; + + new_spte &= ~PT_WRITABLE_MASK; + new_spte &= ~SPTE_HOST_WRITEABLE; + + new_spte = mark_spte_for_access_track(new_spte); + + return new_spte; +} + static int kvm_set_pte_rmapp(struct kvm *kvm, struct kvm_rmap_head *rmap_head, struct kvm_memory_slot *slot, gfn_t gfn, int level, unsigned long data) @@ -1733,13 +1735,8 @@ static int kvm_set_pte_rmapp(struct kvm *kvm, struct kvm_rmap_head *rmap_head, pte_list_remove(rmap_head, sptep); goto restart; } else { - new_spte = *sptep & ~PT64_BASE_ADDR_MASK; - new_spte |= (u64)new_pfn << PAGE_SHIFT; - - new_spte &= ~PT_WRITABLE_MASK; - new_spte &= ~SPTE_HOST_WRITEABLE; - - new_spte = mark_spte_for_access_track(new_spte); + new_spte = kvm_mmu_changed_pte_notifier_make_spte( + *sptep, new_pfn); mmu_spte_clear_track_bits(sptep); mmu_spte_set(sptep, new_spte); @@ -1895,7 +1892,14 @@ int kvm_unmap_hva_range(struct kvm *kvm, unsigned long start, unsigned long end, int kvm_set_spte_hva(struct kvm *kvm, unsigned long hva, pte_t pte) { - return kvm_handle_hva(kvm, hva, (unsigned long)&pte, kvm_set_pte_rmapp); + int r; + + r = kvm_handle_hva(kvm, hva, (unsigned long)&pte, kvm_set_pte_rmapp); + + if (kvm->arch.tdp_mmu_enabled) + r |= kvm_tdp_mmu_set_spte_hva(kvm, hva, &pte); + + return r; } static int kvm_age_rmapp(struct kvm *kvm, struct kvm_rmap_head *rmap_head, diff --git a/arch/x86/kvm/mmu/mmu_internal.h b/arch/x86/kvm/mmu/mmu_internal.h index 228bda0885552..8eaa6e4764bce 100644 --- a/arch/x86/kvm/mmu/mmu_internal.h +++ b/arch/x86/kvm/mmu/mmu_internal.h @@ -80,6 +80,12 @@ bool kvm_mmu_slot_gfn_write_protect(struct kvm *kvm, (PT64_BASE_ADDR_MASK & ((1ULL << (PAGE_SHIFT + (((level) - 1) \ * PT64_LEVEL_BITS))) - 1)) +#ifdef CONFIG_DYNAMIC_PHYSICAL_MASK +#define PT64_BASE_ADDR_MASK (physical_mask & ~(u64)(PAGE_SIZE-1)) +#else +#define PT64_BASE_ADDR_MASK (((1ULL << 52) - 1) & ~(u64)(PAGE_SIZE-1)) +#endif + extern u64 shadow_user_mask; extern u64 shadow_accessed_mask; extern u64 shadow_present_mask; @@ -89,6 +95,12 @@ extern u64 shadow_present_mask; #define ACC_USER_MASK PT_USER_MASK #define ACC_ALL (ACC_EXEC_MASK | ACC_WRITE_MASK | ACC_USER_MASK) +#define PT_FIRST_AVAIL_BITS_SHIFT 10 +#define PT64_SECOND_AVAIL_BITS_SHIFT 54 + +#define SPTE_HOST_WRITEABLE (1ULL << PT_FIRST_AVAIL_BITS_SHIFT) +#define SPTE_MMU_WRITEABLE (1ULL << (PT_FIRST_AVAIL_BITS_SHIFT + 1)) + /* Functions for interpreting SPTEs */ kvm_pfn_t spte_to_pfn(u64 pte); bool is_mmio_spte(u64 spte); @@ -138,5 +150,6 @@ bool is_nx_huge_page_enabled(void); void *mmu_memory_cache_alloc(struct kvm_mmu_memory_cache *mc); u64 mark_spte_for_access_track(u64 spte); +u64 kvm_mmu_changed_pte_notifier_make_spte(u64 old_spte, kvm_pfn_t new_pfn); #endif /* __KVM_X86_MMU_INTERNAL_H */ diff --git a/arch/x86/kvm/mmu/tdp_mmu.c b/arch/x86/kvm/mmu/tdp_mmu.c index 0a4b98669b3ef..3119583409131 100644 --- a/arch/x86/kvm/mmu/tdp_mmu.c +++ b/arch/x86/kvm/mmu/tdp_mmu.c @@ -722,3 +722,64 @@ int kvm_tdp_mmu_test_age_hva(struct kvm *kvm, unsigned long hva) return kvm_tdp_mmu_handle_hva_range(kvm, hva, hva + 1, 0, test_age_gfn); } + +/* + * Handle the changed_pte MMU notifier for the TDP MMU. + * data is a pointer to the new pte_t mapping the HVA specified by the MMU + * notifier. + * Returns non-zero if a flush is needed before releasing the MMU lock. + */ +static int set_tdp_spte(struct kvm *kvm, struct kvm_memory_slot *slot, + struct kvm_mmu_page *root, gfn_t gfn, gfn_t unused, + unsigned long data) +{ + struct tdp_iter iter; + pte_t *ptep = (pte_t *)data; + kvm_pfn_t new_pfn; + u64 new_spte; + int need_flush = 0; + int as_id = kvm_mmu_page_as_id(root); + + WARN_ON(pte_huge(*ptep)); + + new_pfn = pte_pfn(*ptep); + + for_each_tdp_pte_root(iter, root, gfn, gfn + 1) { + if (iter.level != PG_LEVEL_4K) + continue; + + if (!is_shadow_present_pte(iter.old_spte)) + break; + + *iter.sptep = 0; + handle_changed_spte(kvm, as_id, iter.gfn, iter.old_spte, + new_spte, iter.level); + + kvm_flush_remote_tlbs_with_address(kvm, iter.gfn, 1); + + if (!pte_write(*ptep)) { + new_spte = kvm_mmu_changed_pte_notifier_make_spte( + iter.old_spte, new_pfn); + + *iter.sptep = new_spte; + handle_changed_spte(kvm, as_id, iter.gfn, iter.old_spte, + new_spte, iter.level); + } + + need_flush = 1; + } + + if (need_flush) + kvm_flush_remote_tlbs_with_address(kvm, gfn, 1); + + return 0; +} + +int kvm_tdp_mmu_set_spte_hva(struct kvm *kvm, unsigned long address, + pte_t *host_ptep) +{ + return kvm_tdp_mmu_handle_hva_range(kvm, address, address + 1, + (unsigned long)host_ptep, + set_tdp_spte); +} + diff --git a/arch/x86/kvm/mmu/tdp_mmu.h b/arch/x86/kvm/mmu/tdp_mmu.h index f316773b7b5a8..5a399aa60b8d8 100644 --- a/arch/x86/kvm/mmu/tdp_mmu.h +++ b/arch/x86/kvm/mmu/tdp_mmu.h @@ -25,4 +25,7 @@ int kvm_tdp_mmu_zap_hva_range(struct kvm *kvm, unsigned long start, int kvm_tdp_mmu_age_hva_range(struct kvm *kvm, unsigned long start, unsigned long end); int kvm_tdp_mmu_test_age_hva(struct kvm *kvm, unsigned long hva); + +int kvm_tdp_mmu_set_spte_hva(struct kvm *kvm, unsigned long address, + pte_t *host_ptep); #endif /* __KVM_X86_MMU_TDP_MMU_H */ -- 2.28.0.709.gb0816b6eb0-goog ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 15/22] kvm: mmu: Support changed pte notifier in tdp MMU 2020-09-25 21:22 ` [PATCH 15/22] kvm: mmu: Support changed pte notifier in tdp MMU Ben Gardon @ 2020-09-26 0:33 ` Paolo Bonzini 2020-09-28 15:11 ` Paolo Bonzini 2020-10-09 10:59 ` Dan Carpenter 2 siblings, 0 replies; 10+ messages in thread From: Paolo Bonzini @ 2020-09-26 0:33 UTC (permalink / raw) To: Ben Gardon, linux-kernel, kvm Cc: Cannon Matthews, Peter Xu, Sean Christopherson, Peter Shier, Peter Feiner, Junaid Shahid, Jim Mattson, Yulei Zhang, Wanpeng Li, Vitaly Kuznetsov, Xiao Guangrong On 25/09/20 23:22, Ben Gardon wrote: > @@ -1708,6 +1695,21 @@ static int kvm_unmap_rmapp(struct kvm *kvm, struct kvm_rmap_head *rmap_head, > return kvm_zap_rmapp(kvm, rmap_head); > } > > +u64 kvm_mmu_changed_pte_notifier_make_spte(u64 old_spte, kvm_pfn_t new_pfn) > +{ > + u64 new_spte; > + > + new_spte = old_spte & ~PT64_BASE_ADDR_MASK; > + new_spte |= (u64)new_pfn << PAGE_SHIFT; > + > + new_spte &= ~PT_WRITABLE_MASK; > + new_spte &= ~SPTE_HOST_WRITEABLE; > + > + new_spte = mark_spte_for_access_track(new_spte); > + > + return new_spte; > +} > + And another. :) ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 15/22] kvm: mmu: Support changed pte notifier in tdp MMU 2020-09-25 21:22 ` [PATCH 15/22] kvm: mmu: Support changed pte notifier in tdp MMU Ben Gardon 2020-09-26 0:33 ` Paolo Bonzini @ 2020-09-28 15:11 ` Paolo Bonzini 2020-10-07 16:53 ` Ben Gardon 2020-10-09 10:59 ` Dan Carpenter 2 siblings, 1 reply; 10+ messages in thread From: Paolo Bonzini @ 2020-09-28 15:11 UTC (permalink / raw) To: Ben Gardon, linux-kernel, kvm Cc: Cannon Matthews, Peter Xu, Sean Christopherson, Peter Shier, Peter Feiner, Junaid Shahid, Jim Mattson, Yulei Zhang, Wanpeng Li, Vitaly Kuznetsov, Xiao Guangrong On 25/09/20 23:22, Ben Gardon wrote: > + *iter.sptep = 0; > + handle_changed_spte(kvm, as_id, iter.gfn, iter.old_spte, > + new_spte, iter.level); > + Can you explain why new_spte is passed here instead of 0? All calls to handle_changed_spte are preceded by "*something = new_spte" except this one, so I'm thinking of having a change_spte function like static void change_spte(struct kvm *kvm, int as_id, gfn_t gfn, u64 *sptep, u64 new_spte, int level) { u64 old_spte = *sptep; *sptep = new_spte; __handle_changed_spte(kvm, as_id, gfn, old_spte, new_spte, level); handle_changed_spte_acc_track(old_spte, new_spte, level); handle_changed_spte_dirty_log(kvm, as_id, gfn, old_spte, new_spte, level); } in addition to the previously-mentioned cleanup of always calling handle_changed_spte instead of special-casing calls to two of the three functions. It would be a nice place to add the trace_kvm_mmu_set_spte tracepoint, too. Paolo ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 15/22] kvm: mmu: Support changed pte notifier in tdp MMU 2020-09-28 15:11 ` Paolo Bonzini @ 2020-10-07 16:53 ` Ben Gardon 2020-10-07 17:18 ` Paolo Bonzini 0 siblings, 1 reply; 10+ messages in thread From: Ben Gardon @ 2020-10-07 16:53 UTC (permalink / raw) To: Paolo Bonzini Cc: LKML, kvm, Cannon Matthews, Peter Xu, Sean Christopherson, Peter Shier, Peter Feiner, Junaid Shahid, Jim Mattson, Yulei Zhang, Wanpeng Li, Vitaly Kuznetsov, Xiao Guangrong On Mon, Sep 28, 2020 at 8:11 AM Paolo Bonzini <pbonzini@redhat.com> wrote: > > On 25/09/20 23:22, Ben Gardon wrote: > > + *iter.sptep = 0; > > + handle_changed_spte(kvm, as_id, iter.gfn, iter.old_spte, > > + new_spte, iter.level); > > + > > Can you explain why new_spte is passed here instead of 0? That's just a bug. Thank you for catching it. > > All calls to handle_changed_spte are preceded by "*something = > new_spte" except this one, so I'm thinking of having a change_spte > function like > > static void change_spte(struct kvm *kvm, int as_id, gfn_t gfn, > u64 *sptep, u64 new_spte, int level) > { > u64 old_spte = *sptep; > *sptep = new_spte; > > __handle_changed_spte(kvm, as_id, gfn, old_spte, new_spte, level); > handle_changed_spte_acc_track(old_spte, new_spte, level); > handle_changed_spte_dirty_log(kvm, as_id, gfn, old_spte, new_spte, level); > } > > in addition to the previously-mentioned cleanup of always calling > handle_changed_spte instead of special-casing calls to two of the > three functions. It would be a nice place to add the > trace_kvm_mmu_set_spte tracepoint, too. I'm not sure we can avoid special casing calls to the access tracking and dirty logging handler functions. At least in the past that's created bugs with things being marked dirty or accessed when they shouldn't be. I'll revisit those assumptions. It would certainly be nice to get rid of that complexity. I agree that putting the SPTE assignment and handler functions in a helper function would clean up the code. I'll do that. I got some feedback on the RFC I sent last year which led me to open-code a lot more, but I think this is still a good cleanup. Re tracepoints, I was planning to just insert them all once this code is stabilized, if that's alright. > > Paolo > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 15/22] kvm: mmu: Support changed pte notifier in tdp MMU 2020-10-07 16:53 ` Ben Gardon @ 2020-10-07 17:18 ` Paolo Bonzini 2020-10-07 17:30 ` Ben Gardon 0 siblings, 1 reply; 10+ messages in thread From: Paolo Bonzini @ 2020-10-07 17:18 UTC (permalink / raw) To: Ben Gardon Cc: LKML, kvm, Cannon Matthews, Peter Xu, Sean Christopherson, Peter Shier, Peter Feiner, Junaid Shahid, Jim Mattson, Yulei Zhang, Wanpeng Li, Vitaly Kuznetsov, Xiao Guangrong On 07/10/20 18:53, Ben Gardon wrote: >> in addition to the previously-mentioned cleanup of always calling >> handle_changed_spte instead of special-casing calls to two of the >> three functions. It would be a nice place to add the >> trace_kvm_mmu_set_spte tracepoint, too. > I'm not sure we can avoid special casing calls to the access tracking > and dirty logging handler functions. At least in the past that's > created bugs with things being marked dirty or accessed when they > shouldn't be. I'll revisit those assumptions. It would certainly be > nice to get rid of that complexity. > > I agree that putting the SPTE assignment and handler functions in a > helper function would clean up the code. I'll do that. Well that's not easy if you have to think of which functions have to be called. I'll take a closer look at the access tracking and dirty logging cases to try and understand what those bugs can be. Apart from that I have my suggested changes and I can probably finish testing them and send them out tomorrow. Paolo > I got some > feedback on the RFC I sent last year which led me to open-code a lot > more, but I think this is still a good cleanup. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 15/22] kvm: mmu: Support changed pte notifier in tdp MMU 2020-10-07 17:18 ` Paolo Bonzini @ 2020-10-07 17:30 ` Ben Gardon 2020-10-07 17:54 ` Paolo Bonzini 0 siblings, 1 reply; 10+ messages in thread From: Ben Gardon @ 2020-10-07 17:30 UTC (permalink / raw) To: Paolo Bonzini Cc: LKML, kvm, Cannon Matthews, Peter Xu, Sean Christopherson, Peter Shier, Peter Feiner, Junaid Shahid, Jim Mattson, Yulei Zhang, Wanpeng Li, Vitaly Kuznetsov, Xiao Guangrong On Wed, Oct 7, 2020 at 10:18 AM Paolo Bonzini <pbonzini@redhat.com> wrote: > > On 07/10/20 18:53, Ben Gardon wrote: > >> in addition to the previously-mentioned cleanup of always calling > >> handle_changed_spte instead of special-casing calls to two of the > >> three functions. It would be a nice place to add the > >> trace_kvm_mmu_set_spte tracepoint, too. > > I'm not sure we can avoid special casing calls to the access tracking > > and dirty logging handler functions. At least in the past that's > > created bugs with things being marked dirty or accessed when they > > shouldn't be. I'll revisit those assumptions. It would certainly be > > nice to get rid of that complexity. > > > > I agree that putting the SPTE assignment and handler functions in a > > helper function would clean up the code. I'll do that. > > Well that's not easy if you have to think of which functions have to be > called. > > I'll take a closer look at the access tracking and dirty logging cases > to try and understand what those bugs can be. Apart from that I have my > suggested changes and I can probably finish testing them and send them > out tomorrow. Awesome, thank you. I'll look forward to seeing them. Will you be applying those changes to the tdp_mmu branch you created as well? > > Paolo > > > I got some > > feedback on the RFC I sent last year which led me to open-code a lot > > more, but I think this is still a good cleanup. > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 15/22] kvm: mmu: Support changed pte notifier in tdp MMU 2020-10-07 17:30 ` Ben Gardon @ 2020-10-07 17:54 ` Paolo Bonzini 0 siblings, 0 replies; 10+ messages in thread From: Paolo Bonzini @ 2020-10-07 17:54 UTC (permalink / raw) To: Ben Gardon Cc: LKML, kvm, Cannon Matthews, Peter Xu, Sean Christopherson, Peter Shier, Peter Feiner, Junaid Shahid, Jim Mattson, Yulei Zhang, Wanpeng Li, Vitaly Kuznetsov, Xiao Guangrong On 07/10/20 19:30, Ben Gardon wrote: >> Well that's not easy if you have to think of which functions have to be >> called. >> >> I'll take a closer look at the access tracking and dirty logging cases >> to try and understand what those bugs can be. Apart from that I have my >> suggested changes and I can probably finish testing them and send them >> out tomorrow. > Awesome, thank you. I'll look forward to seeing them. Will you be > applying those changes to the tdp_mmu branch you created as well? > No, only to the rebased version. Paolo ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 15/22] kvm: mmu: Support changed pte notifier in tdp MMU 2020-09-25 21:22 ` [PATCH 15/22] kvm: mmu: Support changed pte notifier in tdp MMU Ben Gardon @ 2020-10-09 10:59 ` Dan Carpenter 2020-09-28 15:11 ` Paolo Bonzini 2020-10-09 10:59 ` Dan Carpenter 2 siblings, 0 replies; 10+ messages in thread From: Dan Carpenter @ 2020-10-09 10:59 UTC (permalink / raw) To: kbuild [-- Attachment #1: Type: text/plain, Size: 3962 bytes --] Hi Ben, url: https://github.com/0day-ci/linux/commits/Ben-Gardon/Introduce-the-TDP-MMU/20200926-052649 base: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 6d28cf7dfede6cfca5119a0d415a6a447c68f3a0 config: x86_64-randconfig-m001-20201008 (attached as .config) compiler: gcc-9 (Debian 9.3.0-15) 9.3.0 If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot <lkp@intel.com> Reported-by: Dan Carpenter <dan.carpenter@oracle.com> smatch warnings: arch/x86/kvm/mmu/tdp_mmu.c:756 set_tdp_spte() error: uninitialized symbol 'new_spte'. vim +/new_spte +756 arch/x86/kvm/mmu/tdp_mmu.c 8753659d94fc330 Ben Gardon 2020-09-25 732 static int set_tdp_spte(struct kvm *kvm, struct kvm_memory_slot *slot, 8753659d94fc330 Ben Gardon 2020-09-25 733 struct kvm_mmu_page *root, gfn_t gfn, gfn_t unused, 8753659d94fc330 Ben Gardon 2020-09-25 734 unsigned long data) 8753659d94fc330 Ben Gardon 2020-09-25 735 { 8753659d94fc330 Ben Gardon 2020-09-25 736 struct tdp_iter iter; 8753659d94fc330 Ben Gardon 2020-09-25 737 pte_t *ptep = (pte_t *)data; 8753659d94fc330 Ben Gardon 2020-09-25 738 kvm_pfn_t new_pfn; 8753659d94fc330 Ben Gardon 2020-09-25 739 u64 new_spte; 8753659d94fc330 Ben Gardon 2020-09-25 740 int need_flush = 0; 8753659d94fc330 Ben Gardon 2020-09-25 741 int as_id = kvm_mmu_page_as_id(root); 8753659d94fc330 Ben Gardon 2020-09-25 742 8753659d94fc330 Ben Gardon 2020-09-25 743 WARN_ON(pte_huge(*ptep)); 8753659d94fc330 Ben Gardon 2020-09-25 744 8753659d94fc330 Ben Gardon 2020-09-25 745 new_pfn = pte_pfn(*ptep); 8753659d94fc330 Ben Gardon 2020-09-25 746 8753659d94fc330 Ben Gardon 2020-09-25 747 for_each_tdp_pte_root(iter, root, gfn, gfn + 1) { 8753659d94fc330 Ben Gardon 2020-09-25 748 if (iter.level != PG_LEVEL_4K) 8753659d94fc330 Ben Gardon 2020-09-25 749 continue; 8753659d94fc330 Ben Gardon 2020-09-25 750 8753659d94fc330 Ben Gardon 2020-09-25 751 if (!is_shadow_present_pte(iter.old_spte)) 8753659d94fc330 Ben Gardon 2020-09-25 752 break; 8753659d94fc330 Ben Gardon 2020-09-25 753 8753659d94fc330 Ben Gardon 2020-09-25 754 *iter.sptep = 0; 8753659d94fc330 Ben Gardon 2020-09-25 755 handle_changed_spte(kvm, as_id, iter.gfn, iter.old_spte, 8753659d94fc330 Ben Gardon 2020-09-25 @756 new_spte, iter.level); ^^^^^^^^ Not initialized on first iteration through the loop. 8753659d94fc330 Ben Gardon 2020-09-25 757 8753659d94fc330 Ben Gardon 2020-09-25 758 kvm_flush_remote_tlbs_with_address(kvm, iter.gfn, 1); 8753659d94fc330 Ben Gardon 2020-09-25 759 8753659d94fc330 Ben Gardon 2020-09-25 760 if (!pte_write(*ptep)) { 8753659d94fc330 Ben Gardon 2020-09-25 761 new_spte = kvm_mmu_changed_pte_notifier_make_spte( 8753659d94fc330 Ben Gardon 2020-09-25 762 iter.old_spte, new_pfn); 8753659d94fc330 Ben Gardon 2020-09-25 763 8753659d94fc330 Ben Gardon 2020-09-25 764 *iter.sptep = new_spte; 8753659d94fc330 Ben Gardon 2020-09-25 765 handle_changed_spte(kvm, as_id, iter.gfn, iter.old_spte, 8753659d94fc330 Ben Gardon 2020-09-25 766 new_spte, iter.level); 8753659d94fc330 Ben Gardon 2020-09-25 767 } 8753659d94fc330 Ben Gardon 2020-09-25 768 8753659d94fc330 Ben Gardon 2020-09-25 769 need_flush = 1; 8753659d94fc330 Ben Gardon 2020-09-25 770 } 8753659d94fc330 Ben Gardon 2020-09-25 771 8753659d94fc330 Ben Gardon 2020-09-25 772 if (need_flush) 8753659d94fc330 Ben Gardon 2020-09-25 773 kvm_flush_remote_tlbs_with_address(kvm, gfn, 1); 8753659d94fc330 Ben Gardon 2020-09-25 774 8753659d94fc330 Ben Gardon 2020-09-25 775 return 0; 8753659d94fc330 Ben Gardon 2020-09-25 776 } --- 0-DAY CI Kernel Test Service, Intel Corporation https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org [-- Attachment #2: config.gz --] [-- Type: application/gzip, Size: 41414 bytes --] ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 15/22] kvm: mmu: Support changed pte notifier in tdp MMU @ 2020-10-09 10:59 ` Dan Carpenter 0 siblings, 0 replies; 10+ messages in thread From: Dan Carpenter @ 2020-10-09 10:59 UTC (permalink / raw) To: kbuild-all [-- Attachment #1: Type: text/plain, Size: 3962 bytes --] Hi Ben, url: https://github.com/0day-ci/linux/commits/Ben-Gardon/Introduce-the-TDP-MMU/20200926-052649 base: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 6d28cf7dfede6cfca5119a0d415a6a447c68f3a0 config: x86_64-randconfig-m001-20201008 (attached as .config) compiler: gcc-9 (Debian 9.3.0-15) 9.3.0 If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot <lkp@intel.com> Reported-by: Dan Carpenter <dan.carpenter@oracle.com> smatch warnings: arch/x86/kvm/mmu/tdp_mmu.c:756 set_tdp_spte() error: uninitialized symbol 'new_spte'. vim +/new_spte +756 arch/x86/kvm/mmu/tdp_mmu.c 8753659d94fc330 Ben Gardon 2020-09-25 732 static int set_tdp_spte(struct kvm *kvm, struct kvm_memory_slot *slot, 8753659d94fc330 Ben Gardon 2020-09-25 733 struct kvm_mmu_page *root, gfn_t gfn, gfn_t unused, 8753659d94fc330 Ben Gardon 2020-09-25 734 unsigned long data) 8753659d94fc330 Ben Gardon 2020-09-25 735 { 8753659d94fc330 Ben Gardon 2020-09-25 736 struct tdp_iter iter; 8753659d94fc330 Ben Gardon 2020-09-25 737 pte_t *ptep = (pte_t *)data; 8753659d94fc330 Ben Gardon 2020-09-25 738 kvm_pfn_t new_pfn; 8753659d94fc330 Ben Gardon 2020-09-25 739 u64 new_spte; 8753659d94fc330 Ben Gardon 2020-09-25 740 int need_flush = 0; 8753659d94fc330 Ben Gardon 2020-09-25 741 int as_id = kvm_mmu_page_as_id(root); 8753659d94fc330 Ben Gardon 2020-09-25 742 8753659d94fc330 Ben Gardon 2020-09-25 743 WARN_ON(pte_huge(*ptep)); 8753659d94fc330 Ben Gardon 2020-09-25 744 8753659d94fc330 Ben Gardon 2020-09-25 745 new_pfn = pte_pfn(*ptep); 8753659d94fc330 Ben Gardon 2020-09-25 746 8753659d94fc330 Ben Gardon 2020-09-25 747 for_each_tdp_pte_root(iter, root, gfn, gfn + 1) { 8753659d94fc330 Ben Gardon 2020-09-25 748 if (iter.level != PG_LEVEL_4K) 8753659d94fc330 Ben Gardon 2020-09-25 749 continue; 8753659d94fc330 Ben Gardon 2020-09-25 750 8753659d94fc330 Ben Gardon 2020-09-25 751 if (!is_shadow_present_pte(iter.old_spte)) 8753659d94fc330 Ben Gardon 2020-09-25 752 break; 8753659d94fc330 Ben Gardon 2020-09-25 753 8753659d94fc330 Ben Gardon 2020-09-25 754 *iter.sptep = 0; 8753659d94fc330 Ben Gardon 2020-09-25 755 handle_changed_spte(kvm, as_id, iter.gfn, iter.old_spte, 8753659d94fc330 Ben Gardon 2020-09-25 @756 new_spte, iter.level); ^^^^^^^^ Not initialized on first iteration through the loop. 8753659d94fc330 Ben Gardon 2020-09-25 757 8753659d94fc330 Ben Gardon 2020-09-25 758 kvm_flush_remote_tlbs_with_address(kvm, iter.gfn, 1); 8753659d94fc330 Ben Gardon 2020-09-25 759 8753659d94fc330 Ben Gardon 2020-09-25 760 if (!pte_write(*ptep)) { 8753659d94fc330 Ben Gardon 2020-09-25 761 new_spte = kvm_mmu_changed_pte_notifier_make_spte( 8753659d94fc330 Ben Gardon 2020-09-25 762 iter.old_spte, new_pfn); 8753659d94fc330 Ben Gardon 2020-09-25 763 8753659d94fc330 Ben Gardon 2020-09-25 764 *iter.sptep = new_spte; 8753659d94fc330 Ben Gardon 2020-09-25 765 handle_changed_spte(kvm, as_id, iter.gfn, iter.old_spte, 8753659d94fc330 Ben Gardon 2020-09-25 766 new_spte, iter.level); 8753659d94fc330 Ben Gardon 2020-09-25 767 } 8753659d94fc330 Ben Gardon 2020-09-25 768 8753659d94fc330 Ben Gardon 2020-09-25 769 need_flush = 1; 8753659d94fc330 Ben Gardon 2020-09-25 770 } 8753659d94fc330 Ben Gardon 2020-09-25 771 8753659d94fc330 Ben Gardon 2020-09-25 772 if (need_flush) 8753659d94fc330 Ben Gardon 2020-09-25 773 kvm_flush_remote_tlbs_with_address(kvm, gfn, 1); 8753659d94fc330 Ben Gardon 2020-09-25 774 8753659d94fc330 Ben Gardon 2020-09-25 775 return 0; 8753659d94fc330 Ben Gardon 2020-09-25 776 } --- 0-DAY CI Kernel Test Service, Intel Corporation https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org [-- Attachment #2: config.gz --] [-- Type: application/gzip, Size: 41414 bytes --] ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2020-10-09 10:59 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-10-07 21:20 [PATCH 15/22] kvm: mmu: Support changed pte notifier in tdp MMU kernel test robot -- strict thread matches above, loose matches on Subject: below -- 2020-09-25 21:22 [PATCH 00/22] Introduce the TDP MMU Ben Gardon 2020-09-25 21:22 ` [PATCH 15/22] kvm: mmu: Support changed pte notifier in tdp MMU Ben Gardon 2020-09-26 0:33 ` Paolo Bonzini 2020-09-28 15:11 ` Paolo Bonzini 2020-10-07 16:53 ` Ben Gardon 2020-10-07 17:18 ` Paolo Bonzini 2020-10-07 17:30 ` Ben Gardon 2020-10-07 17:54 ` Paolo Bonzini 2020-10-09 10:59 ` Dan Carpenter 2020-10-09 10:59 ` Dan Carpenter
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.