BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=2198
A GHCB page is needed during the Sec phase, so this new page must be
created. Since the #VC exception handler routines assume that a per-CPU
variable area is immediately after the GHCB, this per-CPU variable area
must also be created. Since the GHCB must be marked as an un-encrypted,
or shared, page, an additional pagetable page is required to break down
the 2MB region where the GHCB page lives into 4K pagetable entries.
Create a new entry in the OVMF memory layout for the new page table
page and for the SEC GHCB and per-CPU variable pages. After breaking down
the 2MB page, update the GHCB page table entry to remove the encryption
mask.
The GHCB page will be used by the SEC #VC exception handler. The #VC
exception handler will fill in the necessary fields of the GHCB and exit
to the hypervisor using the VMGEXIT instruction. The hypervisor then
accesses the GHCB in order to perform the requested function.
Two new fixed PCDs are needed to support the SEC GHCB page:
- PcdOvmfSecGhcbBase UINT64 value that is the base address of the
GHCB used during the SEC phase.
- PcdOvmfSecGhcbSize UINT64 value that is the size, in bytes, of the
GHCB area used during the SEC phase.
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
---
OvmfPkg/OvmfPkg.dec | 9 +++
OvmfPkg/OvmfPkgX64.fdf | 6 ++
OvmfPkg/ResetVector/ResetVector.inf | 5 ++
OvmfPkg/ResetVector/Ia32/PageTables64.asm | 70 +++++++++++++++++++++++
OvmfPkg/ResetVector/ResetVector.nasmb | 17 ++++++
5 files changed, 107 insertions(+)
diff --git a/OvmfPkg/OvmfPkg.dec b/OvmfPkg/OvmfPkg.dec
index 65bb2bb0eb4c..02ad62ed9f43 100644
--- a/OvmfPkg/OvmfPkg.dec
+++ b/OvmfPkg/OvmfPkg.dec
@@ -281,6 +281,15 @@ [PcdsFixedAtBuild]
## Number of page frames to use for storing grant table entries.
gUefiOvmfPkgTokenSpaceGuid.PcdXenGrantFrames|4|UINT32|0x33
+ ## Specify the extra page table needed to mark the GHCB as unencrypted.
+ # The value should be a multiple of 4KB for each.
+ gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbPageTableBase|0x0|UINT32|0x3a
+ gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbPageTableSize|0x0|UINT32|0x3b
+
+ ## The base address of the SEC GHCB page used by SEV-ES.
+ gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbBase|0|UINT32|0x3c
+ gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbSize|0|UINT32|0x3d
+
[PcdsDynamic, PcdsDynamicEx]
gUefiOvmfPkgTokenSpaceGuid.PcdEmuVariableEvent|0|UINT64|2
gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFlashVariablesEnable|FALSE|BOOLEAN|0x10
diff --git a/OvmfPkg/OvmfPkgX64.fdf b/OvmfPkg/OvmfPkgX64.fdf
index bfca1eff9e83..88b1e880e603 100644
--- a/OvmfPkg/OvmfPkgX64.fdf
+++ b/OvmfPkg/OvmfPkgX64.fdf
@@ -76,6 +76,12 @@ [FD.MEMFD]
0x007000|0x001000
gEfiMdePkgTokenSpaceGuid.PcdGuidedExtractHandlerTableAddress|gUefiOvmfPkgTokenSpaceGuid.PcdGuidedExtractHandlerTableSize
+0x008000|0x001000
+gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbPageTableBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbPageTableSize
+
+0x009000|0x002000
+gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbSize
+
0x010000|0x010000
gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamSize
diff --git a/OvmfPkg/ResetVector/ResetVector.inf b/OvmfPkg/ResetVector/ResetVector.inf
index b0ddfa5832a2..483fd90fe785 100644
--- a/OvmfPkg/ResetVector/ResetVector.inf
+++ b/OvmfPkg/ResetVector/ResetVector.inf
@@ -26,6 +26,7 @@ [Sources]
[Packages]
OvmfPkg/OvmfPkg.dec
MdePkg/MdePkg.dec
+ MdeModulePkg/MdeModulePkg.dec
UefiCpuPkg/UefiCpuPkg.dec
[BuildOptions]
@@ -33,5 +34,9 @@ [BuildOptions]
*_*_X64_NASMB_FLAGS = -I$(WORKSPACE)/UefiCpuPkg/ResetVector/Vtf0/
[Pcd]
+ gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbBase
+ gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbSize
+ gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbPageTableBase
+ gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbPageTableSize
gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPageTablesBase
gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPageTablesSize
diff --git a/OvmfPkg/ResetVector/Ia32/PageTables64.asm b/OvmfPkg/ResetVector/Ia32/PageTables64.asm
index abad009f20f5..c3587a1b7814 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 + \
@@ -75,6 +80,37 @@ NoSev:
SevExit:
OneTimeCallRet CheckSevFeature
+; Check if Secure Encrypted Virtualization - Encrypted State (SEV-ES) feature
+; is enabled.
+;
+; Modified: EAX, EBX, ECX
+;
+; If SEV-ES is enabled then EAX will be non-zero.
+; If SEV-ES is disabled then EAX will be zero.
+;
+CheckSevEsFeature:
+ xor eax, eax
+
+ ; SEV-ES can't be enabled if SEV isn't, so first check the encryption
+ ; mask.
+ test edx, edx
+ jz NoSevEs
+
+ ; Save current value of encryption mask
+ mov ebx, edx
+
+ ; Check if SEV-ES is enabled
+ ; MSR_0xC0010131 - Bit 1 (SEV-ES enabled)
+ mov ecx, 0xc0010131
+ rdmsr
+ and eax, 2
+
+ ; Restore encryption mask
+ mov edx, ebx
+
+NoSevEs:
+ OneTimeCallRet CheckSevEsFeature
+
;
; Modified: EAX, EBX, ECX, EDX
;
@@ -139,6 +175,40 @@ pageTableEntriesLoop:
mov [(ecx * 8 + PT_ADDR (0x2000 - 8)) + 4], edx
loop pageTableEntriesLoop
+ OneTimeCall CheckSevEsFeature
+ test eax, eax
+ jz SetCr3
+
+ ;
+ ; The initial GHCB will live at GHCB_BASE and needs to be un-encrypted.
+ ; This requires the 2MB page for this range be broken down into 512 4KB
+ ; pages. All will be marked encrypted, except for the GHCB.
+ ;
+ mov ecx, (GHCB_BASE >> 21)
+ mov eax, GHCB_PT_ADDR + PAGE_PDP_ATTR
+ mov [ecx * 8 + PT_ADDR (0x2000)], eax
+
+ ;
+ ; Page Table Entries (512 * 4KB entries => 2MB)
+ ;
+ mov ecx, 512
+pageTableEntries4kLoop:
+ mov eax, ecx
+ dec eax
+ shl eax, 12
+ add eax, GHCB_BASE & 0xFFE0_0000
+ add eax, PAGE_4K_PDE_ATTR
+ mov [ecx * 8 + GHCB_PT_ADDR - 8], eax
+ mov [(ecx * 8 + GHCB_PT_ADDR - 8) + 4], edx
+ loop pageTableEntries4kLoop
+
+ ;
+ ; Clear the encryption bit from the GHCB entry
+ ;
+ mov ecx, (GHCB_BASE & 0x1F_FFFF) >> 12
+ mov [ecx * 8 + GHCB_PT_ADDR + 4], strict dword 0
+
+SetCr3:
;
; Set CR3 now that the paging structures are available
;
diff --git a/OvmfPkg/ResetVector/ResetVector.nasmb b/OvmfPkg/ResetVector/ResetVector.nasmb
index 75cfe16654b1..bfb77e439105 100644
--- a/OvmfPkg/ResetVector/ResetVector.nasmb
+++ b/OvmfPkg/ResetVector/ResetVector.nasmb
@@ -53,8 +53,25 @@
%error "This implementation inherently depends on PcdOvmfSecPageTablesSize"
%endif
+ %if (FixedPcdGet32 (PcdOvmfSecGhcbPageTableSize) != 0x1000)
+ %error "This implementation inherently depends on PcdOvmfSecGhcbPageTableSize"
+ %endif
+
+ %if (FixedPcdGet32 (PcdOvmfSecGhcbSize) != 0x2000)
+ %error "This implementation inherently depends on PcdOvmfSecGhcbSize"
+ %endif
+
+ %if ((FixedPcdGet32 (PcdOvmfSecGhcbBase) >> 21) != \
+ ((FixedPcdGet32 (PcdOvmfSecGhcbBase) + FixedPcdGet32 (PcdOvmfSecGhcbSize) - 1) >> 21))
+ %error "This implementation inherently depends on PcdOvmfSecGhcbBase not straddling a 2MB boundary"
+ %endif
+
%define PT_ADDR(Offset) (FixedPcdGet32 (PcdOvmfSecPageTablesBase) + (Offset))
%include "Ia32/Flat32ToFlat64.asm"
+
+ %define GHCB_PT_ADDR (FixedPcdGet32 (PcdOvmfSecGhcbPageTableBase))
+ %define GHCB_BASE (FixedPcdGet32 (PcdOvmfSecGhcbBase))
+ %define GHCB_SIZE (FixedPcdGet32 (PcdOvmfSecGhcbSize))
%include "Ia32/PageTables64.asm"
%endif
--
2.17.1
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#59885): https://edk2.groups.io/g/devel/message/59885
Mute This Topic: https://groups.io/mt/74336585/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
On 05/19/20 23:50, Lendacky, Thomas wrote:
> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=2198
>
> A GHCB page is needed during the Sec phase, so this new page must be
> created. Since the #VC exception handler routines assume that a per-CPU
> variable area is immediately after the GHCB, this per-CPU variable area
> must also be created. Since the GHCB must be marked as an un-encrypted,
> or shared, page, an additional pagetable page is required to break down
> the 2MB region where the GHCB page lives into 4K pagetable entries.
>
> Create a new entry in the OVMF memory layout for the new page table
> page and for the SEC GHCB and per-CPU variable pages. After breaking down
> the 2MB page, update the GHCB page table entry to remove the encryption
> mask.
>
> The GHCB page will be used by the SEC #VC exception handler. The #VC
> exception handler will fill in the necessary fields of the GHCB and exit
> to the hypervisor using the VMGEXIT instruction. The hypervisor then
> accesses the GHCB in order to perform the requested function.
>
> Two new fixed PCDs are needed to support the SEC GHCB page:
> - PcdOvmfSecGhcbBase UINT64 value that is the base address of the
> GHCB used during the SEC phase.
> - PcdOvmfSecGhcbSize UINT64 value that is the size, in bytes, of the
> GHCB area used during the SEC phase.
>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
> ---
> OvmfPkg/OvmfPkg.dec | 9 +++
> OvmfPkg/OvmfPkgX64.fdf | 6 ++
> OvmfPkg/ResetVector/ResetVector.inf | 5 ++
> OvmfPkg/ResetVector/Ia32/PageTables64.asm | 70 +++++++++++++++++++++++
> OvmfPkg/ResetVector/ResetVector.nasmb | 17 ++++++
> 5 files changed, 107 insertions(+)
>
> diff --git a/OvmfPkg/OvmfPkg.dec b/OvmfPkg/OvmfPkg.dec
> index 65bb2bb0eb4c..02ad62ed9f43 100644
> --- a/OvmfPkg/OvmfPkg.dec
> +++ b/OvmfPkg/OvmfPkg.dec
> @@ -281,6 +281,15 @@ [PcdsFixedAtBuild]
> ## Number of page frames to use for storing grant table entries.
> gUefiOvmfPkgTokenSpaceGuid.PcdXenGrantFrames|4|UINT32|0x33
>
> + ## Specify the extra page table needed to mark the GHCB as unencrypted.
> + # The value should be a multiple of 4KB for each.
> + gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbPageTableBase|0x0|UINT32|0x3a
> + gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbPageTableSize|0x0|UINT32|0x3b
> +
> + ## The base address of the SEC GHCB page used by SEV-ES.
> + gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbBase|0|UINT32|0x3c
> + gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbSize|0|UINT32|0x3d
> +
> [PcdsDynamic, PcdsDynamicEx]
> gUefiOvmfPkgTokenSpaceGuid.PcdEmuVariableEvent|0|UINT64|2
> gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFlashVariablesEnable|FALSE|BOOLEAN|0x10
OK, the token values have been updated, due to:
- commit 7efce2e59c20 ("OvmfPkg/PvScsiDxe: Report the number of targets
and LUNs", 2020-03-30)
- commit c4c15b870239 ("OvmfPkg/PvScsiDxe: Support sending SCSI request
and receive response", 2020-03-30)
- commit 093cceaf79b5 ("OvmfPkg/MptScsiDxe: Report targets and one LUN",
2020-05-05)
(Independently, when I reviewed what would become 505812ae1d2d
("OvmfPkg/MptScsiDxe: Implement the PassThru method", 2020-05-05), I
missed that 0x39 is followed by 0x3A, not 0x40. Oh well.)
> diff --git a/OvmfPkg/OvmfPkgX64.fdf b/OvmfPkg/OvmfPkgX64.fdf
> index bfca1eff9e83..88b1e880e603 100644
> --- a/OvmfPkg/OvmfPkgX64.fdf
> +++ b/OvmfPkg/OvmfPkgX64.fdf
> @@ -76,6 +76,12 @@ [FD.MEMFD]
> 0x007000|0x001000
> gEfiMdePkgTokenSpaceGuid.PcdGuidedExtractHandlerTableAddress|gUefiOvmfPkgTokenSpaceGuid.PcdGuidedExtractHandlerTableSize
>
> +0x008000|0x001000
> +gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbPageTableBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbPageTableSize
> +
> +0x009000|0x002000
> +gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbSize
> +
> 0x010000|0x010000
> gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamSize
>
> diff --git a/OvmfPkg/ResetVector/ResetVector.inf b/OvmfPkg/ResetVector/ResetVector.inf
> index b0ddfa5832a2..483fd90fe785 100644
> --- a/OvmfPkg/ResetVector/ResetVector.inf
> +++ b/OvmfPkg/ResetVector/ResetVector.inf
> @@ -26,6 +26,7 @@ [Sources]
> [Packages]
> OvmfPkg/OvmfPkg.dec
> MdePkg/MdePkg.dec
> + MdeModulePkg/MdeModulePkg.dec
> UefiCpuPkg/UefiCpuPkg.dec
>
> [BuildOptions]
> @@ -33,5 +34,9 @@ [BuildOptions]
> *_*_X64_NASMB_FLAGS = -I$(WORKSPACE)/UefiCpuPkg/ResetVector/Vtf0/
>
> [Pcd]
> + gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbBase
> + gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbSize
> + gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbPageTableBase
> + gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbPageTableSize
> gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPageTablesBase
> gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPageTablesSize
> diff --git a/OvmfPkg/ResetVector/Ia32/PageTables64.asm b/OvmfPkg/ResetVector/Ia32/PageTables64.asm
> index abad009f20f5..c3587a1b7814 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 + \
> @@ -75,6 +80,37 @@ NoSev:
> SevExit:
> OneTimeCallRet CheckSevFeature
>
> +; Check if Secure Encrypted Virtualization - Encrypted State (SEV-ES) feature
> +; is enabled.
> +;
> +; Modified: EAX, EBX, ECX
> +;
> +; If SEV-ES is enabled then EAX will be non-zero.
> +; If SEV-ES is disabled then EAX will be zero.
> +;
> +CheckSevEsFeature:
> + xor eax, eax
> +
> + ; SEV-ES can't be enabled if SEV isn't, so first check the encryption
> + ; mask.
> + test edx, edx
> + jz NoSevEs
> +
> + ; Save current value of encryption mask
> + mov ebx, edx
> +
> + ; Check if SEV-ES is enabled
> + ; MSR_0xC0010131 - Bit 1 (SEV-ES enabled)
> + mov ecx, 0xc0010131
> + rdmsr
> + and eax, 2
> +
> + ; Restore encryption mask
> + mov edx, ebx
> +
> +NoSevEs:
> + OneTimeCallRet CheckSevEsFeature
> +
> ;
> ; Modified: EAX, EBX, ECX, EDX
> ;
> @@ -139,6 +175,40 @@ pageTableEntriesLoop:
> mov [(ecx * 8 + PT_ADDR (0x2000 - 8)) + 4], edx
> loop pageTableEntriesLoop
>
> + OneTimeCall CheckSevEsFeature
> + test eax, eax
> + jz SetCr3
> +
> + ;
> + ; The initial GHCB will live at GHCB_BASE and needs to be un-encrypted.
> + ; This requires the 2MB page for this range be broken down into 512 4KB
> + ; pages. All will be marked encrypted, except for the GHCB.
> + ;
> + mov ecx, (GHCB_BASE >> 21)
> + mov eax, GHCB_PT_ADDR + PAGE_PDP_ATTR
> + mov [ecx * 8 + PT_ADDR (0x2000)], eax
> +
> + ;
> + ; Page Table Entries (512 * 4KB entries => 2MB)
> + ;
> + mov ecx, 512
> +pageTableEntries4kLoop:
> + mov eax, ecx
> + dec eax
> + shl eax, 12
> + add eax, GHCB_BASE & 0xFFE0_0000
> + add eax, PAGE_4K_PDE_ATTR
> + mov [ecx * 8 + GHCB_PT_ADDR - 8], eax
> + mov [(ecx * 8 + GHCB_PT_ADDR - 8) + 4], edx
> + loop pageTableEntries4kLoop
> +
> + ;
> + ; Clear the encryption bit from the GHCB entry
> + ;
> + mov ecx, (GHCB_BASE & 0x1F_FFFF) >> 12
> + mov [ecx * 8 + GHCB_PT_ADDR + 4], strict dword 0
> +
(1) Why did you remove "clearGhcbMemoryLoop" (in the v6->v7 transition)?
I think that's exactly the clearing loop (minimally for the CPU#0
per-CPU page) that I was just looking for in point (8) under
"OvmfPkg/VmgExitLib: Add support for DR7 Read/Write NAE events" (v8 26/46).
Hm... the v7 blurb says, "Ensure the per-CPU variable page remains
encrypted". OK, but that still doesn't explain why we don't clear it
(just for the guest to see).
Also, if the patch was non-trivially modified in v7, then arguably my
R-b (given originally under "RFC PATCH v3 26/43") should have been removed.
Please re-instate "clearGhcbMemoryLoop" (and then keep the R-b).
Thanks,
Laszlo
> +SetCr3:
> ;
> ; Set CR3 now that the paging structures are available
> ;
> diff --git a/OvmfPkg/ResetVector/ResetVector.nasmb b/OvmfPkg/ResetVector/ResetVector.nasmb
> index 75cfe16654b1..bfb77e439105 100644
> --- a/OvmfPkg/ResetVector/ResetVector.nasmb
> +++ b/OvmfPkg/ResetVector/ResetVector.nasmb
> @@ -53,8 +53,25 @@
> %error "This implementation inherently depends on PcdOvmfSecPageTablesSize"
> %endif
>
> + %if (FixedPcdGet32 (PcdOvmfSecGhcbPageTableSize) != 0x1000)
> + %error "This implementation inherently depends on PcdOvmfSecGhcbPageTableSize"
> + %endif
> +
> + %if (FixedPcdGet32 (PcdOvmfSecGhcbSize) != 0x2000)
> + %error "This implementation inherently depends on PcdOvmfSecGhcbSize"
> + %endif
> +
> + %if ((FixedPcdGet32 (PcdOvmfSecGhcbBase) >> 21) != \
> + ((FixedPcdGet32 (PcdOvmfSecGhcbBase) + FixedPcdGet32 (PcdOvmfSecGhcbSize) - 1) >> 21))
> + %error "This implementation inherently depends on PcdOvmfSecGhcbBase not straddling a 2MB boundary"
> + %endif
> +
> %define PT_ADDR(Offset) (FixedPcdGet32 (PcdOvmfSecPageTablesBase) + (Offset))
> %include "Ia32/Flat32ToFlat64.asm"
> +
> + %define GHCB_PT_ADDR (FixedPcdGet32 (PcdOvmfSecGhcbPageTableBase))
> + %define GHCB_BASE (FixedPcdGet32 (PcdOvmfSecGhcbBase))
> + %define GHCB_SIZE (FixedPcdGet32 (PcdOvmfSecGhcbSize))
> %include "Ia32/PageTables64.asm"
> %endif
>
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#60212): https://edk2.groups.io/g/devel/message/60212
Mute This Topic: https://groups.io/mt/74336585/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
On 5/25/20 10:07 AM, Laszlo Ersek wrote:
> On 05/19/20 23:50, Lendacky, Thomas wrote:
>> BZ: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D2198&data=02%7C01%7Cthomas.lendacky%40amd.com%7C39b71c622d2d4bbf9e5b08d800bd69a5%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637260160817275268&sdata=hz43pd7UO60%2FWfNALLyUuUax8KX%2Bpq4SyU9NIN32Pfc%3D&reserved=0
>>
>> A GHCB page is needed during the Sec phase, so this new page must be
>> created. Since the #VC exception handler routines assume that a per-CPU
>> variable area is immediately after the GHCB, this per-CPU variable area
>> must also be created. Since the GHCB must be marked as an un-encrypted,
>> or shared, page, an additional pagetable page is required to break down
>> the 2MB region where the GHCB page lives into 4K pagetable entries.
>>
>> Create a new entry in the OVMF memory layout for the new page table
>> page and for the SEC GHCB and per-CPU variable pages. After breaking down
>> the 2MB page, update the GHCB page table entry to remove the encryption
>> mask.
>>
>> The GHCB page will be used by the SEC #VC exception handler. The #VC
>> exception handler will fill in the necessary fields of the GHCB and exit
>> to the hypervisor using the VMGEXIT instruction. The hypervisor then
>> accesses the GHCB in order to perform the requested function.
>>
>> Two new fixed PCDs are needed to support the SEC GHCB page:
>> - PcdOvmfSecGhcbBase UINT64 value that is the base address of the
>> GHCB used during the SEC phase.
>> - PcdOvmfSecGhcbSize UINT64 value that is the size, in bytes, of the
>> GHCB area used during the SEC phase.
>>
>> Cc: Jordan Justen <jordan.l.justen@intel.com>
>> Cc: Laszlo Ersek <lersek@redhat.com>
>> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
>> Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
>> ---
>> OvmfPkg/OvmfPkg.dec | 9 +++
>> OvmfPkg/OvmfPkgX64.fdf | 6 ++
>> OvmfPkg/ResetVector/ResetVector.inf | 5 ++
>> OvmfPkg/ResetVector/Ia32/PageTables64.asm | 70 +++++++++++++++++++++++
>> OvmfPkg/ResetVector/ResetVector.nasmb | 17 ++++++
>> 5 files changed, 107 insertions(+)
>>
>> diff --git a/OvmfPkg/OvmfPkg.dec b/OvmfPkg/OvmfPkg.dec
>> index 65bb2bb0eb4c..02ad62ed9f43 100644
>> --- a/OvmfPkg/OvmfPkg.dec
>> +++ b/OvmfPkg/OvmfPkg.dec
>> @@ -281,6 +281,15 @@ [PcdsFixedAtBuild]
>> ## Number of page frames to use for storing grant table entries.
>> gUefiOvmfPkgTokenSpaceGuid.PcdXenGrantFrames|4|UINT32|0x33
>>
>> + ## Specify the extra page table needed to mark the GHCB as unencrypted.
>> + # The value should be a multiple of 4KB for each.
>> + gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbPageTableBase|0x0|UINT32|0x3a
>> + gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbPageTableSize|0x0|UINT32|0x3b
>> +
>> + ## The base address of the SEC GHCB page used by SEV-ES.
>> + gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbBase|0|UINT32|0x3c
>> + gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbSize|0|UINT32|0x3d
>> +
>> [PcdsDynamic, PcdsDynamicEx]
>> gUefiOvmfPkgTokenSpaceGuid.PcdEmuVariableEvent|0|UINT64|2
>> gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFlashVariablesEnable|FALSE|BOOLEAN|0x10
>
> OK, the token values have been updated, due to:
>
> - commit 7efce2e59c20 ("OvmfPkg/PvScsiDxe: Report the number of targets
> and LUNs", 2020-03-30)
>
> - commit c4c15b870239 ("OvmfPkg/PvScsiDxe: Support sending SCSI request
> and receive response", 2020-03-30)
>
> - commit 093cceaf79b5 ("OvmfPkg/MptScsiDxe: Report targets and one LUN",
> 2020-05-05)
>
> (Independently, when I reviewed what would become 505812ae1d2d
> ("OvmfPkg/MptScsiDxe: Implement the PassThru method", 2020-05-05), I
> missed that 0x39 is followed by 0x3A, not 0x40. Oh well.)
>
>
>> diff --git a/OvmfPkg/OvmfPkgX64.fdf b/OvmfPkg/OvmfPkgX64.fdf
>> index bfca1eff9e83..88b1e880e603 100644
>> --- a/OvmfPkg/OvmfPkgX64.fdf
>> +++ b/OvmfPkg/OvmfPkgX64.fdf
>> @@ -76,6 +76,12 @@ [FD.MEMFD]
>> 0x007000|0x001000
>> gEfiMdePkgTokenSpaceGuid.PcdGuidedExtractHandlerTableAddress|gUefiOvmfPkgTokenSpaceGuid.PcdGuidedExtractHandlerTableSize
>>
>> +0x008000|0x001000
>> +gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbPageTableBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbPageTableSize
>> +
>> +0x009000|0x002000
>> +gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbSize
>> +
>> 0x010000|0x010000
>> gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamSize
>>
>> diff --git a/OvmfPkg/ResetVector/ResetVector.inf b/OvmfPkg/ResetVector/ResetVector.inf
>> index b0ddfa5832a2..483fd90fe785 100644
>> --- a/OvmfPkg/ResetVector/ResetVector.inf
>> +++ b/OvmfPkg/ResetVector/ResetVector.inf
>> @@ -26,6 +26,7 @@ [Sources]
>> [Packages]
>> OvmfPkg/OvmfPkg.dec
>> MdePkg/MdePkg.dec
>> + MdeModulePkg/MdeModulePkg.dec
>> UefiCpuPkg/UefiCpuPkg.dec
>>
>> [BuildOptions]
>> @@ -33,5 +34,9 @@ [BuildOptions]
>> *_*_X64_NASMB_FLAGS = -I$(WORKSPACE)/UefiCpuPkg/ResetVector/Vtf0/
>>
>> [Pcd]
>> + gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbBase
>> + gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbSize
>> + gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbPageTableBase
>> + gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbPageTableSize
>> gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPageTablesBase
>> gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPageTablesSize
>> diff --git a/OvmfPkg/ResetVector/Ia32/PageTables64.asm b/OvmfPkg/ResetVector/Ia32/PageTables64.asm
>> index abad009f20f5..c3587a1b7814 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 + \
>> @@ -75,6 +80,37 @@ NoSev:
>> SevExit:
>> OneTimeCallRet CheckSevFeature
>>
>> +; Check if Secure Encrypted Virtualization - Encrypted State (SEV-ES) feature
>> +; is enabled.
>> +;
>> +; Modified: EAX, EBX, ECX
>> +;
>> +; If SEV-ES is enabled then EAX will be non-zero.
>> +; If SEV-ES is disabled then EAX will be zero.
>> +;
>> +CheckSevEsFeature:
>> + xor eax, eax
>> +
>> + ; SEV-ES can't be enabled if SEV isn't, so first check the encryption
>> + ; mask.
>> + test edx, edx
>> + jz NoSevEs
>> +
>> + ; Save current value of encryption mask
>> + mov ebx, edx
>> +
>> + ; Check if SEV-ES is enabled
>> + ; MSR_0xC0010131 - Bit 1 (SEV-ES enabled)
>> + mov ecx, 0xc0010131
>> + rdmsr
>> + and eax, 2
>> +
>> + ; Restore encryption mask
>> + mov edx, ebx
>> +
>> +NoSevEs:
>> + OneTimeCallRet CheckSevEsFeature
>> +
>> ;
>> ; Modified: EAX, EBX, ECX, EDX
>> ;
>> @@ -139,6 +175,40 @@ pageTableEntriesLoop:
>> mov [(ecx * 8 + PT_ADDR (0x2000 - 8)) + 4], edx
>> loop pageTableEntriesLoop
>>
>> + OneTimeCall CheckSevEsFeature
>> + test eax, eax
>> + jz SetCr3
>> +
>> + ;
>> + ; The initial GHCB will live at GHCB_BASE and needs to be un-encrypted.
>> + ; This requires the 2MB page for this range be broken down into 512 4KB
>> + ; pages. All will be marked encrypted, except for the GHCB.
>> + ;
>> + mov ecx, (GHCB_BASE >> 21)
>> + mov eax, GHCB_PT_ADDR + PAGE_PDP_ATTR
>> + mov [ecx * 8 + PT_ADDR (0x2000)], eax
>> +
>> + ;
>> + ; Page Table Entries (512 * 4KB entries => 2MB)
>> + ;
>> + mov ecx, 512
>> +pageTableEntries4kLoop:
>> + mov eax, ecx
>> + dec eax
>> + shl eax, 12
>> + add eax, GHCB_BASE & 0xFFE0_0000
>> + add eax, PAGE_4K_PDE_ATTR
>> + mov [ecx * 8 + GHCB_PT_ADDR - 8], eax
>> + mov [(ecx * 8 + GHCB_PT_ADDR - 8) + 4], edx
>> + loop pageTableEntries4kLoop
>> +
>> + ;
>> + ; Clear the encryption bit from the GHCB entry
>> + ;
>> + mov ecx, (GHCB_BASE & 0x1F_FFFF) >> 12
>> + mov [ecx * 8 + GHCB_PT_ADDR + 4], strict dword 0
>> +
>
> (1) Why did you remove "clearGhcbMemoryLoop" (in the v6->v7 transition)?
I removed it because it actually wasn't clearing the GHCB at all. Since
this occurred before the new page tables are loaded, the page is accessed
encrypted. After loading the new page tables, the GHCB is now referenced
unencrypted and so the "zeroed" page isn't actually zeroes anymore, it is
cipher-text.
Since the GHCB is always cleared on #VC, I dropped it.
>
> I think that's exactly the clearing loop (minimally for the CPU#0
> per-CPU page) that I was just looking for in point (8) under
> "OvmfPkg/VmgExitLib: Add support for DR7 Read/Write NAE events" (v8 26/46).
>
> Hm... the v7 blurb says, "Ensure the per-CPU variable page remains
> encrypted". OK, but that still doesn't explain why we don't clear it
> (just for the guest to see).
I'll add a loop to clear the GHCB page and the per-CPU page after
establishing the new page tables.
>
> Also, if the patch was non-trivially modified in v7, then arguably my
> R-b (given originally under "RFC PATCH v3 26/43") should have been removed.
>
> Please re-instate "clearGhcbMemoryLoop" (and then keep the R-b).
I'll actually drop your Reviewed-by: since I'll need to expand and move
the loop to clear the memory area from the original location in order for
the clearing of the pages to be correct.
Thanks,
Tom
>
> Thanks,
> Laszlo
>
>> +SetCr3:
>> ;
>> ; Set CR3 now that the paging structures are available
>> ;
>> diff --git a/OvmfPkg/ResetVector/ResetVector.nasmb b/OvmfPkg/ResetVector/ResetVector.nasmb
>> index 75cfe16654b1..bfb77e439105 100644
>> --- a/OvmfPkg/ResetVector/ResetVector.nasmb
>> +++ b/OvmfPkg/ResetVector/ResetVector.nasmb
>> @@ -53,8 +53,25 @@
>> %error "This implementation inherently depends on PcdOvmfSecPageTablesSize"
>> %endif
>>
>> + %if (FixedPcdGet32 (PcdOvmfSecGhcbPageTableSize) != 0x1000)
>> + %error "This implementation inherently depends on PcdOvmfSecGhcbPageTableSize"
>> + %endif
>> +
>> + %if (FixedPcdGet32 (PcdOvmfSecGhcbSize) != 0x2000)
>> + %error "This implementation inherently depends on PcdOvmfSecGhcbSize"
>> + %endif
>> +
>> + %if ((FixedPcdGet32 (PcdOvmfSecGhcbBase) >> 21) != \
>> + ((FixedPcdGet32 (PcdOvmfSecGhcbBase) + FixedPcdGet32 (PcdOvmfSecGhcbSize) - 1) >> 21))
>> + %error "This implementation inherently depends on PcdOvmfSecGhcbBase not straddling a 2MB boundary"
>> + %endif
>> +
>> %define PT_ADDR(Offset) (FixedPcdGet32 (PcdOvmfSecPageTablesBase) + (Offset))
>> %include "Ia32/Flat32ToFlat64.asm"
>> +
>> + %define GHCB_PT_ADDR (FixedPcdGet32 (PcdOvmfSecGhcbPageTableBase))
>> + %define GHCB_BASE (FixedPcdGet32 (PcdOvmfSecGhcbBase))
>> + %define GHCB_SIZE (FixedPcdGet32 (PcdOvmfSecGhcbSize))
>> %include "Ia32/PageTables64.asm"
>> %endif
>>
>>
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#60283): https://edk2.groups.io/g/devel/message/60283
Mute This Topic: https://groups.io/mt/74336585/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
On 5/26/20 10:41 AM, Tom Lendacky wrote:
> On 5/25/20 10:07 AM, Laszlo Ersek wrote:
>> On 05/19/20 23:50, Lendacky, Thomas wrote:
>>> BZ:
>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D2198&data=02%7C01%7Cthomas.lendacky%40amd.com%7C39b71c622d2d4bbf9e5b08d800bd69a5%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637260160817275268&sdata=hz43pd7UO60%2FWfNALLyUuUax8KX%2Bpq4SyU9NIN32Pfc%3D&reserved=0
>>>
>>>
>>> A GHCB page is needed during the Sec phase, so this new page must be
>>> created. Since the #VC exception handler routines assume that a per-CPU
>>> variable area is immediately after the GHCB, this per-CPU variable area
>>> must also be created. Since the GHCB must be marked as an un-encrypted,
>>> or shared, page, an additional pagetable page is required to break down
>>> the 2MB region where the GHCB page lives into 4K pagetable entries.
>>>
>>> Create a new entry in the OVMF memory layout for the new page table
>>> page and for the SEC GHCB and per-CPU variable pages. After breaking down
>>> the 2MB page, update the GHCB page table entry to remove the encryption
>>> mask.
>>>
>>> The GHCB page will be used by the SEC #VC exception handler. The #VC
>>> exception handler will fill in the necessary fields of the GHCB and exit
>>> to the hypervisor using the VMGEXIT instruction. The hypervisor then
>>> accesses the GHCB in order to perform the requested function.
>>>
>>> Two new fixed PCDs are needed to support the SEC GHCB page:
>>> - PcdOvmfSecGhcbBase UINT64 value that is the base address of the
>>> GHCB used during the SEC phase.
>>> - PcdOvmfSecGhcbSize UINT64 value that is the size, in bytes, of the
>>> GHCB area used during the SEC phase.
>>>
>>> Cc: Jordan Justen <jordan.l.justen@intel.com>
>>> Cc: Laszlo Ersek <lersek@redhat.com>
>>> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
>>> Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
>>> ---
>>> OvmfPkg/OvmfPkg.dec | 9 +++
>>> OvmfPkg/OvmfPkgX64.fdf | 6 ++
>>> OvmfPkg/ResetVector/ResetVector.inf | 5 ++
>>> OvmfPkg/ResetVector/Ia32/PageTables64.asm | 70 +++++++++++++++++++++++
>>> OvmfPkg/ResetVector/ResetVector.nasmb | 17 ++++++
>>> 5 files changed, 107 insertions(+)
>>>
>>> diff --git a/OvmfPkg/OvmfPkg.dec b/OvmfPkg/OvmfPkg.dec
>>> index 65bb2bb0eb4c..02ad62ed9f43 100644
>>> --- a/OvmfPkg/OvmfPkg.dec
>>> +++ b/OvmfPkg/OvmfPkg.dec
>>> @@ -281,6 +281,15 @@ [PcdsFixedAtBuild]
>>> ## Number of page frames to use for storing grant table entries.
>>> gUefiOvmfPkgTokenSpaceGuid.PcdXenGrantFrames|4|UINT32|0x33
>>> + ## Specify the extra page table needed to mark the GHCB as unencrypted.
>>> + # The value should be a multiple of 4KB for each.
>>> + gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbPageTableBase|0x0|UINT32|0x3a
>>> + gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbPageTableSize|0x0|UINT32|0x3b
>>> +
>>> + ## The base address of the SEC GHCB page used by SEV-ES.
>>> + gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbBase|0|UINT32|0x3c
>>> + gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbSize|0|UINT32|0x3d
>>> +
>>> [PcdsDynamic, PcdsDynamicEx]
>>> gUefiOvmfPkgTokenSpaceGuid.PcdEmuVariableEvent|0|UINT64|2
>>>
>>> gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFlashVariablesEnable|FALSE|BOOLEAN|0x10
>>
>> OK, the token values have been updated, due to:
>>
>> - commit 7efce2e59c20 ("OvmfPkg/PvScsiDxe: Report the number of targets
>> and LUNs", 2020-03-30)
>>
>> - commit c4c15b870239 ("OvmfPkg/PvScsiDxe: Support sending SCSI request
>> and receive response", 2020-03-30)
>>
>> - commit 093cceaf79b5 ("OvmfPkg/MptScsiDxe: Report targets and one LUN",
>> 2020-05-05)
>>
>> (Independently, when I reviewed what would become 505812ae1d2d
>> ("OvmfPkg/MptScsiDxe: Implement the PassThru method", 2020-05-05), I
>> missed that 0x39 is followed by 0x3A, not 0x40. Oh well.)
>>
>>
>>> diff --git a/OvmfPkg/OvmfPkgX64.fdf b/OvmfPkg/OvmfPkgX64.fdf
>>> index bfca1eff9e83..88b1e880e603 100644
>>> --- a/OvmfPkg/OvmfPkgX64.fdf
>>> +++ b/OvmfPkg/OvmfPkgX64.fdf
>>> @@ -76,6 +76,12 @@ [FD.MEMFD]
>>> 0x007000|0x001000
>>>
>>> gEfiMdePkgTokenSpaceGuid.PcdGuidedExtractHandlerTableAddress|gUefiOvmfPkgTokenSpaceGuid.PcdGuidedExtractHandlerTableSize
>>>
>>> +0x008000|0x001000
>>> +gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbPageTableBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbPageTableSize
>>>
>>> +
>>> +0x009000|0x002000
>>> +gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbSize
>>>
>>> +
>>> 0x010000|0x010000
>>>
>>> gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamSize
>>>
>>> diff --git a/OvmfPkg/ResetVector/ResetVector.inf
>>> b/OvmfPkg/ResetVector/ResetVector.inf
>>> index b0ddfa5832a2..483fd90fe785 100644
>>> --- a/OvmfPkg/ResetVector/ResetVector.inf
>>> +++ b/OvmfPkg/ResetVector/ResetVector.inf
>>> @@ -26,6 +26,7 @@ [Sources]
>>> [Packages]
>>> OvmfPkg/OvmfPkg.dec
>>> MdePkg/MdePkg.dec
>>> + MdeModulePkg/MdeModulePkg.dec
>>> UefiCpuPkg/UefiCpuPkg.dec
>>> [BuildOptions]
>>> @@ -33,5 +34,9 @@ [BuildOptions]
>>> *_*_X64_NASMB_FLAGS = -I$(WORKSPACE)/UefiCpuPkg/ResetVector/Vtf0/
>>> [Pcd]
>>> + gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbBase
>>> + gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbSize
>>> + gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbPageTableBase
>>> + gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbPageTableSize
>>> gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPageTablesBase
>>> gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPageTablesSize
>>> diff --git a/OvmfPkg/ResetVector/Ia32/PageTables64.asm
>>> b/OvmfPkg/ResetVector/Ia32/PageTables64.asm
>>> index abad009f20f5..c3587a1b7814 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 + \
>>> @@ -75,6 +80,37 @@ NoSev:
>>> SevExit:
>>> OneTimeCallRet CheckSevFeature
>>> +; Check if Secure Encrypted Virtualization - Encrypted State (SEV-ES)
>>> feature
>>> +; is enabled.
>>> +;
>>> +; Modified: EAX, EBX, ECX
>>> +;
>>> +; If SEV-ES is enabled then EAX will be non-zero.
>>> +; If SEV-ES is disabled then EAX will be zero.
>>> +;
>>> +CheckSevEsFeature:
>>> + xor eax, eax
>>> +
>>> + ; SEV-ES can't be enabled if SEV isn't, so first check the encryption
>>> + ; mask.
>>> + test edx, edx
>>> + jz NoSevEs
>>> +
>>> + ; Save current value of encryption mask
>>> + mov ebx, edx
>>> +
>>> + ; Check if SEV-ES is enabled
>>> + ; MSR_0xC0010131 - Bit 1 (SEV-ES enabled)
>>> + mov ecx, 0xc0010131
>>> + rdmsr
>>> + and eax, 2
>>> +
>>> + ; Restore encryption mask
>>> + mov edx, ebx
>>> +
>>> +NoSevEs:
>>> + OneTimeCallRet CheckSevEsFeature
>>> +
>>> ;
>>> ; Modified: EAX, EBX, ECX, EDX
>>> ;
>>> @@ -139,6 +175,40 @@ pageTableEntriesLoop:
>>> mov [(ecx * 8 + PT_ADDR (0x2000 - 8)) + 4], edx
>>> loop pageTableEntriesLoop
>>> + OneTimeCall CheckSevEsFeature
>>> + test eax, eax
>>> + jz SetCr3
>>> +
>>> + ;
>>> + ; The initial GHCB will live at GHCB_BASE and needs to be
>>> un-encrypted.
>>> + ; This requires the 2MB page for this range be broken down into
>>> 512 4KB
>>> + ; pages. All will be marked encrypted, except for the GHCB.
>>> + ;
>>> + mov ecx, (GHCB_BASE >> 21)
>>> + mov eax, GHCB_PT_ADDR + PAGE_PDP_ATTR
>>> + mov [ecx * 8 + PT_ADDR (0x2000)], eax
>>> +
>>> + ;
>>> + ; Page Table Entries (512 * 4KB entries => 2MB)
>>> + ;
>>> + mov ecx, 512
>>> +pageTableEntries4kLoop:
>>> + mov eax, ecx
>>> + dec eax
>>> + shl eax, 12
>>> + add eax, GHCB_BASE & 0xFFE0_0000
>>> + add eax, PAGE_4K_PDE_ATTR
>>> + mov [ecx * 8 + GHCB_PT_ADDR - 8], eax
>>> + mov [(ecx * 8 + GHCB_PT_ADDR - 8) + 4], edx
>>> + loop pageTableEntries4kLoop
>>> +
>>> + ;
>>> + ; Clear the encryption bit from the GHCB entry
>>> + ;
>>> + mov ecx, (GHCB_BASE & 0x1F_FFFF) >> 12
>>> + mov [ecx * 8 + GHCB_PT_ADDR + 4], strict dword 0
>>> +
>>
>> (1) Why did you remove "clearGhcbMemoryLoop" (in the v6->v7 transition)?
>
> I removed it because it actually wasn't clearing the GHCB at all. Since
> this occurred before the new page tables are loaded, the page is accessed
> encrypted. After loading the new page tables, the GHCB is now referenced
> unencrypted and so the "zeroed" page isn't actually zeroes anymore, it is
> cipher-text.
>
> Since the GHCB is always cleared on #VC, I dropped it.
>
>>
>> I think that's exactly the clearing loop (minimally for the CPU#0
>> per-CPU page) that I was just looking for in point (8) under
>> "OvmfPkg/VmgExitLib: Add support for DR7 Read/Write NAE events" (v8 26/46).
>>
>> Hm... the v7 blurb says, "Ensure the per-CPU variable page remains
>> encrypted". OK, but that still doesn't explain why we don't clear it
>> (just for the guest to see).
>
> I'll add a loop to clear the GHCB page and the per-CPU page after
> establishing the new page tables.
>
>>
>> Also, if the patch was non-trivially modified in v7, then arguably my
>> R-b (given originally under "RFC PATCH v3 26/43") should have been removed.
>>
>> Please re-instate "clearGhcbMemoryLoop" (and then keep the R-b).
>
> I'll actually drop your Reviewed-by: since I'll need to expand and move
> the loop to clear the memory area from the original location in order for
> the clearing of the pages to be correct.
It's not expanding (not sure why I typed that), just moving to after the
setting of CR3.
Thanks,
Tom
>
> Thanks,
> Tom
>
>>
>> Thanks,
>> Laszlo
>>
>>> +SetCr3:
>>> ;
>>> ; Set CR3 now that the paging structures are available
>>> ;
>>> diff --git a/OvmfPkg/ResetVector/ResetVector.nasmb
>>> b/OvmfPkg/ResetVector/ResetVector.nasmb
>>> index 75cfe16654b1..bfb77e439105 100644
>>> --- a/OvmfPkg/ResetVector/ResetVector.nasmb
>>> +++ b/OvmfPkg/ResetVector/ResetVector.nasmb
>>> @@ -53,8 +53,25 @@
>>> %error "This implementation inherently depends on
>>> PcdOvmfSecPageTablesSize"
>>> %endif
>>> + %if (FixedPcdGet32 (PcdOvmfSecGhcbPageTableSize) != 0x1000)
>>> + %error "This implementation inherently depends on
>>> PcdOvmfSecGhcbPageTableSize"
>>> + %endif
>>> +
>>> + %if (FixedPcdGet32 (PcdOvmfSecGhcbSize) != 0x2000)
>>> + %error "This implementation inherently depends on PcdOvmfSecGhcbSize"
>>> + %endif
>>> +
>>> + %if ((FixedPcdGet32 (PcdOvmfSecGhcbBase) >> 21) != \
>>> + ((FixedPcdGet32 (PcdOvmfSecGhcbBase) + FixedPcdGet32
>>> (PcdOvmfSecGhcbSize) - 1) >> 21))
>>> + %error "This implementation inherently depends on
>>> PcdOvmfSecGhcbBase not straddling a 2MB boundary"
>>> + %endif
>>> +
>>> %define PT_ADDR(Offset) (FixedPcdGet32 (PcdOvmfSecPageTablesBase) +
>>> (Offset))
>>> %include "Ia32/Flat32ToFlat64.asm"
>>> +
>>> + %define GHCB_PT_ADDR (FixedPcdGet32 (PcdOvmfSecGhcbPageTableBase))
>>> + %define GHCB_BASE (FixedPcdGet32 (PcdOvmfSecGhcbBase))
>>> + %define GHCB_SIZE (FixedPcdGet32 (PcdOvmfSecGhcbSize))
>>> %include "Ia32/PageTables64.asm"
>>> %endif
>>>
>>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#60284): https://edk2.groups.io/g/devel/message/60284
Mute This Topic: https://groups.io/mt/74336585/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
On 05/26/20 17:41, Tom Lendacky wrote:
> On 5/25/20 10:07 AM, Laszlo Ersek wrote:
>> On 05/19/20 23:50, Lendacky, Thomas wrote:
>>> BZ:
>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D2198&data=02%7C01%7Cthomas.lendacky%40amd.com%7C39b71c622d2d4bbf9e5b08d800bd69a5%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637260160817275268&sdata=hz43pd7UO60%2FWfNALLyUuUax8KX%2Bpq4SyU9NIN32Pfc%3D&reserved=0
>>>
>>>
>>> A GHCB page is needed during the Sec phase, so this new page must be
>>> created. Since the #VC exception handler routines assume that a per-CPU
>>> variable area is immediately after the GHCB, this per-CPU variable area
>>> must also be created. Since the GHCB must be marked as an un-encrypted,
>>> or shared, page, an additional pagetable page is required to break down
>>> the 2MB region where the GHCB page lives into 4K pagetable entries.
>>>
>>> Create a new entry in the OVMF memory layout for the new page table
>>> page and for the SEC GHCB and per-CPU variable pages. After breaking
>>> down
>>> the 2MB page, update the GHCB page table entry to remove the encryption
>>> mask.
>>>
>>> The GHCB page will be used by the SEC #VC exception handler. The #VC
>>> exception handler will fill in the necessary fields of the GHCB and exit
>>> to the hypervisor using the VMGEXIT instruction. The hypervisor then
>>> accesses the GHCB in order to perform the requested function.
>>>
>>> Two new fixed PCDs are needed to support the SEC GHCB page:
>>> - PcdOvmfSecGhcbBase UINT64 value that is the base address of the
>>> GHCB used during the SEC phase.
>>> - PcdOvmfSecGhcbSize UINT64 value that is the size, in bytes, of the
>>> GHCB area used during the SEC phase.
>>>
>>> Cc: Jordan Justen <jordan.l.justen@intel.com>
>>> Cc: Laszlo Ersek <lersek@redhat.com>
>>> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
>>> Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
>>> ---
>>> OvmfPkg/OvmfPkg.dec | 9 +++
>>> OvmfPkg/OvmfPkgX64.fdf | 6 ++
>>> OvmfPkg/ResetVector/ResetVector.inf | 5 ++
>>> OvmfPkg/ResetVector/Ia32/PageTables64.asm | 70 +++++++++++++++++++++++
>>> OvmfPkg/ResetVector/ResetVector.nasmb | 17 ++++++
>>> 5 files changed, 107 insertions(+)
>>>
>>> diff --git a/OvmfPkg/OvmfPkg.dec b/OvmfPkg/OvmfPkg.dec
>>> index 65bb2bb0eb4c..02ad62ed9f43 100644
>>> --- a/OvmfPkg/OvmfPkg.dec
>>> +++ b/OvmfPkg/OvmfPkg.dec
>>> @@ -281,6 +281,15 @@ [PcdsFixedAtBuild]
>>> ## Number of page frames to use for storing grant table entries.
>>> gUefiOvmfPkgTokenSpaceGuid.PcdXenGrantFrames|4|UINT32|0x33
>>> + ## Specify the extra page table needed to mark the GHCB as
>>> unencrypted.
>>> + # The value should be a multiple of 4KB for each.
>>> +
>>> gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbPageTableBase|0x0|UINT32|0x3a
>>> +
>>> gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbPageTableSize|0x0|UINT32|0x3b
>>> +
>>> + ## The base address of the SEC GHCB page used by SEV-ES.
>>> + gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbBase|0|UINT32|0x3c
>>> + gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbSize|0|UINT32|0x3d
>>> +
>>> [PcdsDynamic, PcdsDynamicEx]
>>> gUefiOvmfPkgTokenSpaceGuid.PcdEmuVariableEvent|0|UINT64|2
>>>
>>> gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFlashVariablesEnable|FALSE|BOOLEAN|0x10
>>>
>>
>> OK, the token values have been updated, due to:
>>
>> - commit 7efce2e59c20 ("OvmfPkg/PvScsiDxe: Report the number of targets
>> and LUNs", 2020-03-30)
>>
>> - commit c4c15b870239 ("OvmfPkg/PvScsiDxe: Support sending SCSI request
>> and receive response", 2020-03-30)
>>
>> - commit 093cceaf79b5 ("OvmfPkg/MptScsiDxe: Report targets and one LUN",
>> 2020-05-05)
>>
>> (Independently, when I reviewed what would become 505812ae1d2d
>> ("OvmfPkg/MptScsiDxe: Implement the PassThru method", 2020-05-05), I
>> missed that 0x39 is followed by 0x3A, not 0x40. Oh well.)
>>
>>
>>> diff --git a/OvmfPkg/OvmfPkgX64.fdf b/OvmfPkg/OvmfPkgX64.fdf
>>> index bfca1eff9e83..88b1e880e603 100644
>>> --- a/OvmfPkg/OvmfPkgX64.fdf
>>> +++ b/OvmfPkg/OvmfPkgX64.fdf
>>> @@ -76,6 +76,12 @@ [FD.MEMFD]
>>> 0x007000|0x001000
>>>
>>> gEfiMdePkgTokenSpaceGuid.PcdGuidedExtractHandlerTableAddress|gUefiOvmfPkgTokenSpaceGuid.PcdGuidedExtractHandlerTableSize
>>>
>>> +0x008000|0x001000
>>> +gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbPageTableBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbPageTableSize
>>>
>>> +
>>> +0x009000|0x002000
>>> +gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbSize
>>>
>>> +
>>> 0x010000|0x010000
>>>
>>> gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamSize
>>>
>>> diff --git a/OvmfPkg/ResetVector/ResetVector.inf
>>> b/OvmfPkg/ResetVector/ResetVector.inf
>>> index b0ddfa5832a2..483fd90fe785 100644
>>> --- a/OvmfPkg/ResetVector/ResetVector.inf
>>> +++ b/OvmfPkg/ResetVector/ResetVector.inf
>>> @@ -26,6 +26,7 @@ [Sources]
>>> [Packages]
>>> OvmfPkg/OvmfPkg.dec
>>> MdePkg/MdePkg.dec
>>> + MdeModulePkg/MdeModulePkg.dec
>>> UefiCpuPkg/UefiCpuPkg.dec
>>> [BuildOptions]
>>> @@ -33,5 +34,9 @@ [BuildOptions]
>>> *_*_X64_NASMB_FLAGS = -I$(WORKSPACE)/UefiCpuPkg/ResetVector/Vtf0/
>>> [Pcd]
>>> + gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbBase
>>> + gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbSize
>>> + gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbPageTableBase
>>> + gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbPageTableSize
>>> gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPageTablesBase
>>> gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPageTablesSize
>>> diff --git a/OvmfPkg/ResetVector/Ia32/PageTables64.asm
>>> b/OvmfPkg/ResetVector/Ia32/PageTables64.asm
>>> index abad009f20f5..c3587a1b7814 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 + \
>>> @@ -75,6 +80,37 @@ NoSev:
>>> SevExit:
>>> OneTimeCallRet CheckSevFeature
>>> +; Check if Secure Encrypted Virtualization - Encrypted State
>>> (SEV-ES) feature
>>> +; is enabled.
>>> +;
>>> +; Modified: EAX, EBX, ECX
>>> +;
>>> +; If SEV-ES is enabled then EAX will be non-zero.
>>> +; If SEV-ES is disabled then EAX will be zero.
>>> +;
>>> +CheckSevEsFeature:
>>> + xor eax, eax
>>> +
>>> + ; SEV-ES can't be enabled if SEV isn't, so first check the
>>> encryption
>>> + ; mask.
>>> + test edx, edx
>>> + jz NoSevEs
>>> +
>>> + ; Save current value of encryption mask
>>> + mov ebx, edx
>>> +
>>> + ; Check if SEV-ES is enabled
>>> + ; MSR_0xC0010131 - Bit 1 (SEV-ES enabled)
>>> + mov ecx, 0xc0010131
>>> + rdmsr
>>> + and eax, 2
>>> +
>>> + ; Restore encryption mask
>>> + mov edx, ebx
>>> +
>>> +NoSevEs:
>>> + OneTimeCallRet CheckSevEsFeature
>>> +
>>> ;
>>> ; Modified: EAX, EBX, ECX, EDX
>>> ;
>>> @@ -139,6 +175,40 @@ pageTableEntriesLoop:
>>> mov [(ecx * 8 + PT_ADDR (0x2000 - 8)) + 4], edx
>>> loop pageTableEntriesLoop
>>> + OneTimeCall CheckSevEsFeature
>>> + test eax, eax
>>> + jz SetCr3
>>> +
>>> + ;
>>> + ; The initial GHCB will live at GHCB_BASE and needs to be
>>> un-encrypted.
>>> + ; This requires the 2MB page for this range be broken down into
>>> 512 4KB
>>> + ; pages. All will be marked encrypted, except for the GHCB.
>>> + ;
>>> + mov ecx, (GHCB_BASE >> 21)
>>> + mov eax, GHCB_PT_ADDR + PAGE_PDP_ATTR
>>> + mov [ecx * 8 + PT_ADDR (0x2000)], eax
>>> +
>>> + ;
>>> + ; Page Table Entries (512 * 4KB entries => 2MB)
>>> + ;
>>> + mov ecx, 512
>>> +pageTableEntries4kLoop:
>>> + mov eax, ecx
>>> + dec eax
>>> + shl eax, 12
>>> + add eax, GHCB_BASE & 0xFFE0_0000
>>> + add eax, PAGE_4K_PDE_ATTR
>>> + mov [ecx * 8 + GHCB_PT_ADDR - 8], eax
>>> + mov [(ecx * 8 + GHCB_PT_ADDR - 8) + 4], edx
>>> + loop pageTableEntries4kLoop
>>> +
>>> + ;
>>> + ; Clear the encryption bit from the GHCB entry
>>> + ;
>>> + mov ecx, (GHCB_BASE & 0x1F_FFFF) >> 12
>>> + mov [ecx * 8 + GHCB_PT_ADDR + 4], strict dword 0
>>> +
>>
>> (1) Why did you remove "clearGhcbMemoryLoop" (in the v6->v7 transition)?
>
> I removed it because it actually wasn't clearing the GHCB at all. Since
> this occurred before the new page tables are loaded, the page is
> accessed encrypted. After loading the new page tables, the GHCB is now
> referenced unencrypted and so the "zeroed" page isn't actually zeroes
> anymore, it is cipher-text.
>
> Since the GHCB is always cleared on #VC, I dropped it.
>
>>
>> I think that's exactly the clearing loop (minimally for the CPU#0
>> per-CPU page) that I was just looking for in point (8) under
>> "OvmfPkg/VmgExitLib: Add support for DR7 Read/Write NAE events" (v8
>> 26/46).
>>
>> Hm... the v7 blurb says, "Ensure the per-CPU variable page remains
>> encrypted". OK, but that still doesn't explain why we don't clear it
>> (just for the guest to see).
>
> I'll add a loop to clear the GHCB page and the per-CPU page after
> establishing the new page tables.
>
>>
>> Also, if the patch was non-trivially modified in v7, then arguably my
>> R-b (given originally under "RFC PATCH v3 26/43") should have been
>> removed.
>>
>> Please re-instate "clearGhcbMemoryLoop" (and then keep the R-b).
>
> I'll actually drop your Reviewed-by: since I'll need to expand and move
> the loop to clear the memory area from the original location in order
> for the clearing of the pages to be correct.
Thank you, that works for me (both code-wise and process-wise).
Cheers,
Laszlo
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#60336): https://edk2.groups.io/g/devel/message/60336
Mute This Topic: https://groups.io/mt/74336585/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
© 2016 - 2026 Red Hat, Inc.