Re: [PATCH v3 05/21] KVM: arm64: Add support for creating kernel-agnostic stage-2 page tables

[Will Deacon <will@kernel.org>]

On Wed, Sep 02, 2020 at 04:40:03PM +1000, Gavin Shan wrote:
> On 8/25/20 7:39 PM, Will Deacon wrote:
> > Introduce alloc() and free() functions to the generic page-table code
> > for guest stage-2 page-tables and plumb these into the existing KVM
> > page-table allocator. Subsequent patches will convert other operations
> > within the KVM allocator over to the generic code.
> > 
> > Cc: Marc Zyngier <maz@kernel.org>
> > Cc: Quentin Perret <qperret@google.com>
> > Signed-off-by: Will Deacon <will@kernel.org>
> > ---
> >   arch/arm64/include/asm/kvm_host.h    |  1 +
> >   arch/arm64/include/asm/kvm_pgtable.h | 18 +++++++++
> >   arch/arm64/kvm/hyp/pgtable.c         | 51 ++++++++++++++++++++++++++
> >   arch/arm64/kvm/mmu.c                 | 55 +++++++++++++++-------------
> >   4 files changed, 99 insertions(+), 26 deletions(-)
> > 
> 
> With the following one question resolved:
> 
> Reviewed-by: Gavin Shan <gshan@redhat.com>

Thanks!

> > diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c
> > index fabd72b0c8a4..4607e9ca60a2 100644
> > --- a/arch/arm64/kvm/mmu.c
> > +++ b/arch/arm64/kvm/mmu.c
> > @@ -668,47 +668,49 @@ int create_hyp_exec_mappings(phys_addr_t phys_addr, size_t size,
> >    * @kvm:	The pointer to the KVM structure
> >    * @mmu:	The pointer to the s2 MMU structure
> >    *
> > - * Allocates only the stage-2 HW PGD level table(s) of size defined by
> > - * stage2_pgd_size(mmu->kvm).
> > - *
> > + * Allocates only the stage-2 HW PGD level table(s).
> >    * Note we don't need locking here as this is only called when the VM is
> >    * created, which can only be done once.
> >    */
> >   int kvm_init_stage2_mmu(struct kvm *kvm, struct kvm_s2_mmu *mmu)
> >   {
> > -	phys_addr_t pgd_phys;
> > -	pgd_t *pgd;
> > -	int cpu;
> > +	int cpu, err;
> > +	struct kvm_pgtable *pgt;
> > -	if (mmu->pgd != NULL) {
> > +	if (mmu->pgt != NULL) {
> >   		kvm_err("kvm_arch already initialized?\n");
> >   		return -EINVAL;
> >   	}
> > -	/* Allocate the HW PGD, making sure that each page gets its own refcount */
> > -	pgd = alloc_pages_exact(stage2_pgd_size(kvm), GFP_KERNEL | __GFP_ZERO);
> > -	if (!pgd)
> > +	pgt = kzalloc(sizeof(*pgt), GFP_KERNEL);
> > +	if (!pgt)
> >   		return -ENOMEM;
> > -	pgd_phys = virt_to_phys(pgd);
> > -	if (WARN_ON(pgd_phys & ~kvm_vttbr_baddr_mask(kvm)))
> > -		return -EINVAL;
> > +	err = kvm_pgtable_stage2_init(pgt, kvm);
> > +	if (err)
> > +		goto out_free_pgtable;
> >   	mmu->last_vcpu_ran = alloc_percpu(typeof(*mmu->last_vcpu_ran));
> >   	if (!mmu->last_vcpu_ran) {
> > -		free_pages_exact(pgd, stage2_pgd_size(kvm));
> > -		return -ENOMEM;
> > +		err = -ENOMEM;
> > +		goto out_destroy_pgtable;
> >   	}
> >   	for_each_possible_cpu(cpu)
> >   		*per_cpu_ptr(mmu->last_vcpu_ran, cpu) = -1;
> >   	mmu->kvm = kvm;
> > -	mmu->pgd = pgd;
> > -	mmu->pgd_phys = pgd_phys;
> > +	mmu->pgt = pgt;
> > +	mmu->pgd_phys = __pa(pgt->pgd);
> > +	mmu->pgd = (void *)pgt->pgd;
> >   	mmu->vmid.vmid_gen = 0;
> > -
> >   	return 0;
> > +
> > +out_destroy_pgtable:
> > +	kvm_pgtable_stage2_destroy(pgt);
> > +out_free_pgtable:
> > +	kfree(pgt);
> > +	return err;
> >   }
> > 
> 
> kvm_pgtable_stage2_destroy() might not needed here because
> the stage2 page pgtable is empty so far. However, it should
> be rare to hit the case. If I'm correct, what we need to do
> is just freeing the PGDs.

Right, but kvm_pgtable_stage2_destroy() also frees the PGDs because it
knows how many pages there are and they were allocated by
kvm_pgtable_stage2_init().

Will
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm