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

From: Sean Christopherson <seanjc@google.com>
To: Borislav Petkov <bp@alien8.de>
Cc: Kai Huang <kai.huang@intel.com>,
	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: Mon, 11 Jan 2021 11:20:11 -0800	[thread overview]
Message-ID: <X/yk6zcJTLXJwIrJ@google.com> (raw)
In-Reply-To: <20210111190901.GG25645@zn.tnic>

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.

> 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