Re: [RFC PATCH 04/23] x86/cpufeatures: Add SGX1 and SGX2 sub-features

From: Kai Huang <kai.huang@intel.com>
To: Sean Christopherson <seanjc@google.com>
Cc: Borislav Petkov <bp@alien8.de>,
	Dave Hansen <dave.hansen@intel.com>,
	linux-sgx@vger.kernel.org, kvm@vger.kernel.org, x86@kernel.org,
	jarkko@kernel.org, luto@kernel.org, haitao.huang@intel.com,
	pbonzini@redhat.com, tglx@linutronix.de, mingo@redhat.com,
	hpa@zytor.com
Subject: Re: [RFC PATCH 04/23] x86/cpufeatures: Add SGX1 and SGX2 sub-features
Date: Tue, 12 Jan 2021 15:01:28 +1300	[thread overview]
Message-ID: <20210112150128.1e0d621e053bbbf210bcb946@intel.com> (raw)
In-Reply-To: <X/yk6zcJTLXJwIrJ@google.com>

On Mon, 11 Jan 2021 11:20:11 -0800 Sean Christopherson wrote:
> On Mon, Jan 11, 2021, Borislav Petkov wrote:
> > On Mon, Jan 11, 2021 at 09:54:17AM -0800, Sean Christopherson wrote:
> > > It would be possible for KVM to break the dependency on X86_FEATURE_* bit
> > > offsets by defining a translation layer, but I strongly feel that adding manual
> > > translations will do more harm than good as it increases the odds of us botching
> > > a translation or using the wrong feature flag, creates potential namespace
> > > conflicts, etc...
> > 
> > Ok, lemme see if we might encounter more issues down the road...
> > 
> > +enum kvm_only_cpuid_leafs {
> > +       CPUID_12_EAX     = NCAPINTS,
> > +       NR_KVM_CPU_CAPS,
> > +
> > +       NKVMCAPINTS = NR_KVM_CPU_CAPS - NCAPINTS,
> > +};
> > +
> > 
> > What happens when we decide to allocate a separate leaf for CPUID_12_EAX
> > down the road?
> 
> Well, mechanically, that would generate a build failure if the kernel does the
> obvious things and names the 'enum cpuid_leafs' entry CPUID_12_EAX.  That would
> be an obvious clue that KVM should be updated.
> 
> If the kernel named the enum entry something different, and we botched the code
> review, KVM would continue to work, but would unnecessarily copy the bits it
> cares about to its own word.   E.g. the boot_cpu_has() checks and translation to
> __X86_FEATURE_* would still be valid.  As far as failure modes go, that's not
> terrible.

Should we add a dedicated, i.e. kvm_scattered_cpu_caps[], instead of using
existing kvm_cpu_cap[NCAPINTS]? If so this issue can be avoided??

> 
> > You do it already here
> > 
> > Subject: [PATCH 04/13] x86/cpufeatures: Assign dedicated feature word for AMD mem encryption
> > 
> > for the AMD leaf.
> > 
> > I'm thinking this way around - from scattered to a hw one - should be ok
> > because that should work easily. The other way around, taking a hw leaf
> > and scattering it around x86_capability[] array elems would probably be
> > nasty but with your change that should work too.
> > 
> > Yah, I'm just hypothesizing here - I don't think this "other way around"
> > will ever happen...
> > 
> > Hmm, yap, I can cautiously say that with your change we should be ok...
> > 
> > Thx.
> > 
> > -- 
> > Regards/Gruss,
> >     Boris.
> > 
> > https://people.kernel.org/tglx/notes-about-netiquette