In TDX guests, by default memory is protected from host access. If a
guest needs to communicate with the VMM (like the I/O use case), it uses
a single bit in the physical address to communicate the protected/shared
attribute of the given page.
In the x86 ARCH code, __PHYSICAL_MASK macro represents the width of the
physical address in the given architecture. It is used in creating
physical PAGE_MASK for address bits in the kernel. Since in TDX guest,
a single bit is used as metadata, it needs to be excluded from valid
physical address bits to avoid using incorrect addresses bits in the
kernel.
Enable DYNAMIC_PHYSICAL_MASK to support updating the __PHYSICAL_MASK.
Co-developed-by: Kuppuswamy Sathyanarayanan <[email protected]>
Signed-off-by: Kuppuswamy Sathyanarayanan <[email protected]>
Reviewed-by: Andi Kleen <[email protected]>
Reviewed-by: Tony Luck <[email protected]>
Signed-off-by: Kirill A. Shutemov <[email protected]>
Reviewed-by: Thomas Gleixner <[email protected]>
---
arch/x86/Kconfig | 1 +
arch/x86/coco/tdx/tdx.c | 8 ++++++++
2 files changed, 9 insertions(+)
diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index 93e67842e369..d2f45e58e846 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -885,6 +885,7 @@ config INTEL_TDX_GUEST
depends on X86_64 && CPU_SUP_INTEL
depends on X86_X2APIC
select ARCH_HAS_CC_PLATFORM
+ select DYNAMIC_PHYSICAL_MASK
help
Support running as a guest under Intel TDX. Without this support,
the guest kernel can not boot or run under TDX.
diff --git a/arch/x86/coco/tdx/tdx.c b/arch/x86/coco/tdx/tdx.c
index 9ac066fb3912..069a93d2d331 100644
--- a/arch/x86/coco/tdx/tdx.c
+++ b/arch/x86/coco/tdx/tdx.c
@@ -82,6 +82,14 @@ void __init tdx_early_init(void)
cc_set_vendor(CC_VENDOR_INTEL);
+ /*
+ * All bits above GPA width are reserved and kernel treats shared bit
+ * as flag, not as part of physical address.
+ *
+ * Adjust physical mask to only cover valid GPA bits.
+ */
+ physical_mask &= GENMASK_ULL(gpa_width - 2, 0);
+
/*
* The highest bit of a guest physical address is the "sharing" bit.
* Set it for shared pages and clear it for private pages.
--
2.34.1
On Wed, Mar 16 2022 at 05:08, Kirill A. Shutemov wrote:
> @@ -82,6 +82,14 @@ void __init tdx_early_init(void)
>
> cc_set_vendor(CC_VENDOR_INTEL);
>
> + /*
> + * All bits above GPA width are reserved and kernel treats shared bit
> + * as flag, not as part of physical address.
> + *
> + * Adjust physical mask to only cover valid GPA bits.
> + */
> + physical_mask &= GENMASK_ULL(gpa_width - 2, 0);
> +
Hrm. I forgot about the second use case for gpa_width, but my comment
about ordering still stands. OTOH:
GENMASK_ULL(gpa_width - 2, 0) == BIT_UL(gpa_width - 1) - 1
right? So you really can consolidate on the fact that cc_mask is a
single bit which is above the guests physical address space boundary.
I.e. make the code tell the story instead of adding lengthy comments
explaining the obfuscation.
Thanks,
tglx
On Thu, Mar 17, 2022 at 01:16:00AM +0100, Thomas Gleixner wrote:
> On Wed, Mar 16 2022 at 05:08, Kirill A. Shutemov wrote:
> > @@ -82,6 +82,14 @@ void __init tdx_early_init(void)
> >
> > cc_set_vendor(CC_VENDOR_INTEL);
> >
> > + /*
> > + * All bits above GPA width are reserved and kernel treats shared bit
> > + * as flag, not as part of physical address.
> > + *
> > + * Adjust physical mask to only cover valid GPA bits.
> > + */
> > + physical_mask &= GENMASK_ULL(gpa_width - 2, 0);
> > +
>
> Hrm. I forgot about the second use case for gpa_width, but my comment
> about ordering still stands. OTOH:
>
> GENMASK_ULL(gpa_width - 2, 0) == BIT_UL(gpa_width - 1) - 1
>
> right? So you really can consolidate on the fact that cc_mask is a
> single bit which is above the guests physical address space boundary.
>
> I.e. make the code tell the story instead of adding lengthy comments
> explaining the obfuscation.
So it will looks something like this:
cc_set_vendor(CC_VENDOR_INTEL);
cc_mask = get_cc_mask();
cc_set_mask(cc_mask);
/*
* All bits above GPA width are reserved and kernel treats shared bit
* as flag, not as part of physical address.
*
* Adjust physical mask to only cover valid GPA bits.
*/
physical_mask &= cc_mask - 1;
I still think these comments are useful. I hided comment for cc_mask
calclulation inside get_cc_mask().
Does it look fine to you?
--
Kirill A. Shutemov
On Thu, Mar 17 2022 at 16:58, Kirill A. Shutemov wrote:
> On Thu, Mar 17, 2022 at 01:16:00AM +0100, Thomas Gleixner wrote:
>> On Wed, Mar 16 2022 at 05:08, Kirill A. Shutemov wrote:
>> > @@ -82,6 +82,14 @@ void __init tdx_early_init(void)
>> >
>> > cc_set_vendor(CC_VENDOR_INTEL);
>> >
>> > + /*
>> > + * All bits above GPA width are reserved and kernel treats shared bit
>> > + * as flag, not as part of physical address.
>> > + *
>> > + * Adjust physical mask to only cover valid GPA bits.
>> > + */
>> > + physical_mask &= GENMASK_ULL(gpa_width - 2, 0);
>> > +
>>
>> Hrm. I forgot about the second use case for gpa_width, but my comment
>> about ordering still stands. OTOH:
>>
>> GENMASK_ULL(gpa_width - 2, 0) == BIT_UL(gpa_width - 1) - 1
>>
>> right? So you really can consolidate on the fact that cc_mask is a
>> single bit which is above the guests physical address space boundary.
>>
>> I.e. make the code tell the story instead of adding lengthy comments
>> explaining the obfuscation.
>
> So it will looks something like this:
>
>
> cc_set_vendor(CC_VENDOR_INTEL);
> cc_mask = get_cc_mask();
> cc_set_mask(cc_mask);
>
> /*
> * All bits above GPA width are reserved and kernel treats shared bit
> * as flag, not as part of physical address.
> *
> * Adjust physical mask to only cover valid GPA bits.
> */
> physical_mask &= cc_mask - 1;
>
> I still think these comments are useful. I hided comment for cc_mask
> calclulation inside get_cc_mask().
>
> Does it look fine to you?
Yes.