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.
