RFC: https://bugzilla.tianocore.org/show_bug.cgi?id=3429

MMIO region in Tdx guest is set with PcdTdxSharedBitMask when
creating 1:1 identity page table. In SEV guest the page table
entries is set with PcdPteMemoryEncryptionAddressOrMask when
creating 1:1 identity table.

So the AddressEncMask in GetPageTableEntry (@CpuPageTable.c) is either
PcdPteMemoryEncryptionAddressOrMask (in SEV guest), or
PcdTdxSharedBitMask (in TDX guest), or all-0 (in Legacy guest).

Cc: Brijesh Singh <brijesh.singh@amd.com>
Cc: Erdem Aktas <erdemaktas@google.com>
Cc: James Bottomley <jejb@linux.ibm.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Rahul Kumar <rahul1.kumar@intel.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Signed-off-by: Min Xu <min.m.xu@intel.com>
---
 UefiCpuPkg/CpuDxe/CpuDxe.inf     | 1 +
 UefiCpuPkg/CpuDxe/CpuPageTable.c | 4 ++++
 2 files changed, 5 insertions(+)

diff --git a/UefiCpuPkg/CpuDxe/CpuDxe.inf b/UefiCpuPkg/CpuDxe/CpuDxe.inf
index d87fe503d152..235241899222 100644
--- a/UefiCpuPkg/CpuDxe/CpuDxe.inf
+++ b/UefiCpuPkg/CpuDxe/CpuDxe.inf
@@ -80,6 +80,7 @@
   gEfiMdeModulePkgTokenSpaceGuid.PcdNullPointerDetectionPropertyMask    ## CONSUMES
   gUefiCpuPkgTokenSpaceGuid.PcdCpuStackSwitchExceptionList              ## CONSUMES
   gUefiCpuPkgTokenSpaceGuid.PcdCpuKnownGoodStackSize                    ## CONSUMES
+  gEfiMdeModulePkgTokenSpaceGuid.PcdTdxSharedBitMask                    ## CONSUMES
 
 [Depex]
   TRUE
