Re: [RFC v1 25/26] x86/tdx: Make DMA pages shared

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

On Thu, Apr 01, 2021 at 02:01:15PM -0700, Dave Hansen wrote:
> > +int tdx_map_gpa(phys_addr_t gpa, int numpages, bool private)
> > +{
> > +	int ret, i;
> > +
> > +	ret = __tdx_map_gpa(gpa, numpages, private);
> > +	if (ret || !private)
> > +		return ret;
> > +
> > +	for (i = 0; i < numpages; i++)
> > +		tdx_accept_page(gpa + i*PAGE_SIZE);
> > +
> > +	return 0;
> > +}
> 
> Please do something like this:
> 
> enum tdx_max_type {
> 	TDX_MAP_PRIVATE,
> 	TDX_MAP_SHARED
> }
> 
> Then, your calls will look like:
> 
> 	tdx_map_gpa(gpa, nr, TDX_MAP_SHARED);
> 
> instead of:
> 
> 	tdx_map_gpa(gpa, nr, false);

Okay, makes sense.

> >  static __cpuidle void tdx_halt(void)
> >  {
> >  	register long r10 asm("r10") = TDVMCALL_STANDARD;
> > diff --git a/arch/x86/mm/mem_encrypt_common.c b/arch/x86/mm/mem_encrypt_common.c
> > index 964e04152417..b6d93b0c5dcf 100644
> > --- a/arch/x86/mm/mem_encrypt_common.c
> > +++ b/arch/x86/mm/mem_encrypt_common.c
> > @@ -15,9 +15,9 @@
> >  bool force_dma_unencrypted(struct device *dev)
> >  {
> >  	/*
> > -	 * For SEV, all DMA must be to unencrypted/shared addresses.
> > +	 * For SEV and TDX, all DMA must be to unencrypted/shared addresses.
> >  	 */
> > -	if (sev_active())
> > +	if (sev_active() || is_tdx_guest())
> >  		return true;
> >  
> >  	/*
> > diff --git a/arch/x86/mm/pat/set_memory.c b/arch/x86/mm/pat/set_memory.c
> > index 16f878c26667..6f23a9816ef0 100644
> > --- a/arch/x86/mm/pat/set_memory.c
> > +++ b/arch/x86/mm/pat/set_memory.c
> > @@ -27,6 +27,7 @@
> >  #include <asm/proto.h>
> >  #include <asm/memtype.h>
> >  #include <asm/set_memory.h>
> > +#include <asm/tdx.h>
> >  
> >  #include "../mm_internal.h"
> >  
> > @@ -1977,8 +1978,8 @@ static int __set_memory_enc_dec(unsigned long addr, int numpages, bool enc)
> >  	struct cpa_data cpa;
> >  	int ret;
> >  
> > -	/* Nothing to do if memory encryption is not active */
> > -	if (!mem_encrypt_active())
> > +	/* Nothing to do if memory encryption and TDX are not active */
> > +	if (!mem_encrypt_active() && !is_tdx_guest())
> >  		return 0;
> 
> So, this is starting to look like the "enc" naming is wrong, or at least
> a little misleading.   Should we be talking about "protection" or
> "guards" or something?

Are you talking about the function argument or function name too?

> >  	/* Should not be working on unaligned addresses */
> > @@ -1988,8 +1989,14 @@ static int __set_memory_enc_dec(unsigned long addr, int numpages, bool enc)
> >  	memset(&cpa, 0, sizeof(cpa));
> >  	cpa.vaddr = &addr;
> >  	cpa.numpages = numpages;
> > -	cpa.mask_set = enc ? __pgprot(_PAGE_ENC) : __pgprot(0);
> > -	cpa.mask_clr = enc ? __pgprot(0) : __pgprot(_PAGE_ENC);
> > +	if (is_tdx_guest()) {
> > +		cpa.mask_set = __pgprot(enc ? 0 : tdx_shared_mask());
> > +		cpa.mask_clr = __pgprot(enc ? tdx_shared_mask() : 0);
> > +	} else {
> > +		cpa.mask_set = __pgprot(enc ? _PAGE_ENC : 0);
> > +		cpa.mask_clr = __pgprot(enc ? 0 : _PAGE_ENC);
> > +	}
> 
> OK, this is too hideous to live.  It sucks that the TDX and SEV/SME bits
> are opposite polarity, but oh well.
> 
> To me, this gets a lot clearer, and opens up room for commenting if you
> do something like:
> 
> 	if (is_tdx_guest()) {
> 		mem_enc_bits   = 0;
> 		mem_plain_bits = tdx_shared_mask();
> 	} else {
> 		mem_enc_bits   = _PAGE_ENC;
> 		mem_plain_bits = 0
> 	}
> 
> 	if (enc) {
> 		cpa.mask_set = mem_enc_bits;
> 		cpa.mask_clr = mem_plain_bits;  // clear "plain" bits
> 	} else {
> 		
> 		cpa.mask_set = mem_plain_bits;
> 		cpa.mask_clr = mem_enc_bits;	// clear encryption bits
> 	}

I'm not convinced that your approach it clearer. If you add the missing
__pgprot() it going to as ugly as the original.

But if a maintainer wants... :)

> >  	cpa.pgd = init_mm.pgd;
> >  
> >  	/* Must avoid aliasing mappings in the highmem code */
> > @@ -1999,7 +2006,8 @@ static int __set_memory_enc_dec(unsigned long addr, int numpages, bool enc)
> >  	/*
> >  	 * Before changing the encryption attribute, we need to flush caches.
> >  	 */
> > -	cpa_flush(&cpa, !this_cpu_has(X86_FEATURE_SME_COHERENT));
> > +	if (!enc || !is_tdx_guest())
> > +		cpa_flush(&cpa, !this_cpu_has(X86_FEATURE_SME_COHERENT));
> 
> That "!enc" looks wrong to me.  Caches would need to be flushed whenever
> encryption attributes *change*, not just when they are set.
> 
> Also, cpa_flush() flushes caches *AND* the TLB.  How does TDX manage to
> not need TLB flushes?

I will double-check everthing, but I think we can skip *both* cpa_flush()
for private->shared conversion. VMM and TDX module will take care about
TLB and cache flush in response to MapGPA TDVMCALL.

> >  	ret = __change_page_attr_set_clr(&cpa, 1);
> >  
> > @@ -2012,6 +2020,11 @@ static int __set_memory_enc_dec(unsigned long addr, int numpages, bool enc)
> >  	 */
> >  	cpa_flush(&cpa, 0);
> >  
> > +	if (!ret && is_tdx_guest()) {
> > +		ret = tdx_map_gpa(__pa(addr), numpages, enc);
> > +		// XXX: need to undo on error?
> > +	}
> 
> Time to fix this stuff up if you want folks to take this series more
> seriously.

My bad, will fix it.

-- 
 Kirill A. Shutemov