Re: [PATCH v4 2/9] KVM: x86: MMU: Clear CR3 LAM bits when allocate shadow root

Robert Hoo
2023-02-16T02:14:22Z

On Tue, 2023-02-14 at 10:55 +0800, Binbin Wu wrote:
> On 2/9/2023 9:02 PM, Robert Hoo wrote:
> > On Thu, 2023-02-09 at 17:55 +0800, Chao Gao wrote:
> > > On Thu, Feb 09, 2023 at 10:40:15AM +0800, Robert Hoo wrote:
> > > > --- a/arch/x86/kvm/mmu/mmu.c
> > > > +++ b/arch/x86/kvm/mmu/mmu.c
> > > > @@ -3698,8 +3698,11 @@ static int mmu_alloc_shadow_roots(struct
> > > > kvm_vcpu *vcpu)
> > > > gfn_t root_gfn, root_pgd;
> > > > int quadrant, i, r;
> > > > hpa_t root;
> > > > -
> > >
> > > The blank line should be kept.
> >
> > OK
> > > > +#ifdef CONFIG_X86_64
> > > > + root_pgd = mmu->get_guest_pgd(vcpu) & ~(X86_CR3_LAM_U48
> > > > |
> > > > X86_CR3_LAM_U57);
> > > > +#else
> > > > root_pgd = mmu->get_guest_pgd(vcpu);
> > > > +#endif
> > >
> > > Why are other call sites of mmu->get_guest_pgd() not changed?
> >
> > Emm, the other 3 are
> > FNAME(walk_addr_generic)()
> > kvm_arch_setup_async_pf()
> > kvm_arch_async_page_ready
> >
> > In former version, I clear CR3.LAM bits for guest_pgd inside mmu-
> > > get_guest_pgd(). I think this is generic. Perhaps I should still
> > > do it
> >
> > in that way. Let's wait for other's comments on this.
> > Thanks for pointing out.
>
> I also prefer to handle it inside get_guest_pdg,
> but in kvm_arch_setup_async_pf()/kvm_arch_async_page_ready(), the
> value
> is assigned to
> cr3 of struct kvm_arch_async_pf, does it requires all fileds of cr3?
>
Took a rough look at the code, looks like
kvm_arch_setup_sync_fp() preserves the temporal cr3
kvm_arch_async_page_ready() confirms the ready (resolved) PF does
correspond to current vcpu context.
To be conservative, I prefer keep
kvm_arch_setup_async_pf()/kvm_arch_async_page_ready() as is, i.e. LAM
bits is contained in the checking.

As for FNAME(walk_addr_generic)()
pte = mmu->get_guest_pgd(vcpu);
I think it's like mmu_alloc_shadow_roots(), i.e. LAM bits should be
cleared.

If no objection, I'll update in next version.