[PATCH] x86/cpufeatures: SGX: Adjust the error message when BIOS does not support SGX

[PATCH] x86/cpufeatures: SGX: Adjust the error message when BIOS does not support SGX

[WangYuli]

When SGX is not supported by the BIOS, we still output the error
'SGX disabled by BIOS', which can be confusing since there might not be
an SGX-related option in the BIOS settings.

As a kernel, it's difficult for us to distinguish between the BIOS not
supporting SGX and the BIOS supporting SGX but it's disabled.

Therefore, we should update the error message to
'SGX disabled or unsupported by BIOS' to make it easier for those reading
kernel logs to understand what's happening.

Reported-by: Bo Wu <wubo@uniontech.com>
Link: https://github.com/linuxdeepin/developer-center/issues/10032
Signed-off-by: Zelong Xiang <xiangzelong@uniontech.com>
Signed-off-by: WangYuli <wangyuli@uniontech.com>
---
 arch/x86/kernel/cpu/feat_ctl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/kernel/cpu/feat_ctl.c b/arch/x86/kernel/cpu/feat_ctl.c
index 1640ae76548f..4a4118784c13 100644
--- a/arch/x86/kernel/cpu/feat_ctl.c
+++ b/arch/x86/kernel/cpu/feat_ctl.c
@@ -188,7 +188,7 @@ void init_ia32_feat_ctl(struct cpuinfo_x86 *c)
 update_sgx:
 	if (!(msr & FEAT_CTL_SGX_ENABLED)) {
 		if (enable_sgx_kvm || enable_sgx_driver)
-			pr_err_once("SGX disabled by BIOS.\n");
+			pr_err_once("SGX disabled or unsupported by BIOS.\n");
 		clear_cpu_cap(c, X86_FEATURE_SGX);
 		return;
 	}
-- 
2.43.4

Re: [PATCH] x86/cpufeatures: SGX: Adjust the error message when BIOS does not support SGX

[Huang, Kai]

On Tue, 2024-07-30 at 10:49 +0800, WangYuli wrote:
> When SGX is not supported by the BIOS, we still output the error
> 'SGX disabled by BIOS', which can be confusing since there might not be
> an SGX-related option in the BIOS settings.

+linux-sgx list, Jarkko, Haitao.

This message is only printed when SGX is reported in CPUID but is not
enabled in the FEAT_CTL MSR.  I can only recall this can happen when the
BIOS actually provides an option for the user to turn on/off SGX, in
which case the current message is correct.

I could be wrong, but I don't recall I have met any machine that doesn't
have any SGX option in the BIOS but still reports SGX in the CPUID.  Can
you confirm this is the case?  

I don't see this is mentioned in the github link below which reports this
issue.  In fact, it says:

	非bug，主板bios关闭了SGX，正常内核提醒

.. which is

	Not bug, the motherboard BIOS disabled SGX, normal kernel
message

And the link also shows this issue is "closed".

Please clarify.

> 
> As a kernel, it's difficult for us to distinguish between the BIOS not
> supporting SGX and the BIOS supporting SGX but it's disabled.
> 
> Therefore, we should update the error message to
> 'SGX disabled or unsupported by BIOS' to make it easier for those reading
> kernel logs to understand what's happening.
> 
> Reported-by: Bo Wu <wubo@uniontech.com>
> Link: https://github.com/linuxdeepin/developer-center/issues/10032
> Signed-off-by: Zelong Xiang <xiangzelong@uniontech.com>
> Signed-off-by: WangYuli <wangyuli@uniontech.com>
> ---
>  arch/x86/kernel/cpu/feat_ctl.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kernel/cpu/feat_ctl.c b/arch/x86/kernel/cpu/feat_ctl.c
> index 1640ae76548f..4a4118784c13 100644
> --- a/arch/x86/kernel/cpu/feat_ctl.c
> +++ b/arch/x86/kernel/cpu/feat_ctl.c
> @@ -188,7 +188,7 @@ void init_ia32_feat_ctl(struct cpuinfo_x86 *c)
>  update_sgx:
>  	if (!(msr & FEAT_CTL_SGX_ENABLED)) {
>  		if (enable_sgx_kvm || enable_sgx_driver)
> -			pr_err_once("SGX disabled by BIOS.\n");
> +			pr_err_once("SGX disabled or unsupported by BIOS.\n");
>  		clear_cpu_cap(c, X86_FEATURE_SGX);
>  		return;
>  	}

Re: [PATCH] x86/cpufeatures: SGX: Adjust the error message when BIOS does not support SGX

[Sean Christopherson]

On Tue, Jul 30, 2024, Kai Huang wrote:
> On Tue, 2024-07-30 at 10:49 +0800, WangYuli wrote:
> > When SGX is not supported by the BIOS, we still output the error
> > 'SGX disabled by BIOS', which can be confusing since there might not be
> > an SGX-related option in the BIOS settings.
> 
> +linux-sgx list, Jarkko, Haitao.
> 
> This message is only printed when SGX is reported in CPUID but is not
> enabled in the FEAT_CTL MSR.  I can only recall this can happen when the
> BIOS actually provides an option for the user to turn on/off SGX, in
> which case the current message is correct.

