On Thu, Aug 04, 2022, David Matlack wrote:
> On Thu, May 05, 2022 at 11:14:31AM -0700, isaku.yamahata@xxxxxxxxx wrote:
> > +static inline void kvm_init_shadow_page(void *page)
> > +{
> > +#ifdef CONFIG_X86_64
> > + int ign;
> > +
> > + WARN_ON_ONCE(shadow_nonpresent_value != SHADOW_NONPRESENT_VALUE);
> > + asm volatile (
> > + "rep stosq\n\t"
> > + : "=c"(ign), "=D"(page)
> > + : "a"(SHADOW_NONPRESENT_VALUE), "c"(4096/8), "D"(page)
> > + : "memory"
> > + );
>
> Use memset64()?
Huh. The optimized x86-64 versions were added in 2017 (4c51248533ad ("x86: implement
memset16, memset32 & memset64"), so I can't even claim I wrote this before there
was a perfect fit.
> > @@ -5643,7 +5687,8 @@ int kvm_mmu_create(struct kvm_vcpu *vcpu)
> > vcpu->arch.mmu_page_header_cache.kmem_cache = mmu_page_header_cache;
> > vcpu->arch.mmu_page_header_cache.gfp_zero = __GFP_ZERO;
> >
> > - vcpu->arch.mmu_shadow_page_cache.gfp_zero = __GFP_ZERO;
> > + if (!(is_tdp_mmu_enabled(vcpu->kvm) && shadow_nonpresent_value))
> > + vcpu->arch.mmu_shadow_page_cache.gfp_zero = __GFP_ZERO;
>
> Is there any reason to prefer using __GFP_ZERO? I suspect the code would
> be simpler if KVM unconditionally initialized shadow pages.
Hmm, we'd have to implement kvm_init_shadow_page() for 32-bit builds, and I don't
love having "gfp_zero" but not using it when we need zeros, but if the end result
is simpler, I'm definitely ok with omitting __GFP_ZERO and always flowing through
kvm_init_shadow_page().
> > diff --git a/arch/x86/kvm/mmu/spte.h b/arch/x86/kvm/mmu/spte.h
> > index fbbab180395e..3319ca7f8f48 100644
> > --- a/arch/x86/kvm/mmu/spte.h
> > +++ b/arch/x86/kvm/mmu/spte.h
> > @@ -140,6 +140,19 @@ static_assert(MMIO_SPTE_GEN_LOW_BITS == 8 && MMIO_SPTE_GEN_HIGH_BITS == 11);
> >
> > #define MMIO_SPTE_GEN_MASK GENMASK_ULL(MMIO_SPTE_GEN_LOW_BITS + MMIO_SPTE_GEN_HIGH_BITS - 1, 0)
> >
> > +/*
> > + * non-present SPTE value for both VMX and SVM for TDP MMU.
> > + * For SVM NPT, for non-present spte (bit 0 = 0), other bits are ignored.
> > + * For VMX EPT, bit 63 is ignored if #VE is disabled.
> > + * bit 63 is #VE suppress if #VE is enabled.
> > + */
> > +#ifdef CONFIG_X86_64
> > +#define SHADOW_NONPRESENT_VALUE BIT_ULL(63)
> > +static_assert(!(SHADOW_NONPRESENT_VALUE & SPTE_MMU_PRESENT_MASK));
> > +#else
> > +#define SHADOW_NONPRESENT_VALUE 0ULL
> > +#endif
>
> The terminology "shadow_nonpresent" implies it would be the opposite of
> e.g. is_shadow_present_pte(), when in fact they are completely
> different concepts.
You can fight Paolo over that one :-) I agree it looks a bit odd when juxtaposed
with is_shadow_present_pte(), but at the same time I agree with Paolo that
SHADOW_INIT_VALUE is also funky.
https://lore.kernel.org/all/9dfc44d6-6b20-e864-8d4f-09ab7d489b97@xxxxxxxxxx
> Also, this is a good opportunity to follow the same naming terminology
> as REMOVED_SPTE in the TDP MMU.
>
> How about EMPTY_SPTE?
No, because "empty" implies there's nothing there, and it very much matters that
the SUPPRESS_VE bit is set for TDX.
