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.
