Re: [RFC v2 27/32] x86/tdx: Exclude Shared bit from __PHYSICAL_MASK

From: Kuppuswamy, Sathyanarayanan
Date: Thu May 20 2021 - 14:48:54 EST


Hi Dave,

On 5/19/21 9:14 AM, Dave Hansen wrote:
On 4/26/21 11:01 AM, Kuppuswamy Sathyanarayanan wrote:
From: "Kirill A. Shutemov" <kirill.shutemov@xxxxxxxxxxxxxxx>

tdx_shared_mask() returns the mask that has to be set in a page
table entry to make page shared with VMM.

Here's a rewrite:

Just like MKTME, TDX reassigns bits of the physical address for
metadata. MKTME used several bits for an encryption KeyID. TDX uses a
single bit in guests to communicate whether a physical page should be
protected by TDX as private memory (bit set to 0) or unprotected and
shared with the VMM (bit set to 1).

Add a helper, tdg_shared_mask() (bad name please fix it) to generate the

Initially we have used tdx_* prefix for the guest code. But when the code from
host side got merged together, we came across many name conflicts. So to
avoid such issues in future, we were asked not to use the "tdx_" prefix and
our alternative choice was "tdg_".

Also, IMO, "tdg" prefix is more meaningful for guest code (Trusted Domain Guest)
compared to "tdx" (Trusted Domain eXtensions). I know that it gets confusing
when grepping for TDX related changes. But since these functions are only used
inside arch/x86 it should not be too confusing.

Even if rename is requested, IMO, it is easier to do it in one patch over
making changes in all the patches. So if it is required, we can do it later
once these initial patches were merged.

mask. The processor enumerates its physical address width to include
the shared bit, which means it gets included in __PHYSICAL_MASK by default.

Remove the shared mask from 'physical_mask' since any bits in
tdg_shared_mask() are not used for physical addresses in page table entries.

--

Thanks. I will include it in next version.


BTW, do you find it confusing that the subject says: '__PHYSICAL_MASK'
and yet the code only modifies 'physical_mask'?

Also, note that we cannot club shared mapping configuration between
AMD SME and Intel TDX Guest platforms in common function. SME has
to do it very early in __startup_64() as it sets the bit on all
memory, except what is used for communication. TDX can postpone as
we don't need any shared mapping in very early boot.

Signed-off-by: Kirill A. Shutemov <kirill.shutemov@xxxxxxxxxxxxxxx>
Reviewed-by: Andi Kleen <ak@xxxxxxxxxxxxxxx>
Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@xxxxxxxxxxxxxxx>
---
arch/x86/Kconfig | 1 +
arch/x86/include/asm/tdx.h | 6 ++++++
arch/x86/kernel/tdx.c | 9 +++++++++
3 files changed, 16 insertions(+)

diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index 67f99bf27729..5f92e8205de2 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -882,6 +882,7 @@ config INTEL_TDX_GUEST
select PARAVIRT_XL
select X86_X2APIC
select SECURITY_LOCKDOWN_LSM
+ select X86_MEM_ENCRYPT_COMMON
help
Provide support for running in a trusted domain on Intel processors
equipped with Trusted Domain eXtenstions. TDX is an new Intel
diff --git a/arch/x86/include/asm/tdx.h b/arch/x86/include/asm/tdx.h
index b972c6531a53..dc80cf7f7d08 100644
--- a/arch/x86/include/asm/tdx.h
+++ b/arch/x86/include/asm/tdx.h
@@ -111,6 +111,8 @@ unsigned char tdg_inb(unsigned short port);
unsigned short tdg_inw(unsigned short port);
unsigned int tdg_inl(unsigned short port);
+extern phys_addr_t tdg_shared_mask(void);
+
#else // !CONFIG_INTEL_TDX_GUEST
static inline bool is_tdx_guest(void)
@@ -149,6 +151,10 @@ static inline long tdx_kvm_hypercall4(unsigned int nr, unsigned long p1,
return -ENODEV;
}
+static inline phys_addr_t tdg_shared_mask(void)
+{
+ return 0;
+}
#endif /* CONFIG_INTEL_TDX_GUEST */
#endif /* __ASSEMBLY__ */
#endif /* _ASM_X86_TDX_H */
diff --git a/arch/x86/kernel/tdx.c b/arch/x86/kernel/tdx.c
index 1f1bb98e1d38..7e391cd7aa2b 100644
--- a/arch/x86/kernel/tdx.c
+++ b/arch/x86/kernel/tdx.c
@@ -76,6 +76,12 @@ bool is_tdx_guest(void)
}
EXPORT_SYMBOL_GPL(is_tdx_guest);
+/* The highest bit of a guest physical address is the "sharing" bit */
+phys_addr_t tdg_shared_mask(void)
+{
+ return 1ULL << (td_info.gpa_width - 1);
+}

Why not just inline this thing? Functions don't get any smaller than
that. Or does it not get used anywhere else? Or are you concerned
about exporting td_info?

We don't want to export td_info. It has more information additional to shared
mask details. Any reason for suggesting to use inline?

This function is only used in following files.

arch/x86/include/asm/pgtable.h:25:#define pgprot_tdg_shared(prot) __pgprot(pgprot_val(prot) | tdg_shared_mask())
arch/x86/mm/pat/set_memory.c:1997: mem_plain_bits = __pgprot(tdg_shared_mask());
arch/x86/kernel/tdx.c:134:phys_addr_t tdg_shared_mask(void)
arch/x86/kernel/tdx.c:274: physical_mask &= ~tdg_shared_mask();



static void tdg_get_info(void)
{
u64 ret;
@@ -87,6 +93,9 @@ static void tdg_get_info(void)
td_info.gpa_width = out.rcx & GENMASK(5, 0);
td_info.attributes = out.rdx;
+
+ /* Exclude Shared bit from the __PHYSICAL_MASK */
+ physical_mask &= ~tdg_shared_mask();
}
static __cpuidle void tdg_halt(void)



--
Sathyanarayanan Kuppuswamy
Linux Kernel Developer