From: Tom Lendacky <thomas.lendacky@amd.com>
BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=2198
Allocate memory for the GHCB pages during SEV initialization for use
during Pei and Dxe phases. The GHCB page(s) must be shared pages, so
clear the encryption mask from the current page table entries. Upon
successful allocation, set the GHCB PCDs (PcdGhcbBase and PcdGhcbSize).
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
---
OvmfPkg/OvmfPkgIa32.dsc | 2 ++
OvmfPkg/OvmfPkgIa32X64.dsc | 2 ++
OvmfPkg/OvmfPkgX64.dsc | 2 ++
OvmfPkg/PlatformPei/PlatformPei.inf | 2 ++
OvmfPkg/PlatformPei/AmdSev.c | 36 ++++++++++++++++++++++++++++-
5 files changed, 43 insertions(+), 1 deletion(-)
diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc
index 0ce5c01722ef..4369cf6d55e5 100644
--- a/OvmfPkg/OvmfPkgIa32.dsc
+++ b/OvmfPkg/OvmfPkgIa32.dsc
@@ -560,6 +560,8 @@ [PcdsDynamicDefault]
# Set SEV-ES defaults
gEfiMdeModulePkgTokenSpaceGuid.PcdSevEsActive|0
+ gEfiMdeModulePkgTokenSpaceGuid.PcdGhcbBase|0
+ gEfiMdeModulePkgTokenSpaceGuid.PcdGhcbSize|0
!if $(SMM_REQUIRE) == TRUE
gUefiOvmfPkgTokenSpaceGuid.PcdQ35TsegMbytes|8
diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
index e7455e35a55d..a74f5028068e 100644
--- a/OvmfPkg/OvmfPkgIa32X64.dsc
+++ b/OvmfPkg/OvmfPkgIa32X64.dsc
@@ -572,6 +572,8 @@ [PcdsDynamicDefault]
# Set SEV-ES defaults
gEfiMdeModulePkgTokenSpaceGuid.PcdSevEsActive|0
+ gEfiMdeModulePkgTokenSpaceGuid.PcdGhcbBase|0
+ gEfiMdeModulePkgTokenSpaceGuid.PcdGhcbSize|0
!if $(SMM_REQUIRE) == TRUE
gUefiOvmfPkgTokenSpaceGuid.PcdQ35TsegMbytes|8
diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
index 0b8305cd10a2..fd714d386e75 100644
--- a/OvmfPkg/OvmfPkgX64.dsc
+++ b/OvmfPkg/OvmfPkgX64.dsc
@@ -571,6 +571,8 @@ [PcdsDynamicDefault]
# Set SEV-ES defaults
gEfiMdeModulePkgTokenSpaceGuid.PcdSevEsActive|0
+ gEfiMdeModulePkgTokenSpaceGuid.PcdGhcbBase|0
+ gEfiMdeModulePkgTokenSpaceGuid.PcdGhcbSize|0
!if $(SMM_REQUIRE) == TRUE
gUefiOvmfPkgTokenSpaceGuid.PcdQ35TsegMbytes|8
diff --git a/OvmfPkg/PlatformPei/PlatformPei.inf b/OvmfPkg/PlatformPei/PlatformPei.inf
index a9e424a6012a..62abc99f4622 100644
--- a/OvmfPkg/PlatformPei/PlatformPei.inf
+++ b/OvmfPkg/PlatformPei/PlatformPei.inf
@@ -105,6 +105,8 @@ [Pcd]
gUefiCpuPkgTokenSpaceGuid.PcdCpuApInitTimeOutInMicroSeconds
gUefiCpuPkgTokenSpaceGuid.PcdCpuApStackSize
gEfiMdeModulePkgTokenSpaceGuid.PcdSevEsActive
+ gEfiMdeModulePkgTokenSpaceGuid.PcdGhcbBase
+ gEfiMdeModulePkgTokenSpaceGuid.PcdGhcbSize
[FixedPcd]
gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress
diff --git a/OvmfPkg/PlatformPei/AmdSev.c b/OvmfPkg/PlatformPei/AmdSev.c
index 7ae2f26a2ba7..30c0e4af7252 100644
--- a/OvmfPkg/PlatformPei/AmdSev.c
+++ b/OvmfPkg/PlatformPei/AmdSev.c
@@ -16,6 +16,9 @@
#include <PiPei.h>
#include <Register/Amd/Cpuid.h>
#include <Register/Cpuid.h>
+#include <Register/Amd/Msr.h>
+#include <Library/BaseMemoryLib.h>
+#include <Library/MemoryAllocationLib.h>
#include "Platform.h"
@@ -30,7 +33,10 @@ AmdSevEsInitialize (
VOID
)
{
- RETURN_STATUS PcdStatus;
+ VOID *GhcbBase;
+ PHYSICAL_ADDRESS GhcbBasePa;
+ UINTN GhcbPageCount;
+ RETURN_STATUS PcdStatus, DecryptStatus;
if (!MemEncryptSevEsIsEnabled ()) {
return;
@@ -38,6 +44,34 @@ AmdSevEsInitialize (
PcdStatus = PcdSetBoolS (PcdSevEsActive, 1);
ASSERT_RETURN_ERROR (PcdStatus);
+
+ //
+ // Allocate GHCB pages.
+ //
+ GhcbPageCount = mMaxCpuCount;
+ GhcbBase = AllocatePages (GhcbPageCount);
+ ASSERT (GhcbBase);
+
+ GhcbBasePa = (PHYSICAL_ADDRESS)(UINTN) GhcbBase;
+
+ DecryptStatus = MemEncryptSevClearPageEncMask (
+ 0,
+ GhcbBasePa,
+ GhcbPageCount,
+ TRUE
+ );
+ ASSERT_RETURN_ERROR (DecryptStatus);
+
+ SetMem (GhcbBase, GhcbPageCount * SIZE_4KB, 0);
+
+ PcdStatus = PcdSet64S (PcdGhcbBase, (UINT64)GhcbBasePa);
+ ASSERT_RETURN_ERROR (PcdStatus);
+ PcdStatus = PcdSet64S (PcdGhcbSize, (UINT64)EFI_PAGES_TO_SIZE (GhcbPageCount));
+ ASSERT_RETURN_ERROR (PcdStatus);
+
+ DEBUG ((DEBUG_INFO, "SEV-ES is enabled, %u GHCB pages allocated starting at 0x%lx\n", GhcbPageCount, GhcbBase));
+
+ AsmWriteMsr64 (MSR_SEV_ES_GHCB, (UINT64)GhcbBasePa);
}
/**
--
2.17.1
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#47642): https://edk2.groups.io/g/devel/message/47642
Mute This Topic: https://groups.io/mt/34203543/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Hi Tom,
On 09/19/19 21:52, Lendacky, Thomas wrote:
> From: Tom Lendacky <thomas.lendacky@amd.com>
>
> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=2198
>
> Allocate memory for the GHCB pages during SEV initialization for use
> during Pei and Dxe phases. The GHCB page(s) must be shared pages, so
> clear the encryption mask from the current page table entries. Upon
> successful allocation, set the GHCB PCDs (PcdGhcbBase and PcdGhcbSize).
skimming this patch and the next two ones for OvmfPkg (#10, #11), I'm a
bit lost. I'm missing a parallel between the "early X64 page tables" and
the GHCB-related pages.
The former are set up (in X64 OVMF) in SEC, and are used throughout PEI
until the DXE IPL builds new ones for the DXE phase. The latter also
*seemed* to be set up in SEC, and I thought they'd be used throughout
PEI -- I assumed the next place we'd need to massage GHCB pages would be
similarly in the DXE IPL, or thereabouts.
However, in this patch, we seem to allocate new pages for GHCB, and the
commit message implies they are supposed to be used during PEI. That
diverges from how long the "early X64 page tables" are used.
I guess this difference could be justified, especially because we do MP
stuff in PEI. (And we need separate GHCB stuff per VCPU -- in SEC we
only consider the BSP.)
But then, the question becomes: what exactly do we need the GHCB page
allocated in SEC for? From the blurb, it seems that the GHCB allows the
guest to selectively (actively) share information with the hypervisor --
such as (parts of?) the register file, which the hypervisor cannot
directly access, for a SEV-ES guest.
But, we never seem to place such information at PcdOvmfSecGhcbBase (aka
GHCB_BASE) in SEC. We program the GHCB's base address, and then we clear
the GHCB, but that seems to be it. Do we write anything non-zero to that
block, ever?
Thanks
Laszlo
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
> ---
> OvmfPkg/OvmfPkgIa32.dsc | 2 ++
> OvmfPkg/OvmfPkgIa32X64.dsc | 2 ++
> OvmfPkg/OvmfPkgX64.dsc | 2 ++
> OvmfPkg/PlatformPei/PlatformPei.inf | 2 ++
> OvmfPkg/PlatformPei/AmdSev.c | 36 ++++++++++++++++++++++++++++-
> 5 files changed, 43 insertions(+), 1 deletion(-)
>
> diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc
> index 0ce5c01722ef..4369cf6d55e5 100644
> --- a/OvmfPkg/OvmfPkgIa32.dsc
> +++ b/OvmfPkg/OvmfPkgIa32.dsc
> @@ -560,6 +560,8 @@ [PcdsDynamicDefault]
>
> # Set SEV-ES defaults
> gEfiMdeModulePkgTokenSpaceGuid.PcdSevEsActive|0
> + gEfiMdeModulePkgTokenSpaceGuid.PcdGhcbBase|0
> + gEfiMdeModulePkgTokenSpaceGuid.PcdGhcbSize|0
>
> !if $(SMM_REQUIRE) == TRUE
> gUefiOvmfPkgTokenSpaceGuid.PcdQ35TsegMbytes|8
> diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
> index e7455e35a55d..a74f5028068e 100644
> --- a/OvmfPkg/OvmfPkgIa32X64.dsc
> +++ b/OvmfPkg/OvmfPkgIa32X64.dsc
> @@ -572,6 +572,8 @@ [PcdsDynamicDefault]
>
> # Set SEV-ES defaults
> gEfiMdeModulePkgTokenSpaceGuid.PcdSevEsActive|0
> + gEfiMdeModulePkgTokenSpaceGuid.PcdGhcbBase|0
> + gEfiMdeModulePkgTokenSpaceGuid.PcdGhcbSize|0
>
> !if $(SMM_REQUIRE) == TRUE
> gUefiOvmfPkgTokenSpaceGuid.PcdQ35TsegMbytes|8
> diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
> index 0b8305cd10a2..fd714d386e75 100644
> --- a/OvmfPkg/OvmfPkgX64.dsc
> +++ b/OvmfPkg/OvmfPkgX64.dsc
> @@ -571,6 +571,8 @@ [PcdsDynamicDefault]
>
> # Set SEV-ES defaults
> gEfiMdeModulePkgTokenSpaceGuid.PcdSevEsActive|0
> + gEfiMdeModulePkgTokenSpaceGuid.PcdGhcbBase|0
> + gEfiMdeModulePkgTokenSpaceGuid.PcdGhcbSize|0
>
> !if $(SMM_REQUIRE) == TRUE
> gUefiOvmfPkgTokenSpaceGuid.PcdQ35TsegMbytes|8
> diff --git a/OvmfPkg/PlatformPei/PlatformPei.inf b/OvmfPkg/PlatformPei/PlatformPei.inf
> index a9e424a6012a..62abc99f4622 100644
> --- a/OvmfPkg/PlatformPei/PlatformPei.inf
> +++ b/OvmfPkg/PlatformPei/PlatformPei.inf
> @@ -105,6 +105,8 @@ [Pcd]
> gUefiCpuPkgTokenSpaceGuid.PcdCpuApInitTimeOutInMicroSeconds
> gUefiCpuPkgTokenSpaceGuid.PcdCpuApStackSize
> gEfiMdeModulePkgTokenSpaceGuid.PcdSevEsActive
> + gEfiMdeModulePkgTokenSpaceGuid.PcdGhcbBase
> + gEfiMdeModulePkgTokenSpaceGuid.PcdGhcbSize
>
> [FixedPcd]
> gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress
> diff --git a/OvmfPkg/PlatformPei/AmdSev.c b/OvmfPkg/PlatformPei/AmdSev.c
> index 7ae2f26a2ba7..30c0e4af7252 100644
> --- a/OvmfPkg/PlatformPei/AmdSev.c
> +++ b/OvmfPkg/PlatformPei/AmdSev.c
> @@ -16,6 +16,9 @@
> #include <PiPei.h>
> #include <Register/Amd/Cpuid.h>
> #include <Register/Cpuid.h>
> +#include <Register/Amd/Msr.h>
> +#include <Library/BaseMemoryLib.h>
> +#include <Library/MemoryAllocationLib.h>
>
> #include "Platform.h"
>
> @@ -30,7 +33,10 @@ AmdSevEsInitialize (
> VOID
> )
> {
> - RETURN_STATUS PcdStatus;
> + VOID *GhcbBase;
> + PHYSICAL_ADDRESS GhcbBasePa;
> + UINTN GhcbPageCount;
> + RETURN_STATUS PcdStatus, DecryptStatus;
>
> if (!MemEncryptSevEsIsEnabled ()) {
> return;
> @@ -38,6 +44,34 @@ AmdSevEsInitialize (
>
> PcdStatus = PcdSetBoolS (PcdSevEsActive, 1);
> ASSERT_RETURN_ERROR (PcdStatus);
> +
> + //
> + // Allocate GHCB pages.
> + //
> + GhcbPageCount = mMaxCpuCount;
> + GhcbBase = AllocatePages (GhcbPageCount);
> + ASSERT (GhcbBase);
> +
> + GhcbBasePa = (PHYSICAL_ADDRESS)(UINTN) GhcbBase;
> +
> + DecryptStatus = MemEncryptSevClearPageEncMask (
> + 0,
> + GhcbBasePa,
> + GhcbPageCount,
> + TRUE
> + );
> + ASSERT_RETURN_ERROR (DecryptStatus);
> +
> + SetMem (GhcbBase, GhcbPageCount * SIZE_4KB, 0);
> +
> + PcdStatus = PcdSet64S (PcdGhcbBase, (UINT64)GhcbBasePa);
> + ASSERT_RETURN_ERROR (PcdStatus);
> + PcdStatus = PcdSet64S (PcdGhcbSize, (UINT64)EFI_PAGES_TO_SIZE (GhcbPageCount));
> + ASSERT_RETURN_ERROR (PcdStatus);
> +
> + DEBUG ((DEBUG_INFO, "SEV-ES is enabled, %u GHCB pages allocated starting at 0x%lx\n", GhcbPageCount, GhcbBase));
> +
> + AsmWriteMsr64 (MSR_SEV_ES_GHCB, (UINT64)GhcbBasePa);
> }
>
> /**
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#48080): https://edk2.groups.io/g/devel/message/48080
Mute This Topic: https://groups.io/mt/34203543/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
On 9/26/19 3:00 AM, Laszlo Ersek wrote:
> Hi Tom,
>
> On 09/19/19 21:52, Lendacky, Thomas wrote:
>> From: Tom Lendacky <thomas.lendacky@amd.com>
>>
>> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=2198
>>
>> Allocate memory for the GHCB pages during SEV initialization for use
>> during Pei and Dxe phases. The GHCB page(s) must be shared pages, so
>> clear the encryption mask from the current page table entries. Upon
>> successful allocation, set the GHCB PCDs (PcdGhcbBase and PcdGhcbSize).
>
> skimming this patch and the next two ones for OvmfPkg (#10, #11), I'm a
> bit lost. I'm missing a parallel between the "early X64 page tables" and
> the GHCB-related pages.
>
> The former are set up (in X64 OVMF) in SEC, and are used throughout PEI
> until the DXE IPL builds new ones for the DXE phase. The latter also
> *seemed* to be set up in SEC, and I thought they'd be used throughout
> PEI -- I assumed the next place we'd need to massage GHCB pages would be
> similarly in the DXE IPL, or thereabouts.
>
> However, in this patch, we seem to allocate new pages for GHCB, and the
> commit message implies they are supposed to be used during PEI. That
> diverges from how long the "early X64 page tables" are used.
At this stage, we need a GHCB page for every (v)CPU. So a new allocation
is done and then the pages are marked unencrypted. Once the new GHCB
pages are allocated, the original GHCB page for SEC is no longer needed
because the new allocation replaces it in the BSP. But the early page
table is still required in order to access all of the memory from the 2MB
range (0x800000 to 0x9fffff).
>
> I guess this difference could be justified, especially because we do MP
> stuff in PEI. (And we need separate GHCB stuff per VCPU -- in SEC we
> only consider the BSP.)
>
> But then, the question becomes: what exactly do we need the GHCB page
> allocated in SEC for? From the blurb, it seems that the GHCB allows the
There are lots of different ways to cause a #VC. A #VC is generated for
debug statements that use port I/O, MMIO, intercept-able MSR accesses,
CPUID instructions, WBINVD instructions, etc. Many of these things happen
during SEC. With the debug serial output enabled, over 8,000 #VC
exceptions occur before allocating the new GHCB pages in
AmdSevEsInitialize().
> guest to selectively (actively) share information with the hypervisor --
> such as (parts of?) the register file, which the hypervisor cannot
> directly access, for a SEV-ES guest.
>
> But, we never seem to place such information at PcdOvmfSecGhcbBase (aka
> GHCB_BASE) in SEC. We program the GHCB's base address, and then we clear
> the GHCB, but that seems to be it. Do we write anything non-zero to that
> block, ever?
Yes, that happens in the SEC exception handler. When the #VC occurs, the
GHCB information is filled in and a VMGEXIT instruction is issued to exit
to the hypervisor. The hypervisor then accesses the GHCB in order to
perform the requested function.
Thanks,
Tom
>
> Thanks
> Laszlo
>
>> Cc: Jordan Justen <jordan.l.justen@intel.com>
>> Cc: Laszlo Ersek <lersek@redhat.com>
>> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
>> ---
>> OvmfPkg/OvmfPkgIa32.dsc | 2 ++
>> OvmfPkg/OvmfPkgIa32X64.dsc | 2 ++
>> OvmfPkg/OvmfPkgX64.dsc | 2 ++
>> OvmfPkg/PlatformPei/PlatformPei.inf | 2 ++
>> OvmfPkg/PlatformPei/AmdSev.c | 36 ++++++++++++++++++++++++++++-
>> 5 files changed, 43 insertions(+), 1 deletion(-)
>>
>> diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc
>> index 0ce5c01722ef..4369cf6d55e5 100644
>> --- a/OvmfPkg/OvmfPkgIa32.dsc
>> +++ b/OvmfPkg/OvmfPkgIa32.dsc
>> @@ -560,6 +560,8 @@ [PcdsDynamicDefault]
>>
>> # Set SEV-ES defaults
>> gEfiMdeModulePkgTokenSpaceGuid.PcdSevEsActive|0
>> + gEfiMdeModulePkgTokenSpaceGuid.PcdGhcbBase|0
>> + gEfiMdeModulePkgTokenSpaceGuid.PcdGhcbSize|0
>>
>> !if $(SMM_REQUIRE) == TRUE
>> gUefiOvmfPkgTokenSpaceGuid.PcdQ35TsegMbytes|8
>> diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
>> index e7455e35a55d..a74f5028068e 100644
>> --- a/OvmfPkg/OvmfPkgIa32X64.dsc
>> +++ b/OvmfPkg/OvmfPkgIa32X64.dsc
>> @@ -572,6 +572,8 @@ [PcdsDynamicDefault]
>>
>> # Set SEV-ES defaults
>> gEfiMdeModulePkgTokenSpaceGuid.PcdSevEsActive|0
>> + gEfiMdeModulePkgTokenSpaceGuid.PcdGhcbBase|0
>> + gEfiMdeModulePkgTokenSpaceGuid.PcdGhcbSize|0
>>
>> !if $(SMM_REQUIRE) == TRUE
>> gUefiOvmfPkgTokenSpaceGuid.PcdQ35TsegMbytes|8
>> diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
>> index 0b8305cd10a2..fd714d386e75 100644
>> --- a/OvmfPkg/OvmfPkgX64.dsc
>> +++ b/OvmfPkg/OvmfPkgX64.dsc
>> @@ -571,6 +571,8 @@ [PcdsDynamicDefault]
>>
>> # Set SEV-ES defaults
>> gEfiMdeModulePkgTokenSpaceGuid.PcdSevEsActive|0
>> + gEfiMdeModulePkgTokenSpaceGuid.PcdGhcbBase|0
>> + gEfiMdeModulePkgTokenSpaceGuid.PcdGhcbSize|0
>>
>> !if $(SMM_REQUIRE) == TRUE
>> gUefiOvmfPkgTokenSpaceGuid.PcdQ35TsegMbytes|8
>> diff --git a/OvmfPkg/PlatformPei/PlatformPei.inf b/OvmfPkg/PlatformPei/PlatformPei.inf
>> index a9e424a6012a..62abc99f4622 100644
>> --- a/OvmfPkg/PlatformPei/PlatformPei.inf
>> +++ b/OvmfPkg/PlatformPei/PlatformPei.inf
>> @@ -105,6 +105,8 @@ [Pcd]
>> gUefiCpuPkgTokenSpaceGuid.PcdCpuApInitTimeOutInMicroSeconds
>> gUefiCpuPkgTokenSpaceGuid.PcdCpuApStackSize
>> gEfiMdeModulePkgTokenSpaceGuid.PcdSevEsActive
>> + gEfiMdeModulePkgTokenSpaceGuid.PcdGhcbBase
>> + gEfiMdeModulePkgTokenSpaceGuid.PcdGhcbSize
>>
>> [FixedPcd]
>> gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress
>> diff --git a/OvmfPkg/PlatformPei/AmdSev.c b/OvmfPkg/PlatformPei/AmdSev.c
>> index 7ae2f26a2ba7..30c0e4af7252 100644
>> --- a/OvmfPkg/PlatformPei/AmdSev.c
>> +++ b/OvmfPkg/PlatformPei/AmdSev.c
>> @@ -16,6 +16,9 @@
>> #include <PiPei.h>
>> #include <Register/Amd/Cpuid.h>
>> #include <Register/Cpuid.h>
>> +#include <Register/Amd/Msr.h>
>> +#include <Library/BaseMemoryLib.h>
>> +#include <Library/MemoryAllocationLib.h>
>>
>> #include "Platform.h"
>>
>> @@ -30,7 +33,10 @@ AmdSevEsInitialize (
>> VOID
>> )
>> {
>> - RETURN_STATUS PcdStatus;
>> + VOID *GhcbBase;
>> + PHYSICAL_ADDRESS GhcbBasePa;
>> + UINTN GhcbPageCount;
>> + RETURN_STATUS PcdStatus, DecryptStatus;
>>
>> if (!MemEncryptSevEsIsEnabled ()) {
>> return;
>> @@ -38,6 +44,34 @@ AmdSevEsInitialize (
>>
>> PcdStatus = PcdSetBoolS (PcdSevEsActive, 1);
>> ASSERT_RETURN_ERROR (PcdStatus);
>> +
>> + //
>> + // Allocate GHCB pages.
>> + //
>> + GhcbPageCount = mMaxCpuCount;
>> + GhcbBase = AllocatePages (GhcbPageCount);
>> + ASSERT (GhcbBase);
>> +
>> + GhcbBasePa = (PHYSICAL_ADDRESS)(UINTN) GhcbBase;
>> +
>> + DecryptStatus = MemEncryptSevClearPageEncMask (
>> + 0,
>> + GhcbBasePa,
>> + GhcbPageCount,
>> + TRUE
>> + );
>> + ASSERT_RETURN_ERROR (DecryptStatus);
>> +
>> + SetMem (GhcbBase, GhcbPageCount * SIZE_4KB, 0);
>> +
>> + PcdStatus = PcdSet64S (PcdGhcbBase, (UINT64)GhcbBasePa);
>> + ASSERT_RETURN_ERROR (PcdStatus);
>> + PcdStatus = PcdSet64S (PcdGhcbSize, (UINT64)EFI_PAGES_TO_SIZE (GhcbPageCount));
>> + ASSERT_RETURN_ERROR (PcdStatus);
>> +
>> + DEBUG ((DEBUG_INFO, "SEV-ES is enabled, %u GHCB pages allocated starting at 0x%lx\n", GhcbPageCount, GhcbBase));
>> +
>> + AsmWriteMsr64 (MSR_SEV_ES_GHCB, (UINT64)GhcbBasePa);
>> }
>>
>> /**
>>
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#48111): https://edk2.groups.io/g/devel/message/48111
Mute This Topic: https://groups.io/mt/34203543/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
On 09/26/19 16:00, Lendacky, Thomas wrote: > On 9/26/19 3:00 AM, Laszlo Ersek wrote: >> Hi Tom, >> >> On 09/19/19 21:52, Lendacky, Thomas wrote: >>> From: Tom Lendacky <thomas.lendacky@amd.com> >>> >>> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=2198 >>> >>> Allocate memory for the GHCB pages during SEV initialization for use >>> during Pei and Dxe phases. The GHCB page(s) must be shared pages, so >>> clear the encryption mask from the current page table entries. Upon >>> successful allocation, set the GHCB PCDs (PcdGhcbBase and PcdGhcbSize). >> >> skimming this patch and the next two ones for OvmfPkg (#10, #11), I'm a >> bit lost. I'm missing a parallel between the "early X64 page tables" and >> the GHCB-related pages. >> >> The former are set up (in X64 OVMF) in SEC, and are used throughout PEI >> until the DXE IPL builds new ones for the DXE phase. The latter also >> *seemed* to be set up in SEC, and I thought they'd be used throughout >> PEI -- I assumed the next place we'd need to massage GHCB pages would be >> similarly in the DXE IPL, or thereabouts. >> >> However, in this patch, we seem to allocate new pages for GHCB, and the >> commit message implies they are supposed to be used during PEI. That >> diverges from how long the "early X64 page tables" are used. > > At this stage, we need a GHCB page for every (v)CPU. So a new allocation > is done and then the pages are marked unencrypted. Once the new GHCB > pages are allocated, the original GHCB page for SEC is no longer needed > because the new allocation replaces it in the BSP. But the early page > table is still required in order to access all of the memory from the 2MB > range (0x800000 to 0x9fffff). > >> >> I guess this difference could be justified, especially because we do MP >> stuff in PEI. (And we need separate GHCB stuff per VCPU -- in SEC we >> only consider the BSP.) >> >> But then, the question becomes: what exactly do we need the GHCB page >> allocated in SEC for? From the blurb, it seems that the GHCB allows the > > There are lots of different ways to cause a #VC. A #VC is generated for > debug statements that use port I/O, MMIO, intercept-able MSR accesses, > CPUID instructions, WBINVD instructions, etc. Many of these things happen > during SEC. With the debug serial output enabled, over 8,000 #VC > exceptions occur before allocating the new GHCB pages in > AmdSevEsInitialize(). > >> guest to selectively (actively) share information with the hypervisor -- >> such as (parts of?) the register file, which the hypervisor cannot >> directly access, for a SEV-ES guest. >> >> But, we never seem to place such information at PcdOvmfSecGhcbBase (aka >> GHCB_BASE) in SEC. We program the GHCB's base address, and then we clear >> the GHCB, but that seems to be it. Do we write anything non-zero to that >> block, ever? > > Yes, that happens in the SEC exception handler. When the #VC occurs, the > GHCB information is filled in and a VMGEXIT instruction is issued to exit > to the hypervisor. The hypervisor then accesses the GHCB in order to > perform the requested function. Thanks, very helpful. Can you please work this info into the relevant commit messages? (No need to repost just because of that, of course.) Thanks! Laszlo -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#48288): https://edk2.groups.io/g/devel/message/48288 Mute This Topic: https://groups.io/mt/34203543/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
On 9/30/19 1:52 PM, Laszlo Ersek via Groups.Io wrote: > On 09/26/19 16:00, Lendacky, Thomas wrote: >> On 9/26/19 3:00 AM, Laszlo Ersek wrote: >>> Hi Tom, >>> >>> On 09/19/19 21:52, Lendacky, Thomas wrote: >>>> From: Tom Lendacky <thomas.lendacky@amd.com> >>>> >>>> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=2198 >>>> >>>> Allocate memory for the GHCB pages during SEV initialization for use >>>> during Pei and Dxe phases. The GHCB page(s) must be shared pages, so >>>> clear the encryption mask from the current page table entries. Upon >>>> successful allocation, set the GHCB PCDs (PcdGhcbBase and PcdGhcbSize). >>> >>> skimming this patch and the next two ones for OvmfPkg (#10, #11), I'm a >>> bit lost. I'm missing a parallel between the "early X64 page tables" and >>> the GHCB-related pages. >>> >>> The former are set up (in X64 OVMF) in SEC, and are used throughout PEI >>> until the DXE IPL builds new ones for the DXE phase. The latter also >>> *seemed* to be set up in SEC, and I thought they'd be used throughout >>> PEI -- I assumed the next place we'd need to massage GHCB pages would be >>> similarly in the DXE IPL, or thereabouts. >>> >>> However, in this patch, we seem to allocate new pages for GHCB, and the >>> commit message implies they are supposed to be used during PEI. That >>> diverges from how long the "early X64 page tables" are used. >> >> At this stage, we need a GHCB page for every (v)CPU. So a new allocation >> is done and then the pages are marked unencrypted. Once the new GHCB >> pages are allocated, the original GHCB page for SEC is no longer needed >> because the new allocation replaces it in the BSP. But the early page >> table is still required in order to access all of the memory from the 2MB >> range (0x800000 to 0x9fffff). >> >>> >>> I guess this difference could be justified, especially because we do MP >>> stuff in PEI. (And we need separate GHCB stuff per VCPU -- in SEC we >>> only consider the BSP.) >>> >>> But then, the question becomes: what exactly do we need the GHCB page >>> allocated in SEC for? From the blurb, it seems that the GHCB allows the >> >> There are lots of different ways to cause a #VC. A #VC is generated for >> debug statements that use port I/O, MMIO, intercept-able MSR accesses, >> CPUID instructions, WBINVD instructions, etc. Many of these things happen >> during SEC. With the debug serial output enabled, over 8,000 #VC >> exceptions occur before allocating the new GHCB pages in >> AmdSevEsInitialize(). >> >>> guest to selectively (actively) share information with the hypervisor -- >>> such as (parts of?) the register file, which the hypervisor cannot >>> directly access, for a SEV-ES guest. >>> >>> But, we never seem to place such information at PcdOvmfSecGhcbBase (aka >>> GHCB_BASE) in SEC. We program the GHCB's base address, and then we clear >>> the GHCB, but that seems to be it. Do we write anything non-zero to that >>> block, ever? >> >> Yes, that happens in the SEC exception handler. When the #VC occurs, the >> GHCB information is filled in and a VMGEXIT instruction is issued to exit >> to the hypervisor. The hypervisor then accesses the GHCB in order to >> perform the requested function. > > Thanks, very helpful. > > Can you please work this info into the relevant commit messages? (No > need to repost just because of that, of course.) Will do. Thanks, Tom > > Thanks! > Laszlo > > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#48294): https://edk2.groups.io/g/devel/message/48294 Mute This Topic: https://groups.io/mt/34203543/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
On 09/26/19 16:00, Lendacky, Thomas wrote: > On 9/26/19 3:00 AM, Laszlo Ersek wrote: >> Hi Tom, >> >> On 09/19/19 21:52, Lendacky, Thomas wrote: >>> From: Tom Lendacky <thomas.lendacky@amd.com> >>> >>> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=2198 >>> >>> Allocate memory for the GHCB pages during SEV initialization for use >>> during Pei and Dxe phases. The GHCB page(s) must be shared pages, so >>> clear the encryption mask from the current page table entries. Upon >>> successful allocation, set the GHCB PCDs (PcdGhcbBase and PcdGhcbSize). >> >> skimming this patch and the next two ones for OvmfPkg (#10, #11), I'm a >> bit lost. I'm missing a parallel between the "early X64 page tables" and >> the GHCB-related pages. >> >> The former are set up (in X64 OVMF) in SEC, and are used throughout PEI >> until the DXE IPL builds new ones for the DXE phase. The latter also >> *seemed* to be set up in SEC, and I thought they'd be used throughout >> PEI -- I assumed the next place we'd need to massage GHCB pages would be >> similarly in the DXE IPL, or thereabouts. >> >> However, in this patch, we seem to allocate new pages for GHCB, and the >> commit message implies they are supposed to be used during PEI. That >> diverges from how long the "early X64 page tables" are used. > > At this stage, we need a GHCB page for every (v)CPU. So a new allocation > is done and then the pages are marked unencrypted. Once the new GHCB > pages are allocated, the original GHCB page for SEC is no longer needed > because the new allocation replaces it in the BSP. But the early page > table is still required in order to access all of the memory from the 2MB > range (0x800000 to 0x9fffff). > >> >> I guess this difference could be justified, especially because we do MP >> stuff in PEI. (And we need separate GHCB stuff per VCPU -- in SEC we >> only consider the BSP.) >> >> But then, the question becomes: what exactly do we need the GHCB page >> allocated in SEC for? From the blurb, it seems that the GHCB allows the > > There are lots of different ways to cause a #VC. A #VC is generated for > debug statements that use port I/O, MMIO, intercept-able MSR accesses, > CPUID instructions, WBINVD instructions, etc. Many of these things happen > during SEC. With the debug serial output enabled, over 8,000 #VC > exceptions occur before allocating the new GHCB pages in > AmdSevEsInitialize(). > >> guest to selectively (actively) share information with the hypervisor -- >> such as (parts of?) the register file, which the hypervisor cannot >> directly access, for a SEV-ES guest. >> >> But, we never seem to place such information at PcdOvmfSecGhcbBase (aka >> GHCB_BASE) in SEC. We program the GHCB's base address, and then we clear >> the GHCB, but that seems to be it. Do we write anything non-zero to that >> block, ever? > > Yes, that happens in the SEC exception handler. When the #VC occurs, the > GHCB information is filled in and a VMGEXIT instruction is issued to exit > to the hypervisor. The hypervisor then accesses the GHCB in order to > perform the requested function. (Sorry about sending this in a separate email.) So... Where is the #VC handler that implements this logic in SEC? ... Ah wait, now I understand why I got confused. The patch series thus far has modified SEC code that is specific to OVMF, and then it advances to OvmfPkg/PlatformPei. I thought we were done with SEC changes in OVMF, and I didn't understand where the #VC handler you refer to above was. But, looking a bit ahead in the series, I see the exception handler being built gradually, in UefiCpuPkg, and (I guess) also enabled in OVMF's SEC somewhere / sometime. I wonder what the best ordering would be, for the patches in the series. The middle seems to be alternating between UefiCpuPkg and OvmfPkg. That's quite unusual. I don't have a clear understanding of the feature yet, so I can't authoritatively suggest a "better" structure. As a first guess, I would suggest constructing the building blocks in MdePkg and UefiCpuPkg (libraries, primarily), then utilizing them in MdeModulePkg (core drivers), then adding platform code in OvmfPkg. Also, I would suggest copious cross-references between the patches (identified by subjects). I hope this is not too annoying, just trying to ask for crutches :) I'll try to continue the review of the series this week. Thanks! Laszlo -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#48289): https://edk2.groups.io/g/devel/message/48289 Mute This Topic: https://groups.io/mt/34203543/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
On 9/30/19 2:12 PM, Laszlo Ersek wrote: > On 09/26/19 16:00, Lendacky, Thomas wrote: >> On 9/26/19 3:00 AM, Laszlo Ersek wrote: >>> Hi Tom, >>> >>> On 09/19/19 21:52, Lendacky, Thomas wrote: >>>> From: Tom Lendacky <thomas.lendacky@amd.com> >>>> >>>> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=2198 >>>> >>>> Allocate memory for the GHCB pages during SEV initialization for use >>>> during Pei and Dxe phases. The GHCB page(s) must be shared pages, so >>>> clear the encryption mask from the current page table entries. Upon >>>> successful allocation, set the GHCB PCDs (PcdGhcbBase and PcdGhcbSize). >>> >>> skimming this patch and the next two ones for OvmfPkg (#10, #11), I'm a >>> bit lost. I'm missing a parallel between the "early X64 page tables" and >>> the GHCB-related pages. >>> >>> The former are set up (in X64 OVMF) in SEC, and are used throughout PEI >>> until the DXE IPL builds new ones for the DXE phase. The latter also >>> *seemed* to be set up in SEC, and I thought they'd be used throughout >>> PEI -- I assumed the next place we'd need to massage GHCB pages would be >>> similarly in the DXE IPL, or thereabouts. >>> >>> However, in this patch, we seem to allocate new pages for GHCB, and the >>> commit message implies they are supposed to be used during PEI. That >>> diverges from how long the "early X64 page tables" are used. >> >> At this stage, we need a GHCB page for every (v)CPU. So a new allocation >> is done and then the pages are marked unencrypted. Once the new GHCB >> pages are allocated, the original GHCB page for SEC is no longer needed >> because the new allocation replaces it in the BSP. But the early page >> table is still required in order to access all of the memory from the 2MB >> range (0x800000 to 0x9fffff). >> >>> >>> I guess this difference could be justified, especially because we do MP >>> stuff in PEI. (And we need separate GHCB stuff per VCPU -- in SEC we >>> only consider the BSP.) >>> >>> But then, the question becomes: what exactly do we need the GHCB page >>> allocated in SEC for? From the blurb, it seems that the GHCB allows the >> >> There are lots of different ways to cause a #VC. A #VC is generated for >> debug statements that use port I/O, MMIO, intercept-able MSR accesses, >> CPUID instructions, WBINVD instructions, etc. Many of these things happen >> during SEC. With the debug serial output enabled, over 8,000 #VC >> exceptions occur before allocating the new GHCB pages in >> AmdSevEsInitialize(). >> >>> guest to selectively (actively) share information with the hypervisor -- >>> such as (parts of?) the register file, which the hypervisor cannot >>> directly access, for a SEV-ES guest. >>> >>> But, we never seem to place such information at PcdOvmfSecGhcbBase (aka >>> GHCB_BASE) in SEC. We program the GHCB's base address, and then we clear >>> the GHCB, but that seems to be it. Do we write anything non-zero to that >>> block, ever? >> >> Yes, that happens in the SEC exception handler. When the #VC occurs, the >> GHCB information is filled in and a VMGEXIT instruction is issued to exit >> to the hypervisor. The hypervisor then accesses the GHCB in order to >> perform the requested function. > > (Sorry about sending this in a separate email.) > > So... Where is the #VC handler that implements this logic in SEC? > > ... Ah wait, now I understand why I got confused. The patch series thus > far has modified SEC code that is specific to OVMF, and then it advances > to OvmfPkg/PlatformPei. I thought we were done with SEC changes in OVMF, > and I didn't understand where the #VC handler you refer to above was. > > But, looking a bit ahead in the series, I see the exception handler > being built gradually, in UefiCpuPkg, and (I guess) also enabled in > OVMF's SEC somewhere / sometime. > > I wonder what the best ordering would be, for the patches in the series. > The middle seems to be alternating between UefiCpuPkg and OvmfPkg. > That's quite unusual. I don't have a clear understanding of the feature > yet, so I can't authoritatively suggest a "better" structure. As a first > guess, I would suggest constructing the building blocks in MdePkg and > UefiCpuPkg (libraries, primarily), then utilizing them in MdeModulePkg > (core drivers), then adding platform code in OvmfPkg. Also, I would > suggest copious cross-references between the patches (identified by > subjects). > > I hope this is not too annoying, just trying to ask for crutches :) No worries, I'll see what I can do in ordering the patches so they make more sense to a reviewer looking at the code for the first time. Thanks, Tom > > I'll try to continue the review of the series this week. > > Thanks! > Laszlo > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#48295): https://edk2.groups.io/g/devel/message/48295 Mute This Topic: https://groups.io/mt/34203543/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
After the discussion elsewhere in this patch thread, which related to
commit messages, and patch order in the series, I can make a few coding
style comments on the patch. (No change to functionality.)
On 09/19/19 21:52, Lendacky, Thomas wrote:
> From: Tom Lendacky <thomas.lendacky@amd.com>
>
> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=2198
>
> Allocate memory for the GHCB pages during SEV initialization for use
> during Pei and Dxe phases. The GHCB page(s) must be shared pages, so
> clear the encryption mask from the current page table entries. Upon
> successful allocation, set the GHCB PCDs (PcdGhcbBase and PcdGhcbSize).
>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
> ---
> OvmfPkg/OvmfPkgIa32.dsc | 2 ++
> OvmfPkg/OvmfPkgIa32X64.dsc | 2 ++
> OvmfPkg/OvmfPkgX64.dsc | 2 ++
> OvmfPkg/PlatformPei/PlatformPei.inf | 2 ++
> OvmfPkg/PlatformPei/AmdSev.c | 36 ++++++++++++++++++++++++++++-
> 5 files changed, 43 insertions(+), 1 deletion(-)
>
> diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc
> index 0ce5c01722ef..4369cf6d55e5 100644
> --- a/OvmfPkg/OvmfPkgIa32.dsc
> +++ b/OvmfPkg/OvmfPkgIa32.dsc
> @@ -560,6 +560,8 @@ [PcdsDynamicDefault]
>
> # Set SEV-ES defaults
> gEfiMdeModulePkgTokenSpaceGuid.PcdSevEsActive|0
> + gEfiMdeModulePkgTokenSpaceGuid.PcdGhcbBase|0
> + gEfiMdeModulePkgTokenSpaceGuid.PcdGhcbSize|0
>
> !if $(SMM_REQUIRE) == TRUE
> gUefiOvmfPkgTokenSpaceGuid.PcdQ35TsegMbytes|8
> diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
> index e7455e35a55d..a74f5028068e 100644
> --- a/OvmfPkg/OvmfPkgIa32X64.dsc
> +++ b/OvmfPkg/OvmfPkgIa32X64.dsc
> @@ -572,6 +572,8 @@ [PcdsDynamicDefault]
>
> # Set SEV-ES defaults
> gEfiMdeModulePkgTokenSpaceGuid.PcdSevEsActive|0
> + gEfiMdeModulePkgTokenSpaceGuid.PcdGhcbBase|0
> + gEfiMdeModulePkgTokenSpaceGuid.PcdGhcbSize|0
>
> !if $(SMM_REQUIRE) == TRUE
> gUefiOvmfPkgTokenSpaceGuid.PcdQ35TsegMbytes|8
> diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
> index 0b8305cd10a2..fd714d386e75 100644
> --- a/OvmfPkg/OvmfPkgX64.dsc
> +++ b/OvmfPkg/OvmfPkgX64.dsc
> @@ -571,6 +571,8 @@ [PcdsDynamicDefault]
>
> # Set SEV-ES defaults
> gEfiMdeModulePkgTokenSpaceGuid.PcdSevEsActive|0
> + gEfiMdeModulePkgTokenSpaceGuid.PcdGhcbBase|0
> + gEfiMdeModulePkgTokenSpaceGuid.PcdGhcbSize|0
>
> !if $(SMM_REQUIRE) == TRUE
> gUefiOvmfPkgTokenSpaceGuid.PcdQ35TsegMbytes|8
> diff --git a/OvmfPkg/PlatformPei/PlatformPei.inf b/OvmfPkg/PlatformPei/PlatformPei.inf
> index a9e424a6012a..62abc99f4622 100644
> --- a/OvmfPkg/PlatformPei/PlatformPei.inf
> +++ b/OvmfPkg/PlatformPei/PlatformPei.inf
> @@ -105,6 +105,8 @@ [Pcd]
> gUefiCpuPkgTokenSpaceGuid.PcdCpuApInitTimeOutInMicroSeconds
> gUefiCpuPkgTokenSpaceGuid.PcdCpuApStackSize
> gEfiMdeModulePkgTokenSpaceGuid.PcdSevEsActive
> + gEfiMdeModulePkgTokenSpaceGuid.PcdGhcbBase
> + gEfiMdeModulePkgTokenSpaceGuid.PcdGhcbSize
>
> [FixedPcd]
> gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress
(1) Once you move PcdSevEsActive near the other
gEfiMdeModulePkgTokenSpaceGuid PCDs, per
<http://mid.mail-archive.com/bd38da31-0985-2ffc-b60d-f867a0218ab2@redhat.com>,
please keep these grouped with it, too.
> diff --git a/OvmfPkg/PlatformPei/AmdSev.c b/OvmfPkg/PlatformPei/AmdSev.c
> index 7ae2f26a2ba7..30c0e4af7252 100644
> --- a/OvmfPkg/PlatformPei/AmdSev.c
> +++ b/OvmfPkg/PlatformPei/AmdSev.c
> @@ -16,6 +16,9 @@
> #include <PiPei.h>
> #include <Register/Amd/Cpuid.h>
> #include <Register/Cpuid.h>
> +#include <Register/Amd/Msr.h>
> +#include <Library/BaseMemoryLib.h>
> +#include <Library/MemoryAllocationLib.h>
(2) This #include list is currently sorted. Can you please keep it sorted?
>
> #include "Platform.h"
>
> @@ -30,7 +33,10 @@ AmdSevEsInitialize (
> VOID
> )
> {
> - RETURN_STATUS PcdStatus;
> + VOID *GhcbBase;
> + PHYSICAL_ADDRESS GhcbBasePa;
> + UINTN GhcbPageCount;
> + RETURN_STATUS PcdStatus, DecryptStatus;
>
> if (!MemEncryptSevEsIsEnabled ()) {
> return;
> @@ -38,6 +44,34 @@ AmdSevEsInitialize (
>
> PcdStatus = PcdSetBoolS (PcdSevEsActive, 1);
> ASSERT_RETURN_ERROR (PcdStatus);
> +
> + //
> + // Allocate GHCB pages.
> + //
> + GhcbPageCount = mMaxCpuCount;
> + GhcbBase = AllocatePages (GhcbPageCount);
Yes, AllocatePages() looks safe, per
<http://mid.mail-archive.com/4029f533-9f2a-ba71-2ba6-348187dc7684@redhat.com>.
> + ASSERT (GhcbBase);
(3) I don't really like using *only* an ASSERT for stopping, when we run
out of resources in a place like this (i.e. where there's no way to
recover, or to report the issue nicely). I'd suggest throwing in an
explicit check and a CpuDeadLoop(), in addition to the ASSERT. Anyway,
not really important, up to you.
(4) The expression in the ASSERT() should compare GhcbBase against NULL
explicitly, however. The edk2 coding style permits the implicit "!= 0"
comparison (in controlling expressions) for BOOLEANs only.
> +
> + GhcbBasePa = (PHYSICAL_ADDRESS)(UINTN) GhcbBase;
> +
> + DecryptStatus = MemEncryptSevClearPageEncMask (
> + 0,
> + GhcbBasePa,
> + GhcbPageCount,
> + TRUE
> + );
> + ASSERT_RETURN_ERROR (DecryptStatus);
> +
> + SetMem (GhcbBase, GhcbPageCount * SIZE_4KB, 0);
(5) The following would be more idiomatic:
ZeroMem (GhcbBase, EFI_PAGES_TO_SIZE (GhcbPageCount));
(like you write below already)
> +
> + PcdStatus = PcdSet64S (PcdGhcbBase, (UINT64)GhcbBasePa);
> + ASSERT_RETURN_ERROR (PcdStatus);
> + PcdStatus = PcdSet64S (PcdGhcbSize, (UINT64)EFI_PAGES_TO_SIZE (GhcbPageCount));
> + ASSERT_RETURN_ERROR (PcdStatus);
> +
> + DEBUG ((DEBUG_INFO, "SEV-ES is enabled, %u GHCB pages allocated starting at 0x%lx\n", GhcbPageCount, GhcbBase));
(6) This line is too long; please try to stick with <=80 chars per line.
(7) UINTN values (such as GhcbPageCount) should be converted to UINT64
explicitly, and then formatted with %lu.
(8) GhcbBase is a pointer-to-void; please either format it with %p, or
use GhcbBasePa. (We can rely on the latter being UINT64.)
DEBUG ((DEBUG_INFO,
"SEV-ES is enabled, %lu GHCB pages allocated starting at 0x%lx\n",
(UINT64)GhcbPageCount, GhcbBasePa));
> +
> + AsmWriteMsr64 (MSR_SEV_ES_GHCB, (UINT64)GhcbBasePa);
> }
>
> /**
>
(9) If you like, you can drop the (UINT64) casts from all the
"GhcbBasePa" references; all of edk2 uses PHYSICAL_ADDRESS and
EFI_PHYSICAL_ADDRESS interchangeably, and the latter is UINT64 per spec.
With the above updated -- (3) and (9) are optional --
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Thanks!
Laszlo
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#48366): https://edk2.groups.io/g/devel/message/48366
Mute This Topic: https://groups.io/mt/34203543/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
On 10/2/19 5:23 AM, Laszlo Ersek wrote:
> After the discussion elsewhere in this patch thread, which related to
> commit messages, and patch order in the series, I can make a few coding
> style comments on the patch. (No change to functionality.)
>
> On 09/19/19 21:52, Lendacky, Thomas wrote:
>> From: Tom Lendacky <thomas.lendacky@amd.com>
>>
>> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=2198
>>
>> Allocate memory for the GHCB pages during SEV initialization for use
>> during Pei and Dxe phases. The GHCB page(s) must be shared pages, so
>> clear the encryption mask from the current page table entries. Upon
>> successful allocation, set the GHCB PCDs (PcdGhcbBase and PcdGhcbSize).
>>
>> Cc: Jordan Justen <jordan.l.justen@intel.com>
>> Cc: Laszlo Ersek <lersek@redhat.com>
>> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
>> ---
>> OvmfPkg/OvmfPkgIa32.dsc | 2 ++
>> OvmfPkg/OvmfPkgIa32X64.dsc | 2 ++
>> OvmfPkg/OvmfPkgX64.dsc | 2 ++
>> OvmfPkg/PlatformPei/PlatformPei.inf | 2 ++
>> OvmfPkg/PlatformPei/AmdSev.c | 36 ++++++++++++++++++++++++++++-
>> 5 files changed, 43 insertions(+), 1 deletion(-)
>>
>> diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc
>> index 0ce5c01722ef..4369cf6d55e5 100644
>> --- a/OvmfPkg/OvmfPkgIa32.dsc
>> +++ b/OvmfPkg/OvmfPkgIa32.dsc
>> @@ -560,6 +560,8 @@ [PcdsDynamicDefault]
>>
>> # Set SEV-ES defaults
>> gEfiMdeModulePkgTokenSpaceGuid.PcdSevEsActive|0
>> + gEfiMdeModulePkgTokenSpaceGuid.PcdGhcbBase|0
>> + gEfiMdeModulePkgTokenSpaceGuid.PcdGhcbSize|0
>>
>> !if $(SMM_REQUIRE) == TRUE
>> gUefiOvmfPkgTokenSpaceGuid.PcdQ35TsegMbytes|8
>> diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
>> index e7455e35a55d..a74f5028068e 100644
>> --- a/OvmfPkg/OvmfPkgIa32X64.dsc
>> +++ b/OvmfPkg/OvmfPkgIa32X64.dsc
>> @@ -572,6 +572,8 @@ [PcdsDynamicDefault]
>>
>> # Set SEV-ES defaults
>> gEfiMdeModulePkgTokenSpaceGuid.PcdSevEsActive|0
>> + gEfiMdeModulePkgTokenSpaceGuid.PcdGhcbBase|0
>> + gEfiMdeModulePkgTokenSpaceGuid.PcdGhcbSize|0
>>
>> !if $(SMM_REQUIRE) == TRUE
>> gUefiOvmfPkgTokenSpaceGuid.PcdQ35TsegMbytes|8
>> diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
>> index 0b8305cd10a2..fd714d386e75 100644
>> --- a/OvmfPkg/OvmfPkgX64.dsc
>> +++ b/OvmfPkg/OvmfPkgX64.dsc
>> @@ -571,6 +571,8 @@ [PcdsDynamicDefault]
>>
>> # Set SEV-ES defaults
>> gEfiMdeModulePkgTokenSpaceGuid.PcdSevEsActive|0
>> + gEfiMdeModulePkgTokenSpaceGuid.PcdGhcbBase|0
>> + gEfiMdeModulePkgTokenSpaceGuid.PcdGhcbSize|0
>>
>> !if $(SMM_REQUIRE) == TRUE
>> gUefiOvmfPkgTokenSpaceGuid.PcdQ35TsegMbytes|8
>> diff --git a/OvmfPkg/PlatformPei/PlatformPei.inf b/OvmfPkg/PlatformPei/PlatformPei.inf
>> index a9e424a6012a..62abc99f4622 100644
>> --- a/OvmfPkg/PlatformPei/PlatformPei.inf
>> +++ b/OvmfPkg/PlatformPei/PlatformPei.inf
>> @@ -105,6 +105,8 @@ [Pcd]
>> gUefiCpuPkgTokenSpaceGuid.PcdCpuApInitTimeOutInMicroSeconds
>> gUefiCpuPkgTokenSpaceGuid.PcdCpuApStackSize
>> gEfiMdeModulePkgTokenSpaceGuid.PcdSevEsActive
>> + gEfiMdeModulePkgTokenSpaceGuid.PcdGhcbBase
>> + gEfiMdeModulePkgTokenSpaceGuid.PcdGhcbSize
>>
>> [FixedPcd]
>> gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress
>
> (1) Once you move PcdSevEsActive near the other
> gEfiMdeModulePkgTokenSpaceGuid PCDs, per
> <http://mid.mail-archive.com/bd38da31-0985-2ffc-b60d-f867a0218ab2@redhat.com>,
> please keep these grouped with it, too.
Will do.
>
>> diff --git a/OvmfPkg/PlatformPei/AmdSev.c b/OvmfPkg/PlatformPei/AmdSev.c
>> index 7ae2f26a2ba7..30c0e4af7252 100644
>> --- a/OvmfPkg/PlatformPei/AmdSev.c
>> +++ b/OvmfPkg/PlatformPei/AmdSev.c
>> @@ -16,6 +16,9 @@
>> #include <PiPei.h>
>> #include <Register/Amd/Cpuid.h>
>> #include <Register/Cpuid.h>
>> +#include <Register/Amd/Msr.h>
>> +#include <Library/BaseMemoryLib.h>
>> +#include <Library/MemoryAllocationLib.h>
>
> (2) This #include list is currently sorted. Can you please keep it sorted?
>
>>
>> #include "Platform.h"
>>
>> @@ -30,7 +33,10 @@ AmdSevEsInitialize (
>> VOID
>> )
>> {
>> - RETURN_STATUS PcdStatus;
>> + VOID *GhcbBase;
>> + PHYSICAL_ADDRESS GhcbBasePa;
>> + UINTN GhcbPageCount;
>> + RETURN_STATUS PcdStatus, DecryptStatus;
>>
>> if (!MemEncryptSevEsIsEnabled ()) {
>> return;
>> @@ -38,6 +44,34 @@ AmdSevEsInitialize (
>>
>> PcdStatus = PcdSetBoolS (PcdSevEsActive, 1);
>> ASSERT_RETURN_ERROR (PcdStatus);
>> +
>> + //
>> + // Allocate GHCB pages.
>> + //
>> + GhcbPageCount = mMaxCpuCount;
>> + GhcbBase = AllocatePages (GhcbPageCount);
>
> Yes, AllocatePages() looks safe, per
> <http://mid.mail-archive.com/4029f533-9f2a-ba71-2ba6-348187dc7684@redhat.com>.
>
>> + ASSERT (GhcbBase);
>
> (3) I don't really like using *only* an ASSERT for stopping, when we run
> out of resources in a place like this (i.e. where there's no way to
> recover, or to report the issue nicely). I'd suggest throwing in an
> explicit check and a CpuDeadLoop(), in addition to the ASSERT. Anyway,
> not really important, up to you.
Ok, let me think about that. If ASSERT is enabled, you'll get the ASSERT
message (since the SEC GHCB is in place and OVMF is running in 64-bit
mode). If ASSERT is not enabled, then the ZeroMem will segfault on a NULL
pointer, which will give a bit more info than the CpuDeadLoop() which
would look more like a hang.
>
> (4) The expression in the ASSERT() should compare GhcbBase against NULL
> explicitly, however. The edk2 coding style permits the implicit "!= 0"
> comparison (in controlling expressions) for BOOLEANs only.
Will do.
>
>> +
>> + GhcbBasePa = (PHYSICAL_ADDRESS)(UINTN) GhcbBase;
>> +
>> + DecryptStatus = MemEncryptSevClearPageEncMask (
>> + 0,
>> + GhcbBasePa,
>> + GhcbPageCount,
>> + TRUE
>> + );
>> + ASSERT_RETURN_ERROR (DecryptStatus);
>> +
>> + SetMem (GhcbBase, GhcbPageCount * SIZE_4KB, 0);
>
> (5) The following would be more idiomatic:
>
> ZeroMem (GhcbBase, EFI_PAGES_TO_SIZE (GhcbPageCount));
Will do.
>
> (like you write below already)
>
>> +
>> + PcdStatus = PcdSet64S (PcdGhcbBase, (UINT64)GhcbBasePa);
>> + ASSERT_RETURN_ERROR (PcdStatus);
>> + PcdStatus = PcdSet64S (PcdGhcbSize, (UINT64)EFI_PAGES_TO_SIZE (GhcbPageCount));
>> + ASSERT_RETURN_ERROR (PcdStatus);
>> +
>> + DEBUG ((DEBUG_INFO, "SEV-ES is enabled, %u GHCB pages allocated starting at 0x%lx\n", GhcbPageCount, GhcbBase));
>
> (6) This line is too long; please try to stick with <=80 chars per line.
Huh, don't know how I missed that one. I'll fix that.
>
> (7) UINTN values (such as GhcbPageCount) should be converted to UINT64
> explicitly, and then formatted with %lu.
Ok, will do (and I'll check the rest of my patches for this).
>
> (8) GhcbBase is a pointer-to-void; please either format it with %p, or
> use GhcbBasePa. (We can rely on the latter being UINT64.)
>
> DEBUG ((DEBUG_INFO,
> "SEV-ES is enabled, %lu GHCB pages allocated starting at 0x%lx\n",
> (UINT64)GhcbPageCount, GhcbBasePa));
>
>> +
>> + AsmWriteMsr64 (MSR_SEV_ES_GHCB, (UINT64)GhcbBasePa);
>> }
>>
>> /**
>>
>
> (9) If you like, you can drop the (UINT64) casts from all the
> "GhcbBasePa" references; all of edk2 uses PHYSICAL_ADDRESS and
> EFI_PHYSICAL_ADDRESS interchangeably, and the latter is UINT64 per spec.
Nice, that will clean it up a bit.
Thanks, Tom
>
> With the above updated -- (3) and (9) are optional --
>
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
>
> Thanks!
> Laszlo
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#48373): https://edk2.groups.io/g/devel/message/48373
Mute This Topic: https://groups.io/mt/34203543/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
On 10/02/19 16:43, Lendacky, Thomas wrote:
> On 10/2/19 5:23 AM, Laszlo Ersek wrote:
>> On 09/19/19 21:52, Lendacky, Thomas wrote:
>>> @@ -38,6 +44,34 @@ AmdSevEsInitialize (
>>>
>>> PcdStatus = PcdSetBoolS (PcdSevEsActive, 1);
>>> ASSERT_RETURN_ERROR (PcdStatus);
>>> +
>>> + //
>>> + // Allocate GHCB pages.
>>> + //
>>> + GhcbPageCount = mMaxCpuCount;
>>> + GhcbBase = AllocatePages (GhcbPageCount);
>>
>> Yes, AllocatePages() looks safe, per
>> <http://mid.mail-archive.com/4029f533-9f2a-ba71-2ba6-348187dc7684@redhat.com>.
>>
>>> + ASSERT (GhcbBase);
>>
>> (3) I don't really like using *only* an ASSERT for stopping, when we run
>> out of resources in a place like this (i.e. where there's no way to
>> recover, or to report the issue nicely). I'd suggest throwing in an
>> explicit check and a CpuDeadLoop(), in addition to the ASSERT. Anyway,
>> not really important, up to you.
>
> Ok, let me think about that. If ASSERT is enabled, you'll get the ASSERT
> message (since the SEC GHCB is in place and OVMF is running in 64-bit
> mode). If ASSERT is not enabled, then the ZeroMem will segfault on a NULL
> pointer, which will give a bit more info than the CpuDeadLoop() which
> would look more like a hang.
I don't insist on "strengthening" the ASSERT(). For technical
correctness though: accessing page#0 has -- by default -- no ill effects
in the firmware. Page#0 is normal RAM, and mapped as such (by default).
MdeModulePkg offers null pointer detection; see
"PcdNullPointerDetectionPropertyMask". It's a debug feature (not a
production feature), to my understanding anyway. It's disabled by
default (per DEC file), and OVMF does not enable it.
See also commit 90f3922b018e ("OvmfPkg/QemuVideoDxe: Bypass NULL pointer
detection during VBE SHIM installing", 2017-10-11).
Thanks!
Laszlo
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#48378): https://edk2.groups.io/g/devel/message/48378
Mute This Topic: https://groups.io/mt/34203543/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
© 2016 - 2025 Red Hat, Inc.