linux-kernel - Re: [RFC PATCH v6 037/104] KVM: x86/mmu: Allow non-zero value for non-present SPTE

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <YuxU/VXlSwVip7ys@google.com>
Date:   Thu, 4 Aug 2022 23:23:41 +0000
From:   Sean Christopherson <seanjc@...gle.com>
To:     David Matlack <dmatlack@...gle.com>
Cc:     isaku.yamahata@...el.com, kvm@...r.kernel.org,
        linux-kernel@...r.kernel.org, isaku.yamahata@...il.com,
        Paolo Bonzini <pbonzini@...hat.com>, erdemaktas@...gle.com,
        Sagi Shahar <sagis@...gle.com>
Subject: Re: [RFC PATCH v6 037/104] KVM: x86/mmu: Allow non-zero value for
 non-present SPTE

On Thu, Aug 04, 2022, David Matlack wrote:
> On Thu, May 05, 2022 at 11:14:31AM -0700, isaku.yamahata@...el.com 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@redhat.com

> 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.