| Message ID | 20220909080853.547058-1-kai.huang@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86/intel: Clear SGX bit if both SGX driver and KVM SGX are not enabled | expand |
On Fri, 2022-09-09 at 20:08 +1200, 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. > > Clear SGX feature bit if both SGX driver and KVM SGX are not enabled in > init_ia32_feat_ctl(). > > Signed-off-by: Kai Huang <kai.huang@intel.com> > --- > > Hi Dave, Sean, Jarkko, > > Could you help to review? Tested on SGX (BIOS) enabled machine with > CONFIG_X86_SGX unset. > > This patch is generated on latest tip/master, but it applies to > tip/x86/sgx cleanly as well. > > --- > arch/x86/kernel/cpu/feat_ctl.c | 13 +++++++++++++ > 1 file changed, 13 insertions(+) > > diff --git a/arch/x86/kernel/cpu/feat_ctl.c b/arch/x86/kernel/cpu/feat_ctl.c > index 993697e71854..2f67409f5f00 100644 > --- a/arch/x86/kernel/cpu/feat_ctl.c > +++ b/arch/x86/kernel/cpu/feat_ctl.c > @@ -191,6 +191,19 @@ void init_ia32_feat_ctl(struct cpuinfo_x86 *c) > return; > } > > + /* > + * By reaching here, it is certain that: > + * - CPU supports SGX. > + * - SGX is enabled by BIOS. > + * > + * However if both SGX driver and KVM SGX are not enabled, just > + * need to clear SGX feature bit. > + */ > + if (!enable_sgx_driver && !enable_sgx_kvm) { > + clear_cpu_cap(c, X86_FEATURE_SGX); > + return; > + } > + Sorry my bad. I sent out the patch too quickly. This check should be moved down after checking X86_FEATURE_VMX flag which may result in enable_sgx_kvm being set to false. I'll send out v2.
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?
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@amd.com> 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@amd.com> Signed-off-by: Borislav Petkov <bp@suse.de> Acked-by: Tom Lendacky <thomas.lendacky@amd.com> Link: https://lore.kernel.org/r/20220216034446.2430634-1-mario.limonciello@amd.com 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.
On Fri, Sep 09, 2022 at 08:08:53PM +1200, 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. > > Clear SGX feature bit if both SGX driver and KVM SGX are not enabled in > init_ia32_feat_ctl(). > > Signed-off-by: Kai Huang <kai.huang@intel.com> Is it a pattern that flags are cleared when they are disabled by the kernel? I don't know the answer for this. I could imagine that sometimes you might want to know whether the CPU supports a feature, even if e.g. your distribution kernel does not. There's of course other ways to answer such qeustion, e.g. by using cpuid utility. BR, Jarkko
Hi Jarkko, On 9/12/2022 3:47 AM, Jarkko Sakkinen wrote: > On Fri, Sep 09, 2022 at 08:08:53PM +1200, 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. >> >> Clear SGX feature bit if both SGX driver and KVM SGX are not enabled in >> init_ia32_feat_ctl(). >> >> Signed-off-by: Kai Huang <kai.huang@intel.com> > > Is it a pattern that flags are cleared when they are disabled by the > kernel? I don't know the answer for this. There appears to be relevant text in Documentation/x86/cpuinfo.rst: tl;dr "In general, /proc/cpuinfo shows features which the kernel supports." Longer quote: "If users want to know if a feature is available on a given system, they try to find the flag in /proc/cpuinfo. If a given flag is present, it means that the kernel supports it and is currently making it available. If such flag represents a hardware feature, it also means that the hardware supports it. If the expected flag does not appear in /proc/cpuinfo, things are murkier. Users need to find out the reason why the flag is missing and find the way how to enable it, which is not always easy. There are several factors that can explain missing flags: the expected feature failed to enable, the feature is missing in hardware, platform firmware did not enable it, the feature is disabled at build or run time, an old kernel is in use, or the kernel does not support the feature and thus has not enabled it. In general, /proc/cpuinfo shows features which the kernel supports. For a full list of CPUID flags which the CPU supports, use tools/arch/x86/kcpuid." > > I could imagine that sometimes you might want to know whether the CPU > supports a feature, even if e.g. your distribution kernel does not. Indeed ... this seems to fall into the "murkier" part of the above quote. Reinette
On 9/12/22 07:31, Reinette Chatre wrote: > > On 9/12/2022 3:47 AM, Jarkko Sakkinen wrote: >> On Fri, Sep 09, 2022 at 08:08:53PM +1200, 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. >>> >>> Clear SGX feature bit if both SGX driver and KVM SGX are not enabled in >>> init_ia32_feat_ctl(). >>> >>> Signed-off-by: Kai Huang <kai.huang@intel.com> >> Is it a pattern that flags are cleared when they are disabled by the >> kernel? I don't know the answer for this. There's no good pattern. But, one guideline is that the X86_FEATURE_*'s are for the *kernel*. They are *not* for userspace. The fact that the features are exposed to userspace in cpuinfo is unfortunate, but it doesn't change our guideline. So, if this patch is done only for the benefit of userspace, I don't think we should apply it.
On Mon, 2022-09-12 at 07:34 -0700, Dave Hansen wrote: > On 9/12/22 07:31, Reinette Chatre wrote: > > > > On 9/12/2022 3:47 AM, Jarkko Sakkinen wrote: > > > On Fri, Sep 09, 2022 at 08:08:53PM +1200, 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. > > > > > > > > Clear SGX feature bit if both SGX driver and KVM SGX are not enabled in > > > > init_ia32_feat_ctl(). > > > > > > > > Signed-off-by: Kai Huang <kai.huang@intel.com> > > > Is it a pattern that flags are cleared when they are disabled by the > > > kernel? I don't know the answer for this. > > There's no good pattern. > > But, one guideline is that the X86_FEATURE_*'s are for the *kernel*. > They are *not* for userspace. The fact that the features are exposed to > userspace in cpuinfo is unfortunate, but it doesn't change our guideline. > > So, if this patch is done only for the benefit of userspace, I don't > think we should apply it. It's not done for the userspace. AFAICT no one is complaining about this. I just happened to see this and thought it's a right thing to do. Also, in my other reply to this thread I put one reason that I think it's reasonable: " 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. "
diff --git a/arch/x86/kernel/cpu/feat_ctl.c b/arch/x86/kernel/cpu/feat_ctl.c index 993697e71854..2f67409f5f00 100644 --- a/arch/x86/kernel/cpu/feat_ctl.c +++ b/arch/x86/kernel/cpu/feat_ctl.c @@ -191,6 +191,19 @@ void init_ia32_feat_ctl(struct cpuinfo_x86 *c) return; } + /* + * By reaching here, it is certain that: + * - CPU supports SGX. + * - SGX is enabled by BIOS. + * + * However if both SGX driver and KVM SGX are not enabled, just + * need to clear SGX feature bit. + */ + if (!enable_sgx_driver && !enable_sgx_kvm) { + clear_cpu_cap(c, X86_FEATURE_SGX); + return; + } + /* * VMX feature bit may be cleared due to being disabled in BIOS, * in which case SGX virtualization cannot be supported either.
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. Clear SGX feature bit if both SGX driver and KVM SGX are not enabled in init_ia32_feat_ctl(). Signed-off-by: Kai Huang <kai.huang@intel.com> --- Hi Dave, Sean, Jarkko, Could you help to review? Tested on SGX (BIOS) enabled machine with CONFIG_X86_SGX unset. This patch is generated on latest tip/master, but it applies to tip/x86/sgx cleanly as well. --- arch/x86/kernel/cpu/feat_ctl.c | 13 +++++++++++++ 1 file changed, 13 insertions(+) base-commit: b8b09110cf290fdab4006b717da7a776ffb0cb73