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

["Kuppuswamy, Sathyanarayanan" <sathyanarayanan.kuppuswamy@linux.intel.com>]

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@linux.intel.com>
>>
>> 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@linux.intel.com>
>> Reviewed-by: Andi Kleen <ak@linux.intel.com>
>> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
>> ---
>>   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