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

["Kirill A. Shutemov" <kirill@shutemov.name>]

On Thu, Apr 01, 2021 at 01:06:29PM -0700, Dave Hansen wrote:
> On 2/5/21 3:38 PM, Kuppuswamy Sathyanarayanan wrote:
> > From: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>
> > 
> > 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/

Right.

> > 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.

"
  After setting the shared bit, the conversion must be completed with
  MapGPA TDVMALL. The call informs VMM about the conversion and makes it
  remove the GPA from the S-EPT mapping.
"

> > 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?

"
  AMD SEV makes force_dma_unencrypted() return true which triggers
  set_memory_decrypted() calls on all DMA allocations. TDX will use the
  same code path to make DMA allocations shared.
"

> > 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??)

"
  Only AMD_MEM_ENCRYPT uses the option now. TDX will be the second one.
"

> > 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.

Isn't it what the subject says? :P

> > 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.

Not referenced, but declared.

> Could you explain a bit more?
> 
> I would have expected it tot at *least* be tied to the new #ifdef.

Looks like a fixup got folded into a wrong patch. It supposed to be in
"x86/kvm: Use bounce buffers for TD guest".

This declaration allows to get away without any #ifdefs in
mem_encrypt_init() when !CONFIG_AMD_MEM_ENCRYPT: sev_active() is
false at compile-time and sev_enable_key never referenced.

Sathya, could move it to the right patch?

> > +#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
> ...

-- 
 Kirill A. Shutemov