Messages in this thread |  | | Subject | Re: [RFC v1 25/26] x86/tdx: Make DMA pages shared | From | Dave Hansen <> | Date | Tue, 6 Apr 2021 09:38:42 -0700 |
| |
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.
|  |