[edk2-devel] [RFC PATCH 04/28] OvmfPkg: Create a GHCB page for use during Sec phase

Lendacky, Thomas posted 28 patches 5 years, 3 months ago
There is a newer version of this series
[edk2-devel] [RFC PATCH 04/28] OvmfPkg: Create a GHCB page for use during Sec phase
Posted by Lendacky, Thomas 5 years, 3 months ago
From: Tom Lendacky <thomas.lendacky@amd.com>

A GHCB page is needed during the Sec phase, so this new page must be
created.  Since the GHCB must be marked as an un-encrypted, or shared,
page, an additional pagetable page is required so break down the 2MB
region where the GHCB page lives into 4K pagetable entries.

Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
---
 OvmfPkg/OvmfPkg.dec                        |  5 +++
 OvmfPkg/OvmfPkgX64.fdf                     | 11 ++++---
 OvmfPkg/PlatformPei/PlatformPei.inf        |  2 ++
 OvmfPkg/ResetVector/ResetVector.inf        |  2 ++
 UefiCpuPkg/Include/Register/Amd/Fam17Msr.h | 28 ++++++++++++++++
 OvmfPkg/ResetVector/Ia32/PageTables64.asm  | 37 +++++++++++++++++++++-
 OvmfPkg/ResetVector/ResetVector.nasmb      |  2 +-
 7 files changed, 81 insertions(+), 6 deletions(-)

diff --git a/OvmfPkg/OvmfPkg.dec b/OvmfPkg/OvmfPkg.dec
index 9640360f6245..2ead9a944af4 100644
--- a/OvmfPkg/OvmfPkg.dec
+++ b/OvmfPkg/OvmfPkg.dec
@@ -218,6 +218,11 @@ [PcdsFixedAtBuild]
   #  The value should be a multiple of 4KB.
   gUefiOvmfPkgTokenSpaceGuid.PcdHighPmmMemorySize|0x400000|UINT32|0x31
 
+  ## Specify the GHCB base address and size.
+  #  The value should be a multiple of 4KB for each.
+  gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbBase|0x0|UINT32|0x32
+  gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbSize|0x0|UINT32|0x33
+
 [PcdsDynamic, PcdsDynamicEx]
   gUefiOvmfPkgTokenSpaceGuid.PcdEmuVariableEvent|0|UINT64|2
   gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFlashVariablesEnable|FALSE|BOOLEAN|0x10
diff --git a/OvmfPkg/OvmfPkgX64.fdf b/OvmfPkg/OvmfPkgX64.fdf
index 74407072563b..2a2427092382 100644
--- a/OvmfPkg/OvmfPkgX64.fdf
+++ b/OvmfPkg/OvmfPkgX64.fdf
@@ -67,13 +67,16 @@ [FD.MEMFD]
 BlockSize     = 0x10000
 NumBlocks     = 0xC0
 
-0x000000|0x006000
+0x000000|0x007000
 gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPageTablesBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPageTablesSize
 
-0x006000|0x001000
-gUefiOvmfPkgTokenSpaceGuid.PcdOvmfLockBoxStorageBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfLockBoxStorageSize
-
 0x007000|0x001000
+gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbSize
+
+0x008000|0x001000
+gUefiOvmfPkgTokenSpaceGuid.PcdOvmfLockBoxStorageBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfLockBoxStorageSize
+
+0x009000|0x001000
 gEfiMdePkgTokenSpaceGuid.PcdGuidedExtractHandlerTableAddress|gUefiOvmfPkgTokenSpaceGuid.PcdGuidedExtractHandlerTableSize
 
 0x010000|0x010000
diff --git a/OvmfPkg/PlatformPei/PlatformPei.inf b/OvmfPkg/PlatformPei/PlatformPei.inf
index d9fd9c8f05b3..aed1f64b7c93 100644
--- a/OvmfPkg/PlatformPei/PlatformPei.inf
+++ b/OvmfPkg/PlatformPei/PlatformPei.inf
@@ -72,6 +72,8 @@ [Pcd]
   gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamSize
   gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPageTablesBase
   gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPageTablesSize
