Re: [RFC v1 21/26] x86/mm: Move force_dma_unencrypted() to common code

[Dave Hansen]

On 2/5/21 3:38 PM, Kuppuswamy Sathyanarayanan wrote:
> From: "Kirill A. Shutemov" <kirill.shutemov@xxxxxxxxxxxxxxx>
> 
> Intel TDX doesn't allow VMM to access guest memory. Any memory that is
> required for communication with VMM suppose to be shared explicitly by

s/suppose to/must/

> setting the bit in page table entry. The shared memory is similar to
> unencrypted memory in AMD SME/SEV terminology.

In addition to setting the page table bit, there's also a dance to go
through to convert the memory.  Please mention the procedure here at
least.  It's very different from SME.

> force_dma_unencrypted() has to return true for TDX guest. Move it out of
> AMD SME code.

You lost me here.  What does force_dma_unencrypted() have to do with
host/guest shared memory?

> Introduce new config option X86_MEM_ENCRYPT_COMMON that has to be
> selected by all x86 memory encryption features.

Please also mention what will set it.  I assume TDX guest support will
set this option.  It's probably also worth a sentence to say that
force_dma_unencrypted() will have TDX-specific code added to it.  (It
will, right??)

> This is preparation for TDX changes in DMA code.

Probably best to also mention that this effectively just moves code
around.  This patch should have no functional changes at runtime.


> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> index 0374d9f262a5..8fa654d61ac2 100644
> --- a/arch/x86/Kconfig
> +++ b/arch/x86/Kconfig
> @@ -1538,14 +1538,18 @@ config X86_CPA_STATISTICS
>  	  helps to determine the effectiveness of preserving large and huge
>  	  page mappings when mapping protections are changed.
>  
> +config X86_MEM_ENCRYPT_COMMON
> +	select ARCH_HAS_FORCE_DMA_UNENCRYPTED
> +	select DYNAMIC_PHYSICAL_MASK
> +	def_bool n
> +
>  config AMD_MEM_ENCRYPT
>  	bool "AMD Secure Memory Encryption (SME) support"
>  	depends on X86_64 && CPU_SUP_AMD
>  	select DMA_COHERENT_POOL
> -	select DYNAMIC_PHYSICAL_MASK
>  	select ARCH_USE_MEMREMAP_PROT
> -	select ARCH_HAS_FORCE_DMA_UNENCRYPTED
>  	select INSTRUCTION_DECODER
> +	select X86_MEM_ENCRYPT_COMMON
>  	help
>  	  Say yes to enable support for the encryption of system memory.
>  	  This requires an AMD processor that supports Secure Memory
> diff --git a/arch/x86/include/asm/io.h b/arch/x86/include/asm/io.h
> index 30a3b30395ad..95e534cffa99 100644
> --- a/arch/x86/include/asm/io.h
> +++ b/arch/x86/include/asm/io.h
> @@ -257,10 +257,12 @@ static inline void slow_down_io(void)
>  
>  #endif
>  
> -#ifdef CONFIG_AMD_MEM_ENCRYPT
>  #include <linux/jump_label.h>
>  
>  extern struct static_key_false sev_enable_key;

This _looks_ odd.  sev_enable_key went from being under
CONFIG_AMD_MEM_ENCRYPT to being unconditionally referenced.

Could you explain a bit more?

I would have expected it tot at *least* be tied to the new #ifdef.

> +#ifdef CONFIG_AMD_MEM_ENCRYPT
> +
>  static inline bool sev_key_active(void)
>  {
>  	return static_branch_unlikely(&sev_enable_key);
> diff --git a/arch/x86/mm/Makefile b/arch/x86/mm/Makefile
> index 5864219221ca..b31cb52bf1bd 100644
> --- a/arch/x86/mm/Makefile
...