UefiCpuPkg/Library/MpInitLib/DxeMpLib.c | 19 +++++++- UefiCpuPkg/Library/MpInitLib/MpLib.c | 49 +++++++++++++------- UefiCpuPkg/Library/MpInitLib/PeiMpLib.c | 19 +++++++- 3 files changed, 69 insertions(+), 18 deletions(-)
BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3324
The SEV-ES stacks currently share a page with the reset code and data.
Separate the SEV-ES stacks from the reset vector code and data to avoid
possible stack overflows from overwriting the code and/or data.
When SEV-ES is enabled, invoke the GetWakeupBuffer() routine a second time
to allocate a new area, below the reset vector and data.
Both the PEI and DXE versions of GetWakeupBuffer() are changed so that
when PcdSevEsIsEnabled is true, they will track the previous reset buffer
allocation in order to ensure that the new buffer allocation is below the
previous allocation. When PcdSevEsIsEnabled is false, the original logic
is followed.
Fixes: 7b7508ad784d16a5208c8d12dff43aef6df0835b
Cc: Eric Dong <eric.dong@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Rahul Kumar <rahul1.kumar@intel.com>
Cc: Marvin Häuser <mhaeuser@posteo.de>
Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
---
Changes since v1:
- Renamed the wakeup buffer variables to be unique in the PEI and DXE
libraries and to make it obvious they are SEV-ES specific.
- Use PcdGetBool (PcdSevEsIsEnabled) to make the changes regression-free
so that the new support is only use for SEV-ES guests.
---
UefiCpuPkg/Library/MpInitLib/DxeMpLib.c | 19 +++++++-
UefiCpuPkg/Library/MpInitLib/MpLib.c | 49 +++++++++++++-------
UefiCpuPkg/Library/MpInitLib/PeiMpLib.c | 19 +++++++-
3 files changed, 69 insertions(+), 18 deletions(-)
diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
index 7839c249760e..93fc63bf93e3 100644
--- a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
+++ b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
@@ -29,6 +29,11 @@ VOID *mReservedApLoopFunc = NULL;
UINTN mReservedTopOfApStack;
volatile UINT32 mNumberToFinish = 0;
+//
+// Begin wakeup buffer allocation below 0x88000
+//
+STATIC EFI_PHYSICAL_ADDRESS mSevEsDxeWakeupBuffer = 0x88000;
+
/**
Enable Debug Agent to support source debugging on AP function.
@@ -102,7 +107,14 @@ GetWakeupBuffer (
// LagacyBios driver depends on CPU Arch protocol which guarantees below
// allocation runs earlier than LegacyBios driver.
//
- StartAddress = 0x88000;
+ if (PcdGetBool (PcdSevEsIsEnabled)) {
+ //
+ // SEV-ES Wakeup buffer should be under 0x88000 and under any previous one
+ //
+ StartAddress = mSevEsDxeWakeupBuffer;
+ } else {
+ StartAddress = 0x88000;
+ }
Status = gBS->AllocatePages (
AllocateMaxAddress,
MemoryType,
@@ -112,6 +124,11 @@ GetWakeupBuffer (
ASSERT_EFI_ERROR (Status);
if (EFI_ERROR (Status)) {
StartAddress = (EFI_PHYSICAL_ADDRESS) -1;
+ } else if (PcdGetBool (PcdSevEsIsEnabled)) {
+ //
+ // Next SEV-ES wakeup buffer allocation must be below this allocation
+ //
+ mSevEsDxeWakeupBuffer = StartAddress;
}
DEBUG ((DEBUG_INFO, "WakeupBufferStart = %x, WakeupBufferSize = %x\n",
diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Library/MpInitLib/MpLib.c
index dc2a54aa31e8..b9a06747edbf 100644
--- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
+++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
@@ -1164,20 +1164,6 @@ GetApResetVectorSize (
AddressMap->SwitchToRealSize +
sizeof (MP_CPU_EXCHANGE_INFO);
- //
- // The AP reset stack is only used by SEV-ES guests. Do not add to the
- // allocation if SEV-ES is not enabled.
- //
- if (PcdGetBool (PcdSevEsIsEnabled)) {
- //
- // Stack location is based on APIC ID, so use the total number of
- // processors for calculating the total stack area.
- //
- Size += AP_RESET_STACK_SIZE * PcdGet32 (PcdCpuMaxLogicalProcessorNumber);
-
- Size = ALIGN_VALUE (Size, CPU_STACK_ALIGNMENT);
- }
-
return Size;
}
@@ -1192,6 +1178,7 @@ AllocateResetVector (
)
{
UINTN ApResetVectorSize;
+ UINTN ApResetStackSize;
if (CpuMpData->WakeupBuffer == (UINTN) -1) {
ApResetVectorSize = GetApResetVectorSize (&CpuMpData->AddressMap);
@@ -1207,9 +1194,39 @@ AllocateResetVector (
CpuMpData->AddressMap.ModeTransitionOffset
);
//
- // The reset stack starts at the end of the buffer.
+ // The AP reset stack is only used by SEV-ES guests. Do not allocate it
+ // if SEV-ES is not enabled.
//
- CpuMpData->SevEsAPResetStackStart = CpuMpData->WakeupBuffer + ApResetVectorSize;
+ if (PcdGetBool (PcdSevEsIsEnabled)) {
+ //
+ // Stack location is based on ProcessorNumber, so use the total number
+ // of processors for calculating the total stack area.
+ //
+ ApResetStackSize = (AP_RESET_STACK_SIZE *
+ PcdGet32 (PcdCpuMaxLogicalProcessorNumber));
+
+ //
+ // Invoke GetWakeupBuffer a second time to allocate the stack area
+ // below 1MB. The returned buffer will be page aligned and sized and
+ // below the previously allocated buffer.
+ //
+ CpuMpData->SevEsAPResetStackStart = GetWakeupBuffer (ApResetStackSize);
+
+ //
+ // Check to be sure that the "allocate below" behavior hasn't changed.
+ // This will also catch a failed allocation, as "-1" is returned on
+ // failure.
+ //
+ if (CpuMpData->SevEsAPResetStackStart >= CpuMpData->WakeupBuffer) {
+ DEBUG ((
+ DEBUG_ERROR,
+ "SEV-ES AP reset stack is not below wakeup buffer\n"
+ ));
+
+ ASSERT (FALSE);
+ CpuDeadLoop ();
+ }
+ }
}
BackupAndPrepareWakeupBuffer (CpuMpData);
}
diff --git a/UefiCpuPkg/Library/MpInitLib/PeiMpLib.c b/UefiCpuPkg/Library/MpInitLib/PeiMpLib.c
index 3989bd6a7a9f..90015c650c68 100644
--- a/UefiCpuPkg/Library/MpInitLib/PeiMpLib.c
+++ b/UefiCpuPkg/Library/MpInitLib/PeiMpLib.c
@@ -11,6 +11,8 @@
#include <Guid/S3SmmInitDone.h>
#include <Ppi/ShadowMicrocode.h>
+STATIC UINT64 mSevEsPeiWakeupBuffer = BASE_1MB;
+
/**
S3 SMM Init Done notification function.
@@ -220,7 +222,13 @@ GetWakeupBuffer (
// Need memory under 1MB to be collected here
//
WakeupBufferEnd = Hob.ResourceDescriptor->PhysicalStart + Hob.ResourceDescriptor->ResourceLength;
- if (WakeupBufferEnd > BASE_1MB) {
+ if (PcdGetBool (PcdSevEsIsEnabled) &&
+ WakeupBufferEnd > mSevEsPeiWakeupBuffer) {
+ //
+ // SEV-ES Wakeup buffer should be under 1MB and under any previous one
+ //
+ WakeupBufferEnd = mSevEsPeiWakeupBuffer;
+ } else if (WakeupBufferEnd > BASE_1MB) {
//
// Wakeup buffer should be under 1MB
//
@@ -244,6 +252,15 @@ GetWakeupBuffer (
}
DEBUG ((DEBUG_INFO, "WakeupBufferStart = %x, WakeupBufferSize = %x\n",
WakeupBufferStart, WakeupBufferSize));
+
+ if (PcdGetBool (PcdSevEsIsEnabled)) {
+ //
+ // Next SEV-ES wakeup buffer allocation must be below this
+ // allocation
+ //
+ mSevEsPeiWakeupBuffer = WakeupBufferStart;
+ }
+
return (UINTN)WakeupBufferStart;
}
}
--
2.31.0
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#75132): https://edk2.groups.io/g/devel/message/75132
Mute This Topic: https://groups.io/mt/82833645/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
On 05/14/21 22:28, Lendacky, Thomas wrote:
> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3324
>
> The SEV-ES stacks currently share a page with the reset code and data.
> Separate the SEV-ES stacks from the reset vector code and data to avoid
> possible stack overflows from overwriting the code and/or data.
>
> When SEV-ES is enabled, invoke the GetWakeupBuffer() routine a second time
> to allocate a new area, below the reset vector and data.
>
> Both the PEI and DXE versions of GetWakeupBuffer() are changed so that
> when PcdSevEsIsEnabled is true, they will track the previous reset buffer
> allocation in order to ensure that the new buffer allocation is below the
> previous allocation. When PcdSevEsIsEnabled is false, the original logic
> is followed.
>
> Fixes: 7b7508ad784d16a5208c8d12dff43aef6df0835b
Is this really a *bugfix*?
I called what's being fixed "a gap in a generic protection mechanism",
in <https://bugzilla.tianocore.org/show_bug.cgi?id=3324#c9>.
I don't know if that makes this patch a feature addition (or feature
completion -- the feature being "more extensive page protections"), or a
bugfix.
The distinction matters because of the soft feature freeze:
https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Release-Planning
We still have approximately 2 hours before the SFF starts. So if we can
*finish* reviewing this in 2 hours, then it can be merged during the
SFF, even if we call it a feature. But I'd like Marvin to take a look as
well, plus I'd like at least one of Eric and Ray to check.
... I'm tempted not to call it a bugfix, because the lack of this patch
does not break SEV-ES usage, as far as I understand.
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Rahul Kumar <rahul1.kumar@intel.com>
> Cc: Marvin Häuser <mhaeuser@posteo.de>
> Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
>
> ---
>
> Changes since v1:
> - Renamed the wakeup buffer variables to be unique in the PEI and DXE
> libraries and to make it obvious they are SEV-ES specific.
> - Use PcdGetBool (PcdSevEsIsEnabled) to make the changes regression-free
> so that the new support is only use for SEV-ES guests.
> ---
> UefiCpuPkg/Library/MpInitLib/DxeMpLib.c | 19 +++++++-
> UefiCpuPkg/Library/MpInitLib/MpLib.c | 49 +++++++++++++-------
> UefiCpuPkg/Library/MpInitLib/PeiMpLib.c | 19 +++++++-
> 3 files changed, 69 insertions(+), 18 deletions(-)
>
> diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
> index 7839c249760e..93fc63bf93e3 100644
> --- a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
> +++ b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
> @@ -29,6 +29,11 @@ VOID *mReservedApLoopFunc = NULL;
> UINTN mReservedTopOfApStack;
> volatile UINT32 mNumberToFinish = 0;
>
> +//
> +// Begin wakeup buffer allocation below 0x88000
> +//
> +STATIC EFI_PHYSICAL_ADDRESS mSevEsDxeWakeupBuffer = 0x88000;
> +
> /**
> Enable Debug Agent to support source debugging on AP function.
>
> @@ -102,7 +107,14 @@ GetWakeupBuffer (
> // LagacyBios driver depends on CPU Arch protocol which guarantees below
> // allocation runs earlier than LegacyBios driver.
> //
> - StartAddress = 0x88000;
> + if (PcdGetBool (PcdSevEsIsEnabled)) {
> + //
> + // SEV-ES Wakeup buffer should be under 0x88000 and under any previous one
> + //
> + StartAddress = mSevEsDxeWakeupBuffer;
> + } else {
> + StartAddress = 0x88000;
> + }
> Status = gBS->AllocatePages (
> AllocateMaxAddress,
> MemoryType,
> @@ -112,6 +124,11 @@ GetWakeupBuffer (
> ASSERT_EFI_ERROR (Status);
> if (EFI_ERROR (Status)) {
> StartAddress = (EFI_PHYSICAL_ADDRESS) -1;
> + } else if (PcdGetBool (PcdSevEsIsEnabled)) {
> + //
> + // Next SEV-ES wakeup buffer allocation must be below this allocation
> + //
> + mSevEsDxeWakeupBuffer = StartAddress;
> }
>
> DEBUG ((DEBUG_INFO, "WakeupBufferStart = %x, WakeupBufferSize = %x\n",
> diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> index dc2a54aa31e8..b9a06747edbf 100644
> --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
> +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> @@ -1164,20 +1164,6 @@ GetApResetVectorSize (
> AddressMap->SwitchToRealSize +
> sizeof (MP_CPU_EXCHANGE_INFO);
>
> - //
> - // The AP reset stack is only used by SEV-ES guests. Do not add to the
> - // allocation if SEV-ES is not enabled.
> - //
> - if (PcdGetBool (PcdSevEsIsEnabled)) {
> - //
> - // Stack location is based on APIC ID, so use the total number of
> - // processors for calculating the total stack area.
> - //
> - Size += AP_RESET_STACK_SIZE * PcdGet32 (PcdCpuMaxLogicalProcessorNumber);
> -
> - Size = ALIGN_VALUE (Size, CPU_STACK_ALIGNMENT);
> - }
> -
> return Size;
> }
>
> @@ -1192,6 +1178,7 @@ AllocateResetVector (
> )
> {
> UINTN ApResetVectorSize;
> + UINTN ApResetStackSize;
>
> if (CpuMpData->WakeupBuffer == (UINTN) -1) {
> ApResetVectorSize = GetApResetVectorSize (&CpuMpData->AddressMap);
> @@ -1207,9 +1194,39 @@ AllocateResetVector (
> CpuMpData->AddressMap.ModeTransitionOffset
> );
> //
> - // The reset stack starts at the end of the buffer.
> + // The AP reset stack is only used by SEV-ES guests. Do not allocate it
> + // if SEV-ES is not enabled.
> //
> - CpuMpData->SevEsAPResetStackStart = CpuMpData->WakeupBuffer + ApResetVectorSize;
> + if (PcdGetBool (PcdSevEsIsEnabled)) {
> + //
> + // Stack location is based on ProcessorNumber, so use the total number
sneaky documenation fix :) I vaguely remember discussing earlier that
the APIC ID reference was incorrect. OK.
> + // of processors for calculating the total stack area.
> + //
> + ApResetStackSize = (AP_RESET_STACK_SIZE *
> + PcdGet32 (PcdCpuMaxLogicalProcessorNumber));
> +
> + //
> + // Invoke GetWakeupBuffer a second time to allocate the stack area
> + // below 1MB. The returned buffer will be page aligned and sized and
> + // below the previously allocated buffer.
> + //
> + CpuMpData->SevEsAPResetStackStart = GetWakeupBuffer (ApResetStackSize);
> +
> + //
> + // Check to be sure that the "allocate below" behavior hasn't changed.
> + // This will also catch a failed allocation, as "-1" is returned on
> + // failure.
> + //
> + if (CpuMpData->SevEsAPResetStackStart >= CpuMpData->WakeupBuffer) {
> + DEBUG ((
> + DEBUG_ERROR,
> + "SEV-ES AP reset stack is not below wakeup buffer\n"
> + ));
> +
> + ASSERT (FALSE);
> + CpuDeadLoop ();
> + }
> + }
> }
> BackupAndPrepareWakeupBuffer (CpuMpData);
> }
> diff --git a/UefiCpuPkg/Library/MpInitLib/PeiMpLib.c b/UefiCpuPkg/Library/MpInitLib/PeiMpLib.c
> index 3989bd6a7a9f..90015c650c68 100644
> --- a/UefiCpuPkg/Library/MpInitLib/PeiMpLib.c
> +++ b/UefiCpuPkg/Library/MpInitLib/PeiMpLib.c
> @@ -11,6 +11,8 @@
> #include <Guid/S3SmmInitDone.h>
> #include <Ppi/ShadowMicrocode.h>
>
> +STATIC UINT64 mSevEsPeiWakeupBuffer = BASE_1MB;
> +
> /**
> S3 SMM Init Done notification function.
>
> @@ -220,7 +222,13 @@ GetWakeupBuffer (
> // Need memory under 1MB to be collected here
> //
> WakeupBufferEnd = Hob.ResourceDescriptor->PhysicalStart + Hob.ResourceDescriptor->ResourceLength;
> - if (WakeupBufferEnd > BASE_1MB) {
> + if (PcdGetBool (PcdSevEsIsEnabled) &&
> + WakeupBufferEnd > mSevEsPeiWakeupBuffer) {
> + //
> + // SEV-ES Wakeup buffer should be under 1MB and under any previous one
> + //
> + WakeupBufferEnd = mSevEsPeiWakeupBuffer;
> + } else if (WakeupBufferEnd > BASE_1MB) {
> //
> // Wakeup buffer should be under 1MB
> //
> @@ -244,6 +252,15 @@ GetWakeupBuffer (
> }
> DEBUG ((DEBUG_INFO, "WakeupBufferStart = %x, WakeupBufferSize = %x\n",
> WakeupBufferStart, WakeupBufferSize));
> +
> + if (PcdGetBool (PcdSevEsIsEnabled)) {
> + //
> + // Next SEV-ES wakeup buffer allocation must be below this
> + // allocation
> + //
> + mSevEsPeiWakeupBuffer = WakeupBufferStart;
> + }
> +
> return (UINTN)WakeupBufferStart;
> }
> }
>
The code in the patch seems sound to me, but, now that I've managed to
take a bit more careful look, I think the patch is incomplete.
Note the BackupAndPrepareWakeupBuffer() function call -- visible in the
context --, at the end of the AllocateResetVector() function! Before, we
had a single area allocated for the reset vector, which was
appropriately sized for SEV-ES stacks too, in case SEV-ES was enabled.
That was because MpInitLibInitialize() called GetApResetVectorSize()
too, and stored the result to the "BackupBufferSize" field. Thus, the
BackupAndPrepareWakeupBuffer() function, and its counterpart
RestoreWakeupBuffer(), would include the SEV-ES AP stacks area in the
backup/restore operations.
But now, with SEV-ES enabled, we'll have a separate, discontiguous area
-- and neither BackupAndPrepareWakeupBuffer(), nor its counterpart
RestoreWakeupBuffer() take that into account.
Therefore I think, while this patch is regression-free for the SEV-ES
*disabled* case, it may corrupt memory (through not restoring the AP
stack area's original contents) with SEV-ES enabled.
I think we need to turn "ApResetStackSize" into an explicit field. It
should have a defined value only when SEV-ES is enabled. And for that
case, we seem to need a *separate backup buffer* too.
FWIW, I'd really like to re-classify this BZ as a Feature Request (see
the Product field on BZ#3324), and I'd really like to delay the patch
until after edk2-stable202105. The patch is not necessary for using
SEV-ES, but it has a chance to break SEV-ES.
Thanks
Laszlo
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#75169): https://edk2.groups.io/g/devel/message/75169
Mute This Topic: https://groups.io/mt/82833645/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
On 5/16/21 11:22 PM, Laszlo Ersek wrote:
> On 05/14/21 22:28, Lendacky, Thomas wrote:
>> BZ: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D3324&data=04%7C01%7Cthomas.lendacky%40amd.com%7C8142daa079b04c0b3b5508d918eb6417%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637568221542784370%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=N9MNXaBazq2tiVRHWSPVXRdlcZ97JOf24mc7p0m5Tqw%3D&reserved=0
>>
>> The SEV-ES stacks currently share a page with the reset code and data.
>> Separate the SEV-ES stacks from the reset vector code and data to avoid
>> possible stack overflows from overwriting the code and/or data.
>>
>> When SEV-ES is enabled, invoke the GetWakeupBuffer() routine a second time
>> to allocate a new area, below the reset vector and data.
>>
>> Both the PEI and DXE versions of GetWakeupBuffer() are changed so that
>> when PcdSevEsIsEnabled is true, they will track the previous reset buffer
>> allocation in order to ensure that the new buffer allocation is below the
>> previous allocation. When PcdSevEsIsEnabled is false, the original logic
>> is followed.
>>
>> Fixes: 7b7508ad784d16a5208c8d12dff43aef6df0835b
>
> Is this really a *bugfix*?
>
> I called what's being fixed "a gap in a generic protection mechanism",
> in <https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D3324%23c9&data=04%7C01%7Cthomas.lendacky%40amd.com%7C8142daa079b04c0b3b5508d918eb6417%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637568221542784370%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=BiE2cBZRVRT3anwklHEKBHdhg8v2KjV%2FiiPwtx%2Fpon4%3D&reserved=0>.
>
> I don't know if that makes this patch a feature addition (or feature
> completion -- the feature being "more extensive page protections"), or a
> bugfix.
>
> The distinction matters because of the soft feature freeze:
>
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Ftianocore%2Ftianocore.github.io%2Fwiki%2FEDK-II-Release-Planning&data=04%7C01%7Cthomas.lendacky%40amd.com%7C8142daa079b04c0b3b5508d918eb6417%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637568221542784370%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=1n8z7KFAlm3Vb7fPOFpYQFlZ5lQFOF%2FdLtujjqhns9s%3D&reserved=0
>
> We still have approximately 2 hours before the SFF starts. So if we can
> *finish* reviewing this in 2 hours, then it can be merged during the
> SFF, even if we call it a feature. But I'd like Marvin to take a look as
> well, plus I'd like at least one of Eric and Ray to check.
>
> ... I'm tempted not to call it a bugfix, because the lack of this patch
> does not break SEV-ES usage, as far as I understand.
>
>> Cc: Eric Dong <eric.dong@intel.com>
>> Cc: Ray Ni <ray.ni@intel.com>
>> Cc: Laszlo Ersek <lersek@redhat.com>
>> Cc: Rahul Kumar <rahul1.kumar@intel.com>
>> Cc: Marvin Häuser <mhaeuser@posteo.de>
>> Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
>>
>> ---
>>
>> Changes since v1:
>> - Renamed the wakeup buffer variables to be unique in the PEI and DXE
>> libraries and to make it obvious they are SEV-ES specific.
>> - Use PcdGetBool (PcdSevEsIsEnabled) to make the changes regression-free
>> so that the new support is only use for SEV-ES guests.
>> ---
>> UefiCpuPkg/Library/MpInitLib/DxeMpLib.c | 19 +++++++-
>> UefiCpuPkg/Library/MpInitLib/MpLib.c | 49 +++++++++++++-------
>> UefiCpuPkg/Library/MpInitLib/PeiMpLib.c | 19 +++++++-
>> 3 files changed, 69 insertions(+), 18 deletions(-)
>>
>> diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
>> index 7839c249760e..93fc63bf93e3 100644
>> --- a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
>> +++ b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
>> @@ -29,6 +29,11 @@ VOID *mReservedApLoopFunc = NULL;
>> UINTN mReservedTopOfApStack;
>> volatile UINT32 mNumberToFinish = 0;
>>
>> +//
>> +// Begin wakeup buffer allocation below 0x88000
>> +//
>> +STATIC EFI_PHYSICAL_ADDRESS mSevEsDxeWakeupBuffer = 0x88000;
>> +
>> /**
>> Enable Debug Agent to support source debugging on AP function.
>>
>> @@ -102,7 +107,14 @@ GetWakeupBuffer (
>> // LagacyBios driver depends on CPU Arch protocol which guarantees below
>> // allocation runs earlier than LegacyBios driver.
>> //
>> - StartAddress = 0x88000;
>> + if (PcdGetBool (PcdSevEsIsEnabled)) {
>> + //
>> + // SEV-ES Wakeup buffer should be under 0x88000 and under any previous one
>> + //
>> + StartAddress = mSevEsDxeWakeupBuffer;
>> + } else {
>> + StartAddress = 0x88000;
>> + }
>> Status = gBS->AllocatePages (
>> AllocateMaxAddress,
>> MemoryType,
>> @@ -112,6 +124,11 @@ GetWakeupBuffer (
>> ASSERT_EFI_ERROR (Status);
>> if (EFI_ERROR (Status)) {
>> StartAddress = (EFI_PHYSICAL_ADDRESS) -1;
>> + } else if (PcdGetBool (PcdSevEsIsEnabled)) {
>> + //
>> + // Next SEV-ES wakeup buffer allocation must be below this allocation
>> + //
>> + mSevEsDxeWakeupBuffer = StartAddress;
>> }
>>
>> DEBUG ((DEBUG_INFO, "WakeupBufferStart = %x, WakeupBufferSize = %x\n",
>> diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Library/MpInitLib/MpLib.c
>> index dc2a54aa31e8..b9a06747edbf 100644
>> --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
>> +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
>> @@ -1164,20 +1164,6 @@ GetApResetVectorSize (
>> AddressMap->SwitchToRealSize +
>> sizeof (MP_CPU_EXCHANGE_INFO);
>>
>> - //
>> - // The AP reset stack is only used by SEV-ES guests. Do not add to the
>> - // allocation if SEV-ES is not enabled.
>> - //
>> - if (PcdGetBool (PcdSevEsIsEnabled)) {
>> - //
>> - // Stack location is based on APIC ID, so use the total number of
>> - // processors for calculating the total stack area.
>> - //
>> - Size += AP_RESET_STACK_SIZE * PcdGet32 (PcdCpuMaxLogicalProcessorNumber);
>> -
>> - Size = ALIGN_VALUE (Size, CPU_STACK_ALIGNMENT);
>> - }
>> -
>> return Size;
>> }
>>
>> @@ -1192,6 +1178,7 @@ AllocateResetVector (
>> )
>> {
>> UINTN ApResetVectorSize;
>> + UINTN ApResetStackSize;
>>
>> if (CpuMpData->WakeupBuffer == (UINTN) -1) {
>> ApResetVectorSize = GetApResetVectorSize (&CpuMpData->AddressMap);
>> @@ -1207,9 +1194,39 @@ AllocateResetVector (
>> CpuMpData->AddressMap.ModeTransitionOffset
>> );
>> //
>> - // The reset stack starts at the end of the buffer.
>> + // The AP reset stack is only used by SEV-ES guests. Do not allocate it
>> + // if SEV-ES is not enabled.
>> //
>> - CpuMpData->SevEsAPResetStackStart = CpuMpData->WakeupBuffer + ApResetVectorSize;
>> + if (PcdGetBool (PcdSevEsIsEnabled)) {
>> + //
>> + // Stack location is based on ProcessorNumber, so use the total number
>
> sneaky documenation fix :) I vaguely remember discussing earlier that
> the APIC ID reference was incorrect. OK.
Yeah, I should have made mention of that in the commit message.
>
>> + // of processors for calculating the total stack area.
>> + //
>> + ApResetStackSize = (AP_RESET_STACK_SIZE *
>> + PcdGet32 (PcdCpuMaxLogicalProcessorNumber));
>> +
>> + //
>> + // Invoke GetWakeupBuffer a second time to allocate the stack area
>> + // below 1MB. The returned buffer will be page aligned and sized and
>> + // below the previously allocated buffer.
>> + //
>> + CpuMpData->SevEsAPResetStackStart = GetWakeupBuffer (ApResetStackSize);
>> +
>> + //
>> + // Check to be sure that the "allocate below" behavior hasn't changed.
>> + // This will also catch a failed allocation, as "-1" is returned on
>> + // failure.
>> + //
>> + if (CpuMpData->SevEsAPResetStackStart >= CpuMpData->WakeupBuffer) {
>> + DEBUG ((
>> + DEBUG_ERROR,
>> + "SEV-ES AP reset stack is not below wakeup buffer\n"
>> + ));
>> +
>> + ASSERT (FALSE);
>> + CpuDeadLoop ();
>> + }
>> + }
>> }
>> BackupAndPrepareWakeupBuffer (CpuMpData);
>> }
>> diff --git a/UefiCpuPkg/Library/MpInitLib/PeiMpLib.c b/UefiCpuPkg/Library/MpInitLib/PeiMpLib.c
>> index 3989bd6a7a9f..90015c650c68 100644
>> --- a/UefiCpuPkg/Library/MpInitLib/PeiMpLib.c
>> +++ b/UefiCpuPkg/Library/MpInitLib/PeiMpLib.c
>> @@ -11,6 +11,8 @@
>> #include <Guid/S3SmmInitDone.h>
>> #include <Ppi/ShadowMicrocode.h>
>>
>> +STATIC UINT64 mSevEsPeiWakeupBuffer = BASE_1MB;
>> +
>> /**
>> S3 SMM Init Done notification function.
>>
>> @@ -220,7 +222,13 @@ GetWakeupBuffer (
>> // Need memory under 1MB to be collected here
>> //
>> WakeupBufferEnd = Hob.ResourceDescriptor->PhysicalStart + Hob.ResourceDescriptor->ResourceLength;
>> - if (WakeupBufferEnd > BASE_1MB) {
>> + if (PcdGetBool (PcdSevEsIsEnabled) &&
>> + WakeupBufferEnd > mSevEsPeiWakeupBuffer) {
>> + //
>> + // SEV-ES Wakeup buffer should be under 1MB and under any previous one
>> + //
>> + WakeupBufferEnd = mSevEsPeiWakeupBuffer;
>> + } else if (WakeupBufferEnd > BASE_1MB) {
>> //
>> // Wakeup buffer should be under 1MB
>> //
>> @@ -244,6 +252,15 @@ GetWakeupBuffer (
>> }
>> DEBUG ((DEBUG_INFO, "WakeupBufferStart = %x, WakeupBufferSize = %x\n",
>> WakeupBufferStart, WakeupBufferSize));
>> +
>> + if (PcdGetBool (PcdSevEsIsEnabled)) {
>> + //
>> + // Next SEV-ES wakeup buffer allocation must be below this
>> + // allocation
>> + //
>> + mSevEsPeiWakeupBuffer = WakeupBufferStart;
>> + }
>> +
>> return (UINTN)WakeupBufferStart;
>> }
>> }
>>
>
> The code in the patch seems sound to me, but, now that I've managed to
> take a bit more careful look, I think the patch is incomplete.
>
> Note the BackupAndPrepareWakeupBuffer() function call -- visible in the
> context --, at the end of the AllocateResetVector() function! Before, we
> had a single area allocated for the reset vector, which was
> appropriately sized for SEV-ES stacks too, in case SEV-ES was enabled.
>
> That was because MpInitLibInitialize() called GetApResetVectorSize()
> too, and stored the result to the "BackupBufferSize" field. Thus, the
> BackupAndPrepareWakeupBuffer() function, and its counterpart
> RestoreWakeupBuffer(), would include the SEV-ES AP stacks area in the
> backup/restore operations.
The restore is not performed for an SEV-ES guest (see FreeResetVector()),
because the memory is still needed.
>
> But now, with SEV-ES enabled, we'll have a separate, discontiguous area
> -- and neither BackupAndPrepareWakeupBuffer(), nor its counterpart
> RestoreWakeupBuffer() take that into account.
>
> Therefore I think, while this patch is regression-free for the SEV-ES
> *disabled* case, it may corrupt memory (through not restoring the AP
> stack area's original contents) with SEV-ES enabled.
This is the current behavior for SEV-ES. The wakeup buffer memory is
marked as reserved, at least in the DXE phase.
>
> I think we need to turn "ApResetStackSize" into an explicit field. It
> should have a defined value only when SEV-ES is enabled. And for that
> case, we seem to need a *separate backup buffer* too.
>
> FWIW, I'd really like to re-classify this BZ as a Feature Request (see
> the Product field on BZ#3324), and I'd really like to delay the patch
> until after edk2-stable202105. The patch is not necessary for using
> SEV-ES, but it has a chance to break SEV-ES.
I'm ok with delaying this if you want.
Thanks,
Tom
>
> Thanks
> Laszlo
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#75211): https://edk2.groups.io/g/devel/message/75211
Mute This Topic: https://groups.io/mt/82833645/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
On 05/17/21 17:03, Lendacky, Thomas wrote:
> On 5/16/21 11:22 PM, Laszlo Ersek wrote:
>> On 05/14/21 22:28, Lendacky, Thomas wrote:
>>> BZ: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D3324&data=04%7C01%7Cthomas.lendacky%40amd.com%7C8142daa079b04c0b3b5508d918eb6417%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637568221542784370%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=N9MNXaBazq2tiVRHWSPVXRdlcZ97JOf24mc7p0m5Tqw%3D&reserved=0
>>>
>>> The SEV-ES stacks currently share a page with the reset code and data.
>>> Separate the SEV-ES stacks from the reset vector code and data to avoid
>>> possible stack overflows from overwriting the code and/or data.
>>>
>>> When SEV-ES is enabled, invoke the GetWakeupBuffer() routine a second time
>>> to allocate a new area, below the reset vector and data.
>>>
>>> Both the PEI and DXE versions of GetWakeupBuffer() are changed so that
>>> when PcdSevEsIsEnabled is true, they will track the previous reset buffer
>>> allocation in order to ensure that the new buffer allocation is below the
>>> previous allocation. When PcdSevEsIsEnabled is false, the original logic
>>> is followed.
>>>
>>> Fixes: 7b7508ad784d16a5208c8d12dff43aef6df0835b
>>
>> Is this really a *bugfix*?
>>
>> I called what's being fixed "a gap in a generic protection mechanism",
>> in <https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D3324%23c9&data=04%7C01%7Cthomas.lendacky%40amd.com%7C8142daa079b04c0b3b5508d918eb6417%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637568221542784370%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=BiE2cBZRVRT3anwklHEKBHdhg8v2KjV%2FiiPwtx%2Fpon4%3D&reserved=0>.
>>
>> I don't know if that makes this patch a feature addition (or feature
>> completion -- the feature being "more extensive page protections"), or a
>> bugfix.
>>
>> The distinction matters because of the soft feature freeze:
>>
>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Ftianocore%2Ftianocore.github.io%2Fwiki%2FEDK-II-Release-Planning&data=04%7C01%7Cthomas.lendacky%40amd.com%7C8142daa079b04c0b3b5508d918eb6417%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637568221542784370%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=1n8z7KFAlm3Vb7fPOFpYQFlZ5lQFOF%2FdLtujjqhns9s%3D&reserved=0
>>
>> We still have approximately 2 hours before the SFF starts. So if we can
>> *finish* reviewing this in 2 hours, then it can be merged during the
>> SFF, even if we call it a feature. But I'd like Marvin to take a look as
>> well, plus I'd like at least one of Eric and Ray to check.
>>
>> ... I'm tempted not to call it a bugfix, because the lack of this patch
>> does not break SEV-ES usage, as far as I understand.
>>
>>> Cc: Eric Dong <eric.dong@intel.com>
>>> Cc: Ray Ni <ray.ni@intel.com>
>>> Cc: Laszlo Ersek <lersek@redhat.com>
>>> Cc: Rahul Kumar <rahul1.kumar@intel.com>
>>> Cc: Marvin Häuser <mhaeuser@posteo.de>
>>> Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
>>>
>>> ---
>>>
>>> Changes since v1:
>>> - Renamed the wakeup buffer variables to be unique in the PEI and DXE
>>> libraries and to make it obvious they are SEV-ES specific.
>>> - Use PcdGetBool (PcdSevEsIsEnabled) to make the changes regression-free
>>> so that the new support is only use for SEV-ES guests.
>>> ---
>>> UefiCpuPkg/Library/MpInitLib/DxeMpLib.c | 19 +++++++-
>>> UefiCpuPkg/Library/MpInitLib/MpLib.c | 49 +++++++++++++-------
>>> UefiCpuPkg/Library/MpInitLib/PeiMpLib.c | 19 +++++++-
>>> 3 files changed, 69 insertions(+), 18 deletions(-)
>>>
>>> diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
>>> index 7839c249760e..93fc63bf93e3 100644
>>> --- a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
>>> +++ b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
>>> @@ -29,6 +29,11 @@ VOID *mReservedApLoopFunc = NULL;
>>> UINTN mReservedTopOfApStack;
>>> volatile UINT32 mNumberToFinish = 0;
>>>
>>> +//
>>> +// Begin wakeup buffer allocation below 0x88000
>>> +//
>>> +STATIC EFI_PHYSICAL_ADDRESS mSevEsDxeWakeupBuffer = 0x88000;
>>> +
>>> /**
>>> Enable Debug Agent to support source debugging on AP function.
>>>
>>> @@ -102,7 +107,14 @@ GetWakeupBuffer (
>>> // LagacyBios driver depends on CPU Arch protocol which guarantees below
>>> // allocation runs earlier than LegacyBios driver.
>>> //
>>> - StartAddress = 0x88000;
>>> + if (PcdGetBool (PcdSevEsIsEnabled)) {
>>> + //
>>> + // SEV-ES Wakeup buffer should be under 0x88000 and under any previous one
>>> + //
>>> + StartAddress = mSevEsDxeWakeupBuffer;
>>> + } else {
>>> + StartAddress = 0x88000;
>>> + }
>>> Status = gBS->AllocatePages (
>>> AllocateMaxAddress,
>>> MemoryType,
>>> @@ -112,6 +124,11 @@ GetWakeupBuffer (
>>> ASSERT_EFI_ERROR (Status);
>>> if (EFI_ERROR (Status)) {
>>> StartAddress = (EFI_PHYSICAL_ADDRESS) -1;
>>> + } else if (PcdGetBool (PcdSevEsIsEnabled)) {
>>> + //
>>> + // Next SEV-ES wakeup buffer allocation must be below this allocation
>>> + //
>>> + mSevEsDxeWakeupBuffer = StartAddress;
>>> }
>>>
>>> DEBUG ((DEBUG_INFO, "WakeupBufferStart = %x, WakeupBufferSize = %x\n",
>>> diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Library/MpInitLib/MpLib.c
>>> index dc2a54aa31e8..b9a06747edbf 100644
>>> --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
>>> +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
>>> @@ -1164,20 +1164,6 @@ GetApResetVectorSize (
>>> AddressMap->SwitchToRealSize +
>>> sizeof (MP_CPU_EXCHANGE_INFO);
>>>
>>> - //
>>> - // The AP reset stack is only used by SEV-ES guests. Do not add to the
>>> - // allocation if SEV-ES is not enabled.
>>> - //
>>> - if (PcdGetBool (PcdSevEsIsEnabled)) {
>>> - //
>>> - // Stack location is based on APIC ID, so use the total number of
>>> - // processors for calculating the total stack area.
>>> - //
>>> - Size += AP_RESET_STACK_SIZE * PcdGet32 (PcdCpuMaxLogicalProcessorNumber);
>>> -
>>> - Size = ALIGN_VALUE (Size, CPU_STACK_ALIGNMENT);
>>> - }
>>> -
>>> return Size;
>>> }
>>>
>>> @@ -1192,6 +1178,7 @@ AllocateResetVector (
>>> )
>>> {
>>> UINTN ApResetVectorSize;
>>> + UINTN ApResetStackSize;
>>>
>>> if (CpuMpData->WakeupBuffer == (UINTN) -1) {
>>> ApResetVectorSize = GetApResetVectorSize (&CpuMpData->AddressMap);
>>> @@ -1207,9 +1194,39 @@ AllocateResetVector (
>>> CpuMpData->AddressMap.ModeTransitionOffset
>>> );
>>> //
>>> - // The reset stack starts at the end of the buffer.
>>> + // The AP reset stack is only used by SEV-ES guests. Do not allocate it
>>> + // if SEV-ES is not enabled.
>>> //
>>> - CpuMpData->SevEsAPResetStackStart = CpuMpData->WakeupBuffer + ApResetVectorSize;
>>> + if (PcdGetBool (PcdSevEsIsEnabled)) {
>>> + //
>>> + // Stack location is based on ProcessorNumber, so use the total number
>>
>> sneaky documenation fix :) I vaguely remember discussing earlier that
>> the APIC ID reference was incorrect. OK.
>
> Yeah, I should have made mention of that in the commit message.
>
>>
>>> + // of processors for calculating the total stack area.
>>> + //
>>> + ApResetStackSize = (AP_RESET_STACK_SIZE *
>>> + PcdGet32 (PcdCpuMaxLogicalProcessorNumber));
>>> +
>>> + //
>>> + // Invoke GetWakeupBuffer a second time to allocate the stack area
>>> + // below 1MB. The returned buffer will be page aligned and sized and
>>> + // below the previously allocated buffer.
>>> + //
>>> + CpuMpData->SevEsAPResetStackStart = GetWakeupBuffer (ApResetStackSize);
>>> +
>>> + //
>>> + // Check to be sure that the "allocate below" behavior hasn't changed.
>>> + // This will also catch a failed allocation, as "-1" is returned on
>>> + // failure.
>>> + //
>>> + if (CpuMpData->SevEsAPResetStackStart >= CpuMpData->WakeupBuffer) {
>>> + DEBUG ((
>>> + DEBUG_ERROR,
>>> + "SEV-ES AP reset stack is not below wakeup buffer\n"
>>> + ));
>>> +
>>> + ASSERT (FALSE);
>>> + CpuDeadLoop ();
>>> + }
>>> + }
>>> }
>>> BackupAndPrepareWakeupBuffer (CpuMpData);
>>> }
>>> diff --git a/UefiCpuPkg/Library/MpInitLib/PeiMpLib.c b/UefiCpuPkg/Library/MpInitLib/PeiMpLib.c
>>> index 3989bd6a7a9f..90015c650c68 100644
>>> --- a/UefiCpuPkg/Library/MpInitLib/PeiMpLib.c
>>> +++ b/UefiCpuPkg/Library/MpInitLib/PeiMpLib.c
>>> @@ -11,6 +11,8 @@
>>> #include <Guid/S3SmmInitDone.h>
>>> #include <Ppi/ShadowMicrocode.h>
>>>
>>> +STATIC UINT64 mSevEsPeiWakeupBuffer = BASE_1MB;
>>> +
>>> /**
>>> S3 SMM Init Done notification function.
>>>
>>> @@ -220,7 +222,13 @@ GetWakeupBuffer (
>>> // Need memory under 1MB to be collected here
>>> //
>>> WakeupBufferEnd = Hob.ResourceDescriptor->PhysicalStart + Hob.ResourceDescriptor->ResourceLength;
>>> - if (WakeupBufferEnd > BASE_1MB) {
>>> + if (PcdGetBool (PcdSevEsIsEnabled) &&
>>> + WakeupBufferEnd > mSevEsPeiWakeupBuffer) {
>>> + //
>>> + // SEV-ES Wakeup buffer should be under 1MB and under any previous one
>>> + //
>>> + WakeupBufferEnd = mSevEsPeiWakeupBuffer;
>>> + } else if (WakeupBufferEnd > BASE_1MB) {
>>> //
>>> // Wakeup buffer should be under 1MB
>>> //
>>> @@ -244,6 +252,15 @@ GetWakeupBuffer (
>>> }
>>> DEBUG ((DEBUG_INFO, "WakeupBufferStart = %x, WakeupBufferSize = %x\n",
>>> WakeupBufferStart, WakeupBufferSize));
>>> +
>>> + if (PcdGetBool (PcdSevEsIsEnabled)) {
>>> + //
>>> + // Next SEV-ES wakeup buffer allocation must be below this
>>> + // allocation
>>> + //
>>> + mSevEsPeiWakeupBuffer = WakeupBufferStart;
>>> + }
>>> +
>>> return (UINTN)WakeupBufferStart;
>>> }
>>> }
>>>
>>
>> The code in the patch seems sound to me, but, now that I've managed to
>> take a bit more careful look, I think the patch is incomplete.
>>
>> Note the BackupAndPrepareWakeupBuffer() function call -- visible in the
>> context --, at the end of the AllocateResetVector() function! Before, we
>> had a single area allocated for the reset vector, which was
>> appropriately sized for SEV-ES stacks too, in case SEV-ES was enabled.
>>
>> That was because MpInitLibInitialize() called GetApResetVectorSize()
>> too, and stored the result to the "BackupBufferSize" field. Thus, the
>> BackupAndPrepareWakeupBuffer() function, and its counterpart
>> RestoreWakeupBuffer(), would include the SEV-ES AP stacks area in the
>> backup/restore operations.
>
> The restore is not performed for an SEV-ES guest (see FreeResetVector()),
> because the memory is still needed.
I apologize for missing this. I'm not too familiar with the SEV-ES
specifics in UefiCpuPkg.
One question: given that FreeResetVector() does not call
RestoreWakeupBuffer() when SEV-ES is enabled, would it make sense for
AllocateResetVector() to not call BackupAndPrepareWakeupBuffer() either,
in case SEV-ES is enabled? Because, if we never restore, do we really
need the backup? I wonder if such a patch could be prepended to this one
(in order to form a 2-patch series).
(Well, BackupAndPrepareWakeupBuffer() performs two things, backup and
preparation -- I guess we certainly need the preparation of the wake up
buffer, but do we need to back up the original contents of the area?
Including the backup buffer allocation.)
>
>>
>> But now, with SEV-ES enabled, we'll have a separate, discontiguous area
>> -- and neither BackupAndPrepareWakeupBuffer(), nor its counterpart
>> RestoreWakeupBuffer() take that into account.
>>
>> Therefore I think, while this patch is regression-free for the SEV-ES
>> *disabled* case, it may corrupt memory (through not restoring the AP
>> stack area's original contents) with SEV-ES enabled.
>
> This is the current behavior for SEV-ES. The wakeup buffer memory is
> marked as reserved, at least in the DXE phase.
>
>>
>> I think we need to turn "ApResetStackSize" into an explicit field. It
>> should have a defined value only when SEV-ES is enabled. And for that
>> case, we seem to need a *separate backup buffer* too.
>>
>> FWIW, I'd really like to re-classify this BZ as a Feature Request (see
>> the Product field on BZ#3324), and I'd really like to delay the patch
>> until after edk2-stable202105. The patch is not necessary for using
>> SEV-ES, but it has a chance to break SEV-ES.
>
> I'm ok with delaying this if you want.
Here's what I'd like to do:
- Reach an agreement with Marvin about the ASSERT(). I'm fine if we drop
it, and fine if we keep it.
- Eric or Ray to check the patch as well, because (as I said above) I
didn't follow the SEV-ES patches for UefiCpuPkg (that series was just
huge, apologies), and so now I don't have memories to reach back to.
- Figure out if we need to conditionalize the
BackupAndPrepareWakeupBuffer() call (or a part of that function anyway).
We can and should continue discussing these things during the feature
freeze; I'd like us to merge the patch after the tag.
Sorry again that I'm missing bits and pieces; I'm this close |<->| to
email bankruptcy.
Thanks
Laszlo
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#75279): https://edk2.groups.io/g/devel/message/75279
Mute This Topic: https://groups.io/mt/82833645/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
On 18.05.21 19:17, Laszlo Ersek wrote:
> On 05/17/21 17:03, Lendacky, Thomas wrote:
>> On 5/16/21 11:22 PM, Laszlo Ersek wrote:
>>> On 05/14/21 22:28, Lendacky, Thomas wrote:
>>>> BZ: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D3324&data=04%7C01%7Cthomas.lendacky%40amd.com%7C8142daa079b04c0b3b5508d918eb6417%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637568221542784370%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=N9MNXaBazq2tiVRHWSPVXRdlcZ97JOf24mc7p0m5Tqw%3D&reserved=0
>>>>
>>>> The SEV-ES stacks currently share a page with the reset code and data.
>>>> Separate the SEV-ES stacks from the reset vector code and data to avoid
>>>> possible stack overflows from overwriting the code and/or data.
>>>>
>>>> When SEV-ES is enabled, invoke the GetWakeupBuffer() routine a second time
>>>> to allocate a new area, below the reset vector and data.
>>>>
>>>> Both the PEI and DXE versions of GetWakeupBuffer() are changed so that
>>>> when PcdSevEsIsEnabled is true, they will track the previous reset buffer
>>>> allocation in order to ensure that the new buffer allocation is below the
>>>> previous allocation. When PcdSevEsIsEnabled is false, the original logic
>>>> is followed.
>>>>
>>>> Fixes: 7b7508ad784d16a5208c8d12dff43aef6df0835b
>>> Is this really a *bugfix*?
>>>
>>> I called what's being fixed "a gap in a generic protection mechanism",
>>> in <https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D3324%23c9&data=04%7C01%7Cthomas.lendacky%40amd.com%7C8142daa079b04c0b3b5508d918eb6417%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637568221542784370%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=BiE2cBZRVRT3anwklHEKBHdhg8v2KjV%2FiiPwtx%2Fpon4%3D&reserved=0>.
>>>
>>> I don't know if that makes this patch a feature addition (or feature
>>> completion -- the feature being "more extensive page protections"), or a
>>> bugfix.
>>>
>>> The distinction matters because of the soft feature freeze:
>>>
>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Ftianocore%2Ftianocore.github.io%2Fwiki%2FEDK-II-Release-Planning&data=04%7C01%7Cthomas.lendacky%40amd.com%7C8142daa079b04c0b3b5508d918eb6417%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637568221542784370%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=1n8z7KFAlm3Vb7fPOFpYQFlZ5lQFOF%2FdLtujjqhns9s%3D&reserved=0
>>>
>>> We still have approximately 2 hours before the SFF starts. So if we can
>>> *finish* reviewing this in 2 hours, then it can be merged during the
>>> SFF, even if we call it a feature. But I'd like Marvin to take a look as
>>> well, plus I'd like at least one of Eric and Ray to check.
>>>
>>> ... I'm tempted not to call it a bugfix, because the lack of this patch
>>> does not break SEV-ES usage, as far as I understand.
>>>
>>>> Cc: Eric Dong <eric.dong@intel.com>
>>>> Cc: Ray Ni <ray.ni@intel.com>
>>>> Cc: Laszlo Ersek <lersek@redhat.com>
>>>> Cc: Rahul Kumar <rahul1.kumar@intel.com>
>>>> Cc: Marvin Häuser <mhaeuser@posteo.de>
>>>> Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
>>>>
>>>> ---
>>>>
>>>> Changes since v1:
>>>> - Renamed the wakeup buffer variables to be unique in the PEI and DXE
>>>> libraries and to make it obvious they are SEV-ES specific.
>>>> - Use PcdGetBool (PcdSevEsIsEnabled) to make the changes regression-free
>>>> so that the new support is only use for SEV-ES guests.
>>>> ---
>>>> UefiCpuPkg/Library/MpInitLib/DxeMpLib.c | 19 +++++++-
>>>> UefiCpuPkg/Library/MpInitLib/MpLib.c | 49 +++++++++++++-------
>>>> UefiCpuPkg/Library/MpInitLib/PeiMpLib.c | 19 +++++++-
>>>> 3 files changed, 69 insertions(+), 18 deletions(-)
>>>>
>>>> diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
>>>> index 7839c249760e..93fc63bf93e3 100644
>>>> --- a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
>>>> +++ b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
>>>> @@ -29,6 +29,11 @@ VOID *mReservedApLoopFunc = NULL;
>>>> UINTN mReservedTopOfApStack;
>>>> volatile UINT32 mNumberToFinish = 0;
>>>>
>>>> +//
>>>> +// Begin wakeup buffer allocation below 0x88000
>>>> +//
>>>> +STATIC EFI_PHYSICAL_ADDRESS mSevEsDxeWakeupBuffer = 0x88000;
>>>> +
>>>> /**
>>>> Enable Debug Agent to support source debugging on AP function.
>>>>
>>>> @@ -102,7 +107,14 @@ GetWakeupBuffer (
>>>> // LagacyBios driver depends on CPU Arch protocol which guarantees below
>>>> // allocation runs earlier than LegacyBios driver.
>>>> //
>>>> - StartAddress = 0x88000;
>>>> + if (PcdGetBool (PcdSevEsIsEnabled)) {
>>>> + //
>>>> + // SEV-ES Wakeup buffer should be under 0x88000 and under any previous one
>>>> + //
>>>> + StartAddress = mSevEsDxeWakeupBuffer;
>>>> + } else {
>>>> + StartAddress = 0x88000;
>>>> + }
>>>> Status = gBS->AllocatePages (
>>>> AllocateMaxAddress,
>>>> MemoryType,
>>>> @@ -112,6 +124,11 @@ GetWakeupBuffer (
>>>> ASSERT_EFI_ERROR (Status);
>>>> if (EFI_ERROR (Status)) {
>>>> StartAddress = (EFI_PHYSICAL_ADDRESS) -1;
>>>> + } else if (PcdGetBool (PcdSevEsIsEnabled)) {
>>>> + //
>>>> + // Next SEV-ES wakeup buffer allocation must be below this allocation
>>>> + //
>>>> + mSevEsDxeWakeupBuffer = StartAddress;
>>>> }
>>>>
>>>> DEBUG ((DEBUG_INFO, "WakeupBufferStart = %x, WakeupBufferSize = %x\n",
>>>> diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Library/MpInitLib/MpLib.c
>>>> index dc2a54aa31e8..b9a06747edbf 100644
>>>> --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
>>>> +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
>>>> @@ -1164,20 +1164,6 @@ GetApResetVectorSize (
>>>> AddressMap->SwitchToRealSize +
>>>> sizeof (MP_CPU_EXCHANGE_INFO);
>>>>
>>>> - //
>>>> - // The AP reset stack is only used by SEV-ES guests. Do not add to the
>>>> - // allocation if SEV-ES is not enabled.
>>>> - //
>>>> - if (PcdGetBool (PcdSevEsIsEnabled)) {
>>>> - //
>>>> - // Stack location is based on APIC ID, so use the total number of
>>>> - // processors for calculating the total stack area.
>>>> - //
>>>> - Size += AP_RESET_STACK_SIZE * PcdGet32 (PcdCpuMaxLogicalProcessorNumber);
>>>> -
>>>> - Size = ALIGN_VALUE (Size, CPU_STACK_ALIGNMENT);
>>>> - }
>>>> -
>>>> return Size;
>>>> }
>>>>
>>>> @@ -1192,6 +1178,7 @@ AllocateResetVector (
>>>> )
>>>> {
>>>> UINTN ApResetVectorSize;
>>>> + UINTN ApResetStackSize;
>>>>
>>>> if (CpuMpData->WakeupBuffer == (UINTN) -1) {
>>>> ApResetVectorSize = GetApResetVectorSize (&CpuMpData->AddressMap);
>>>> @@ -1207,9 +1194,39 @@ AllocateResetVector (
>>>> CpuMpData->AddressMap.ModeTransitionOffset
>>>> );
>>>> //
>>>> - // The reset stack starts at the end of the buffer.
>>>> + // The AP reset stack is only used by SEV-ES guests. Do not allocate it
>>>> + // if SEV-ES is not enabled.
>>>> //
>>>> - CpuMpData->SevEsAPResetStackStart = CpuMpData->WakeupBuffer + ApResetVectorSize;
>>>> + if (PcdGetBool (PcdSevEsIsEnabled)) {
>>>> + //
>>>> + // Stack location is based on ProcessorNumber, so use the total number
>>> sneaky documenation fix :) I vaguely remember discussing earlier that
>>> the APIC ID reference was incorrect. OK.
>> Yeah, I should have made mention of that in the commit message.
>>
>>>> + // of processors for calculating the total stack area.
>>>> + //
>>>> + ApResetStackSize = (AP_RESET_STACK_SIZE *
>>>> + PcdGet32 (PcdCpuMaxLogicalProcessorNumber));
>>>> +
>>>> + //
>>>> + // Invoke GetWakeupBuffer a second time to allocate the stack area
>>>> + // below 1MB. The returned buffer will be page aligned and sized and
>>>> + // below the previously allocated buffer.
>>>> + //
>>>> + CpuMpData->SevEsAPResetStackStart = GetWakeupBuffer (ApResetStackSize);
>>>> +
>>>> + //
>>>> + // Check to be sure that the "allocate below" behavior hasn't changed.
>>>> + // This will also catch a failed allocation, as "-1" is returned on
>>>> + // failure.
>>>> + //
>>>> + if (CpuMpData->SevEsAPResetStackStart >= CpuMpData->WakeupBuffer) {
>>>> + DEBUG ((
>>>> + DEBUG_ERROR,
>>>> + "SEV-ES AP reset stack is not below wakeup buffer\n"
>>>> + ));
>>>> +
>>>> + ASSERT (FALSE);
>>>> + CpuDeadLoop ();
>>>> + }
>>>> + }
>>>> }
>>>> BackupAndPrepareWakeupBuffer (CpuMpData);
>>>> }
>>>> diff --git a/UefiCpuPkg/Library/MpInitLib/PeiMpLib.c b/UefiCpuPkg/Library/MpInitLib/PeiMpLib.c
>>>> index 3989bd6a7a9f..90015c650c68 100644
>>>> --- a/UefiCpuPkg/Library/MpInitLib/PeiMpLib.c
>>>> +++ b/UefiCpuPkg/Library/MpInitLib/PeiMpLib.c
>>>> @@ -11,6 +11,8 @@
>>>> #include <Guid/S3SmmInitDone.h>
>>>> #include <Ppi/ShadowMicrocode.h>
>>>>
>>>> +STATIC UINT64 mSevEsPeiWakeupBuffer = BASE_1MB;
>>>> +
>>>> /**
>>>> S3 SMM Init Done notification function.
>>>>
>>>> @@ -220,7 +222,13 @@ GetWakeupBuffer (
>>>> // Need memory under 1MB to be collected here
>>>> //
>>>> WakeupBufferEnd = Hob.ResourceDescriptor->PhysicalStart + Hob.ResourceDescriptor->ResourceLength;
>>>> - if (WakeupBufferEnd > BASE_1MB) {
>>>> + if (PcdGetBool (PcdSevEsIsEnabled) &&
>>>> + WakeupBufferEnd > mSevEsPeiWakeupBuffer) {
>>>> + //
>>>> + // SEV-ES Wakeup buffer should be under 1MB and under any previous one
>>>> + //
>>>> + WakeupBufferEnd = mSevEsPeiWakeupBuffer;
>>>> + } else if (WakeupBufferEnd > BASE_1MB) {
>>>> //
>>>> // Wakeup buffer should be under 1MB
>>>> //
>>>> @@ -244,6 +252,15 @@ GetWakeupBuffer (
>>>> }
>>>> DEBUG ((DEBUG_INFO, "WakeupBufferStart = %x, WakeupBufferSize = %x\n",
>>>> WakeupBufferStart, WakeupBufferSize));
>>>> +
>>>> + if (PcdGetBool (PcdSevEsIsEnabled)) {
>>>> + //
>>>> + // Next SEV-ES wakeup buffer allocation must be below this
>>>> + // allocation
>>>> + //
>>>> + mSevEsPeiWakeupBuffer = WakeupBufferStart;
>>>> + }
>>>> +
>>>> return (UINTN)WakeupBufferStart;
>>>> }
>>>> }
>>>>
>>> The code in the patch seems sound to me, but, now that I've managed to
>>> take a bit more careful look, I think the patch is incomplete.
>>>
>>> Note the BackupAndPrepareWakeupBuffer() function call -- visible in the
>>> context --, at the end of the AllocateResetVector() function! Before, we
>>> had a single area allocated for the reset vector, which was
>>> appropriately sized for SEV-ES stacks too, in case SEV-ES was enabled.
>>>
>>> That was because MpInitLibInitialize() called GetApResetVectorSize()
>>> too, and stored the result to the "BackupBufferSize" field. Thus, the
>>> BackupAndPrepareWakeupBuffer() function, and its counterpart
>>> RestoreWakeupBuffer(), would include the SEV-ES AP stacks area in the
>>> backup/restore operations.
>> The restore is not performed for an SEV-ES guest (see FreeResetVector()),
>> because the memory is still needed.
> I apologize for missing this. I'm not too familiar with the SEV-ES
> specifics in UefiCpuPkg.
>
> One question: given that FreeResetVector() does not call
> RestoreWakeupBuffer() when SEV-ES is enabled, would it make sense for
> AllocateResetVector() to not call BackupAndPrepareWakeupBuffer() either,
> in case SEV-ES is enabled? Because, if we never restore, do we really
> need the backup? I wonder if such a patch could be prepended to this one
> (in order to form a 2-patch series).
>
> (Well, BackupAndPrepareWakeupBuffer() performs two things, backup and
> preparation -- I guess we certainly need the preparation of the wake up
> buffer, but do we need to back up the original contents of the area?
> Including the backup buffer allocation.)
>
>>> But now, with SEV-ES enabled, we'll have a separate, discontiguous area
>>> -- and neither BackupAndPrepareWakeupBuffer(), nor its counterpart
>>> RestoreWakeupBuffer() take that into account.
>>>
>>> Therefore I think, while this patch is regression-free for the SEV-ES
>>> *disabled* case, it may corrupt memory (through not restoring the AP
>>> stack area's original contents) with SEV-ES enabled.
>> This is the current behavior for SEV-ES. The wakeup buffer memory is
>> marked as reserved, at least in the DXE phase.
>>
>>> I think we need to turn "ApResetStackSize" into an explicit field. It
>>> should have a defined value only when SEV-ES is enabled. And for that
>>> case, we seem to need a *separate backup buffer* too.
>>>
>>> FWIW, I'd really like to re-classify this BZ as a Feature Request (see
>>> the Product field on BZ#3324), and I'd really like to delay the patch
>>> until after edk2-stable202105. The patch is not necessary for using
>>> SEV-ES, but it has a chance to break SEV-ES.
>> I'm ok with delaying this if you want.
> Here's what I'd like to do:
>
> - Reach an agreement with Marvin about the ASSERT(). I'm fine if we drop
> it, and fine if we keep it.
As I said, it's a strong recommendation from me, but I am fine either
way as well. If it ever causes any of the issues I outlined, it could be
fixed later as well.
Best regards,
Marvin
>
> - Eric or Ray to check the patch as well, because (as I said above) I
> didn't follow the SEV-ES patches for UefiCpuPkg (that series was just
> huge, apologies), and so now I don't have memories to reach back to.
>
> - Figure out if we need to conditionalize the
> BackupAndPrepareWakeupBuffer() call (or a part of that function anyway).
>
> We can and should continue discussing these things during the feature
> freeze; I'd like us to merge the patch after the tag.
>
> Sorry again that I'm missing bits and pieces; I'm this close |<->| to
> email bankruptcy.
>
> Thanks
> Laszlo
>
>
>
>
>
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#75280): https://edk2.groups.io/g/devel/message/75280
Mute This Topic: https://groups.io/mt/82833645/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
On 5/18/21 12:17 PM, Laszlo Ersek wrote:
> On 05/17/21 17:03, Lendacky, Thomas wrote:
>> On 5/16/21 11:22 PM, Laszlo Ersek wrote:
>>> On 05/14/21 22:28, Lendacky, Thomas wrote:
>>>> BZ: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D3324&data=04%7C01%7Cthomas.lendacky%40amd.com%7Cec6b583f740d45dfb96408d91a20d05a%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637569550503778789%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=194iu8I6ucUBr6bNR0fIBa%2FgQhtUQQpJdN55gzOHlmY%3D&reserved=0
>>>>
>>>> The SEV-ES stacks currently share a page with the reset code and data.
>>>> Separate the SEV-ES stacks from the reset vector code and data to avoid
>>>> possible stack overflows from overwriting the code and/or data.
>>>>
>>>> When SEV-ES is enabled, invoke the GetWakeupBuffer() routine a second time
>>>> to allocate a new area, below the reset vector and data.
>>>>
>>>> Both the PEI and DXE versions of GetWakeupBuffer() are changed so that
>>>> when PcdSevEsIsEnabled is true, they will track the previous reset buffer
>>>> allocation in order to ensure that the new buffer allocation is below the
>>>> previous allocation. When PcdSevEsIsEnabled is false, the original logic
>>>> is followed.
>>>>
>>>> Fixes: 7b7508ad784d16a5208c8d12dff43aef6df0835b
>>>
>>> Is this really a *bugfix*?
>>>
>>> I called what's being fixed "a gap in a generic protection mechanism",
>>> in <https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D3324%23c9&data=04%7C01%7Cthomas.lendacky%40amd.com%7Cec6b583f740d45dfb96408d91a20d05a%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637569550503778789%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=QFwYOCIEZb8Uc6zX60a7p6QWIkPdzTInggQQv3dRVlQ%3D&reserved=0>.
>>>
>>> I don't know if that makes this patch a feature addition (or feature
>>> completion -- the feature being "more extensive page protections"), or a
>>> bugfix.
>>>
>>> The distinction matters because of the soft feature freeze:
>>>
>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Ftianocore%2Ftianocore.github.io%2Fwiki%2FEDK-II-Release-Planning&data=04%7C01%7Cthomas.lendacky%40amd.com%7Cec6b583f740d45dfb96408d91a20d05a%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637569550503788787%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=bZJ%2BK1Y%2Fs4CXXy%2Fz3IKVnWWCUSz3Noo3cPsWR%2Fby5H0%3D&reserved=0
>>>
>>> We still have approximately 2 hours before the SFF starts. So if we can
>>> *finish* reviewing this in 2 hours, then it can be merged during the
>>> SFF, even if we call it a feature. But I'd like Marvin to take a look as
>>> well, plus I'd like at least one of Eric and Ray to check.
>>>
>>> ... I'm tempted not to call it a bugfix, because the lack of this patch
>>> does not break SEV-ES usage, as far as I understand.
>>>
>>>> Cc: Eric Dong <eric.dong@intel.com>
>>>> Cc: Ray Ni <ray.ni@intel.com>
>>>> Cc: Laszlo Ersek <lersek@redhat.com>
>>>> Cc: Rahul Kumar <rahul1.kumar@intel.com>
>>>> Cc: Marvin Häuser <mhaeuser@posteo.de>
>>>> Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
>>>>
>>>> ---
>>>>
>>>> Changes since v1:
>>>> - Renamed the wakeup buffer variables to be unique in the PEI and DXE
>>>> libraries and to make it obvious they are SEV-ES specific.
>>>> - Use PcdGetBool (PcdSevEsIsEnabled) to make the changes regression-free
>>>> so that the new support is only use for SEV-ES guests.
>>>> ---
>>>> UefiCpuPkg/Library/MpInitLib/DxeMpLib.c | 19 +++++++-
>>>> UefiCpuPkg/Library/MpInitLib/MpLib.c | 49 +++++++++++++-------
>>>> UefiCpuPkg/Library/MpInitLib/PeiMpLib.c | 19 +++++++-
>>>> 3 files changed, 69 insertions(+), 18 deletions(-)
>>>>
>>>> diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
>>>> index 7839c249760e..93fc63bf93e3 100644
>>>> --- a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
>>>> +++ b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
>>>> @@ -29,6 +29,11 @@ VOID *mReservedApLoopFunc = NULL;
>>>> UINTN mReservedTopOfApStack;
>>>> volatile UINT32 mNumberToFinish = 0;
>>>>
>>>> +//
>>>> +// Begin wakeup buffer allocation below 0x88000
>>>> +//
>>>> +STATIC EFI_PHYSICAL_ADDRESS mSevEsDxeWakeupBuffer = 0x88000;
>>>> +
>>>> /**
>>>> Enable Debug Agent to support source debugging on AP function.
>>>>
>>>> @@ -102,7 +107,14 @@ GetWakeupBuffer (
>>>> // LagacyBios driver depends on CPU Arch protocol which guarantees below
>>>> // allocation runs earlier than LegacyBios driver.
>>>> //
>>>> - StartAddress = 0x88000;
>>>> + if (PcdGetBool (PcdSevEsIsEnabled)) {
>>>> + //
>>>> + // SEV-ES Wakeup buffer should be under 0x88000 and under any previous one
>>>> + //
>>>> + StartAddress = mSevEsDxeWakeupBuffer;
>>>> + } else {
>>>> + StartAddress = 0x88000;
>>>> + }
>>>> Status = gBS->AllocatePages (
>>>> AllocateMaxAddress,
>>>> MemoryType,
>>>> @@ -112,6 +124,11 @@ GetWakeupBuffer (
>>>> ASSERT_EFI_ERROR (Status);
>>>> if (EFI_ERROR (Status)) {
>>>> StartAddress = (EFI_PHYSICAL_ADDRESS) -1;
>>>> + } else if (PcdGetBool (PcdSevEsIsEnabled)) {
>>>> + //
>>>> + // Next SEV-ES wakeup buffer allocation must be below this allocation
>>>> + //
>>>> + mSevEsDxeWakeupBuffer = StartAddress;
>>>> }
>>>>
>>>> DEBUG ((DEBUG_INFO, "WakeupBufferStart = %x, WakeupBufferSize = %x\n",
>>>> diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Library/MpInitLib/MpLib.c
>>>> index dc2a54aa31e8..b9a06747edbf 100644
>>>> --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
>>>> +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
>>>> @@ -1164,20 +1164,6 @@ GetApResetVectorSize (
>>>> AddressMap->SwitchToRealSize +
>>>> sizeof (MP_CPU_EXCHANGE_INFO);
>>>>
>>>> - //
>>>> - // The AP reset stack is only used by SEV-ES guests. Do not add to the
>>>> - // allocation if SEV-ES is not enabled.
>>>> - //
>>>> - if (PcdGetBool (PcdSevEsIsEnabled)) {
>>>> - //
>>>> - // Stack location is based on APIC ID, so use the total number of
>>>> - // processors for calculating the total stack area.
>>>> - //
>>>> - Size += AP_RESET_STACK_SIZE * PcdGet32 (PcdCpuMaxLogicalProcessorNumber);
>>>> -
>>>> - Size = ALIGN_VALUE (Size, CPU_STACK_ALIGNMENT);
>>>> - }
>>>> -
>>>> return Size;
>>>> }
>>>>
>>>> @@ -1192,6 +1178,7 @@ AllocateResetVector (
>>>> )
>>>> {
>>>> UINTN ApResetVectorSize;
>>>> + UINTN ApResetStackSize;
>>>>
>>>> if (CpuMpData->WakeupBuffer == (UINTN) -1) {
>>>> ApResetVectorSize = GetApResetVectorSize (&CpuMpData->AddressMap);
>>>> @@ -1207,9 +1194,39 @@ AllocateResetVector (
>>>> CpuMpData->AddressMap.ModeTransitionOffset
>>>> );
>>>> //
>>>> - // The reset stack starts at the end of the buffer.
>>>> + // The AP reset stack is only used by SEV-ES guests. Do not allocate it
>>>> + // if SEV-ES is not enabled.
>>>> //
>>>> - CpuMpData->SevEsAPResetStackStart = CpuMpData->WakeupBuffer + ApResetVectorSize;
>>>> + if (PcdGetBool (PcdSevEsIsEnabled)) {
>>>> + //
>>>> + // Stack location is based on ProcessorNumber, so use the total number
>>>
>>> sneaky documenation fix :) I vaguely remember discussing earlier that
>>> the APIC ID reference was incorrect. OK.
>>
>> Yeah, I should have made mention of that in the commit message.
>>
>>>
>>>> + // of processors for calculating the total stack area.
>>>> + //
>>>> + ApResetStackSize = (AP_RESET_STACK_SIZE *
>>>> + PcdGet32 (PcdCpuMaxLogicalProcessorNumber));
>>>> +
>>>> + //
>>>> + // Invoke GetWakeupBuffer a second time to allocate the stack area
>>>> + // below 1MB. The returned buffer will be page aligned and sized and
>>>> + // below the previously allocated buffer.
>>>> + //
>>>> + CpuMpData->SevEsAPResetStackStart = GetWakeupBuffer (ApResetStackSize);
>>>> +
>>>> + //
>>>> + // Check to be sure that the "allocate below" behavior hasn't changed.
>>>> + // This will also catch a failed allocation, as "-1" is returned on
>>>> + // failure.
>>>> + //
>>>> + if (CpuMpData->SevEsAPResetStackStart >= CpuMpData->WakeupBuffer) {
>>>> + DEBUG ((
>>>> + DEBUG_ERROR,
>>>> + "SEV-ES AP reset stack is not below wakeup buffer\n"
>>>> + ));
>>>> +
>>>> + ASSERT (FALSE);
>>>> + CpuDeadLoop ();
>>>> + }
>>>> + }
>>>> }
>>>> BackupAndPrepareWakeupBuffer (CpuMpData);
>>>> }
>>>> diff --git a/UefiCpuPkg/Library/MpInitLib/PeiMpLib.c b/UefiCpuPkg/Library/MpInitLib/PeiMpLib.c
>>>> index 3989bd6a7a9f..90015c650c68 100644
>>>> --- a/UefiCpuPkg/Library/MpInitLib/PeiMpLib.c
>>>> +++ b/UefiCpuPkg/Library/MpInitLib/PeiMpLib.c
>>>> @@ -11,6 +11,8 @@
>>>> #include <Guid/S3SmmInitDone.h>
>>>> #include <Ppi/ShadowMicrocode.h>
>>>>
>>>> +STATIC UINT64 mSevEsPeiWakeupBuffer = BASE_1MB;
>>>> +
>>>> /**
>>>> S3 SMM Init Done notification function.
>>>>
>>>> @@ -220,7 +222,13 @@ GetWakeupBuffer (
>>>> // Need memory under 1MB to be collected here
>>>> //
>>>> WakeupBufferEnd = Hob.ResourceDescriptor->PhysicalStart + Hob.ResourceDescriptor->ResourceLength;
>>>> - if (WakeupBufferEnd > BASE_1MB) {
>>>> + if (PcdGetBool (PcdSevEsIsEnabled) &&
>>>> + WakeupBufferEnd > mSevEsPeiWakeupBuffer) {
>>>> + //
>>>> + // SEV-ES Wakeup buffer should be under 1MB and under any previous one
>>>> + //
>>>> + WakeupBufferEnd = mSevEsPeiWakeupBuffer;
>>>> + } else if (WakeupBufferEnd > BASE_1MB) {
>>>> //
>>>> // Wakeup buffer should be under 1MB
>>>> //
>>>> @@ -244,6 +252,15 @@ GetWakeupBuffer (
>>>> }
>>>> DEBUG ((DEBUG_INFO, "WakeupBufferStart = %x, WakeupBufferSize = %x\n",
>>>> WakeupBufferStart, WakeupBufferSize));
>>>> +
>>>> + if (PcdGetBool (PcdSevEsIsEnabled)) {
>>>> + //
>>>> + // Next SEV-ES wakeup buffer allocation must be below this
>>>> + // allocation
>>>> + //
>>>> + mSevEsPeiWakeupBuffer = WakeupBufferStart;
>>>> + }
>>>> +
>>>> return (UINTN)WakeupBufferStart;
>>>> }
>>>> }
>>>>
>>>
>>> The code in the patch seems sound to me, but, now that I've managed to
>>> take a bit more careful look, I think the patch is incomplete.
>>>
>>> Note the BackupAndPrepareWakeupBuffer() function call -- visible in the
>>> context --, at the end of the AllocateResetVector() function! Before, we
>>> had a single area allocated for the reset vector, which was
>>> appropriately sized for SEV-ES stacks too, in case SEV-ES was enabled.
>>>
>>> That was because MpInitLibInitialize() called GetApResetVectorSize()
>>> too, and stored the result to the "BackupBufferSize" field. Thus, the
>>> BackupAndPrepareWakeupBuffer() function, and its counterpart
>>> RestoreWakeupBuffer(), would include the SEV-ES AP stacks area in the
>>> backup/restore operations.
>>
>> The restore is not performed for an SEV-ES guest (see FreeResetVector()),
>> because the memory is still needed.
>
> I apologize for missing this. I'm not too familiar with the SEV-ES
> specifics in UefiCpuPkg.
>
> One question: given that FreeResetVector() does not call
> RestoreWakeupBuffer() when SEV-ES is enabled, would it make sense for
> AllocateResetVector() to not call BackupAndPrepareWakeupBuffer() either,
> in case SEV-ES is enabled? Because, if we never restore, do we really
> need the backup? I wonder if such a patch could be prepended to this one
> (in order to form a 2-patch series).
>
> (Well, BackupAndPrepareWakeupBuffer() performs two things, backup and
> preparation -- I guess we certainly need the preparation of the wake up
> buffer, but do we need to back up the original contents of the area?
> Including the backup buffer allocation.)
We don't need to, but it doesn't hurt. I wanted to avoid sprinkling too
many SEV-ES specific checks throughout the code.
>
>>
>>>
>>> But now, with SEV-ES enabled, we'll have a separate, discontiguous area
>>> -- and neither BackupAndPrepareWakeupBuffer(), nor its counterpart
>>> RestoreWakeupBuffer() take that into account.
>>>
>>> Therefore I think, while this patch is regression-free for the SEV-ES
>>> *disabled* case, it may corrupt memory (through not restoring the AP
>>> stack area's original contents) with SEV-ES enabled.
>>
>> This is the current behavior for SEV-ES. The wakeup buffer memory is
>> marked as reserved, at least in the DXE phase.
>>
>>>
>>> I think we need to turn "ApResetStackSize" into an explicit field. It
>>> should have a defined value only when SEV-ES is enabled. And for that
>>> case, we seem to need a *separate backup buffer* too.
>>>
>>> FWIW, I'd really like to re-classify this BZ as a Feature Request (see
>>> the Product field on BZ#3324), and I'd really like to delay the patch
>>> until after edk2-stable202105. The patch is not necessary for using
>>> SEV-ES, but it has a chance to break SEV-ES.
>>
>> I'm ok with delaying this if you want.
>
> Here's what I'd like to do:
>
> - Reach an agreement with Marvin about the ASSERT(). I'm fine if we drop
> it, and fine if we keep it.
I can drop it since there's already a debug message issued at that point.
>
> - Eric or Ray to check the patch as well, because (as I said above) I
> didn't follow the SEV-ES patches for UefiCpuPkg (that series was just
> huge, apologies), and so now I don't have memories to reach back to.
>
> - Figure out if we need to conditionalize the
> BackupAndPrepareWakeupBuffer() call (or a part of that function anyway).
>
> We can and should continue discussing these things during the feature
> freeze; I'd like us to merge the patch after the tag.
>
> Sorry again that I'm missing bits and pieces; I'm this close |<->| to
> email bankruptcy.
I know that feeling, stay solvent!
Thanks,
Tom
>
> Thanks
> Laszlo
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#75420): https://edk2.groups.io/g/devel/message/75420
Mute This Topic: https://groups.io/mt/82833645/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
On 05/17/21 17:03, Lendacky, Thomas wrote: > On 5/16/21 11:22 PM, Laszlo Ersek wrote: >> But now, with SEV-ES enabled, we'll have a separate, discontiguous area >> -- and neither BackupAndPrepareWakeupBuffer(), nor its counterpart >> RestoreWakeupBuffer() take that into account. >> >> Therefore I think, while this patch is regression-free for the SEV-ES >> *disabled* case, it may corrupt memory (through not restoring the AP >> stack area's original contents) with SEV-ES enabled. > > This is the current behavior for SEV-ES. The wakeup buffer memory is > marked as reserved, at least in the DXE phase. Another question that occurred to me later: where does this reservation happen? If we have a separate allocation for the AP stacks now, do we need to reserve that too, separately? What about PEI in general? Why is there no risk of corruption there? (Sorry if these are lame questions!) Thanks, Laszlo -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#75359): https://edk2.groups.io/g/devel/message/75359 Mute This Topic: https://groups.io/mt/82833645/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
On 5/19/21 2:27 AM, Laszlo Ersek wrote: > On 05/17/21 17:03, Lendacky, Thomas wrote: >> On 5/16/21 11:22 PM, Laszlo Ersek wrote: > >>> But now, with SEV-ES enabled, we'll have a separate, discontiguous area >>> -- and neither BackupAndPrepareWakeupBuffer(), nor its counterpart >>> RestoreWakeupBuffer() take that into account. >>> >>> Therefore I think, while this patch is regression-free for the SEV-ES >>> *disabled* case, it may corrupt memory (through not restoring the AP >>> stack area's original contents) with SEV-ES enabled. >> >> This is the current behavior for SEV-ES. The wakeup buffer memory is >> marked as reserved, at least in the DXE phase. > > Another question that occurred to me later: where does this reservation > happen? If we have a separate allocation for the AP stacks now, do we > need to reserve that too, separately? The GetWakeupBuffer() in DxeMpLib.c will perform AllocatePages() with a memory type of "EfiReservedMemoryType" for SEV-ES. Since, with this patch, it is called twice (once for the reset vector area and once for the stack area) and both allocations will be EfiReservedMemoryType. > > What about PEI in general? Why is there no risk of corruption there? That's a good question. The PEI GetWakeupBuffer() looks for memory that hasn't been allocated below 1MB and just uses it - no reservation or allocation. For OVMF, I imagine that nothing is really populated there and so it is truly just "free" memory that doesn't need to be restored. Thanks, Tom > > (Sorry if these are lame questions!) > > Thanks, > Laszlo > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#75422): https://edk2.groups.io/g/devel/message/75422 Mute This Topic: https://groups.io/mt/82833645/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
On 05/14/21 22:28, Lendacky, Thomas wrote:
> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3324
>
> The SEV-ES stacks currently share a page with the reset code and data.
> Separate the SEV-ES stacks from the reset vector code and data to avoid
> possible stack overflows from overwriting the code and/or data.
>
> When SEV-ES is enabled, invoke the GetWakeupBuffer() routine a second time
> to allocate a new area, below the reset vector and data.
>
> Both the PEI and DXE versions of GetWakeupBuffer() are changed so that
> when PcdSevEsIsEnabled is true, they will track the previous reset buffer
> allocation in order to ensure that the new buffer allocation is below the
> previous allocation. When PcdSevEsIsEnabled is false, the original logic
> is followed.
>
> Fixes: 7b7508ad784d16a5208c8d12dff43aef6df0835b
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Rahul Kumar <rahul1.kumar@intel.com>
> Cc: Marvin Häuser <mhaeuser@posteo.de>
> Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
>
> ---
>
> Changes since v1:
> - Renamed the wakeup buffer variables to be unique in the PEI and DXE
> libraries and to make it obvious they are SEV-ES specific.
> - Use PcdGetBool (PcdSevEsIsEnabled) to make the changes regression-free
> so that the new support is only use for SEV-ES guests.
> ---
> UefiCpuPkg/Library/MpInitLib/DxeMpLib.c | 19 +++++++-
> UefiCpuPkg/Library/MpInitLib/MpLib.c | 49 +++++++++++++-------
> UefiCpuPkg/Library/MpInitLib/PeiMpLib.c | 19 +++++++-
> 3 files changed, 69 insertions(+), 18 deletions(-)
>
> diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
> index 7839c249760e..93fc63bf93e3 100644
> --- a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
> +++ b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
> @@ -29,6 +29,11 @@ VOID *mReservedApLoopFunc = NULL;
> UINTN mReservedTopOfApStack;
> volatile UINT32 mNumberToFinish = 0;
>
> +//
> +// Begin wakeup buffer allocation below 0x88000
> +//
> +STATIC EFI_PHYSICAL_ADDRESS mSevEsDxeWakeupBuffer = 0x88000;
> +
> /**
> Enable Debug Agent to support source debugging on AP function.
>
> @@ -102,7 +107,14 @@ GetWakeupBuffer (
> // LagacyBios driver depends on CPU Arch protocol which guarantees below
> // allocation runs earlier than LegacyBios driver.
> //
> - StartAddress = 0x88000;
> + if (PcdGetBool (PcdSevEsIsEnabled)) {
> + //
> + // SEV-ES Wakeup buffer should be under 0x88000 and under any previous one
> + //
> + StartAddress = mSevEsDxeWakeupBuffer;
> + } else {
> + StartAddress = 0x88000;
> + }
> Status = gBS->AllocatePages (
> AllocateMaxAddress,
> MemoryType,
> @@ -112,6 +124,11 @@ GetWakeupBuffer (
> ASSERT_EFI_ERROR (Status);
> if (EFI_ERROR (Status)) {
> StartAddress = (EFI_PHYSICAL_ADDRESS) -1;
> + } else if (PcdGetBool (PcdSevEsIsEnabled)) {
> + //
> + // Next SEV-ES wakeup buffer allocation must be below this allocation
> + //
> + mSevEsDxeWakeupBuffer = StartAddress;
> }
>
> DEBUG ((DEBUG_INFO, "WakeupBufferStart = %x, WakeupBufferSize = %x\n",
> diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> index dc2a54aa31e8..b9a06747edbf 100644
> --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
> +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> @@ -1164,20 +1164,6 @@ GetApResetVectorSize (
> AddressMap->SwitchToRealSize +
> sizeof (MP_CPU_EXCHANGE_INFO);
>
> - //
> - // The AP reset stack is only used by SEV-ES guests. Do not add to the
> - // allocation if SEV-ES is not enabled.
> - //
> - if (PcdGetBool (PcdSevEsIsEnabled)) {
> - //
> - // Stack location is based on APIC ID, so use the total number of
> - // processors for calculating the total stack area.
> - //
> - Size += AP_RESET_STACK_SIZE * PcdGet32 (PcdCpuMaxLogicalProcessorNumber);
> -
> - Size = ALIGN_VALUE (Size, CPU_STACK_ALIGNMENT);
> - }
> -
> return Size;
> }
>
> @@ -1192,6 +1178,7 @@ AllocateResetVector (
> )
> {
> UINTN ApResetVectorSize;
> + UINTN ApResetStackSize;
>
> if (CpuMpData->WakeupBuffer == (UINTN) -1) {
> ApResetVectorSize = GetApResetVectorSize (&CpuMpData->AddressMap);
> @@ -1207,9 +1194,39 @@ AllocateResetVector (
> CpuMpData->AddressMap.ModeTransitionOffset
> );
> //
> - // The reset stack starts at the end of the buffer.
> + // The AP reset stack is only used by SEV-ES guests. Do not allocate it
> + // if SEV-ES is not enabled.
> //
> - CpuMpData->SevEsAPResetStackStart = CpuMpData->WakeupBuffer + ApResetVectorSize;
> + if (PcdGetBool (PcdSevEsIsEnabled)) {
> + //
> + // Stack location is based on ProcessorNumber, so use the total number
> + // of processors for calculating the total stack area.
> + //
> + ApResetStackSize = (AP_RESET_STACK_SIZE *
> + PcdGet32 (PcdCpuMaxLogicalProcessorNumber));
> +
> + //
> + // Invoke GetWakeupBuffer a second time to allocate the stack area
> + // below 1MB. The returned buffer will be page aligned and sized and
> + // below the previously allocated buffer.
> + //
> + CpuMpData->SevEsAPResetStackStart = GetWakeupBuffer (ApResetStackSize);
> +
> + //
> + // Check to be sure that the "allocate below" behavior hasn't changed.
> + // This will also catch a failed allocation, as "-1" is returned on
> + // failure.
> + //
> + if (CpuMpData->SevEsAPResetStackStart >= CpuMpData->WakeupBuffer) {
> + DEBUG ((
> + DEBUG_ERROR,
> + "SEV-ES AP reset stack is not below wakeup buffer\n"
> + ));
> +
> + ASSERT (FALSE);
> + CpuDeadLoop ();
> + }
> + }
> }
> BackupAndPrepareWakeupBuffer (CpuMpData);
> }
> diff --git a/UefiCpuPkg/Library/MpInitLib/PeiMpLib.c b/UefiCpuPkg/Library/MpInitLib/PeiMpLib.c
> index 3989bd6a7a9f..90015c650c68 100644
> --- a/UefiCpuPkg/Library/MpInitLib/PeiMpLib.c
> +++ b/UefiCpuPkg/Library/MpInitLib/PeiMpLib.c
> @@ -11,6 +11,8 @@
> #include <Guid/S3SmmInitDone.h>
> #include <Ppi/ShadowMicrocode.h>
>
> +STATIC UINT64 mSevEsPeiWakeupBuffer = BASE_1MB;
> +
> /**
> S3 SMM Init Done notification function.
>
> @@ -220,7 +222,13 @@ GetWakeupBuffer (
> // Need memory under 1MB to be collected here
> //
> WakeupBufferEnd = Hob.ResourceDescriptor->PhysicalStart + Hob.ResourceDescriptor->ResourceLength;
> - if (WakeupBufferEnd > BASE_1MB) {
> + if (PcdGetBool (PcdSevEsIsEnabled) &&
> + WakeupBufferEnd > mSevEsPeiWakeupBuffer) {
> + //
> + // SEV-ES Wakeup buffer should be under 1MB and under any previous one
> + //
> + WakeupBufferEnd = mSevEsPeiWakeupBuffer;
> + } else if (WakeupBufferEnd > BASE_1MB) {
> //
> // Wakeup buffer should be under 1MB
> //
> @@ -244,6 +252,15 @@ GetWakeupBuffer (
> }
> DEBUG ((DEBUG_INFO, "WakeupBufferStart = %x, WakeupBufferSize = %x\n",
> WakeupBufferStart, WakeupBufferSize));
> +
> + if (PcdGetBool (PcdSevEsIsEnabled)) {
> + //
> + // Next SEV-ES wakeup buffer allocation must be below this
> + // allocation
> + //
> + mSevEsPeiWakeupBuffer = WakeupBufferStart;
> + }
> +
> return (UINTN)WakeupBufferStart;
> }
> }
>
Acked-by: Laszlo Ersek <lersek@redhat.com>
Should I forget to merge this after the stable tag, please remind me.
Thanks!
Laszlo
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#75415): https://edk2.groups.io/g/devel/message/75415
Mute This Topic: https://groups.io/mt/82833645/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
On 05/14/21 22:28, Lendacky, Thomas wrote:
> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3324
>
> The SEV-ES stacks currently share a page with the reset code and data.
> Separate the SEV-ES stacks from the reset vector code and data to avoid
> possible stack overflows from overwriting the code and/or data.
>
> When SEV-ES is enabled, invoke the GetWakeupBuffer() routine a second time
> to allocate a new area, below the reset vector and data.
>
> Both the PEI and DXE versions of GetWakeupBuffer() are changed so that
> when PcdSevEsIsEnabled is true, they will track the previous reset buffer
> allocation in order to ensure that the new buffer allocation is below the
> previous allocation. When PcdSevEsIsEnabled is false, the original logic
> is followed.
>
> Fixes: 7b7508ad784d16a5208c8d12dff43aef6df0835b
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Rahul Kumar <rahul1.kumar@intel.com>
> Cc: Marvin Häuser <mhaeuser@posteo.de>
> Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
>
> ---
>
> Changes since v1:
> - Renamed the wakeup buffer variables to be unique in the PEI and DXE
> libraries and to make it obvious they are SEV-ES specific.
> - Use PcdGetBool (PcdSevEsIsEnabled) to make the changes regression-free
> so that the new support is only use for SEV-ES guests.
> ---
> UefiCpuPkg/Library/MpInitLib/DxeMpLib.c | 19 +++++++-
> UefiCpuPkg/Library/MpInitLib/MpLib.c | 49 +++++++++++++-------
> UefiCpuPkg/Library/MpInitLib/PeiMpLib.c | 19 +++++++-
> 3 files changed, 69 insertions(+), 18 deletions(-)
>
> diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
> index 7839c249760e..93fc63bf93e3 100644
> --- a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
> +++ b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
> @@ -29,6 +29,11 @@ VOID *mReservedApLoopFunc = NULL;
> UINTN mReservedTopOfApStack;
> volatile UINT32 mNumberToFinish = 0;
>
> +//
> +// Begin wakeup buffer allocation below 0x88000
> +//
> +STATIC EFI_PHYSICAL_ADDRESS mSevEsDxeWakeupBuffer = 0x88000;
> +
> /**
> Enable Debug Agent to support source debugging on AP function.
>
> @@ -102,7 +107,14 @@ GetWakeupBuffer (
> // LagacyBios driver depends on CPU Arch protocol which guarantees below
> // allocation runs earlier than LegacyBios driver.
> //
> - StartAddress = 0x88000;
> + if (PcdGetBool (PcdSevEsIsEnabled)) {
> + //
> + // SEV-ES Wakeup buffer should be under 0x88000 and under any previous one
> + //
> + StartAddress = mSevEsDxeWakeupBuffer;
> + } else {
> + StartAddress = 0x88000;
> + }
> Status = gBS->AllocatePages (
> AllocateMaxAddress,
> MemoryType,
> @@ -112,6 +124,11 @@ GetWakeupBuffer (
> ASSERT_EFI_ERROR (Status);
> if (EFI_ERROR (Status)) {
> StartAddress = (EFI_PHYSICAL_ADDRESS) -1;
> + } else if (PcdGetBool (PcdSevEsIsEnabled)) {
> + //
> + // Next SEV-ES wakeup buffer allocation must be below this allocation
> + //
> + mSevEsDxeWakeupBuffer = StartAddress;
> }
>
> DEBUG ((DEBUG_INFO, "WakeupBufferStart = %x, WakeupBufferSize = %x\n",
> diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> index dc2a54aa31e8..b9a06747edbf 100644
> --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
> +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> @@ -1164,20 +1164,6 @@ GetApResetVectorSize (
> AddressMap->SwitchToRealSize +
> sizeof (MP_CPU_EXCHANGE_INFO);
>
> - //
> - // The AP reset stack is only used by SEV-ES guests. Do not add to the
> - // allocation if SEV-ES is not enabled.
> - //
> - if (PcdGetBool (PcdSevEsIsEnabled)) {
> - //
> - // Stack location is based on APIC ID, so use the total number of
> - // processors for calculating the total stack area.
> - //
> - Size += AP_RESET_STACK_SIZE * PcdGet32 (PcdCpuMaxLogicalProcessorNumber);
> -
> - Size = ALIGN_VALUE (Size, CPU_STACK_ALIGNMENT);
> - }
> -
> return Size;
> }
>
> @@ -1192,6 +1178,7 @@ AllocateResetVector (
> )
> {
> UINTN ApResetVectorSize;
> + UINTN ApResetStackSize;
>
> if (CpuMpData->WakeupBuffer == (UINTN) -1) {
> ApResetVectorSize = GetApResetVectorSize (&CpuMpData->AddressMap);
> @@ -1207,9 +1194,39 @@ AllocateResetVector (
> CpuMpData->AddressMap.ModeTransitionOffset
> );
> //
> - // The reset stack starts at the end of the buffer.
> + // The AP reset stack is only used by SEV-ES guests. Do not allocate it
> + // if SEV-ES is not enabled.
> //
> - CpuMpData->SevEsAPResetStackStart = CpuMpData->WakeupBuffer + ApResetVectorSize;
> + if (PcdGetBool (PcdSevEsIsEnabled)) {
> + //
> + // Stack location is based on ProcessorNumber, so use the total number
> + // of processors for calculating the total stack area.
> + //
> + ApResetStackSize = (AP_RESET_STACK_SIZE *
> + PcdGet32 (PcdCpuMaxLogicalProcessorNumber));
> +
> + //
> + // Invoke GetWakeupBuffer a second time to allocate the stack area
> + // below 1MB. The returned buffer will be page aligned and sized and
> + // below the previously allocated buffer.
> + //
> + CpuMpData->SevEsAPResetStackStart = GetWakeupBuffer (ApResetStackSize);
> +
> + //
> + // Check to be sure that the "allocate below" behavior hasn't changed.
> + // This will also catch a failed allocation, as "-1" is returned on
> + // failure.
> + //
> + if (CpuMpData->SevEsAPResetStackStart >= CpuMpData->WakeupBuffer) {
> + DEBUG ((
> + DEBUG_ERROR,
> + "SEV-ES AP reset stack is not below wakeup buffer\n"
> + ));
> +
> + ASSERT (FALSE);
> + CpuDeadLoop ();
> + }
> + }
> }
> BackupAndPrepareWakeupBuffer (CpuMpData);
> }
> diff --git a/UefiCpuPkg/Library/MpInitLib/PeiMpLib.c b/UefiCpuPkg/Library/MpInitLib/PeiMpLib.c
> index 3989bd6a7a9f..90015c650c68 100644
> --- a/UefiCpuPkg/Library/MpInitLib/PeiMpLib.c
> +++ b/UefiCpuPkg/Library/MpInitLib/PeiMpLib.c
> @@ -11,6 +11,8 @@
> #include <Guid/S3SmmInitDone.h>
> #include <Ppi/ShadowMicrocode.h>
>
> +STATIC UINT64 mSevEsPeiWakeupBuffer = BASE_1MB;
> +
> /**
> S3 SMM Init Done notification function.
>
> @@ -220,7 +222,13 @@ GetWakeupBuffer (
> // Need memory under 1MB to be collected here
> //
> WakeupBufferEnd = Hob.ResourceDescriptor->PhysicalStart + Hob.ResourceDescriptor->ResourceLength;
> - if (WakeupBufferEnd > BASE_1MB) {
> + if (PcdGetBool (PcdSevEsIsEnabled) &&
> + WakeupBufferEnd > mSevEsPeiWakeupBuffer) {
> + //
> + // SEV-ES Wakeup buffer should be under 1MB and under any previous one
> + //
> + WakeupBufferEnd = mSevEsPeiWakeupBuffer;
> + } else if (WakeupBufferEnd > BASE_1MB) {
> //
> // Wakeup buffer should be under 1MB
> //
> @@ -244,6 +252,15 @@ GetWakeupBuffer (
> }
> DEBUG ((DEBUG_INFO, "WakeupBufferStart = %x, WakeupBufferSize = %x\n",
> WakeupBufferStart, WakeupBufferSize));
> +
> + if (PcdGetBool (PcdSevEsIsEnabled)) {
> + //
> + // Next SEV-ES wakeup buffer allocation must be below this
> + // allocation
> + //
> + mSevEsPeiWakeupBuffer = WakeupBufferStart;
> + }
> +
> return (UINTN)WakeupBufferStart;
> }
> }
>
Merged in commit dbc22a178546 via
<https://github.com/tianocore/edk2/pull/1674>.
Thanks
Laszlo
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#75848): https://edk2.groups.io/g/devel/message/75848
Mute This Topic: https://groups.io/mt/82833645/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
© 2016 - 2024 Red Hat, Inc.