+  gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbBase
+  gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbSize
   gUefiOvmfPkgTokenSpaceGuid.PcdOvmfLockBoxStorageBase
   gUefiOvmfPkgTokenSpaceGuid.PcdOvmfLockBoxStorageSize
   gUefiOvmfPkgTokenSpaceGuid.PcdGuidedExtractHandlerTableSize
diff --git a/OvmfPkg/ResetVector/ResetVector.inf b/OvmfPkg/ResetVector/ResetVector.inf
index 960b47cd0797..d66f4dc29737 100644
--- a/OvmfPkg/ResetVector/ResetVector.inf
+++ b/OvmfPkg/ResetVector/ResetVector.inf
@@ -37,3 +37,5 @@ [Pcd]
   gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPageTablesSize
   gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamBase
   gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamSize
+  gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbBase
+  gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbSize
diff --git a/UefiCpuPkg/Include/Register/Amd/Fam17Msr.h b/UefiCpuPkg/Include/Register/Amd/Fam17Msr.h
index 37b935dcdb30..55a5723e164e 100644
--- a/UefiCpuPkg/Include/Register/Amd/Fam17Msr.h
+++ b/UefiCpuPkg/Include/Register/Amd/Fam17Msr.h
@@ -17,6 +17,34 @@
 #ifndef __FAM17_MSR_H__
 #define __FAM17_MSR_H__
 