FWIW, this could easily happen with a virtual machine, e.g. running an old QEMU
with `-cpu host`.

Re: [PATCH] x86/cpufeatures: SGX: Adjust the error message when BIOS does not support SGX

[Huang, Kai]

On Tue, 2024-07-30 at 10:56 -0700, Sean Christopherson wrote:
> On Tue, Jul 30, 2024, Kai Huang wrote:
> > On Tue, 2024-07-30 at 10:49 +0800, WangYuli wrote:
> > > When SGX is not supported by the BIOS, we still output the error
> > > 'SGX disabled by BIOS', which can be confusing since there might not be
> > > an SGX-related option in the BIOS settings.
> > 
> > +linux-sgx list, Jarkko, Haitao.
> > 
> > This message is only printed when SGX is reported in CPUID but is not
> > enabled in the FEAT_CTL MSR.  I can only recall this can happen when the
> > BIOS actually provides an option for the user to turn on/off SGX, in
> > which case the current message is correct.
> 
> FWIW, this could easily happen with a virtual machine, e.g. running an old QEMU
> with `-cpu host`.
> 

Hmm.. it appears so, if the old Qemu doesn't have SGX support.  Thanks.

Perhaps "SGX disabled or unsupported by BIOS." or "SGX not enabled by
BIOS" is slightly better than the current "SGX disabled by BIOS", but I
am not sure whether it is worth patching.

Re: [PATCH] x86/cpufeatures: SGX: Adjust the error message when BIOS does not support SGX

[WangYuli]

On 2024/7/31 06:48, Huang, Kai wrote:

>> FWIW, this could easily happen with a virtual machine, e.g. running an old QEMU
>> with `-cpu host`.
>>
> Hmm.. it appears so, if the old Qemu doesn't have SGX support.  Thanks.
>
> Perhaps "SGX disabled or unsupported by BIOS." or "SGX not enabled by
> BIOS" is slightly better than the current "SGX disabled by BIOS", but I
> am not sure whether it is worth patching.

Of course, most kernel developers are incredibly busy and are reluctant 
to spend their time

on those little changes.


However, the user base of Linux is gradually shifting from 
'professionals' to 'general users'.

As I mentioned in the commit msg. I hope to make kernel logs clearer and 
more understandable,

reducing confusion for those who are unfamiliar with the codebase.


It's important that any change in upstream can potentially impact 
countless users.


Sincerely,

-- 
WangYuli

Re: [PATCH] x86/cpufeatures: SGX: Adjust the error message when BIOS does not support SGX

[WangYuli]

On 2024/7/30 19:57, Huang, Kai wrote:

> +linux-sgx list, Jarkko, Haitao.
>
> This message is only printed when SGX is reported in CPUID but is not
> enabled in the FEAT_CTL MSR.  I can only recall this can happen when the
> BIOS actually provides an option for the user to turn on/off SGX, in
> which case the current message is correct.
>
> I could be wrong, but I don't recall I have met any machine that doesn't
> have any SGX option in the BIOS but still reports SGX in the CPUID.  Can
> you confirm this is the case?

Sure.

For example, Lenovo ThinkPad T480s that compliance id is TP00092A.

>
> I don't see this is mentioned in the github link below which reports this
> issue.  In fact, it says:
>
> 	非bug，主板bios关闭了SGX，正常内核提醒
>
> .. which is
>
> 	Not bug, the motherboard BIOS disabled SGX, normal kernel
> message
hah, that's a typo.
>
> And the link also shows this issue is "closed".
>
> Please clarify.
-- 
WangYuli

Re: [PATCH] x86/cpufeatures: SGX: Adjust the error message when BIOS does not support SGX

[Huang, Kai]

On Wed, 2024-07-31 at 11:22 +0800, WangYuli wrote:
> On 2024/7/30 19:57, Huang, Kai wrote:
> 
> > +linux-sgx list, Jarkko, Haitao.
> > 
> > This message is only printed when SGX is reported in CPUID but is not
> > enabled in the FEAT_CTL MSR.  I can only recall this can happen when the
> > BIOS actually provides an option for the user to turn on/off SGX, in
> > which case the current message is correct.
> > 
> > I could be wrong, but I don't recall I have met any machine that doesn't
> > have any SGX option in the BIOS but still reports SGX in the CPUID.  Can
> > you confirm this is the case?
> 
> Sure.
> 
> For example, Lenovo ThinkPad T480s that compliance id is TP00092A.
> 
> 

Fair enough.  I guess the updated message is slightly better:

Acked-by: Kai Huang <kai.huang@intel.com>

Btw, I think there are some issues in the patch title/changelog.  Please
fix them.

E.g., I think the format of patch title should be:

	x86/cpu: ...

In the changelog please avoid using "we", and please use imperative mode
to describe the change.  More information please see: 

https://docs.kernel.org/process/maintainer-tip.html