| Message ID | 20230209024022.3371768-3-robert.hu@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Linear Address Masking (LAM) KVM Enabling | expand |
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. >+#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? And what's the value of the #ifdef? > root_gfn = root_pgd >> PAGE_SHIFT; > > if (mmu_check_root(vcpu, root_gfn)) >-- >2.31.1 >
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. > And what's > the value of the #ifdef? LAM is only available in 64 bit mode. > > > root_gfn = root_pgd >> PAGE_SHIFT; > > > > if (mmu_check_root(vcpu, root_gfn)) > > -- > > 2.31.1 > >
On 2/9/2023 10:40 AM, Robert Hoo wrote: [...] > - > +#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 I prefer using: root_pgd = mmu->get_guest_pgd(vcpu); if (IS_ENABLED(CONFIG_X86_64)) root_pgd &= ~(X86_CR3_LAM_U48 | X86_CR3_LAM_U57); I looks more structured. > root_gfn = root_pgd >> PAGE_SHIFT; > > if (mmu_check_root(vcpu, root_gfn))
On Fri, 2023-02-10 at 11:38 +0800, Yang, Weijiang wrote: > On 2/9/2023 10:40 AM, Robert Hoo wrote: > > [...] > > > > - > > +#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 > > I prefer using: > > root_pgd = mmu->get_guest_pgd(vcpu); > > if (IS_ENABLED(CONFIG_X86_64)) > > root_pgd &= ~(X86_CR3_LAM_U48 | X86_CR3_LAM_U57); > > I looks more structured. > OK, thanks, both look all right to me. Just don't quite understand the "structured" meaning, would you elaborate more? Same as 8d5265b1016 ("KVM: x86/mmu: Use IS_ENABLED() to avoid RETPOLINE for TDP page faults")?
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? > >> And what's >> the value of the #ifdef? > LAM is only available in 64 bit mode. In other modes, the two bits are either ignored or not defined, seems safe to clear the two bits unconditionally. >>> root_gfn = root_pgd >> PAGE_SHIFT; >>> >>> if (mmu_check_root(vcpu, root_gfn)) >>> -- >>> 2.31.1 >>>
On Tue, 2023-02-14 at 10:55 +0800, Binbin Wu wrote: > > 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? > I'm looking into the async PF. Anyone who's familiar with async PF can shed some light? If kvm_arch_setup_async_pf()/kvm_arch_async_page_ready() needs whole CR3, would you agree we have 2 methods: get_cr3() and get_pgd()?
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.
diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c index 835426254e76..1d61dfe37c77 100644 --- 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; - +#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 root_gfn = root_pgd >> PAGE_SHIFT; if (mmu_check_root(vcpu, root_gfn))
mmu->get_guest_pgd()'s implementation is get_cr3(), clear the LAM bits for root_pgd, which needs a pure address, plus (possible) PCID info (low 12 bits). Signed-off-by: Robert Hoo <robert.hu@linux.intel.com> --- arch/x86/kvm/mmu/mmu.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)