+/**
+  Secure Encrypted Virtualization - Encrypted State (SEV-ES) GHCB register
+
+**/
+#define MSR_SEV_ES_GHCB                    0xc0010130
+
+/**
+  MSR information returned for #MSR_SEV_ES_GHCB
+**/
+typedef union {
+  struct {
+    UINT32  GhcbNegotiateBit:1;
+
+    UINT32  Reserved:31;
+  } Bits;
+
+  struct {
+    UINT8   Reserved[3];
+    UINT8   SevEncryptionBitPos;
+    UINT16  SevEsProtocolMin;
+    UINT16  SevEsProtocolMax;
+  } GhcbProtocol;
+
+  VOID    *Ghcb;
+
+  UINT64  GhcbPhysicalAddress;
+} MSR_SEV_ES_GHCB_REGISTER;
+
 /**
   Secure Encrypted Virtualization (SEV) status register
 
diff --git a/OvmfPkg/ResetVector/Ia32/PageTables64.asm b/OvmfPkg/ResetVector/Ia32/PageTables64.asm
index c6071fe934de..fd4d5b1d8661 100644
--- a/OvmfPkg/ResetVector/Ia32/PageTables64.asm
+++ b/OvmfPkg/ResetVector/Ia32/PageTables64.asm
@@ -21,6 +21,11 @@ BITS    32
 %define PAGE_2M_MBO            0x080
 %define PAGE_2M_PAT          0x01000
 
+%define PAGE_4K_PDE_ATTR (PAGE_ACCESSED + \
+                          PAGE_DIRTY + \
+                          PAGE_READ_WRITE + \
+                          PAGE_PRESENT)
+
 %define PAGE_2M_PDE_ATTR (PAGE_2M_MBO + \
                           PAGE_ACCESSED + \
                           PAGE_DIRTY + \
@@ -120,7 +125,7 @@ SevNotActive:
     ; more permanent location by DxeIpl.
     ;
 
-    mov     ecx, 6 * 0x1000 / 4
+    mov     ecx, 7 * 0x1000 / 4
     xor     eax, eax
 clearPageTablesMemoryLoop:
     mov     dword[ecx * 4 + PT_ADDR (0) - 4], eax
@@ -157,6 +162,36 @@ pageTableEntriesLoop:
     mov     [(ecx * 8 + PT_ADDR (0x2000 - 8)) + 4], edx
     loop    pageTableEntriesLoop
 
+    ;
+    ; The GHCB will live at 0x807000 (just after the page tables)
+    ; and needs to be un-encrypted.  This requires the 2MB page
+    ; (index 4 in the first 1GB page) for this range be broken down
+    ; into 512 4KB pages.  All will be marked as encrypted, except
+    ; for the GHCB.
+    ;
+    mov     ecx, 4
+    mov     eax, PT_ADDR (0x6000) + PAGE_PDP_ATTR
+    mov     [ecx * 8 + PT_ADDR (0x2000)], eax
+
+    mov     ecx, 512
+pageTableEntries4kLoop:
+    mov     eax, ecx
+    dec     eax
+    shl     eax, 12
+    add     eax, 0x800000
+    add     eax, PAGE_4K_PDE_ATTR
+    mov     [ecx * 8 + PT_ADDR (0x6000 - 8)], eax
+    mov     [(ecx * 8 + PT_ADDR (0x6000 - 8)) + 4], edx
+    loop    pageTableEntries4kLoop
+
+    ;
+    ; Clear the encryption bit from the GHCB entry (index 7 in the
+    ; new PTE table - (0x807000 - 0x800000) >> 12).
+    ;
+    mov     ecx, 7
+    xor     edx, edx
+    mov     [(ecx * 8 + PT_ADDR (0x6000)) + 4], edx
+
     ;
     ; Set CR3 now that the paging structures are available
     ;
diff --git a/OvmfPkg/ResetVector/ResetVector.nasmb b/OvmfPkg/ResetVector/ResetVector.nasmb
index 3b213cd05ab2..56d9b86ed943 100644
--- a/OvmfPkg/ResetVector/ResetVector.nasmb
+++ b/OvmfPkg/ResetVector/ResetVector.nasmb
@@ -49,7 +49,7 @@
 %ifdef ARCH_X64
   #include <AutoGen.h>
 
-  %if (FixedPcdGet32 (PcdOvmfSecPageTablesSize) != 0x6000)
+  %if (FixedPcdGet32 (PcdOvmfSecPageTablesSize) != 0x7000)
     %error "This implementation inherently depends on PcdOvmfSecPageTablesSize"
   %endif
 
-- 
2.17.1


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#46104): https://edk2.groups.io/g/devel/message/46104
Mute This Topic: https://groups.io/mt/32966276/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [RFC PATCH 04/28] OvmfPkg: Create a GHCB page for use during Sec phase
Posted by Laszlo Ersek 5 years, 3 months ago
On 08/19/19 23:35, Lendacky, Thomas wrote:
> From: Tom Lendacky <thomas.lendacky@amd.com>
> 
> A GHCB page is needed during the Sec phase, so this new page must be
> created.  Since the GHCB must be marked as an un-encrypted, or shared,
> page, an additional pagetable page is required so break down the 2MB
> region where the GHCB page lives into 4K pagetable entries.
> 
> Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
> ---
>  OvmfPkg/OvmfPkg.dec                        |  5 +++
>  OvmfPkg/OvmfPkgX64.fdf                     | 11 ++++---
>  OvmfPkg/PlatformPei/PlatformPei.inf        |  2 ++
>  OvmfPkg/ResetVector/ResetVector.inf        |  2 ++
>  UefiCpuPkg/Include/Register/Amd/Fam17Msr.h | 28 ++++++++++++++++
>  OvmfPkg/ResetVector/Ia32/PageTables64.asm  | 37 +++++++++++++++++++++-
>  OvmfPkg/ResetVector/ResetVector.nasmb      |  2 +-
>  7 files changed, 81 insertions(+), 6 deletions(-)

I've skipped patches 02 and 03 for now, because I'll have to go through
them with a fine toothed comb -- in a subsequent submission, most
probably. I'm just trying to provide formal comments, so that I do the
actual review more easily, later.

As I requested under the blurb, this patch should be split in at least
three parts, if possible -- OvmfPkg/PlatformPei, OvmfPkg/ResetVector,
UefiCpuPkg. (The DEC and FDF changes can be kept squashed with the
OvmfPkg patch that seems more suitable for that.)

... Having said that, why do you add PCDs to the PlatformPei INF file?
The code in PlatformPei doesn't change. Could be a leftover from an
earlier (abandoned) approach.

Thanks
Laszlo

> 
> diff --git a/OvmfPkg/OvmfPkg.dec b/OvmfPkg/OvmfPkg.dec
> index 9640360f6245..2ead9a944af4 100644
> --- a/OvmfPkg/OvmfPkg.dec
> +++ b/OvmfPkg/OvmfPkg.dec
> @@ -218,6 +218,11 @@ [PcdsFixedAtBuild]
>    #  The value should be a multiple of 4KB.
>    gUefiOvmfPkgTokenSpaceGuid.PcdHighPmmMemorySize|0x400000|UINT32|0x31
>  
> +  ## Specify the GHCB base address and size.
> +  #  The value should be a multiple of 4KB for each.
> +  gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbBase|0x0|UINT32|0x32
> +  gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbSize|0x0|UINT32|0x33
> +
>  [PcdsDynamic, PcdsDynamicEx]
>    gUefiOvmfPkgTokenSpaceGuid.PcdEmuVariableEvent|0|UINT64|2
>    gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFlashVariablesEnable|FALSE|BOOLEAN|0x10
> diff --git a/OvmfPkg/OvmfPkgX64.fdf b/OvmfPkg/OvmfPkgX64.fdf
> index 74407072563b..2a2427092382 100644
> --- a/OvmfPkg/OvmfPkgX64.fdf
> +++ b/OvmfPkg/OvmfPkgX64.fdf
> @@ -67,13 +67,16 @@ [FD.MEMFD]
>  BlockSize     = 0x10000
>  NumBlocks     = 0xC0
>  
> -0x000000|0x006000
> +0x000000|0x007000
>  gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPageTablesBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPageTablesSize
>  
> -0x006000|0x001000
> -gUefiOvmfPkgTokenSpaceGuid.PcdOvmfLockBoxStorageBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfLockBoxStorageSize
> -
>  0x007000|0x001000
> +gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbSize
> +
> +0x008000|0x001000
> +gUefiOvmfPkgTokenSpaceGuid.PcdOvmfLockBoxStorageBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfLockBoxStorageSize
> +
> +0x009000|0x001000
>  gEfiMdePkgTokenSpaceGuid.PcdGuidedExtractHandlerTableAddress|gUefiOvmfPkgTokenSpaceGuid.PcdGuidedExtractHandlerTableSize
>  
>  0x010000|0x010000
> diff --git a/OvmfPkg/PlatformPei/PlatformPei.inf b/OvmfPkg/PlatformPei/PlatformPei.inf
> index d9fd9c8f05b3..aed1f64b7c93 100644
> --- a/OvmfPkg/PlatformPei/PlatformPei.inf
> +++ b/OvmfPkg/PlatformPei/PlatformPei.inf
> @@ -72,6 +72,8 @@ [Pcd]
>    gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamSize
>    gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPageTablesBase
>    gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPageTablesSize
> +  gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbBase
> +  gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbSize
>    gUefiOvmfPkgTokenSpaceGuid.PcdOvmfLockBoxStorageBase
>    gUefiOvmfPkgTokenSpaceGuid.PcdOvmfLockBoxStorageSize
>    gUefiOvmfPkgTokenSpaceGuid.PcdGuidedExtractHandlerTableSize
> diff --git a/OvmfPkg/ResetVector/ResetVector.inf b/OvmfPkg/ResetVector/ResetVector.inf
> index 960b47cd0797..d66f4dc29737 100644
> --- a/OvmfPkg/ResetVector/ResetVector.inf
> +++ b/OvmfPkg/ResetVector/ResetVector.inf
> @@ -37,3 +37,5 @@ [Pcd]
>    gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPageTablesSize
>    gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamBase
>    gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamSize
> +  gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbBase
> +  gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbSize
> diff --git a/UefiCpuPkg/Include/Register/Amd/Fam17Msr.h b/UefiCpuPkg/Include/Register/Amd/Fam17Msr.h
> index 37b935dcdb30..55a5723e164e 100644
> --- a/UefiCpuPkg/Include/Register/Amd/Fam17Msr.h
> +++ b/UefiCpuPkg/Include/Register/Amd/Fam17Msr.h
> @@ -17,6 +17,34 @@
>  #ifndef __FAM17_MSR_H__
>  #define __FAM17_MSR_H__
>  
> +/**
> +  Secure Encrypted Virtualization - Encrypted State (SEV-ES) GHCB register
> +
> +**/
> +#define MSR_SEV_ES_GHCB                    0xc0010130
> +
> +/**
> +  MSR information returned for #MSR_SEV_ES_GHCB
> +**/
> +typedef union {
> +  struct {
> +    UINT32  GhcbNegotiateBit:1;
> +
> +    UINT32  Reserved:31;
> +  } Bits;
> +
> +  struct {
> +    UINT8   Reserved[3];
> +    UINT8   SevEncryptionBitPos;
> +    UINT16  SevEsProtocolMin;
> +    UINT16  SevEsProtocolMax;
> +  } GhcbProtocol;
> +
> +  VOID    *Ghcb;
> +
> +  UINT64  GhcbPhysicalAddress;
> +} MSR_SEV_ES_GHCB_REGISTER;
> +
>  /**
>    Secure Encrypted Virtualization (SEV) status register
>  
> diff --git a/OvmfPkg/ResetVector/Ia32/PageTables64.asm b/OvmfPkg/ResetVector/Ia32/PageTables64.asm
> index c6071fe934de..fd4d5b1d8661 100644
> --- a/OvmfPkg/ResetVector/Ia32/PageTables64.asm
> +++ b/OvmfPkg/ResetVector/Ia32/PageTables64.asm
> @@ -21,6 +21,11 @@ BITS    32
>  %define PAGE_2M_MBO            0x080
>  %define PAGE_2M_PAT          0x01000
>  
> +%define PAGE_4K_PDE_ATTR (PAGE_ACCESSED + \
> +                          PAGE_DIRTY + \
> +                          PAGE_READ_WRITE + \
> +                          PAGE_PRESENT)
> +
>  %define PAGE_2M_PDE_ATTR (PAGE_2M_MBO + \
>                            PAGE_ACCESSED + \
>                            PAGE_DIRTY + \
> @@ -120,7 +125,7 @@ SevNotActive:
>      ; more permanent location by DxeIpl.
>      ;
>  
> -    mov     ecx, 6 * 0x1000 / 4
> +    mov     ecx, 7 * 0x1000 / 4
>      xor     eax, eax
>  clearPageTablesMemoryLoop:
>      mov     dword[ecx * 4 + PT_ADDR (0) - 4], eax
> @@ -157,6 +162,36 @@ pageTableEntriesLoop:
>      mov     [(ecx * 8 + PT_ADDR (0x2000 - 8)) + 4], edx
>      loop    pageTableEntriesLoop
>  
> +    ;
> +    ; The GHCB will live at 0x807000 (just after the page tables)
> +    ; and needs to be un-encrypted.  This requires the 2MB page
> +    ; (index 4 in the first 1GB page) for this range be broken down
> +    ; into 512 4KB pages.  All will be marked as encrypted, except
> +    ; for the GHCB.
> +    ;
> +    mov     ecx, 4
> +    mov     eax, PT_ADDR (0x6000) + PAGE_PDP_ATTR
> +    mov     [ecx * 8 + PT_ADDR (0x2000)], eax
> +
> +    mov     ecx, 512
> +pageTableEntries4kLoop:
> +    mov     eax, ecx
> +    dec     eax
> +    shl     eax, 12
> +    add     eax, 0x800000
> +    add     eax, PAGE_4K_PDE_ATTR
> +    mov     [ecx * 8 + PT_ADDR (0x6000 - 8)], eax
> +    mov     [(ecx * 8 + PT_ADDR (0x6000 - 8)) + 4], edx
> +    loop    pageTableEntries4kLoop
> +
> +    ;
> +    ; Clear the encryption bit from the GHCB entry (index 7 in the
> +    ; new PTE table - (0x807000 - 0x800000) >> 12).
> +    ;
> +    mov     ecx, 7
> +    xor     edx, edx
> +    mov     [(ecx * 8 + PT_ADDR (0x6000)) + 4], edx
> +
>      ;
>      ; Set CR3 now that the paging structures are available
>      ;
> diff --git a/OvmfPkg/ResetVector/ResetVector.nasmb b/OvmfPkg/ResetVector/ResetVector.nasmb
> index 3b213cd05ab2..56d9b86ed943 100644
> --- a/OvmfPkg/ResetVector/ResetVector.nasmb
> +++ b/OvmfPkg/ResetVector/ResetVector.nasmb
> @@ -49,7 +49,7 @@
>  %ifdef ARCH_X64
>    #include <AutoGen.h>
>  
> -  %if (FixedPcdGet32 (PcdOvmfSecPageTablesSize) != 0x6000)
> +  %if (FixedPcdGet32 (PcdOvmfSecPageTablesSize) != 0x7000)
>      %error "This implementation inherently depends on PcdOvmfSecPageTablesSize"
>    %endif
>  
> 


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#46157): https://edk2.groups.io/g/devel/message/46157
Mute This Topic: https://groups.io/mt/32966276/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [RFC PATCH 04/28] OvmfPkg: Create a GHCB page for use during Sec phase
Posted by Lendacky, Thomas 5 years, 3 months ago
On 8/21/19 9:25 AM, Laszlo Ersek via Groups.Io wrote:
> On 08/19/19 23:35, Lendacky, Thomas wrote:
>> From: Tom Lendacky <thomas.lendacky@amd.com>
>>
>> A GHCB page is needed during the Sec phase, so this new page must be
>> created.  Since the GHCB must be marked as an un-encrypted, or shared,
>> page, an additional pagetable page is required so break down the 2MB
>> region where the GHCB page lives into 4K pagetable entries.
>>
>> Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
>> ---
>>  OvmfPkg/OvmfPkg.dec                        |  5 +++
>>  OvmfPkg/OvmfPkgX64.fdf                     | 11 ++++---
>>  OvmfPkg/PlatformPei/PlatformPei.inf        |  2 ++
>>  OvmfPkg/ResetVector/ResetVector.inf        |  2 ++
>>  UefiCpuPkg/Include/Register/Amd/Fam17Msr.h | 28 ++++++++++++++++
>>  OvmfPkg/ResetVector/Ia32/PageTables64.asm  | 37 +++++++++++++++++++++-
>>  OvmfPkg/ResetVector/ResetVector.nasmb      |  2 +-
>>  7 files changed, 81 insertions(+), 6 deletions(-)
> 
> I've skipped patches 02 and 03 for now, because I'll have to go through
> them with a fine toothed comb -- in a subsequent submission, most
> probably. I'm just trying to provide formal comments, so that I do the
> actual review more easily, later.
> 
> As I requested under the blurb, this patch should be split in at least
> three parts, if possible -- OvmfPkg/PlatformPei, OvmfPkg/ResetVector,
> UefiCpuPkg. (The DEC and FDF changes can be kept squashed with the
> OvmfPkg patch that seems more suitable for that.)

Ok.

> 
> ... Having said that, why do you add PCDs to the PlatformPei INF file?
> The code in PlatformPei doesn't change. Could be a leftover from an
> earlier (abandoned) approach.

Yeah, most likely. I'll remove that.

Thanks,
Tom

> 
> Thanks
> Laszlo
> 
>>
>> diff --git a/OvmfPkg/OvmfPkg.dec b/OvmfPkg/OvmfPkg.dec
>> index 9640360f6245..2ead9a944af4 100644
>> --- a/OvmfPkg/OvmfPkg.dec
>> +++ b/OvmfPkg/OvmfPkg.dec
>> @@ -218,6 +218,11 @@ [PcdsFixedAtBuild]
>>    #  The value should be a multiple of 4KB.
>>    gUefiOvmfPkgTokenSpaceGuid.PcdHighPmmMemorySize|0x400000|UINT32|0x31
>>  
>> +  ## Specify the GHCB base address and size.
>> +  #  The value should be a multiple of 4KB for each.
>> +  gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbBase|0x0|UINT32|0x32
>> +  gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbSize|0x0|UINT32|0x33
>> +
>>  [PcdsDynamic, PcdsDynamicEx]
>>    gUefiOvmfPkgTokenSpaceGuid.PcdEmuVariableEvent|0|UINT64|2
>>    gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFlashVariablesEnable|FALSE|BOOLEAN|0x10
>> diff --git a/OvmfPkg/OvmfPkgX64.fdf b/OvmfPkg/OvmfPkgX64.fdf
>> index 74407072563b..2a2427092382 100644
>> --- a/OvmfPkg/OvmfPkgX64.fdf
>> +++ b/OvmfPkg/OvmfPkgX64.fdf
>> @@ -67,13 +67,16 @@ [FD.MEMFD]
>>  BlockSize     = 0x10000
>>  NumBlocks     = 0xC0
>>  
>> -0x000000|0x006000
>> +0x000000|0x007000
>>  gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPageTablesBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPageTablesSize
>>  
>> -0x006000|0x001000
>> -gUefiOvmfPkgTokenSpaceGuid.PcdOvmfLockBoxStorageBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfLockBoxStorageSize
>> -
>>  0x007000|0x001000
>> +gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbSize
>> +
>> +0x008000|0x001000
>> +gUefiOvmfPkgTokenSpaceGuid.PcdOvmfLockBoxStorageBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfLockBoxStorageSize
>> +
>> +0x009000|0x001000
>>  gEfiMdePkgTokenSpaceGuid.PcdGuidedExtractHandlerTableAddress|gUefiOvmfPkgTokenSpaceGuid.PcdGuidedExtractHandlerTableSize
>>  
>>  0x010000|0x010000
>> diff --git a/OvmfPkg/PlatformPei/PlatformPei.inf b/OvmfPkg/PlatformPei/PlatformPei.inf
>> index d9fd9c8f05b3..aed1f64b7c93 100644
>> --- a/OvmfPkg/PlatformPei/PlatformPei.inf
>> +++ b/OvmfPkg/PlatformPei/PlatformPei.inf
>> @@ -72,6 +72,8 @@ [Pcd]
>>    gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamSize
>>    gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPageTablesBase
>>    gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPageTablesSize
>> +  gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbBase
>> +  gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbSize
>>    gUefiOvmfPkgTokenSpaceGuid.PcdOvmfLockBoxStorageBase
>>    gUefiOvmfPkgTokenSpaceGuid.PcdOvmfLockBoxStorageSize
>>    gUefiOvmfPkgTokenSpaceGuid.PcdGuidedExtractHandlerTableSize
>> diff --git a/OvmfPkg/ResetVector/ResetVector.inf b/OvmfPkg/ResetVector/ResetVector.inf
>> index 960b47cd0797..d66f4dc29737 100644
>> --- a/OvmfPkg/ResetVector/ResetVector.inf
>> +++ b/OvmfPkg/ResetVector/ResetVector.inf
>> @@ -37,3 +37,5 @@ [Pcd]
>>    gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPageTablesSize
>>    gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamBase
>>    gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamSize
>> +  gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbBase
>> +  gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbSize
>> diff --git a/UefiCpuPkg/Include/Register/Amd/Fam17Msr.h b/UefiCpuPkg/Include/Register/Amd/Fam17Msr.h
>> index 37b935dcdb30..55a5723e164e 100644
>> --- a/UefiCpuPkg/Include/Register/Amd/Fam17Msr.h
>> +++ b/UefiCpuPkg/Include/Register/Amd/Fam17Msr.h
>> @@ -17,6 +17,34 @@
>>  #ifndef __FAM17_MSR_H__
>>  #define __FAM17_MSR_H__
>>  
>> +/**
>> +  Secure Encrypted Virtualization - Encrypted State (SEV-ES) GHCB register
>> +
>> +**/
>> +#define MSR_SEV_ES_GHCB                    0xc0010130
>> +
>> +/**
>> +  MSR information returned for #MSR_SEV_ES_GHCB
>> +**/
>> +typedef union {
>> +  struct {
>> +    UINT32  GhcbNegotiateBit:1;
>> +
>> +    UINT32  Reserved:31;
>> +  } Bits;
>> +
>> +  struct {
>> +    UINT8   Reserved[3];
>> +    UINT8   SevEncryptionBitPos;
>> +    UINT16  SevEsProtocolMin;
>> +    UINT16  SevEsProtocolMax;
>> +  } GhcbProtocol;
>> +
>> +  VOID    *Ghcb;
>> +
>> +  UINT64  GhcbPhysicalAddress;
>> +} MSR_SEV_ES_GHCB_REGISTER;
>> +
>>  /**
>>    Secure Encrypted Virtualization (SEV) status register
>>  
>> diff --git a/OvmfPkg/ResetVector/Ia32/PageTables64.asm b/OvmfPkg/ResetVector/Ia32/PageTables64.asm
>> index c6071fe934de..fd4d5b1d8661 100644
>> --- a/OvmfPkg/ResetVector/Ia32/PageTables64.asm
>> +++ b/OvmfPkg/ResetVector/Ia32/PageTables64.asm
>> @@ -21,6 +21,11 @@ BITS    32
>>  %define PAGE_2M_MBO            0x080
>>  %define PAGE_2M_PAT          0x01000
>>  
>> +%define PAGE_4K_PDE_ATTR (PAGE_ACCESSED + \
>> +                          PAGE_DIRTY + \
>> +                          PAGE_READ_WRITE + \
>> +                          PAGE_PRESENT)
>> +
>>  %define PAGE_2M_PDE_ATTR (PAGE_2M_MBO + \
>>                            PAGE_ACCESSED + \
>>                            PAGE_DIRTY + \
>> @@ -120,7 +125,7 @@ SevNotActive:
>>      ; more permanent location by DxeIpl.
>>      ;
>>  
>> -    mov     ecx, 6 * 0x1000 / 4
>> +    mov     ecx, 7 * 0x1000 / 4
>>      xor     eax, eax
>>  clearPageTablesMemoryLoop:
>>      mov     dword[ecx * 4 + PT_ADDR (0) - 4], eax
>> @@ -157,6 +162,36 @@ pageTableEntriesLoop:
>>      mov     [(ecx * 8 + PT_ADDR (0x2000 - 8)) + 4], edx
>>      loop    pageTableEntriesLoop
>>  
>> +    ;
>> +    ; The GHCB will live at 0x807000 (just after the page tables)
>> +    ; and needs to be un-encrypted.  This requires the 2MB page
>> +    ; (index 4 in the first 1GB page) for this range be broken down
>> +    ; into 512 4KB pages.  All will be marked as encrypted, except
>> +    ; for the GHCB.
>> +    ;
>> +    mov     ecx, 4
>> +    mov     eax, PT_ADDR (0x6000) + PAGE_PDP_ATTR
>> +    mov     [ecx * 8 + PT_ADDR (0x2000)], eax
>> +
>> +    mov     ecx, 512
>> +pageTableEntries4kLoop:
>> +    mov     eax, ecx
>> +    dec     eax
>> +    shl     eax, 12
>> +    add     eax, 0x800000
>> +    add     eax, PAGE_4K_PDE_ATTR
>> +    mov     [ecx * 8 + PT_ADDR (0x6000 - 8)], eax
>> +    mov     [(ecx * 8 + PT_ADDR (0x6000 - 8)) + 4], edx
>> +    loop    pageTableEntries4kLoop
>> +
>> +    ;
>> +    ; Clear the encryption bit from the GHCB entry (index 7 in the
>> +    ; new PTE table - (0x807000 - 0x800000) >> 12).
>> +    ;
>> +    mov     ecx, 7
>> +    xor     edx, edx
>> +    mov     [(ecx * 8 + PT_ADDR (0x6000)) + 4], edx
>> +
>>      ;
>>      ; Set CR3 now that the paging structures are available
>>      ;
>> diff --git a/OvmfPkg/ResetVector/ResetVector.nasmb b/OvmfPkg/ResetVector/ResetVector.nasmb
>> index 3b213cd05ab2..56d9b86ed943 100644
>> --- a/OvmfPkg/ResetVector/ResetVector.nasmb
>> +++ b/OvmfPkg/ResetVector/ResetVector.nasmb
>> @@ -49,7 +49,7 @@
>>  %ifdef ARCH_X64
>>    #include <AutoGen.h>
>>  
>> -  %if (FixedPcdGet32 (PcdOvmfSecPageTablesSize) != 0x6000)
>> +  %if (FixedPcdGet32 (PcdOvmfSecPageTablesSize) != 0x7000)
>>      %error "This implementation inherently depends on PcdOvmfSecPageTablesSize"
>>    %endif
>>  
>>
> 
> 
> 
> 

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#46179): https://edk2.groups.io/g/devel/message/46179
Mute This Topic: https://groups.io/mt/32966276/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-