Re: [PATCH] x86/intel: Clear SGX bit if both SGX driver and KVM SGX are not enabled

[Huang, Kai]

On Fri, 2022-09-09 at 05:11 -0700, Dave Hansen wrote:
> On 9/9/22 01:08, Kai Huang wrote:
> > Currently on platform which has SGX enabled, if CONFIG_X86_SGX is not
> > enabled, the X86_FEATURE_SGX is not cleared, resulting in /proc/cpuinfo
> > shows "sgx" feature.  This is not desired.
> 
> Why is it not desired?

My understanding is if a feature is present in /proc/cpuinfo, that feature
should be supported by the kernel and should be usable to the userspace.  For
example, it seems AMD's SME was always present in /proc/cpuinfo at the very
beginning, but later it was cleared if SME is not activated by the kernel in
below commit:

commit 08f253ec3767bcfafc5d32617a92cee57c63968e (tag: x86_cpu_for_v5.18_rc1)
Author: Mario Limonciello <mario.limonciello@xxxxxxx>
Date:   Tue Feb 15 21:44:46 2022 -0600

    x86/cpu: Clear SME feature flag when not in use
    
    Currently, the SME CPU feature flag is reflective of whether the CPU
    supports the feature but not whether it has been activated by the
    kernel.
    
    Change this around to clear the SME feature flag if the kernel is not
    using it so userspace can determine if it is available and in use
    from /proc/cpuinfo.
    
    As the feature flag is cleared on systems where SME isn't active, use
    CPUID 0x8000001f to confirm SME availability before calling
    native_wbinvd().
    
    Signed-off-by: Mario Limonciello <mario.limonciello@xxxxxxx>
    Signed-off-by: Borislav Petkov <bp@xxxxxxx>
    Acked-by: Tom Lendacky <thomas.lendacky@xxxxxxx>
    Link:
https://lore.kernel.org/r/20220216034446.2430634-1-mario.limonciello@xxxxxxx

I am not 100% sure whether this rule should always be applied to other features,
though.  But for SGX even the current upstream code clears SGX feature in some
conditions, for example, when SGX_LC is disabled by BIOS (in which case only KVM
SGX can be supported) and KVM SGX is also disabled, i.e. due to
CONFIG_X86_SGX_KVM isn't set:

    if (!(msr & FEAT_CTL_SGX_LC_ENABLED) && enable_sgx_driver) {
            if (!enable_sgx_kvm) {
                    pr_err_once("SGX Launch Control is locked. Disable SGX.\n");
                    clear_cpu_cap(c, X86_FEATURE_SGX);
	    else {
	            ...
	    }
    }

So I think it makes sense to clear SGX if both SGX driver and KVM SGX are not
enabled by the kernel.


-- 
Thanks,
-Kai