Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 24 Jun 2022 18:50:27 +0000
From:   Sean Christopherson <seanjc@google.com>
To:     David Matlack <dmatlack@google.com>
Cc:     Paolo Bonzini <pbonzini@redhat.com>,
        Vitaly Kuznetsov <vkuznets@redhat.com>,
        Wanpeng Li <wanpengli@tencent.com>,
        Jim Mattson <jmattson@google.com>,
        Joerg Roedel <joro@8bytes.org>, kvm@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH 1/3] KVM: x86/mmu: Avoid subtle pointer arithmetic in
 kvm_mmu_child_role()
Message-ID: <YrYHc4BIAf+pGRhW@google.com>
References: <20220624171808.2845941-1-seanjc@google.com>
 <20220624171808.2845941-2-seanjc@google.com>
 <YrX1WB1FZzXiR+Io@google.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <YrX1WB1FZzXiR+Io@google.com>
Precedence: bulk

On Fri, Jun 24, 2022, David Matlack wrote:
> On Fri, Jun 24, 2022 at 05:18:06PM +0000, Sean Christopherson wrote:
> > --- a/arch/x86/kvm/mmu/mmu.c
> > +++ b/arch/x86/kvm/mmu/mmu.c
> > @@ -2168,7 +2168,8 @@ static struct kvm_mmu_page *kvm_mmu_get_shadow_page(struct kvm_vcpu *vcpu,
> >  	return __kvm_mmu_get_shadow_page(vcpu->kvm, vcpu, &caches, gfn, role);
> >  }
> >  
> > -static union kvm_mmu_page_role kvm_mmu_child_role(u64 *sptep, bool direct, unsigned int access)
> > +static union kvm_mmu_page_role kvm_mmu_child_role(u64 *sptep, bool direct,
> > +						  unsigned int access)
> >  {
> >  	struct kvm_mmu_page *parent_sp = sptep_to_sp(sptep);
> >  	union kvm_mmu_page_role role;
> > @@ -2195,13 +2196,19 @@ static union kvm_mmu_page_role kvm_mmu_child_role(u64 *sptep, bool direct, unsig
> >  	 * uses 2 PAE page tables, each mapping a 2MiB region. For these,
> >  	 * @role.quadrant encodes which half of the region they map.
> >  	 *
> > -	 * Note, the 4 PAE page directories are pre-allocated and the quadrant
> > -	 * assigned in mmu_alloc_root(). So only page tables need to be handled
> > -	 * here.
> > +	 * Concretely, a 4-byte PDE consumes bits 31:22, while an 8-byte PDE
> > +	 * consumes bits 29:21.  To consume bits 31:30, KVM's uses 4 shadow
> > +	 * PDPTEs; those 4 PAE page directories are pre-allocated and their
> > +	 * quadrant is assigned in mmu_alloc_root().   A 4-byte PTE consumes
> > +	 * bits 21:12, while an 8-byte PTE consumes bits 20:12.  To consume
> > +	 * bit 21 in the PTE (the child here), KVM propagates that bit to the
> > +	 * quadrant, i.e. sets quadrant to '0' or '1'.  The parent 8-byte PDE
> > +	 * covers bit 21 (see above), thus the quadrant is calculated from the
> > +	 * _least_ significant bit of the PDE index.
> >  	 */
> >  	if (role.has_4_byte_gpte) {
> >  		WARN_ON_ONCE(role.level != PG_LEVEL_4K);
> > -		role.quadrant = (sptep - parent_sp->spt) % 2;
> > +		role.quadrant = ((unsigned long)sptep / sizeof(*sptep)) & 1;
> >  	}
> 
> I find both difficult to read TBH.

No argument there.  My objection to the pointer arithmetic is that it's easy to
misread.

> And "sptep -> sp->spt" is repeated in other places.

> 
> How about using this oppotunity to introduce a helper that turns an
> sptep into an index to use here and clean up the other users?
> 
> e.g.
> 
> static inline int spte_index(u64 *sptep)
> {
>         return ((unsigned long)sptep / sizeof(*sptep)) & (SPTE_ENT_PER_PAGE - 1);
> }
> 
> Then kvm_mmu_child_role() becomes:
> 
>         if (role.has_4_byte_gpte) {
>         	WARN_ON_ONCE(role.level != PG_LEVEL_4K);
>         	role.quadrant = spte_index(sptep) & 1;
>         }

Nice!  I like this a lot.  Will do in v2.