On 08/19/19 23:35, Lendacky, Thomas wrote:
> From: Tom Lendacky <thomas.lenda...@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.lenda...@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/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
