| Message ID | 20220422210546.458943-20-dmatlack@google.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | KVM: Extend Eager Page Splitting to the shadow MMU | expand |
Hi David,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on 150866cd0ec871c765181d145aa0912628289c8a]
url: https://github.com/intel-lab-lkp/linux/commits/David-Matlack/KVM-Extend-Eager-Page-Splitting-to-the-shadow-MMU/20220423-062108
base: 150866cd0ec871c765181d145aa0912628289c8a
config: riscv-randconfig-r005-20220422 (https://download.01.org/0day-ci/archive/20220423/202204231516.bclimUe4-lkp@intel.com/config)
compiler: riscv64-linux-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/871c5afc76a6f414c03f433d06bacfd928910b1b
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review David-Matlack/KVM-Extend-Eager-Page-Splitting-to-the-shadow-MMU/20220423-062108
git checkout 871c5afc76a6f414c03f433d06bacfd928910b1b
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross W=1 O=build_dir ARCH=riscv SHELL=/bin/bash
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All error/warnings (new ones prefixed by >>):
arch/riscv/kvm/mmu.c: In function 'stage2_ioremap':
>> arch/riscv/kvm/mmu.c:364:56: error: 'struct kvm_mmu_memory_cache' has no member named 'cache'
364 | ret = kvm_mmu_topup_memory_cache(&cache.cache, stage2_pgd_levels);
| ^
arch/riscv/kvm/mmu.c:369:52: error: 'struct kvm_mmu_memory_cache' has no member named 'cache'
369 | ret = stage2_set_pte(kvm, 0, &cache.cache, addr, &pte);
| ^
arch/riscv/kvm/mmu.c:378:41: error: 'struct kvm_mmu_memory_cache' has no member named 'cache'
378 | kvm_mmu_free_memory_cache(&cache.cache);
| ^
>> arch/riscv/kvm/mmu.c:350:37: warning: variable 'cache' set but not used [-Wunused-but-set-variable]
350 | struct kvm_mmu_memory_cache cache = {
| ^~~~~
vim +364 arch/riscv/kvm/mmu.c
342
343 static int stage2_ioremap(struct kvm *kvm, gpa_t gpa, phys_addr_t hpa,
344 unsigned long size, bool writable)
345 {
346 pte_t pte;
347 int ret = 0;
348 unsigned long pfn;
349 phys_addr_t addr, end;
> 350 struct kvm_mmu_memory_cache cache = {
351 .capacity = KVM_ARCH_NR_OBJS_PER_MEMORY_CACHE,
352 .gfp_zero = __GFP_ZERO,
353 };
354
355 end = (gpa + size + PAGE_SIZE - 1) & PAGE_MASK;
356 pfn = __phys_to_pfn(hpa);
357
358 for (addr = gpa; addr < end; addr += PAGE_SIZE) {
359 pte = pfn_pte(pfn, PAGE_KERNEL);
360
361 if (!writable)
362 pte = pte_wrprotect(pte);
363
> 364 ret = kvm_mmu_topup_memory_cache(&cache.cache, stage2_pgd_levels);
365 if (ret)
366 goto out;
367
368 spin_lock(&kvm->mmu_lock);
369 ret = stage2_set_pte(kvm, 0, &cache.cache, addr, &pte);
370 spin_unlock(&kvm->mmu_lock);
371 if (ret)
372 goto out;
373
374 pfn++;
375 }
376
377 out:
378 kvm_mmu_free_memory_cache(&cache.cache);
379 return ret;
380 }
381
Hi David,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on 150866cd0ec871c765181d145aa0912628289c8a]
url: https://github.com/intel-lab-lkp/linux/commits/David-Matlack/KVM-Extend-Eager-Page-Splitting-to-the-shadow-MMU/20220423-062108
base: 150866cd0ec871c765181d145aa0912628289c8a
config: riscv-randconfig-r016-20220424 (https://download.01.org/0day-ci/archive/20220424/202204242355.T1SzNT9S-lkp@intel.com/config)
compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project 1cddcfdc3c683b393df1a5c9063252eb60e52818)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# install riscv cross compiling tool for clang build
# apt-get install binutils-riscv64-linux-gnu
# https://github.com/intel-lab-lkp/linux/commit/871c5afc76a6f414c03f433d06bacfd928910b1b
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review David-Matlack/KVM-Extend-Eager-Page-Splitting-to-the-shadow-MMU/20220423-062108
git checkout 871c5afc76a6f414c03f433d06bacfd928910b1b
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=riscv SHELL=/bin/bash
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
>> arch/riscv/kvm/mmu.c:364:43: error: no member named 'cache' in 'struct kvm_mmu_memory_cache'
ret = kvm_mmu_topup_memory_cache(&cache.cache, stage2_pgd_levels);
~~~~~ ^
arch/riscv/kvm/mmu.c:369:39: error: no member named 'cache' in 'struct kvm_mmu_memory_cache'
ret = stage2_set_pte(kvm, 0, &cache.cache, addr, &pte);
~~~~~ ^
arch/riscv/kvm/mmu.c:378:35: error: no member named 'cache' in 'struct kvm_mmu_memory_cache'
kvm_mmu_free_memory_cache(&cache.cache);
~~~~~ ^
3 errors generated.
vim +364 arch/riscv/kvm/mmu.c
342
343 static int stage2_ioremap(struct kvm *kvm, gpa_t gpa, phys_addr_t hpa,
344 unsigned long size, bool writable)
345 {
346 pte_t pte;
347 int ret = 0;
348 unsigned long pfn;
349 phys_addr_t addr, end;
350 struct kvm_mmu_memory_cache cache = {
351 .capacity = KVM_ARCH_NR_OBJS_PER_MEMORY_CACHE,
352 .gfp_zero = __GFP_ZERO,
353 };
354
355 end = (gpa + size + PAGE_SIZE - 1) & PAGE_MASK;
356 pfn = __phys_to_pfn(hpa);
357
358 for (addr = gpa; addr < end; addr += PAGE_SIZE) {
359 pte = pfn_pte(pfn, PAGE_KERNEL);
360
361 if (!writable)
362 pte = pte_wrprotect(pte);
363
> 364 ret = kvm_mmu_topup_memory_cache(&cache.cache, stage2_pgd_levels);
365 if (ret)
366 goto out;
367
368 spin_lock(&kvm->mmu_lock);
369 ret = stage2_set_pte(kvm, 0, &cache.cache, addr, &pte);
370 spin_unlock(&kvm->mmu_lock);
371 if (ret)
372 goto out;
373
374 pfn++;
375 }
376
377 out:
378 kvm_mmu_free_memory_cache(&cache.cache);
379 return ret;
380 }
381
diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c index 523bc934fe2f..66f6706a1037 100644 --- a/arch/arm64/kvm/arm.c +++ b/arch/arm64/kvm/arm.c @@ -320,6 +320,7 @@ int kvm_arch_vcpu_create(struct kvm_vcpu *vcpu) vcpu->arch.target = -1; bitmap_zero(vcpu->arch.features, KVM_VCPU_MAX_FEATURES); + vcpu->arch.mmu_page_cache.capacity = KVM_ARCH_NR_OBJS_PER_MEMORY_CACHE; vcpu->arch.mmu_page_cache.gfp_zero = __GFP_ZERO; /* Set up the timer */ diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c index 53ae2c0640bc..2f2ef6b60ff4 100644 --- a/arch/arm64/kvm/mmu.c +++ b/arch/arm64/kvm/mmu.c @@ -764,7 +764,10 @@ int kvm_phys_addr_ioremap(struct kvm *kvm, phys_addr_t guest_ipa, { phys_addr_t addr; int ret = 0; - struct kvm_mmu_memory_cache cache = { 0, __GFP_ZERO, NULL, }; + struct kvm_mmu_memory_cache cache = { + .capacity = KVM_ARCH_NR_OBJS_PER_MEMORY_CACHE, + .gfp_zero = __GFP_ZERO, + }; struct kvm_pgtable *pgt = kvm->arch.mmu.pgt; enum kvm_pgtable_prot prot = KVM_PGTABLE_PROT_DEVICE | KVM_PGTABLE_PROT_R | diff --git a/arch/mips/kvm/mips.c b/arch/mips/kvm/mips.c index a25e0b73ee70..45c7179144dc 100644 --- a/arch/mips/kvm/mips.c +++ b/arch/mips/kvm/mips.c @@ -387,6 +387,8 @@ int kvm_arch_vcpu_create(struct kvm_vcpu *vcpu) if (err) goto out_free_gebase; + vcpu->arch.mmu_page_cache.capacity = KVM_ARCH_NR_OBJS_PER_MEMORY_CACHE; + return 0; out_free_gebase: diff --git a/arch/riscv/kvm/mmu.c b/arch/riscv/kvm/mmu.c index f80a34fbf102..0e042f40d737 100644 --- a/arch/riscv/kvm/mmu.c +++ b/arch/riscv/kvm/mmu.c @@ -347,10 +347,10 @@ static int stage2_ioremap(struct kvm *kvm, gpa_t gpa, phys_addr_t hpa, int ret = 0; unsigned long pfn; phys_addr_t addr, end; - struct kvm_mmu_memory_cache pcache; - - memset(&pcache, 0, sizeof(pcache)); - pcache.gfp_zero = __GFP_ZERO; + struct kvm_mmu_memory_cache cache = { + .capacity = KVM_ARCH_NR_OBJS_PER_MEMORY_CACHE, + .gfp_zero = __GFP_ZERO, + }; end = (gpa + size + PAGE_SIZE - 1) & PAGE_MASK; pfn = __phys_to_pfn(hpa); @@ -361,12 +361,12 @@ static int stage2_ioremap(struct kvm *kvm, gpa_t gpa, phys_addr_t hpa, if (!writable) pte = pte_wrprotect(pte); - ret = kvm_mmu_topup_memory_cache(&pcache, stage2_pgd_levels); + ret = kvm_mmu_topup_memory_cache(&cache.cache, stage2_pgd_levels); if (ret) goto out; spin_lock(&kvm->mmu_lock); - ret = stage2_set_pte(kvm, 0, &pcache, addr, &pte); + ret = stage2_set_pte(kvm, 0, &cache.cache, addr, &pte); spin_unlock(&kvm->mmu_lock); if (ret) goto out; @@ -375,7 +375,7 @@ static int stage2_ioremap(struct kvm *kvm, gpa_t gpa, phys_addr_t hpa, } out: - kvm_mmu_free_memory_cache(&pcache); + kvm_mmu_free_memory_cache(&cache.cache); return ret; } diff --git a/arch/riscv/kvm/vcpu.c b/arch/riscv/kvm/vcpu.c index 6785aef4cbd4..bbcb9d4a04fb 100644 --- a/arch/riscv/kvm/vcpu.c +++ b/arch/riscv/kvm/vcpu.c @@ -94,6 +94,7 @@ int kvm_arch_vcpu_create(struct kvm_vcpu *vcpu) /* Mark this VCPU never ran */ vcpu->arch.ran_atleast_once = false; + vcpu->arch.mmu_page_cache.capacity = KVM_ARCH_NR_OBJS_PER_MEMORY_CACHE; vcpu->arch.mmu_page_cache.gfp_zero = __GFP_ZERO; /* Setup ISA features available to VCPU */ diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c index a5961c17eb36..5b1458b911ab 100644 --- a/arch/x86/kvm/mmu/mmu.c +++ b/arch/x86/kvm/mmu/mmu.c @@ -5719,12 +5719,21 @@ int kvm_mmu_create(struct kvm_vcpu *vcpu) { int ret; + vcpu->arch.mmu_pte_list_desc_cache.capacity = + KVM_ARCH_NR_OBJS_PER_MEMORY_CACHE; vcpu->arch.mmu_pte_list_desc_cache.kmem_cache = pte_list_desc_cache; vcpu->arch.mmu_pte_list_desc_cache.gfp_zero = __GFP_ZERO; + vcpu->arch.mmu_page_header_cache.capacity = + KVM_ARCH_NR_OBJS_PER_MEMORY_CACHE; vcpu->arch.mmu_page_header_cache.kmem_cache = mmu_page_header_cache; vcpu->arch.mmu_page_header_cache.gfp_zero = __GFP_ZERO; + vcpu->arch.mmu_shadowed_info_cache.capacity = + KVM_ARCH_NR_OBJS_PER_MEMORY_CACHE; + + vcpu->arch.mmu_shadow_page_cache.capacity = + KVM_ARCH_NR_OBJS_PER_MEMORY_CACHE; vcpu->arch.mmu_shadow_page_cache.gfp_zero = __GFP_ZERO; vcpu->arch.mmu = &vcpu->arch.root_mmu; diff --git a/include/linux/kvm_types.h b/include/linux/kvm_types.h index ac1ebb37a0ff..549103a4f7bc 100644 --- a/include/linux/kvm_types.h +++ b/include/linux/kvm_types.h @@ -83,14 +83,19 @@ struct gfn_to_pfn_cache { * MMU flows is problematic, as is triggering reclaim, I/O, etc... while * holding MMU locks. Note, these caches act more like prefetch buffers than * classical caches, i.e. objects are not returned to the cache on being freed. + * + * The storage for the cache object pointers is allocated dynamically when the + * cache is topped-up. The capacity field defines the number of object pointers + * available after the struct. */ struct kvm_mmu_memory_cache { int nobjs; + int capacity; gfp_t gfp_zero; struct kmem_cache *kmem_cache; - void *objects[KVM_ARCH_NR_OBJS_PER_MEMORY_CACHE]; + void **objects; }; -#endif +#endif /* KVM_ARCH_NR_OBJS_PER_MEMORY_CACHE */ #define HALT_POLL_HIST_COUNT 32 diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c index dfb7dabdbc63..29f84d7c1950 100644 --- a/virt/kvm/kvm_main.c +++ b/virt/kvm/kvm_main.c @@ -371,12 +371,23 @@ static inline void *mmu_memory_cache_alloc_obj(struct kvm_mmu_memory_cache *mc, int kvm_mmu_topup_memory_cache(struct kvm_mmu_memory_cache *mc, int min) { + gfp_t gfp = GFP_KERNEL_ACCOUNT; void *obj; if (mc->nobjs >= min) return 0; - while (mc->nobjs < ARRAY_SIZE(mc->objects)) { - obj = mmu_memory_cache_alloc_obj(mc, GFP_KERNEL_ACCOUNT); + + if (WARN_ON(mc->capacity == 0)) + return -EINVAL; + + if (!mc->objects) { + mc->objects = kvmalloc_array(sizeof(void *), mc->capacity, gfp); + if (!mc->objects) + return -ENOMEM; + } + + while (mc->nobjs < mc->capacity) { + obj = mmu_memory_cache_alloc_obj(mc, gfp); if (!obj) return mc->nobjs >= min ? 0 : -ENOMEM; mc->objects[mc->nobjs++] = obj; @@ -397,6 +408,11 @@ void kvm_mmu_free_memory_cache(struct kvm_mmu_memory_cache *mc) else free_page((unsigned long)mc->objects[--mc->nobjs]); } + + kvfree(mc->objects); + + /* Note, must set to NULL to avoid use-after-free in the next top-up. */ + mc->objects = NULL; } void *kvm_mmu_memory_cache_alloc(struct kvm_mmu_memory_cache *mc)
Allow the capacity of the kvm_mmu_memory_cache struct to be chosen at declaration time rather than being fixed for all declarations. This will be used in a follow-up commit to declare an cache in x86 with a capacity of 512+ objects without having to increase the capacity of all caches in KVM. This change requires each cache now specify its capacity at runtime, since the cache struct itself no longer has a fixed capacity known at compile time. To protect against someone accidentally defining a kvm_mmu_memory_cache struct directly (without the extra storage), this commit includes a WARN_ON() in kvm_mmu_topup_memory_cache(). In order to support different capacities, this commit changes the objects pointer array to be dynamically allocated the first time the cache is topped-up. An alternative would be to lay out the objects array after the kvm_mmu_memory_cache struct, which can be done at compile time. But that change, unfortunately, adds some grottiness to arm64 and riscv, which uses a function-local (i.e. stack-allocated) kvm_mmu_memory_cache struct. Since C does not allow anonymous structs in functions, the new wrapper struct that contains kvm_mmu_memory_cache and the objects pointer array, must be named, which means dealing with an outer and inner struct. The outer struct can't be dropped since then there would be no guarantee the kvm_mmu_memory_cache struct and objects array would be laid out consecutively on the stack. No functional change intended. Signed-off-by: David Matlack <dmatlack@google.com> --- arch/arm64/kvm/arm.c | 1 + arch/arm64/kvm/mmu.c | 5 ++++- arch/mips/kvm/mips.c | 2 ++ arch/riscv/kvm/mmu.c | 14 +++++++------- arch/riscv/kvm/vcpu.c | 1 + arch/x86/kvm/mmu/mmu.c | 9 +++++++++ include/linux/kvm_types.h | 9 +++++++-- virt/kvm/kvm_main.c | 20 ++++++++++++++++++-- 8 files changed, 49 insertions(+), 12 deletions(-)