On Tue, Apr 04, 2017 at 11:28:09AM +0100, Marc Zyngier wrote: > On 04/04/17 11:14, Suzuki K Poulose wrote: > > On 03/04/17 22:15, kbuild test robot wrote: > >> tree: https://git.kernel.org/pub/scm/linux/kernel/git/kvmarm/kvmarm.git master > >> head: 1f1c45c6f66a586ca420ca02cbd93a35690394f9 > >> commit: f9d9eb7f7a2c7e388861fe1cdb253f63e63555fe [1/3] kvm: arm/arm64: Fix locking for kvm_free_stage2_pgd > >> config: arm-axm55xx_defconfig (attached as .config) > >> compiler: arm-linux-gnueabi-gcc (Debian 6.1.1-9) 6.1.1 20160705 > >> reproduce: > >> wget https://raw.githubusercontent.com/01org/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross > >> chmod +x ~/bin/make.cross > >> git checkout f9d9eb7f7a2c7e388861fe1cdb253f63e63555fe > >> # save the attached .config to linux build tree > >> make.cross ARCH=arm > >> > >> All errors (new ones prefixed by >>): > >> > >> arch/arm/kvm/mmu.c: In function 'unmap_stage2_range': > >>>> arch/arm/kvm/mmu.c:302:14: error: 'S2_PUD_SIZE' undeclared (first use in this function) > >> if (size > S2_PUD_SIZE) > >> ^~~~~~~~~~~ > > > > Thanks kbuild for catching this one ! > > > >> arch/arm/kvm/mmu.c:302:14: note: each undeclared identifier is reported only once for each function it appears in > >> > >> vim +/S2_PUD_SIZE +302 arch/arm/kvm/mmu.c > >> > >> 296 pgd = kvm->arch.pgd + stage2_pgd_index(addr); > >> 297 do { > >> 298 /* > >> 299 * If the range is too large, release the kvm->mmu_lock > >> 300 * to prevent starvation and lockup detector warnings. > >> 301 */ > >> > 302 if (size > S2_PUD_SIZE) > >> 303 cond_resched_lock(&kvm->mmu_lock); > >> 304 next = stage2_pgd_addr_end(addr, end); > >> 305 if (!stage2_pgd_none(*pgd)) > >> > > > > > > Marc, Christoffer, > > > > Ah! I didn't test this on arm32. We have two options : > > > > 1) Define S2_P{U,M}_SIZE for arm32 in asm/stage2_pgtable.h > > > > or, > > > > 2) use the following hunk on top of the patch, which changes the lock > > release after we process one PGDIR entry. As for the first time we enter > > the loop we haven't done much with the lock held, hence it may make > > sense to do it after the first round and we have more work to do. > > > > Let me know what you think > > > > > > diff --git a/arch/arm/kvm/mmu.c b/arch/arm/kvm/mmu.c > > index db94f3a..582a972 100644 > > --- a/arch/arm/kvm/mmu.c > > +++ b/arch/arm/kvm/mmu.c > > @@ -295,15 +295,15 @@ static void unmap_stage2_range(struct kvm *kvm, phys_addr_t start, u64 size) > > assert_spin_locked(&kvm->mmu_lock); > > pgd = kvm->arch.pgd + stage2_pgd_index(addr); > > do { > > + next = stage2_pgd_addr_end(addr, end); > > + if (!stage2_pgd_none(*pgd)) > > + unmap_stage2_puds(kvm, pgd, addr, next); > > /* > > * If the range is too large, release the kvm->mmu_lock > > * to prevent starvation and lockup detector warnings. > > */ > > - if (size > S2_PUD_SIZE) > > + if (next != end) > > cond_resched_lock(&kvm->mmu_lock); > > - next = stage2_pgd_addr_end(addr, end); > > - if (!stage2_pgd_none(*pgd)) > > - unmap_stage2_puds(kvm, pgd, addr, next); > > } while (pgd++, addr = next, addr != end); > > } > > Yup, I quite like this last option, as it doesn't rely on a particular > size (or just implicitly that of the PGD). Can you respin this? > Agreed, I prefer this over my suggestion (sent as a reply to the original patch). Thanks, -Christoffer _______________________________________________ kvmarm mailing list kvmarm@xxxxxxxxxxxxxxxxxxxxx https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
