Re: [PATCH] x86/mm: Check cc_vendor when printing memory encryption info

[Jeremi Piotrowski <jpiotrowski@linux.microsoft.com>]

On 10/11/2023 13:06, kirill.shutemov@linux.intel.com wrote:
> On Thu, Nov 09, 2023 at 07:41:33PM +0100, Jeremi Piotrowski wrote:
>> It's not disregard, the way the kernel behaves in this case is correct except
>> for the error in reporting CPU vendor. Users care about seeing the correct
>> information in dmesg.
> 
> I think it is wrong place to advertise CoCo features. It better fits into
> Intel/AMD-specific code that knows better what it is running on. Inferring
> it from generic code has been proved problematic as you showed.
> 

Let's not make this into a bigger issue than it is. There is a check that does
not cover all cases, and needs to be changed into a different check that covers
all cases. I'd wouldn't call that problematic.

I would be open to moving the function to arch/x86/coco/core.c or providing a
mechanism for cpu/paravisor-specific code to do the reporting (which is what
Dexuan originally proposed [1]).

[1]: https://lore.kernel.org/lkml/20231019062030.3206-1-decui@microsoft.com/

> Maybe just remove incorrect info and that's it?
> 

I disagree, other users and I find the print very useful to see which coco
platform the kernel is running on and which confidential computing features
the kernel detected. I'm willing to fix the code to report correct info.

> diff --git a/arch/x86/mm/mem_encrypt.c b/arch/x86/mm/mem_encrypt.c
> index c290c55b632b..f573a97c0524 100644
> --- a/arch/x86/mm/mem_encrypt.c
> +++ b/arch/x86/mm/mem_encrypt.c
> @@ -40,42 +40,6 @@ bool force_dma_unencrypted(struct device *dev)
>  	return false;
>  }
>  
> -static void print_mem_encrypt_feature_info(void)
> -{
> -	pr_info("Memory Encryption Features active:");
> -
> -	if (cpu_feature_enabled(X86_FEATURE_TDX_GUEST)) {
> -		pr_cont(" Intel TDX\n");
> -		return;
> -	}
> -
> -	pr_cont(" AMD");
> -
> -	/* Secure Memory Encryption */
> -	if (cc_platform_has(CC_ATTR_HOST_MEM_ENCRYPT)) {
> -		/*
> -		 * SME is mutually exclusive with any of the SEV
> -		 * features below.
> -		 */
> -		pr_cont(" SME\n");
> -		return;
> -	}
> -
> -	/* Secure Encrypted Virtualization */
> -	if (cc_platform_has(CC_ATTR_GUEST_MEM_ENCRYPT))
> -		pr_cont(" SEV");
> -
> -	/* Encrypted Register State */
> -	if (cc_platform_has(CC_ATTR_GUEST_STATE_ENCRYPT))
> -		pr_cont(" SEV-ES");
> -
> -	/* Secure Nested Paging */
> -	if (cc_platform_has(CC_ATTR_GUEST_SEV_SNP))
> -		pr_cont(" SEV-SNP");
> -
> -	pr_cont("\n");
> -}
> -
>  /* Architecture __weak replacement functions */
>  void __init mem_encrypt_init(void)
>  {
> @@ -85,7 +49,7 @@ void __init mem_encrypt_init(void)
>  	/* Call into SWIOTLB to update the SWIOTLB DMA buffers */
>  	swiotlb_update_mem_attributes();
>  
> -	print_mem_encrypt_feature_info();
> +	pr_info("Memory Encryption is active\n");
>  }
>  
>  void __init mem_encrypt_setup_arch(void)