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

[Sean Christopherson <seanjc@google.com>]

On Tue, Jan 12, 2021, Borislav Petkov wrote:
> On Mon, Jan 11, 2021 at 11:20:11AM -0800, Sean Christopherson wrote:
> > 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.
> 
> Then we need to properly document that whenever someone does that
> change, someone needs to touch the proper places.
> 
> > 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.
> 
> Right, which reminds me: with your prototype patch, we would have:
> 
> static __always_inline void __kvm_cpu_cap_mask(enum cpuid_leafs leaf)
> {
>         const struct cpuid_reg cpuid = x86_feature_cpuid(leaf * 32);
>         struct kvm_cpuid_entry2 entry;
> 
>         reverse_cpuid_check(leaf);
> 
>         cpuid_count(cpuid.function, cpuid.index,
>                     &entry.eax, &entry.ebx, &entry.ecx, &entry.edx);
> 
>         kvm_cpu_caps[leaf] &= *__cpuid_entry_get_reg(&entry, cpuid.reg);
> }
> 
> which does read CPUID from the hw and kvm_cpu_caps[] has already the
> copied bits from boot_cpu_data.x86_capability.
> 
> Now you said that reading the CPUID is mostly redundant but we're
> paranoid so we do it anyway, just in case, so how about we remove the
> copying of boot_cpu_data.x86_capability? That's one less dependency
> on the baremetal implementation.
>
> Practically, nothing changes for kvm because it will read CPUID which is
> the canonical thing anyway. And this should simplify the deal more and
> keep it simple(r).

We want the boot_cpu_data.x86_capability memcpy() so that KVM doesn't advertise
support for features that are intentionally disabled in the kernel, e.g. via
kernel params.  Except for a few special cases, e.g. LA57, KVM doesn't enable
features in the guest if they're disabled in the host, even if the features are
supported in hardware.

For some features, e.g. SMEP and SMAP, honoring boot_cpu_data is mostly about
respecting the kernel's wishes, i.e. barring hardware bugs, enabling such
features in the guest won't break anything.  But for other features, e.g. XSAVE
based features, enabling them in the guest without proper support in the host
will corrupt guest and/or host state.

So it's really the CPUID read that is (mostly) superfluous.