Re: [PATCH 03/26] KVM: arm64: Factor out stage 2 page table data from struct kvm

[Marc Zyngier <maz@kernel.org>]

Hi Andrew,

On Tue, 05 May 2020 16:26:48 +0100,
Andrew Scull <ascull@google.com> wrote:
> 
> Having a go at reviewing. Might turn out to be more useful as a learning
> exercise for me rather than useful feedback but we've got to start
> somewhere..

Thanks for making the effort. Asking questions is never a pointless
exercise, as it usually means that something isn't as crystal clear as
the author expects... ;-)

> 
> > -struct kvm_arch {
> > +struct kvm_s2_mmu {
> >  	struct kvm_vmid vmid;
> >  
> > -	/* stage2 entry level table */
> > -	pgd_t *pgd;
> > -	phys_addr_t pgd_phys;
> > -
> > -	/* VTCR_EL2 value for this VM */
> > -	u64    vtcr;
> > +	/*
> > +	 * stage2 entry level table
> > +	 *
> > +	 * Two kvm_s2_mmu structures in the same VM can point to the same pgd
> > +	 * here.  This happens when running a non-VHE guest hypervisor which
> > +	 * uses the canonical stage 2 page table for both vEL2 and for vEL1/0
> > +	 * with vHCR_EL2.VM == 0.
> > +	 */
> > +	pgd_t		*pgd;
> > +	phys_addr_t	pgd_phys;
> >  
> >  	/* The last vcpu id that ran on each physical CPU */
> >  	int __percpu *last_vcpu_ran;
> >  
> > +	struct kvm *kvm;
> > +};
> > +
> > +struct kvm_arch {
> > +	struct kvm_s2_mmu mmu;
> > +
> > +	/* VTCR_EL2 value for this VM */
> > +	u64    vtcr;
> 
> VTCR seems quite strongly tied to the MMU config. Is it not controlled
> independently for the nested MMUs and so remains in this struct?

This particular instance of VTCR_EL2 is the host's version. Which
means it describes the virtual HW for the EL1 guest. It constraints,
among other things, the number of IPA bits for the guest, for example,
and is configured by the VMM.

Once you start nesting, each vcpu has its own VTCR_EL2 which is still
constrained by the main one (no nested guest can have a T0SZ bigger
than the value imposed by userspace for this guest as a whole).

Does it make sense?

> 
> > -static void stage2_dissolve_pmd(struct kvm *kvm, phys_addr_t addr, pmd_t *pmd)
> > +static void stage2_dissolve_pmd(struct kvm_s2_mmu *mmu, phys_addr_t addr, pmd_t *pmd)
> 
> How strictly is the long line style rule enforced? checkpatch has 16
> such warnings on this patch.

It isn't enforced at all for KVM/arm. I am perfectly happy with
longish lines (I stupidly gave away my vt100 a very long time ago).
In general, checkpatch warnings are to be looked into (it sometimes
brings interesting stuff up), but this falls into the *cosmetic*
department, and I cannot be bothered.

> 
> > -static void stage2_dissolve_pud(struct kvm *kvm, phys_addr_t addr, pud_t *pudp)
> > +static void stage2_dissolve_pud(struct kvm_s2_mmu *mmu, phys_addr_t addr, pud_t *pudp)
> >  {
> > +	struct kvm *kvm __maybe_unused = mmu->kvm;
> > +
> >  	if (!stage2_pud_huge(kvm, *pudp))
> >  		return;
> 
> There're a couple places with `__maybe_unused` on variables that are
> then used soon after. Can they be dropped in these cases so as not to
> hide legitimate warning?

Absolutely. I'll have a look.

Thanks for the review!

	M.

-- 
Jazz is not dead, it just smells funny.
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm