Hi Gavin, Cheers for taking a look. On Wed, Aug 19, 2020 at 02:38:39PM +1000, Gavin Shan wrote: > On 8/18/20 11:27 PM, Will Deacon wrote: > > diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c > > index 662b0c99a63d..4a24ebdc6fc6 100644 > > --- a/arch/arm64/kvm/mmu.c > > +++ b/arch/arm64/kvm/mmu.c > > @@ -1489,19 +1489,17 @@ int kvm_phys_addr_ioremap(struct kvm *kvm, phys_addr_t guest_ipa, > > ret = kvm_mmu_topup_memory_cache(&cache, > > kvm_mmu_cache_min_pages(kvm)); > > if (ret) > > - goto out; > > + break; > > spin_lock(&kvm->mmu_lock); > > ret = stage2_set_pte(&kvm->arch.mmu, &cache, addr, &pte, > > KVM_S2PTE_FLAG_IS_IOMAP); > > spin_unlock(&kvm->mmu_lock); > > if (ret) > > - goto out; > > + break; > > pfn++; > > } > > -out: > > - kvm_mmu_free_memory_cache(&cache); > > return ret; > > } > > It seems incorrect. The cache is tracked by local variable (@cache), > meaning the cache is only visible to kvm_phys_addr_ioremap() and its > callee. So it's correct to free unused pages in two cases: (1) error > is returned (2) high level of page tables were previously populated > and not all pre-allocated pages are used. Otherwise, this leads to > memory leakage. Well spotted, you're completely right. I was _sure_ this was the vCPU memcache and I even said as much in the commit meesage, but it's not, and it never was, so I can drop this patch. If there are any other patches I can drop in the series, please let me know! I did test with kmemleak enabled, but I guess that doesn't track page allocations into the memcache. It _might_ be an idea to have a per-VM memcache to handle these allocations, as that might offer some reuse over sticking one on the stack each time, but then again kvm_phys_addr_ioremap() is hardly a fastpath. Will _______________________________________________ kvmarm mailing list kvmarm@xxxxxxxxxxxxxxxxxxxxx https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
