Re: [PATCH v2] x86/cpuinfo: Clear X86_FEATURE_TME if TME/MKTME is disabled by BIOS

[Kai Huang <kai.huang@intel.com>]

On Mon, 2022-07-04 at 11:22 -0300, Martin Fernandez wrote:
> Right now the only way to check this is by greping the kernel logs,
> which is inconvenient. This is currently checked for fwupd for
> example.
> 
> I understand that cpuinfo is supposed to report every feature in the
> cpu but since AMD is doing the same (and it also broke backwards
> compatibility [1]) for sme/sev I think it's good to have this for
> Intel too.
> 
> Another option to prevent greping the logs would be a file in
> sysfs. I'm open to suggestions to where to place this infomartion. I
> saw a proposal about a firmware security filesystem [2]; that would
> fit perfectly.
> 
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git/patch/?id=08f253ec3767bcfafc5d32617a92cee57c63968e
> 
> [2] https://lore.kernel.org/all/20220622215648.96723-3-nayna@linux.ibm.com/

Leave above to others, but... 
> 
> Changelog since v1
> 
> Clear the flag not only for BSP but for every cpu in the system.

... the changelog history shouldn't be in the commit message.

You can put one additional '---' after your 'Signed-off-by' and add your
changelog after it.  The content between two '---'s will be stripped away by
'git am' when maintainer takes the patch:

	Signed-off-by: Martin Fernandez <martin.fernandez@eclypsium.com>
	---
	v1->v2:
		xxx
	---
	arch/x86/kernel/cpu/intel.c | 1 +
	1 file changed, 1 insertion(+)
> 
> Signed-off-by: Martin Fernandez <martin.fernandez@eclypsium.com>
> ---
>  arch/x86/kernel/cpu/intel.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c
> index fd5dead8371c..17f23e23f911 100644
> --- a/arch/x86/kernel/cpu/intel.c
> +++ b/arch/x86/kernel/cpu/intel.c
> @@ -570,6 +570,7 @@ static void detect_tme(struct cpuinfo_x86 *c)
> 
>  	if (!TME_ACTIVATE_LOCKED(tme_activate) || !TME_ACTIVATE_ENABLED(tme_activate)) {
>  		pr_info_once("x86/tme: not enabled by BIOS\n");
> +		clear_cpu_cap(c, X86_FEATURE_TME);
>  		mktme_status = MKTME_DISABLED;
>  		return;

This code change itself looks good to me.

But, TME actually supports bypassing TME encryption/decryption by setting 1 to
bit 31 to IA32_TME_ACTIVATE MSR.  See 'Table 4-2 IA32_TME_ACTIVATE MSR' in MKTME
spec below:

https://edc.intel.com/content/www/us/en/design/ipla/software-development-platforms/client/platforms/alder-lake-desktop/12th-generation-intel-core-processors-datasheet-volume-1-of-2/002/intel-multi-key-total-memory-encryption/

When bit 31 is set, the TME is bypassed (no encryption/decryption for KeyID 0).

So looks userspace also needs to check this if it wants to truly check whether
"TME memory encryption" is active.

But perhaps it's arguable whether we can also clear TME flag in this case.

So:

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


-- 
Thanks,
-Kai