diff --git a/UefiCpuPkg/CpuDxe/CpuPageTable.c b/UefiCpuPkg/CpuDxe/CpuPageTable.c
index 06ee1b823171..7982ebd23f98 100644
--- a/UefiCpuPkg/CpuDxe/CpuPageTable.c
+++ b/UefiCpuPkg/CpuDxe/CpuPageTable.c
@@ -299,6 +299,9 @@ GetPageTableEntry (
   // Make sure AddressEncMask is contained to smallest supported address field.
   //
   AddressEncMask = PcdGet64 (PcdPteMemoryEncryptionAddressOrMask) & PAGING_1G_ADDRESS_MASK_64;
+  if (AddressEncMask == 0) {
+    AddressEncMask = PcdGet64 (PcdTdxSharedBitMask) & PAGING_1G_ADDRESS_MASK_64;
+  }
 
   if (PagingContext->MachineType == IMAGE_FILE_MACHINE_X64) {
     if ((PagingContext->ContextData.X64.Attributes & PAGE_TABLE_LIB_PAGING_CONTEXT_IA32_X64_ATTRIBUTES_5_LEVEL) != 0) {
@@ -345,6 +348,7 @@ GetPageTableEntry (
 
   // 4k
   L1PageTable = (UINT64 *)(UINTN)(L2PageTable[Index2] & ~AddressEncMask & PAGING_4K_ADDRESS_MASK_64);
+
   if ((L1PageTable[Index1] == 0) && (Address != 0)) {
     *PageAttribute = PageNone;
     return NULL;
-- 
2.29.2.windows.2



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#82978): https://edk2.groups.io/g/devel/message/82978
Mute This Topic: https://groups.io/mt/86739889/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

  Hi,

> +  gEfiMdeModulePkgTokenSpaceGuid.PcdTdxSharedBitMask                    ## CONSUMES

>    AddressEncMask = PcdGet64 (PcdPteMemoryEncryptionAddressOrMask) & PAGING_1G_ADDRESS_MASK_64;
> +  if (AddressEncMask == 0) {
> +    AddressEncMask = PcdGet64 (PcdTdxSharedBitMask) & PAGING_1G_ADDRESS_MASK_64;
> +  }

Looks like two PCDs for basically the same thing.
Should we create a common CC PCD here?

take care,
  Gerd



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#83226): https://edk2.groups.io/g/devel/message/83226
Mute This Topic: https://groups.io/mt/86739889/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Gerd, thanks. I am about to raise the same comments...


-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Gerd Hoffmann
Sent: Wednesday, November 3, 2021 3:00 PM
To: Xu, Min M <min.m.xu@intel.com>
Cc: devel@edk2.groups.io; Brijesh Singh <brijesh.singh@amd.com>; Erdem Aktas <erdemaktas@google.com>; James Bottomley <jejb@linux.ibm.com>; Yao, Jiewen <jiewen.yao@intel.com>; Tom Lendacky <thomas.lendacky@amd.com>; Dong, Eric <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>; Kumar, Rahul1 <rahul1.kumar@intel.com>
Subject: Re: [edk2-devel] [PATCH V3 23/29] UefiCpuPkg: Update AddressEncMask in CpuPageTable

  Hi,

> +  gEfiMdeModulePkgTokenSpaceGuid.PcdTdxSharedBitMask                    ## CONSUMES

>    AddressEncMask = PcdGet64 (PcdPteMemoryEncryptionAddressOrMask) & 
> PAGING_1G_ADDRESS_MASK_64;
> +  if (AddressEncMask == 0) {
> +    AddressEncMask = PcdGet64 (PcdTdxSharedBitMask) & 
> + PAGING_1G_ADDRESS_MASK_64;  }

Looks like two PCDs for basically the same thing.
Should we create a common CC PCD here?

take care,
  Gerd








-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#83906): https://edk2.groups.io/g/devel/message/83906
Mute This Topic: https://groups.io/mt/86739889/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

On November 22, 2021 11:09 AM, Ni Ray wrote:
> Gerd, thanks. I am about to raise the same comments...
> > +  gEfiMdeModulePkgTokenSpaceGuid.PcdTdxSharedBitMask                    ##
> CONSUMES
> 
> >    AddressEncMask = PcdGet64 (PcdPteMemoryEncryptionAddressOrMask)
> &
> > PAGING_1G_ADDRESS_MASK_64;
> > +  if (AddressEncMask == 0) {
> > +    AddressEncMask = PcdGet64 (PcdTdxSharedBitMask) &
> > + PAGING_1G_ADDRESS_MASK_64;  }
> 
> Looks like two PCDs for basically the same thing.
> Should we create a common CC PCD here?
> 
1. The current situation of PcdPteMemoryEncryptionAddressOrMask is:
1.1 PcdPteMemoryEncryptionAddressOrMask is now set by AmdSev.
1.2 In CreateIdentityMappingPageTables(), this value (AddressEncMask) is set to the page tables in SEV guest.
1.3 This PCD is also used as an indicator in InternalMemEncryptSevStatus() if ReadSevMsr is TRUE or FALSE.
1.4 This PCD is also used in BootScriptExecutorEntryPoint()

2. The meaning and usage scenario of PcdTdxSharedBitMask are somehow different from PcdPteMemoryEncryptionAddressOrMask.
2.1 Guest physical address (GPA) space of Td guest is divided into private and shared sub-spaces, determined by the shared bit of GPA.[1]
2.2 PcdTdxSharedBitMask indicates the above shared bit of GPA. And only the shared GPA has the shared bit set. This breaks 1.2.
2.3 It also breaks above 1.3. Because not all the MSR can be read in Td guest (It will trigger #VE).
2.4 It breaks above 1.4 as well. Because the private GPA doesn't have the shared bit set (2.2). So BootScriptExecutorEntryPoint() has to check Td guest in run-time so that the correct AddressEncMask is used. 

Based on above investigation and consideration, I suggest use PcdTdxSharedBitMask for Td guest and PcdPteMemoryEncryptionAddressOrMask for SEV guest. We can re-visit it later.

[1] https://www.intel.com/content/dam/develop/external/us/en/documents/tdx-module-1.0-public-spec-v0.931.pdf Section 2.4.2

Thanks
Min


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#84441): https://edk2.groups.io/g/devel/message/84441
Mute This Topic: https://groups.io/mt/86739889/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

  Hi,

> > Looks like two PCDs for basically the same thing.
> > Should we create a common CC PCD here?
> > 
> 1. The current situation of PcdPteMemoryEncryptionAddressOrMask is:
> 1.1 PcdPteMemoryEncryptionAddressOrMask is now set by AmdSev.
> 1.2 In CreateIdentityMappingPageTables(), this value (AddressEncMask) is set to the page tables in SEV guest.
> 1.3 This PCD is also used as an indicator in InternalMemEncryptSevStatus() if ReadSevMsr is TRUE or FALSE.
> 1.4 This PCD is also used in BootScriptExecutorEntryPoint()

Yes.  Creating a common CC PCD may require some changes on the SEV side
too.  The code (1.3 for example) assumes sev is active when
PcdPteMemoryEncryptionAddressOrMask is set, which will obviously not be
the case any more when tdx uses it too.  But there are other ways to
check for sev which can be used instead ...

> 2. The meaning and usage scenario of PcdTdxSharedBitMask are somehow different from PcdPteMemoryEncryptionAddressOrMask.
> 2.1 Guest physical address (GPA) space of Td guest is divided into private and shared sub-spaces, determined by the shared bit of GPA.[1]

Well, there are some differences in detail but the underlying concept is
the same.  The page table bit says whenever the page is private to the vm
or not.  With SEV the bit enables/disables encryption.  With TDX the bit
switches between private and shared encryption key.

> 2.2 PcdTdxSharedBitMask indicates the above shared bit of GPA. And
> only the shared GPA has the shared bit set. This breaks 1.2.

Hmm, ok.  So the logic is different.  SEV enables the bit for private
pages whereas TDX enables the bit for shared pages.

Too bad.  That indeed makes it impossible to share a single PCD.

We could still define something generic, like a
"set-this-bit-for-shared-pages" pcd and a
"set-this-bit-for-private-pages" pcd.  But at the end of the day that
probably wouldn't be very different from having
PcdPteMemoryEncryptionAddressOrMask + PcdTdxSharedBitMask ...

take care,
  Gerd



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#84446): https://edk2.groups.io/g/devel/message/84446
Mute This Topic: https://groups.io/mt/86739889/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-