> +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);
> 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?
> /* 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
}
> 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?
> 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.
