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

[Dave Hansen <dave.hansen@intel.com>]

On 4/6/21 9:31 AM, Kirill A. Shutemov wrote:
> On Thu, Apr 01, 2021 at 02:01:15PM -0700, Dave Hansen wrote:
>>> @@ -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?

Yes, __set_memory_enc_dec() isn't really just doing "enc"ryption any more.

>>>  	/* 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... :)

Yes, please.  I think my version (with the added __pgprot() conversions)
clearly separates out the two thing that are going on.

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

Oh, interesting.  You might also want to double check if there are any
more places where X86_FEATURE_SME_COHERENT and TDX have similar properties.