Synchronize with edk2 package where StandaloneMmCpu component has moved
from StandaloneMmPkg/Drivers/StandaloneMmCpu/AArch64/StandaloneMmCpu.inf
to StandaloneMmPkg/Drivers/StandaloneMmCpu/StandaloneMmCpu.inf

Cc: Ard Biesheuvel <ardb+tianocore@...>
Cc: Ilias Apalodimas <ilias.apalodimas@...>
Cc: Leif Lindholm <leif@...>
Cc: Sami Mujawar <sami.mujawar@...>
Cc: Sughosh Ganu <sughosh.ganu@...>
Cc: Thomas Abraham <thomas.abraham@...>
Signed-off-by: Etienne Carriere <etienne.carriere@...>
---
Changes since v1:
- split change in 3: this change relates to Platform/ARM/SgiPkg only.
---
Platform/ARM/SgiPkg/PlatformStandaloneMm.dsc | 2 +-
Platform/ARM/SgiPkg/PlatformStandaloneMm.fdf | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/Platform/ARM/SgiPkg/PlatformStandaloneMm.dsc b/Platform/ARM/SgiPkg/PlatformStandaloneMm.dsc
index e281d54909..1e0af23711 100644
--- a/Platform/ARM/SgiPkg/PlatformStandaloneMm.dsc
+++ b/Platform/ARM/SgiPkg/PlatformStandaloneMm.dsc
@@ -122,7 +122,7 @@
StandaloneMmPkg/Core/StandaloneMmCore.inf

[Components.AARCH64]
- StandaloneMmPkg/Drivers/StandaloneMmCpu/AArch64/StandaloneMmCpu.inf
+ StandaloneMmPkg/Drivers/StandaloneMmCpu/StandaloneMmCpu.inf

###################################################################################################
#
diff --git a/Platform/ARM/SgiPkg/PlatformStandaloneMm.fdf b/Platform/ARM/SgiPkg/PlatformStandaloneMm.fdf
index 5a0772cd85..96b4272dd6 100644
--- a/Platform/ARM/SgiPkg/PlatformStandaloneMm.fdf
+++ b/Platform/ARM/SgiPkg/PlatformStandaloneMm.fdf
@@ -49,7 +49,7 @@ READ_LOCK_CAP = TRUE
READ_LOCK_STATUS = TRUE

INF StandaloneMmPkg/Core/StandaloneMmCore.inf
- INF StandaloneMmPkg/Drivers/StandaloneMmCpu/AArch64/StandaloneMmCpu.inf
+ INF StandaloneMmPkg/Drivers/StandaloneMmCpu/StandaloneMmCpu.inf

################################################################################
#
--
2.17.1

This change allows to build StandaloneMmPkg components for 32bit Arm
StandaloneMm firmware.

This change mainly moves AArch64/ source files to Arm/ side directory
for several components: StandaloneMmCpu, StandaloneMmCoreEntryPoint
and StandaloneMmMemLib. The source file is built for both 32b and 64b
Arm targets.

Cc: Achin Gupta <achin.gupta@...>
Cc: Ard Biesheuvel <ardb+tianocore@...>
Cc: Jiewen Yao <jiewen.yao@...>
Cc: Leif Lindholm <leif@...>
Cc: Sami Mujawar <sami.mujawar@...>
Cc: Sughosh Ganu <sughosh.ganu@...>
Signed-off-by: Etienne Carriere <etienne.carriere@...>
---
Changes since v1:
- ARM_SMC_ID_MM_COMMUNICATE 32b/64b agnostic helper ID is defined
in ArmStdSmc.h (see 1st commit in this series) instead of being
local to EventHandle.c.
- Fix void occurrence to VOID.
- Fix path in StandaloneMmPkg/StandaloneMmPkg.dsc
---
StandaloneMmPkg/Core/StandaloneMmCore.inf | 2 +-
StandaloneMmPkg/Drivers/StandaloneMmCpu/{AArch64 => }/EventHandle.c | 5 +++--
StandaloneMmPkg/Drivers/StandaloneMmCpu/{AArch64 => }/StandaloneMmCpu.c | 2 +-
StandaloneMmPkg/Drivers/StandaloneMmCpu/{AArch64 => }/StandaloneMmCpu.h | 0
StandaloneMmPkg/Drivers/StandaloneMmCpu/{AArch64 => }/StandaloneMmCpu.inf | 0
StandaloneMmPkg/Include/Library/{AArch64 => Arm}/StandaloneMmCoreEntryPoint.h | 0
StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/{AArch64 => Arm}/CreateHobList.c | 2 +-
StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/{AArch64 => Arm}/SetPermissions.c | 2 +-
StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/{AArch64 => Arm}/StandaloneMmCoreEntryPoint.c | 16 ++++++++--------
StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/StandaloneMmCoreEntryPoint.inf | 14 +++++++-------
StandaloneMmPkg/Library/StandaloneMmCoreHobLib/{AArch64 => Arm}/StandaloneMmCoreHobLib.c | 0
StandaloneMmPkg/Library/StandaloneMmCoreHobLib/{AArch64 => Arm}/StandaloneMmCoreHobLibInternal.c | 0
StandaloneMmPkg/Library/StandaloneMmCoreHobLib/StandaloneMmCoreHobLib.inf | 8 ++++----
StandaloneMmPkg/Library/StandaloneMmMemLib/{AArch64/StandaloneMmMemLibInternal.c => ArmStandaloneMmMemLibInternal.c} | 9 ++++++++-
StandaloneMmPkg/Library/StandaloneMmMemLib/StandaloneMmMemLib.inf | 6 +++---
StandaloneMmPkg/Library/VariableMmDependency/VariableMmDependency.inf | 2 +-
StandaloneMmPkg/StandaloneMmPkg.dsc | 10 +++++-----
17 files changed, 43 insertions(+), 35 deletions(-)

diff --git a/StandaloneMmPkg/Core/StandaloneMmCore.inf b/StandaloneMmPkg/Core/StandaloneMmCore.inf
index 87bf6e9440..56042b7b39 100644
--- a/StandaloneMmPkg/Core/StandaloneMmCore.inf
+++ b/StandaloneMmPkg/Core/StandaloneMmCore.inf
@@ -17,7 +17,7 @@
PI_SPECIFICATION_VERSION = 0x00010032
ENTRY_POINT = StandaloneMmMain

-# VALID_ARCHITECTURES = IA32 X64 AARCH64
+# VALID_ARCHITECTURES = IA32 X64 AARCH64 ARM

[Sources]
StandaloneMmCore.c
diff --git a/StandaloneMmPkg/Drivers/StandaloneMmCpu/AArch64/EventHandle.c b/StandaloneMmPkg/Drivers/StandaloneMmCpu/EventHandle.c
similarity index 95%
rename from StandaloneMmPkg/Drivers/StandaloneMmCpu/AArch64/EventHandle.c
rename to StandaloneMmPkg/Drivers/StandaloneMmCpu/EventHandle.c
index 63fbe26642..165d696f99 100644
--- a/StandaloneMmPkg/Drivers/StandaloneMmCpu/AArch64/EventHandle.c
+++ b/StandaloneMmPkg/Drivers/StandaloneMmCpu/EventHandle.c
@@ -2,6 +2,7 @@

Copyright (c) 2016 HP Development Company, L.P.
Copyright (c) 2016 - 2021, Arm Limited. All rights reserved.
+ Copyright (c) 2021, Linaro Limited

SPDX-License-Identifier: BSD-2-Clause-Patent

@@ -92,8 +93,8 @@ PiMmStandaloneArmTfCpuDriverEntry (
// receipt of a synchronous MM request. Use the Event ID to distinguish
// between synchronous and asynchronous events.
//
- if ((ARM_SMC_ID_MM_COMMUNICATE_AARCH64 != EventId) &&
- (ARM_SVC_ID_FFA_MSG_SEND_DIRECT_REQ_AARCH64 != EventId)) {
+ if ((ARM_SMC_ID_MM_COMMUNICATE != EventId) &&
+ (ARM_SVC_ID_FFA_MSG_SEND_DIRECT_REQ != EventId)) {
DEBUG ((DEBUG_INFO, "UnRecognized Event - 0x%x\n", EventId));
return EFI_INVALID_PARAMETER;
}
diff --git a/StandaloneMmPkg/Drivers/StandaloneMmCpu/AArch64/StandaloneMmCpu.c b/StandaloneMmPkg/Drivers/StandaloneMmCpu/StandaloneMmCpu.c
similarity index 96%
rename from StandaloneMmPkg/Drivers/StandaloneMmCpu/AArch64/StandaloneMmCpu.c
rename to StandaloneMmPkg/Drivers/StandaloneMmCpu/StandaloneMmCpu.c
index d4590bcd19..10097f792f 100644
--- a/StandaloneMmPkg/Drivers/StandaloneMmCpu/AArch64/StandaloneMmCpu.c
+++ b/StandaloneMmPkg/Drivers/StandaloneMmCpu/StandaloneMmCpu.c
@@ -10,7 +10,7 @@

#include <Base.h>
#include <Pi/PiMmCis.h>
-#include <Library/AArch64/StandaloneMmCoreEntryPoint.h>
+#include <Library/Arm/StandaloneMmCoreEntryPoint.h>
#include <Library/DebugLib.h>
#include <Library/ArmSvcLib.h>
#include <Library/ArmLib.h>
diff --git a/StandaloneMmPkg/Drivers/StandaloneMmCpu/AArch64/StandaloneMmCpu.h b/StandaloneMmPkg/Drivers/StandaloneMmCpu/StandaloneMmCpu.h
similarity index 100%
rename from StandaloneMmPkg/Drivers/StandaloneMmCpu/AArch64/StandaloneMmCpu.h
rename to StandaloneMmPkg/Drivers/StandaloneMmCpu/StandaloneMmCpu.h
diff --git a/StandaloneMmPkg/Drivers/StandaloneMmCpu/AArch64/StandaloneMmCpu.inf b/StandaloneMmPkg/Drivers/StandaloneMmCpu/StandaloneMmCpu.inf
similarity index 100%
rename from StandaloneMmPkg/Drivers/StandaloneMmCpu/AArch64/StandaloneMmCpu.inf
rename to StandaloneMmPkg/Drivers/StandaloneMmCpu/StandaloneMmCpu.inf
diff --git a/StandaloneMmPkg/Include/Library/AArch64/StandaloneMmCoreEntryPoint.h b/StandaloneMmPkg/Include/Library/Arm/StandaloneMmCoreEntryPoint.h
similarity index 100%
rename from StandaloneMmPkg/Include/Library/AArch64/StandaloneMmCoreEntryPoint.h
rename to StandaloneMmPkg/Include/Library/Arm/StandaloneMmCoreEntryPoint.h
diff --git a/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/AArch64/CreateHobList.c b/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/Arm/CreateHobList.c
similarity index 97%
rename from StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/AArch64/CreateHobList.c
rename to StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/Arm/CreateHobList.c
index 4d4cf3d5ff..85f8194687 100644
--- a/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/AArch64/CreateHobList.c
+++ b/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/Arm/CreateHobList.c
@@ -14,7 +14,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
#include <Guid/MmramMemoryReserve.h>
#include <Guid/MpInformation.h>

-#include <Library/AArch64/StandaloneMmCoreEntryPoint.h>
+#include <Library/Arm/StandaloneMmCoreEntryPoint.h>
#include <Library/ArmMmuLib.h>
#include <Library/ArmSvcLib.h>
#include <Library/DebugLib.h>
diff --git a/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/AArch64/SetPermissions.c b/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/Arm/SetPermissions.c
similarity index 96%
rename from StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/AArch64/SetPermissions.c
rename to StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/Arm/SetPermissions.c
index 4a380df4a6..cd4b90823e 100644
--- a/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/AArch64/SetPermissions.c
+++ b/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/Arm/SetPermissions.c
@@ -14,7 +14,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
#include <Guid/MmramMemoryReserve.h>
#include <Guid/MpInformation.h>

-#include <Library/AArch64/StandaloneMmCoreEntryPoint.h>
+#include <Library/Arm/StandaloneMmCoreEntryPoint.h>
#include <Library/ArmMmuLib.h>
#include <Library/ArmSvcLib.h>
#include <Library/DebugLib.h>
diff --git a/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/AArch64/StandaloneMmCoreEntryPoint.c b/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/Arm/StandaloneMmCoreEntryPoint.c
similarity index 94%
rename from StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/AArch64/StandaloneMmCoreEntryPoint.c
rename to StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/Arm/StandaloneMmCoreEntryPoint.c
index b445d6942e..49cf51a789 100644
--- a/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/AArch64/StandaloneMmCoreEntryPoint.c
+++ b/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/Arm/StandaloneMmCoreEntryPoint.c
@@ -10,7 +10,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent

#include <PiMm.h>

-#include <Library/AArch64/StandaloneMmCoreEntryPoint.h>
+#include <Library/Arm/StandaloneMmCoreEntryPoint.h>

#include <PiPei.h>
#include <Guid/MmramMemoryReserve.h>
@@ -182,13 +182,13 @@ DelegatedEventLoop (
}

if (FfaEnabled) {
- EventCompleteSvcArgs->Arg0 = ARM_SVC_ID_FFA_MSG_SEND_DIRECT_RESP_AARCH64;
+ EventCompleteSvcArgs->Arg0 = ARM_SVC_ID_FFA_MSG_SEND_DIRECT_RESP;
EventCompleteSvcArgs->Arg1 = 0;
EventCompleteSvcArgs->Arg2 = 0;
- EventCompleteSvcArgs->Arg3 = ARM_SVC_ID_SP_EVENT_COMPLETE_AARCH64;
+ EventCompleteSvcArgs->Arg3 = ARM_SVC_ID_SP_EVENT_COMPLETE;
EventCompleteSvcArgs->Arg4 = SvcStatus;
} else {
- EventCompleteSvcArgs->Arg0 = ARM_SVC_ID_SP_EVENT_COMPLETE_AARCH64;
+ EventCompleteSvcArgs->Arg0 = ARM_SVC_ID_SP_EVENT_COMPLETE;
EventCompleteSvcArgs->Arg1 = SvcStatus;
}
}
@@ -273,13 +273,13 @@ InitArmSvcArgs (
)
{
if (FeaturePcdGet (PcdFfaEnable)) {
- InitMmFoundationSvcArgs->Arg0 = ARM_SVC_ID_FFA_MSG_SEND_DIRECT_RESP_AARCH64;
+ InitMmFoundationSvcArgs->Arg0 = ARM_SVC_ID_FFA_MSG_SEND_DIRECT_RESP;
InitMmFoundationSvcArgs->Arg1 = 0;
InitMmFoundationSvcArgs->Arg2 = 0;
- InitMmFoundationSvcArgs->Arg3 = ARM_SVC_ID_SP_EVENT_COMPLETE_AARCH64;
+ InitMmFoundationSvcArgs->Arg3 = ARM_SVC_ID_SP_EVENT_COMPLETE;
InitMmFoundationSvcArgs->Arg4 = *Ret;
} else {
- InitMmFoundationSvcArgs->Arg0 = ARM_SVC_ID_SP_EVENT_COMPLETE_AARCH64;
+ InitMmFoundationSvcArgs->Arg0 = ARM_SVC_ID_SP_EVENT_COMPLETE;
InitMmFoundationSvcArgs->Arg1 = *Ret;
}
}
@@ -395,7 +395,7 @@ _ModuleEntryPoint (
//
ProcessModuleEntryPointList (HobStart);

- DEBUG ((DEBUG_INFO, "Shared Cpu Driver EP 0x%lx\n", (UINT64) CpuDriverEntryPoint));
+ DEBUG ((DEBUG_INFO, "Shared Cpu Driver EP %p\n", (VOID *) CpuDriverEntryPoint));

finish:
if (Status == RETURN_UNSUPPORTED) {
diff --git a/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/StandaloneMmCoreEntryPoint.inf b/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/StandaloneMmCoreEntryPoint.inf
index 4fa426f58e..1762586cfa 100644
--- a/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/StandaloneMmCoreEntryPoint.inf
+++ b/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/StandaloneMmCoreEntryPoint.inf
@@ -21,10 +21,10 @@
# VALID_ARCHITECTURES = IA32 X64 IPF EBC (EBC is for build only)
#

-[Sources.AARCH64]
- AArch64/StandaloneMmCoreEntryPoint.c
- AArch64/SetPermissions.c
- AArch64/CreateHobList.c
+[Sources.AARCH64, Sources.ARM]
+ Arm/StandaloneMmCoreEntryPoint.c
+ Arm/SetPermissions.c
+ Arm/CreateHobList.c

[Sources.X64]
X64/StandaloneMmCoreEntryPoint.c
@@ -34,14 +34,14 @@
MdeModulePkg/MdeModulePkg.dec
StandaloneMmPkg/StandaloneMmPkg.dec

-[Packages.AARCH64]
+[Packages.ARM, Packages.AARCH64]
ArmPkg/ArmPkg.dec

[LibraryClasses]
BaseLib
DebugLib

-[LibraryClasses.AARCH64]
+[LibraryClasses.ARM, LibraryClasses.AARCH64]
StandaloneMmMmuLib
ArmSvcLib

@@ -51,7 +51,7 @@
gEfiStandaloneMmNonSecureBufferGuid
gEfiArmTfCpuDriverEpDescriptorGuid

-[FeaturePcd.AARCH64]
+[FeaturePcd.ARM, FeaturePcd.AARCH64]
gArmTokenSpaceGuid.PcdFfaEnable

[BuildOptions]
diff --git a/StandaloneMmPkg/Library/StandaloneMmCoreHobLib/AArch64/StandaloneMmCoreHobLib.c b/StandaloneMmPkg/Library/StandaloneMmCoreHobLib/Arm/StandaloneMmCoreHobLib.c
similarity index 100%
rename from StandaloneMmPkg/Library/StandaloneMmCoreHobLib/AArch64/StandaloneMmCoreHobLib.c
rename to StandaloneMmPkg/Library/StandaloneMmCoreHobLib/Arm/StandaloneMmCoreHobLib.c
diff --git a/StandaloneMmPkg/Library/StandaloneMmCoreHobLib/AArch64/StandaloneMmCoreHobLibInternal.c b/StandaloneMmPkg/Library/StandaloneMmCoreHobLib/Arm/StandaloneMmCoreHobLibInternal.c
similarity index 100%
rename from StandaloneMmPkg/Library/StandaloneMmCoreHobLib/AArch64/StandaloneMmCoreHobLibInternal.c
rename to StandaloneMmPkg/Library/StandaloneMmCoreHobLib/Arm/StandaloneMmCoreHobLibInternal.c
diff --git a/StandaloneMmPkg/Library/StandaloneMmCoreHobLib/StandaloneMmCoreHobLib.inf b/StandaloneMmPkg/Library/StandaloneMmCoreHobLib/StandaloneMmCoreHobLib.inf
index a2559920e8..34ed536480 100644
--- a/StandaloneMmPkg/Library/StandaloneMmCoreHobLib/StandaloneMmCoreHobLib.inf
+++ b/StandaloneMmPkg/Library/StandaloneMmCoreHobLib/StandaloneMmCoreHobLib.inf
@@ -22,7 +22,7 @@
LIBRARY_CLASS = HobLib|MM_CORE_STANDALONE

#
-# VALID_ARCHITECTURES = X64 AARCH64
+# VALID_ARCHITECTURES = X64 AARCH64 ARM
#
[Sources.common]
Common.c
@@ -30,9 +30,9 @@
[Sources.X64]
X64/StandaloneMmCoreHobLib.c

-[Sources.AARCH64]
- AArch64/StandaloneMmCoreHobLib.c
- AArch64/StandaloneMmCoreHobLibInternal.c
+[Sources.AARCH64, Sources.ARM]
+ Arm/StandaloneMmCoreHobLib.c
+ Arm/StandaloneMmCoreHobLibInternal.c

[Packages]
MdePkg/MdePkg.dec
diff --git a/StandaloneMmPkg/Library/StandaloneMmMemLib/AArch64/StandaloneMmMemLibInternal.c b/StandaloneMmPkg/Library/StandaloneMmMemLib/ArmStandaloneMmMemLibInternal.c
similarity index 86%
rename from StandaloneMmPkg/Library/StandaloneMmMemLib/AArch64/StandaloneMmMemLibInternal.c
rename to StandaloneMmPkg/Library/StandaloneMmMemLib/ArmStandaloneMmMemLibInternal.c
index 4124959e04..fa7df46413 100644
--- a/StandaloneMmPkg/Library/StandaloneMmMemLib/AArch64/StandaloneMmMemLibInternal.c
+++ b/StandaloneMmPkg/Library/StandaloneMmMemLib/ArmStandaloneMmMemLibInternal.c
@@ -20,6 +20,13 @@
//
extern EFI_PHYSICAL_ADDRESS mMmMemLibInternalMaximumSupportAddress;

+#ifdef MDE_CPU_AARCH64
+#define ARM_PHYSICAL_ADDRESS_BITS 36
+#endif
+#ifdef MDE_CPU_ARM
+#define ARM_PHYSICAL_ADDRESS_BITS 32
+#endif
+
/**
Calculate and save the maximum support address.

@@ -31,7 +38,7 @@ MmMemLibInternalCalculateMaximumSupportAddress (
{
UINT8 PhysicalAddressBits;

- PhysicalAddressBits = 36;
+ PhysicalAddressBits = ARM_PHYSICAL_ADDRESS_BITS;

//
// Save the maximum support address in one global variable
diff --git a/StandaloneMmPkg/Library/StandaloneMmMemLib/StandaloneMmMemLib.inf b/StandaloneMmPkg/Library/StandaloneMmMemLib/StandaloneMmMemLib.inf
index 062b0d7a11..b29d97a746 100644
--- a/StandaloneMmPkg/Library/StandaloneMmMemLib/StandaloneMmMemLib.inf
+++ b/StandaloneMmPkg/Library/StandaloneMmMemLib/StandaloneMmMemLib.inf
@@ -28,7 +28,7 @@
#
# The following information is for reference only and not required by the build tools.
#
-# VALID_ARCHITECTURES = IA32 X64 AARCH64
+# VALID_ARCHITECTURES = IA32 X64 AARCH64 ARM
#

[Sources.Common]
@@ -37,8 +37,8 @@
[Sources.IA32, Sources.X64]
X86StandaloneMmMemLibInternal.c

-[Sources.AARCH64]
- AArch64/StandaloneMmMemLibInternal.c
+[Sources.AARCH64, Sources.ARM]
+ ArmStandaloneMmMemLibInternal.c

[Packages]
MdePkg/MdePkg.dec
diff --git a/StandaloneMmPkg/Library/VariableMmDependency/VariableMmDependency.inf b/StandaloneMmPkg/Library/VariableMmDependency/VariableMmDependency.inf
index a2a059c5d6..ffb2a6d083 100644
--- a/StandaloneMmPkg/Library/VariableMmDependency/VariableMmDependency.inf
+++ b/StandaloneMmPkg/Library/VariableMmDependency/VariableMmDependency.inf
@@ -20,7 +20,7 @@
#
# The following information is for reference only and not required by the build tools.
#
-# VALID_ARCHITECTURES = AARCH64
+# VALID_ARCHITECTURES = AARCH64|ARM
#
#

diff --git a/StandaloneMmPkg/StandaloneMmPkg.dsc b/StandaloneMmPkg/StandaloneMmPkg.dsc
index 0c45df95e2..772af1b72b 100644
--- a/StandaloneMmPkg/StandaloneMmPkg.dsc
+++ b/StandaloneMmPkg/StandaloneMmPkg.dsc
@@ -20,7 +20,7 @@
PLATFORM_VERSION = 1.0
DSC_SPECIFICATION = 0x00010011
OUTPUT_DIRECTORY = Build/StandaloneMm
- SUPPORTED_ARCHITECTURES = AARCH64|X64
+ SUPPORTED_ARCHITECTURES = AARCH64|X64|ARM
BUILD_TARGETS = DEBUG|RELEASE
SKUID_IDENTIFIER = DEFAULT

@@ -60,7 +60,7 @@
StandaloneMmDriverEntryPoint|MdePkg/Library/StandaloneMmDriverEntryPoint/StandaloneMmDriverEntryPoint.inf
VariableMmDependency|StandaloneMmPkg/Library/VariableMmDependency/VariableMmDependency.inf

-[LibraryClasses.AARCH64]
+[LibraryClasses.AARCH64, LibraryClasses.ARM]
ArmLib|ArmPkg/Library/ArmLib/ArmBaseLib.inf
StandaloneMmMmuLib|ArmPkg/Library/StandaloneMmMmuLib/ArmMmuStandaloneMmLib.inf
ArmSvcLib|ArmPkg/Library/ArmSvcLib/ArmSvcLib.inf
@@ -118,8 +118,8 @@
StandaloneMmPkg/Library/StandaloneMmMemoryAllocationLib/StandaloneMmMemoryAllocationLib.inf
StandaloneMmPkg/Library/VariableMmDependency/VariableMmDependency.inf

-[Components.AARCH64]
- StandaloneMmPkg/Drivers/StandaloneMmCpu/AArch64/StandaloneMmCpu.inf
+[Components.AARCH64, Components.ARM]
+ StandaloneMmPkg/Drivers/StandaloneMmCpu/StandaloneMmCpu.inf
StandaloneMmPkg/Library/StandaloneMmPeCoffExtraActionLib/StandaloneMmPeCoffExtraActionLib.inf

###################################################################################################
@@ -131,7 +131,7 @@
# module style (EDK or EDKII) specified in [Components] section.
#
###################################################################################################
-[BuildOptions.AARCH64]
+[BuildOptions.AARCH64, BuildOptions.ARM]
GCC:*_*_*_DLINK_FLAGS = -z common-page-size=0x1000 -march=armv8-a+nofp -mstrict-align
GCC:*_*_*_CC_FLAGS = -mstrict-align

--
2.17.1

Use intermediate (UINTN) cast when casting int from/to pointer. This
is needed as UINT64 values cast from/to 32bit pointer for 32bit
architectures.

Cc: Achin Gupta <achin.gupta@...>
Cc: Ard Biesheuvel <ardb+tianocore@...>
Cc: Jiewen Yao <jiewen.yao@...>
Cc: Leif Lindholm <leif@...>
Cc: Sami Mujawar <sami.mujawar@...>
Cc: Sughosh Ganu <sughosh.ganu@...>
Signed-off-by: Etienne Carriere <etienne.carriere@...>
---
No change since v1
---
StandaloneMmPkg/Drivers/StandaloneMmCpu/AArch64/StandaloneMmCpu.c | 8 ++++----
StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/AArch64/CreateHobList.c | 14 +++++++-------
StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/AArch64/StandaloneMmCoreEntryPoint.c | 2 +-
3 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/StandaloneMmPkg/Drivers/StandaloneMmCpu/AArch64/StandaloneMmCpu.c b/StandaloneMmPkg/Drivers/StandaloneMmCpu/AArch64/StandaloneMmCpu.c
index 6884095c49..d4590bcd19 100644
--- a/StandaloneMmPkg/Drivers/StandaloneMmCpu/AArch64/StandaloneMmCpu.c
+++ b/StandaloneMmPkg/Drivers/StandaloneMmCpu/AArch64/StandaloneMmCpu.c
@@ -164,8 +164,8 @@ StandaloneMmCpuInitialize (

// Share the entry point of the CPU driver
DEBUG ((DEBUG_INFO, "Sharing Cpu Driver EP *0x%lx = 0x%lx\n",
- (UINT64) CpuDriverEntryPointDesc->ArmTfCpuDriverEpPtr,
- (UINT64) PiMmStandaloneArmTfCpuDriverEntry));
+ (UINTN) CpuDriverEntryPointDesc->ArmTfCpuDriverEpPtr,
+ (UINTN) PiMmStandaloneArmTfCpuDriverEntry));
*(CpuDriverEntryPointDesc->ArmTfCpuDriverEpPtr) = PiMmStandaloneArmTfCpuDriverEntry;

// Find the descriptor that contains the whereabouts of the buffer for
@@ -180,8 +180,8 @@ StandaloneMmCpuInitialize (
return Status;
}

- DEBUG ((DEBUG_INFO, "mNsCommBuffer.PhysicalStart - 0x%lx\n", (UINT64) NsCommBufMmramRange->PhysicalStart));
- DEBUG ((DEBUG_INFO, "mNsCommBuffer.PhysicalSize - 0x%lx\n", (UINT64) NsCommBufMmramRange->PhysicalSize));
+ DEBUG ((DEBUG_INFO, "mNsCommBuffer.PhysicalStart - 0x%lx\n", (UINTN) NsCommBufMmramRange->PhysicalStart));
+ DEBUG ((DEBUG_INFO, "mNsCommBuffer.PhysicalSize - 0x%lx\n", (UINTN) NsCommBufMmramRange->PhysicalSize));

CopyMem (&mNsCommBuffer, NsCommBufMmramRange, sizeof(EFI_MMRAM_DESCRIPTOR));
DEBUG ((DEBUG_INFO, "mNsCommBuffer: 0x%016lx - 0x%lx\n", mNsCommBuffer.CpuStart, mNsCommBuffer.PhysicalSize));
diff --git a/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/AArch64/CreateHobList.c b/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/AArch64/CreateHobList.c
index e8fb96bd6e..4d4cf3d5ff 100644
--- a/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/AArch64/CreateHobList.c
+++ b/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/AArch64/CreateHobList.c
@@ -72,14 +72,14 @@ CreateHobListFromBootInfo (

// Create a hoblist with a PHIT and EOH
HobStart = HobConstructor (
- (VOID *) PayloadBootInfo->SpMemBase,
+ (VOID *) (UINTN) PayloadBootInfo->SpMemBase,
(UINTN) PayloadBootInfo->SpMemLimit - PayloadBootInfo->SpMemBase,
- (VOID *) PayloadBootInfo->SpHeapBase,
- (VOID *) (PayloadBootInfo->SpHeapBase + PayloadBootInfo->SpHeapSize)
+ (VOID *) (UINTN) PayloadBootInfo->SpHeapBase,
+ (VOID *) (UINTN) (PayloadBootInfo->SpHeapBase + PayloadBootInfo->SpHeapSize)
);

// Check that the Hoblist starts at the bottom of the Heap
- ASSERT (HobStart == (VOID *) PayloadBootInfo->SpHeapBase);
+ ASSERT (HobStart == (VOID *) (UINTN) PayloadBootInfo->SpHeapBase);

// Build a Boot Firmware Volume HOB
BuildFvHob (PayloadBootInfo->SpImageBase, PayloadBootInfo->SpImageSize);
@@ -190,9 +190,9 @@ CreateHobListFromBootInfo (
MmramRanges[3].RegionState = EFI_CACHEABLE | EFI_ALLOCATED;

// Base and size of heap memory shared by all cpus
- MmramRanges[4].PhysicalStart = (EFI_PHYSICAL_ADDRESS) HobStart;
- MmramRanges[4].CpuStart = (EFI_PHYSICAL_ADDRESS) HobStart;
- MmramRanges[4].PhysicalSize = HobStart->EfiFreeMemoryBottom - (EFI_PHYSICAL_ADDRESS) HobStart;
+ MmramRanges[4].PhysicalStart = (EFI_PHYSICAL_ADDRESS) (UINTN) HobStart;
+ MmramRanges[4].CpuStart = (EFI_PHYSICAL_ADDRESS) (UINTN) HobStart;
+ MmramRanges[4].PhysicalSize = HobStart->EfiFreeMemoryBottom - (EFI_PHYSICAL_ADDRESS) (UINTN) HobStart;
MmramRanges[4].RegionState = EFI_CACHEABLE | EFI_ALLOCATED;

// Base and size of heap memory shared by all cpus
diff --git a/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/AArch64/StandaloneMmCoreEntryPoint.c b/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/AArch64/StandaloneMmCoreEntryPoint.c
index 6c50f470aa..b445d6942e 100644
--- a/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/AArch64/StandaloneMmCoreEntryPoint.c
+++ b/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/AArch64/StandaloneMmCoreEntryPoint.c
@@ -328,7 +328,7 @@ _ModuleEntryPoint (

// Locate PE/COFF File information for the Standalone MM core module
Status = LocateStandaloneMmCorePeCoffData (
- (EFI_FIRMWARE_VOLUME_HEADER *) PayloadBootInfo->SpImageBase,
+ (EFI_FIRMWARE_VOLUME_HEADER *) (UINTN) PayloadBootInfo->SpImageBase,
&TeData,
&TeDataSize
);
--
2.17.1

Change GenFv for Arm architecture to generate a specific jump
instruction as image entry instruction, when the target entry label
is assembled with Thumb instruction set. This is possible since
SecCoreEntryAddress value fetched from the PE32 has its LSBit set when
the entry instruction executes in Thumb mode.

Cc: Bob Feng <bob.c.feng@...>
Cc: Liming Gao <gaoliming@...>
Cc: Achin Gupta <achin.gupta@...>
Cc: Ard Biesheuvel <ardb+tianocore@...>
Cc: Leif Lindholm <leif@...>
Cc: Sughosh Ganu <sughosh.ganu@...>
Signed-off-by: Etienne Carriere <etienne.carriere@...>
---
Changes since v1:
- Fix typos in commit log and inline comments
- Change if() test operand to be an explicit boolean
---
BaseTools/Source/C/GenFv/GenFvInternalLib.c | 38 +++++++++++++++-----
1 file changed, 29 insertions(+), 9 deletions(-)

diff --git a/BaseTools/Source/C/GenFv/GenFvInternalLib.c b/BaseTools/Source/C/GenFv/GenFvInternalLib.c
index 6e296b8ad6..5f3fd4f808 100644
--- a/BaseTools/Source/C/GenFv/GenFvInternalLib.c
+++ b/BaseTools/Source/C/GenFv/GenFvInternalLib.c
@@ -34,9 +34,27 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
#include "FvLib.h"
#include "PeCoffLib.h"

-#define ARMT_UNCONDITIONAL_JUMP_INSTRUCTION 0xEB000000
#define ARM64_UNCONDITIONAL_JUMP_INSTRUCTION 0x14000000

+/*
+ * Arm instruction to jump to Fv entry instruction in Arm or Thumb mode.
+ * From ARM Arch Ref Manual versions b/c/d, section A8.8.25 BL, BLX (immediate)
+ * BLX (encoding A2) branches to offset in Thumb instruction set mode.
+ * BL (encoding A1) branches to offset in Arm instruction set mode.
+ */
+#define ARM_JUMP_OFFSET_MAX 0xffffff
+#define ARM_JUMP_TO_ARM(Offset) (0xeb000000 | ((Offset - 8) >> 2))
+
+#define _ARM_JUMP_TO_THUMB(Imm32) (0xfa000000 | \
+ (((Imm32) & (1 << 1)) << (24 - 1)) | \
+ (((Imm32) >> 2) & 0x7fffff))
+#define ARM_JUMP_TO_THUMB(Offset) _ARM_JUMP_TO_THUMB((Offset) - 8)
+
+/*
+ * Arm instruction to retrun from exception (MOVS PC, LR)
+ */
+#define ARM_RETURN_FROM_EXCEPTION 0xE1B0F07E
+
BOOLEAN mArm = FALSE;
BOOLEAN mRiscV = FALSE;
STATIC UINT32 MaxFfsAlignment = 0;
@@ -2203,23 +2221,25 @@ Returns:
// if we found an SEC core entry point then generate a branch instruction
// to it and populate a debugger SWI entry as well
if (UpdateVectorSec) {
+ UINT32 EntryOffset;

VerboseMsg("UpdateArmResetVectorIfNeeded updating ARM SEC vector");

- // B SecEntryPoint - signed_immed_24 part +/-32MB offset
- // on ARM, the PC is always 8 ahead, so we're not really jumping from the base address, but from base address + 8
- ResetVector[0] = (INT32)(SecCoreEntryAddress - FvInfo->BaseAddress - 8) >> 2;
+ EntryOffset = (INT32)(SecCoreEntryAddress - FvInfo->BaseAddress);

- if (ResetVector[0] > 0x00FFFFFF) {
- Error(NULL, 0, 3000, "Invalid", "SEC Entry point must be within 32MB of the start of the FV");
+ if (EntryOffset > ARM_JUMP_OFFSET_MAX) {
+ Error(NULL, 0, 3000, "Invalid", "SEC Entry point offset above 1MB of the start of the FV");
return EFI_ABORTED;
}

- // Add opcode for an unconditional branch with no link. i.e.: " B SecEntryPoint"
- ResetVector[0] |= ARMT_UNCONDITIONAL_JUMP_INSTRUCTION;
+ if (SecCoreEntryAddress & 1 != 0) {
+ ResetVector[0] = ARM_JUMP_TO_THUMB(EntryOffset);
+ } else {
+ ResetVector[0] = ARM_JUMP_TO_ARM(EntryOffset);
+ }

// SWI handler movs pc,lr. Just in case a debugger uses SWI
- ResetVector[2] = 0xE1B0F07E;
+ ResetVector[2] = ARM_RETURN_FROM_EXCEPTION;

// Place holder to support a common interrupt handler from ROM.
// Currently not supported. For this to be used the reset vector would not be in this FV
--
2.17.1

Changes in ArmPkg to prepare building StandaloneMm firmware for
32bit Arm architectures.

Adds MmCommunicationDxe driver and ArmMmuPeiLib and
ArmmmuStandaloneMmLib libraries to the list of the standard
components build for ArmPkg on when ARM architectures.

Changes path of source file AArch64/ArmMmuStandaloneMmLib.c
and compile it for both 32bit and 64bit architectures.

Cc: Achin Gupta <achin.gupta@...>
Cc: Ard Biesheuvel <ardb+tianocore@...>
Cc: Leif Lindholm <leif@...>
Cc: Sughosh Ganu <sughosh.ganu@...>
Signed-off-by: Etienne Carriere <etienne.carriere@...>
---
No change since v1
---
ArmPkg/ArmPkg.dec | 2 +-
ArmPkg/ArmPkg.dsc | 2 +-
ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.c | 2 +-
ArmPkg/Library/StandaloneMmMmuLib/{AArch64 => }/ArmMmuStandaloneMmLib.c | 15 ++++++++-------
ArmPkg/Library/StandaloneMmMmuLib/ArmMmuStandaloneMmLib.inf | 6 +++---
5 files changed, 14 insertions(+), 13 deletions(-)

diff --git a/ArmPkg/ArmPkg.dec b/ArmPkg/ArmPkg.dec
index 214b2f5892..6ed51edd03 100644
--- a/ArmPkg/ArmPkg.dec
+++ b/ArmPkg/ArmPkg.dec
@@ -137,7 +137,7 @@
# hardware coherency (i.e., no virtualization or cache coherent DMA)
gArmTokenSpaceGuid.PcdNormalMemoryNonshareableOverride|FALSE|BOOLEAN|0x00000043

-[PcdsFeatureFlag.AARCH64]
+[PcdsFeatureFlag.AARCH64, PcdsFeatureFlag.ARM]
## Used to select method for requesting services from S-EL1.<BR><BR>
# TRUE - Selects FF-A calls for communication between S-EL0 and SPMC.<BR>
# FALSE - Selects SVC calls for communication between S-EL0 and SPMC.<BR>
diff --git a/ArmPkg/ArmPkg.dsc b/ArmPkg/ArmPkg.dsc
index 926986cf7f..4c79dadf9e 100644
--- a/ArmPkg/ArmPkg.dsc
+++ b/ArmPkg/ArmPkg.dsc
@@ -158,7 +158,7 @@
ArmPkg/Universal/Smbios/SmbiosMiscDxe/SmbiosMiscDxe.inf
ArmPkg/Universal/Smbios/OemMiscLibNull/OemMiscLibNull.inf

-[Components.AARCH64]
+[Components.AARCH64, Components.ARM]
ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.inf
ArmPkg/Library/ArmMmuLib/ArmMmuPeiLib.inf
ArmPkg/Library/StandaloneMmMmuLib/ArmMmuStandaloneMmLib.inf
diff --git a/ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.c b/ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.c
index b1e3095809..4ae38a9f22 100644
--- a/ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.c
+++ b/ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.c
@@ -125,7 +125,7 @@ MmCommunication2Communicate (
}

// SMC Function ID
- CommunicateSmcArgs.Arg0 = ARM_SMC_ID_MM_COMMUNICATE_AARCH64;
+ CommunicateSmcArgs.Arg0 = ARM_SMC_ID_MM_COMMUNICATE;

// Cookie
CommunicateSmcArgs.Arg1 = 0;
diff --git a/ArmPkg/Library/StandaloneMmMmuLib/AArch64/ArmMmuStandaloneMmLib.c b/ArmPkg/Library/StandaloneMmMmuLib/ArmMmuStandaloneMmLib.c
similarity index 92%
rename from ArmPkg/Library/StandaloneMmMmuLib/AArch64/ArmMmuStandaloneMmLib.c
rename to ArmPkg/Library/StandaloneMmMmuLib/ArmMmuStandaloneMmLib.c
index dd014beec8..20f873e680 100644
--- a/ArmPkg/Library/StandaloneMmMmuLib/AArch64/ArmMmuStandaloneMmLib.c
+++ b/ArmPkg/Library/StandaloneMmMmuLib/ArmMmuStandaloneMmLib.c
@@ -2,6 +2,7 @@
File managing the MMU for ARMv8 architecture in S-EL0

Copyright (c) 2017 - 2021, Arm Limited. All rights reserved.<BR>
+ Copyright (c) 2021, Linaro Limited
SPDX-License-Identifier: BSD-2-Clause-Patent

@par Reference(s):
@@ -62,7 +63,7 @@ SendMemoryPermissionRequest (
// for other Direct Request calls which are not atomic
// We therefore check only for Direct Response by the
// callee.
- if (SvcArgs->Arg0 == ARM_SVC_ID_FFA_MSG_SEND_DIRECT_RESP_AARCH64) {
+ if (SvcArgs->Arg0 == ARM_SVC_ID_FFA_MSG_SEND_DIRECT_RESP) {
// A Direct Response means FF-A success
// Now check the payload for errors
// The callee sends back the return value
@@ -164,13 +165,13 @@ GetMemoryPermissions (
ZeroMem (&SvcArgs, sizeof (ARM_SVC_ARGS));
if (FeaturePcdGet (PcdFfaEnable)) {
// See [2], Section 10.2 FFA_MSG_SEND_DIRECT_REQ.
- SvcArgs.Arg0 = ARM_SVC_ID_FFA_MSG_SEND_DIRECT_REQ_AARCH64;
+ SvcArgs.Arg0 = ARM_SVC_ID_FFA_MSG_SEND_DIRECT_REQ;
SvcArgs.Arg1 = ARM_FFA_DESTINATION_ENDPOINT_ID;
SvcArgs.Arg2 = 0;
- SvcArgs.Arg3 = ARM_SVC_ID_SP_GET_MEM_ATTRIBUTES_AARCH64;
+ SvcArgs.Arg3 = ARM_SVC_ID_SP_GET_MEM_ATTRIBUTES;
SvcArgs.Arg4 = BaseAddress;
} else {
- SvcArgs.Arg0 = ARM_SVC_ID_SP_GET_MEM_ATTRIBUTES_AARCH64;
+ SvcArgs.Arg0 = ARM_SVC_ID_SP_GET_MEM_ATTRIBUTES;
SvcArgs.Arg1 = BaseAddress;
SvcArgs.Arg2 = 0;
SvcArgs.Arg3 = 0;
@@ -219,15 +220,15 @@ RequestMemoryPermissionChange (
ZeroMem (&SvcArgs, sizeof (ARM_SVC_ARGS));
if (FeaturePcdGet (PcdFfaEnable)) {
// See [2], Section 10.2 FFA_MSG_SEND_DIRECT_REQ.
- SvcArgs.Arg0 = ARM_SVC_ID_FFA_MSG_SEND_DIRECT_REQ_AARCH64;
+ SvcArgs.Arg0 = ARM_SVC_ID_FFA_MSG_SEND_DIRECT_REQ;
SvcArgs.Arg1 = ARM_FFA_DESTINATION_ENDPOINT_ID;
SvcArgs.Arg2 = 0;
- SvcArgs.Arg3 = ARM_SVC_ID_SP_SET_MEM_ATTRIBUTES_AARCH64;
+ SvcArgs.Arg3 = ARM_SVC_ID_SP_SET_MEM_ATTRIBUTES;
SvcArgs.Arg4 = BaseAddress;
SvcArgs.Arg5 = EFI_SIZE_TO_PAGES (Length);
SvcArgs.Arg6 = Permissions;
} else {
- SvcArgs.Arg0 = ARM_SVC_ID_SP_SET_MEM_ATTRIBUTES_AARCH64;
+ SvcArgs.Arg0 = ARM_SVC_ID_SP_SET_MEM_ATTRIBUTES;
SvcArgs.Arg1 = BaseAddress;
SvcArgs.Arg2 = EFI_SIZE_TO_PAGES (Length);
SvcArgs.Arg3 = Permissions;
diff --git a/ArmPkg/Library/StandaloneMmMmuLib/ArmMmuStandaloneMmLib.inf b/ArmPkg/Library/StandaloneMmMmuLib/ArmMmuStandaloneMmLib.inf
index 6c71fe0023..ff20e58980 100644
--- a/ArmPkg/Library/StandaloneMmMmuLib/ArmMmuStandaloneMmLib.inf
+++ b/ArmPkg/Library/StandaloneMmMmuLib/ArmMmuStandaloneMmLib.inf
@@ -16,14 +16,14 @@
LIBRARY_CLASS = StandaloneMmMmuLib
PI_SPECIFICATION_VERSION = 0x00010032

-[Sources.AARCH64]
- AArch64/ArmMmuStandaloneMmLib.c
+[Sources]
+ ArmMmuStandaloneMmLib.c

[Packages]
ArmPkg/ArmPkg.dec
MdePkg/MdePkg.dec

-[FeaturePcd.AARCH64]
+[FeaturePcd.ARM, FeaturePcd.AARCH64]
gArmTokenSpaceGuid.PcdFfaEnable

[LibraryClasses]
--
2.17.1

Defines ARM_SVC_ID_FFA_* and ARM_SVC_ID_SP_* identifiers for 32bit
function IDs as per SMCCC specification. Defines also generic ARM
SVC identifier macros to wrap 32bit or 64bit identifiers upon target
built architecture.

Cc: Achin Gupta <achin.gupta@...>
Cc: Ard Biesheuvel <ardb+tianocore@...>
Cc: Leif Lindholm <leif@...>
Cc: Sughosh Ganu <sughosh.ganu@...>
Signed-off-by: Etienne Carriere <etienne.carriere@...>
---
Changes since v1:
- Define ARM_SMC_ID_MM_COMMUNICATE 32b/64b agnostic helper ID in
ArmStdSmc.h, as expected by few following commits in this series.
---
ArmPkg/Include/IndustryStandard/ArmFfaSvc.h | 12 ++++++++++++
ArmPkg/Include/IndustryStandard/ArmMmSvc.h | 15 +++++++++++++++
ArmPkg/Include/IndustryStandard/ArmStdSmc.h | 8 ++++++++
3 files changed, 35 insertions(+)

diff --git a/ArmPkg/Include/IndustryStandard/ArmFfaSvc.h b/ArmPkg/Include/IndustryStandard/ArmFfaSvc.h
index 65b8343ade..ebcb54b28b 100644
--- a/ArmPkg/Include/IndustryStandard/ArmFfaSvc.h
+++ b/ArmPkg/Include/IndustryStandard/ArmFfaSvc.h
@@ -17,9 +17,21 @@
#define ARM_FFA_SVC_H_

#define ARM_SVC_ID_FFA_VERSION_AARCH32 0x84000063
+#define ARM_SVC_ID_FFA_MSG_SEND_DIRECT_REQ_AARCH32 0x8400006F
+#define ARM_SVC_ID_FFA_MSG_SEND_DIRECT_RESP_AARCH32 0x84000070
#define ARM_SVC_ID_FFA_MSG_SEND_DIRECT_REQ_AARCH64 0xC400006F
#define ARM_SVC_ID_FFA_MSG_SEND_DIRECT_RESP_AARCH64 0xC4000070

+/* Generic IDs when using AArch32 or AArch64 execution state */
+#ifdef MDE_CPU_AARCH64
+#define ARM_SVC_ID_FFA_MSG_SEND_DIRECT_REQ ARM_SVC_ID_FFA_MSG_SEND_DIRECT_REQ_AARCH64
+#define ARM_SVC_ID_FFA_MSG_SEND_DIRECT_RESP ARM_SVC_ID_FFA_MSG_SEND_DIRECT_RESP_AARCH64
+#endif
+#ifdef MDE_CPU_ARM
+#define ARM_SVC_ID_FFA_MSG_SEND_DIRECT_REQ ARM_SVC_ID_FFA_MSG_SEND_DIRECT_REQ_AARCH32
+#define ARM_SVC_ID_FFA_MSG_SEND_DIRECT_RESP ARM_SVC_ID_FFA_MSG_SEND_DIRECT_RESP_AARCH32
+#endif
+
#define SPM_MAJOR_VERSION_FFA 1
#define SPM_MINOR_VERSION_FFA 0

diff --git a/ArmPkg/Include/IndustryStandard/ArmMmSvc.h b/ArmPkg/Include/IndustryStandard/ArmMmSvc.h
index 33d60ccf17..deb3bc99d2 100644
--- a/ArmPkg/Include/IndustryStandard/ArmMmSvc.h
+++ b/ArmPkg/Include/IndustryStandard/ArmMmSvc.h
@@ -15,10 +15,25 @@
* privileged operations on its behalf.
*/
#define ARM_SVC_ID_SPM_VERSION_AARCH32 0x84000060
+#define ARM_SVC_ID_SP_EVENT_COMPLETE_AARCH32 0x84000061
+#define ARM_SVC_ID_SP_GET_MEM_ATTRIBUTES_AARCH32 0x84000064
+#define ARM_SVC_ID_SP_SET_MEM_ATTRIBUTES_AARCH32 0x84000065
#define ARM_SVC_ID_SP_EVENT_COMPLETE_AARCH64 0xC4000061
#define ARM_SVC_ID_SP_GET_MEM_ATTRIBUTES_AARCH64 0xC4000064
#define ARM_SVC_ID_SP_SET_MEM_ATTRIBUTES_AARCH64 0xC4000065

+/* Generic IDs when using AArch32 or AArch64 execution state */
+#ifdef MDE_CPU_AARCH64
+#define ARM_SVC_ID_SP_EVENT_COMPLETE ARM_SVC_ID_SP_EVENT_COMPLETE_AARCH64
+#define ARM_SVC_ID_SP_GET_MEM_ATTRIBUTES ARM_SVC_ID_SP_GET_MEM_ATTRIBUTES_AARCH64
+#define ARM_SVC_ID_SP_SET_MEM_ATTRIBUTES ARM_SVC_ID_SP_SET_MEM_ATTRIBUTES_AARCH64
+#endif
+#ifdef MDE_CPU_ARM
+#define ARM_SVC_ID_SP_EVENT_COMPLETE ARM_SVC_ID_SP_EVENT_COMPLETE_AARCH32
+#define ARM_SVC_ID_SP_GET_MEM_ATTRIBUTES ARM_SVC_ID_SP_GET_MEM_ATTRIBUTES_AARCH32
+#define ARM_SVC_ID_SP_SET_MEM_ATTRIBUTES ARM_SVC_ID_SP_SET_MEM_ATTRIBUTES_AARCH32
+#endif
+
#define SET_MEM_ATTR_DATA_PERM_MASK 0x3
#define SET_MEM_ATTR_DATA_PERM_SHIFT 0
#define SET_MEM_ATTR_DATA_PERM_NO_ACCESS 0
diff --git a/ArmPkg/Include/IndustryStandard/ArmStdSmc.h b/ArmPkg/Include/IndustryStandard/ArmStdSmc.h
index 67afb0ea2d..9116a291da 100644
--- a/ArmPkg/Include/IndustryStandard/ArmStdSmc.h
+++ b/ArmPkg/Include/IndustryStandard/ArmStdSmc.h
@@ -49,6 +49,14 @@
#define ARM_SMC_ID_MM_COMMUNICATE_AARCH32 0x84000041
#define ARM_SMC_ID_MM_COMMUNICATE_AARCH64 0xC4000041

+/* Generic ID when using AArch32 or AArch64 execution state */
+#ifdef MDE_CPU_AARCH64
+#define ARM_SMC_ID_MM_COMMUNICATE ARM_SMC_ID_MM_COMMUNICATE_AARCH64
+#endif
+#ifdef MDE_CPU_ARM
+#define ARM_SMC_ID_MM_COMMUNICATE ARM_SMC_ID_MM_COMMUNICATE_AARCH32
+#endif
+
/* MM return error codes */
#define ARM_SMC_MM_RET_SUCCESS 0
#define ARM_SMC_MM_RET_NOT_SUPPORTED -1
--
2.17.1

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@...>
Cc: Ray Ni <ray.ni@...>
Cc: Laszlo Ersek <lersek@...>
Cc: Rahul Kumar <rahul1.kumar@...>
Cc: Marvin Häuser <mhaeuser@...>
Signed-off-by: Tom Lendacky <thomas.lendacky@...>

---

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

REF:https://bugzilla.tianocore.org/show_bug.cgi?id=3D3395

This patch fixes the issue observed during
BSF file to YAML file conversion. It also
addresses the issue during multibyte array
data conversion check, for example the data
representation of 0xFFFF instead of 0xFF, 0xFF
would be thrown exception "Array size is not
proper" without this patch.

Cc: Maurice Ma <maurice.ma@...>
Cc: Nate DeSimone <nathaniel.l.desimone@...>
Cc: Star Zeng <star.zeng@...>
Cc: Chasel Chiu <chasel.chiu@...>
Signed-off-by: Loo Tung Lun <tung.lun.loo@...>
---
IntelFsp2Pkg/Tools/FspDscBsf2Yaml.py | 11 +++++++++--
IntelFsp2Pkg/Tools/GenCfgOpt.py | 3 ++-
2 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/IntelFsp2Pkg/Tools/FspDscBsf2Yaml.py b/IntelFsp2Pkg/Tools/FspD=
scBsf2Yaml.py
index cad9b60e73..d2ca7145ae 100644
--- a/IntelFsp2Pkg/Tools/FspDscBsf2Yaml.py
+++ b/IntelFsp2Pkg/Tools/FspDscBsf2Yaml.py
@@ -46,6 +46,13 @@ def Bytes2Val(Bytes):
return reduce(lambda x, y: (x << 8) | y, Bytes[::-1])=0D
=0D
=0D
+def Str2Bytes(Value, Blen):=0D
+ Result =3D bytearray(Value[1:-1], 'utf-8') # Excluding quotes=0D
+ if len(Result) < Blen:=0D
+ Result.extend(b'\x00' * (Blen - len(Result)))=0D
+ return Result=0D
+=0D
+=0D
class CFspBsf2Dsc:=0D
=0D
def __init__(self, bsf_file):=0D
@@ -108,7 +115,8 @@ class CFspBsf2Dsc:
cfg_item['find'] =3D prefix=0D
cfg_item['cname'] =3D 'Signature'=0D
cfg_item['length'] =3D len(finds[0][1])=0D
- cfg_item['value'] =3D '0x%X' % Bytes2Val(finds[0][1].encod=
e('UTF-8'))=0D
+ str2byte =3D Str2Bytes("'" + finds[0][1] + "'", len(finds[=
0][1]))=0D
+ cfg_item['value'] =3D '0x%X' % Bytes2Val(str2byte)=0D
cfg_list.append(dict(cfg_item))=0D
cfg_item =3D dict(cfg_temp)=0D
find_list.pop(0)=0D
@@ -291,7 +299,6 @@ class CFspDsc2Yaml():
raise Exception('DSC variable creation error !')=0D
else:=0D
raise Exception('Unsupported file "%s" !' % file_name)=0D
- gen_cfg_data.UpdateDefaultValue()=0D
self.gen_cfg_data =3D gen_cfg_data=0D
=0D
def print_dsc_line(self):=0D
diff --git a/IntelFsp2Pkg/Tools/GenCfgOpt.py b/IntelFsp2Pkg/Tools/GenCfgOpt=
.py
index 660824b740..714b2d8b1a 100644
--- a/IntelFsp2Pkg/Tools/GenCfgOpt.py
+++ b/IntelFsp2Pkg/Tools/GenCfgOpt.py
@@ -708,7 +708,8 @@ EndList
for Page in PageList:=0D
Page =3D Page.strip()=0D
Match =3D re.match("(\w+):\"(.+)\"", Page)=
=0D
- self._CfgPageDict[Match.group(1)] =3D Matc=
h.group(2)=0D
+ if Match !=3D None:=0D
+ self._CfgPageDict[Match.group(1)] =3D =
Match.group(2)=0D
=0D
Match =3D re.match("(?:^|.+\s+)BLOCK:{NAME:\"(.+)\=
"\s*,\s*VER:\"(.+)\"\s*}", Remaining)=0D
if Match:=0D
--=20
2.28.0.windows.1

Create a PR for this patch. https://github.com/tianocore/edk2/pull/1649

Thanks,
Bob

This patch fixes the issue observed during
BSF file to YAML file conversion. It also
addresses the issue during multibyte array
data conversion check, for example the data
representation of 0xFFFF instead of 0xFF, 0xFF
would be thrown exception "Array size is not
proper" without this patch.

Cc: Maurice Ma <maurice.ma@...>
Cc: Nate DeSimone <nathaniel.l.desimone@...>
Cc: Star Zeng <star.zeng@...>
Cc: Chasel Chiu <chasel.chiu@...>
Signed-off-by: Loo Tung Lun <tung.lun.loo@...>
---
IntelFsp2Pkg/Tools/FspDscBsf2Yaml.py | 11 +++++++++--
IntelFsp2Pkg/Tools/GenCfgOpt.py | 3 ++-
2 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/IntelFsp2Pkg/Tools/FspDscBsf2Yaml.py b/IntelFsp2Pkg/Tools/FspD=
scBsf2Yaml.py
index cad9b60e73..d2ca7145ae 100644
--- a/IntelFsp2Pkg/Tools/FspDscBsf2Yaml.py
+++ b/IntelFsp2Pkg/Tools/FspDscBsf2Yaml.py
@@ -46,6 +46,13 @@ def Bytes2Val(Bytes):
return reduce(lambda x, y: (x << 8) | y, Bytes[::-1])=0D
=0D
=0D
+def Str2Bytes(Value, Blen):=0D
+ Result =3D bytearray(Value[1:-1], 'utf-8') # Excluding quotes=0D
+ if len(Result) < Blen:=0D
+ Result.extend(b'\x00' * (Blen - len(Result)))=0D
+ return Result=0D
+=0D
+=0D
class CFspBsf2Dsc:=0D
=0D
def __init__(self, bsf_file):=0D
@@ -108,7 +115,8 @@ class CFspBsf2Dsc:
cfg_item['find'] =3D prefix=0D
cfg_item['cname'] =3D 'Signature'=0D
cfg_item['length'] =3D len(finds[0][1])=0D
- cfg_item['value'] =3D '0x%X' % Bytes2Val(finds[0][1].encod=
e('UTF-8'))=0D
+ str2byte =3D Str2Bytes("'" + finds[0][1] + "'", len(finds[=
0][1]))=0D
+ cfg_item['value'] =3D '0x%X' % Bytes2Val(str2byte)=0D
cfg_list.append(dict(cfg_item))=0D
cfg_item =3D dict(cfg_temp)=0D
find_list.pop(0)=0D
@@ -291,7 +299,6 @@ class CFspDsc2Yaml():
raise Exception('DSC variable creation error !')=0D
else:=0D
raise Exception('Unsupported file "%s" !' % file_name)=0D
- gen_cfg_data.UpdateDefaultValue()=0D
self.gen_cfg_data =3D gen_cfg_data=0D
=0D
def print_dsc_line(self):=0D
diff --git a/IntelFsp2Pkg/Tools/GenCfgOpt.py b/IntelFsp2Pkg/Tools/GenCfgOpt=
.py
index 660824b740..714b2d8b1a 100644
--- a/IntelFsp2Pkg/Tools/GenCfgOpt.py
+++ b/IntelFsp2Pkg/Tools/GenCfgOpt.py
@@ -708,7 +708,8 @@ EndList
for Page in PageList:=0D
Page =3D Page.strip()=0D
Match =3D re.match("(\w+):\"(.+)\"", Page)=
=0D
- self._CfgPageDict[Match.group(1)] =3D Matc=
h.group(2)=0D
+ if Match !=3D None:=0D
+ self._CfgPageDict[Match.group(1)] =3D =
Match.group(2)=0D
=0D
Match =3D re.match("(?:^|.+\s+)BLOCK:{NAME:\"(.+)\=
"\s*,\s*VER:\"(.+)\"\s*}", Remaining)=0D
if Match:=0D
--=20
2.28.0.windows.1

On 05/13/21 01:46, Brijesh Singh wrote:

BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3275

The Flush parameter is used to provide a hint whether the specified range
is Mmio address. Now that we have a dedicated helper to clear the
memory encryption mask for the Mmio address range, its safe to remove the
Flush parameter from MemEncryptSev{Set,Clear}PageEncMask().

Since the address specified in the MemEncryptSev{Set,Clear}PageEncMask()
points to a system RAM, thus a cache flush is required during the
encryption mask update.

Cc: James Bottomley <jejb@...>
Cc: Min Xu <min.m.xu@...>
Cc: Jiewen Yao <jiewen.yao@...>
Cc: Tom Lendacky <thomas.lendacky@...>
Cc: Jordan Justen <jordan.l.justen@...>
Cc: Ard Biesheuvel <ardb+tianocore@...>
Cc: Laszlo Ersek <lersek@...>
Cc: Erdem Aktas <erdemaktas@...>
Signed-off-by: Brijesh Singh <brijesh.singh@...>
---
OvmfPkg/Include/Library/MemEncryptSevLib.h | 10 ++--------
.../BaseMemEncryptSevLib/X64/VirtualMemory.h | 10 ++--------
OvmfPkg/AmdSevDxe/AmdSevDxe.c | 3 +--
OvmfPkg/IoMmuDxe/AmdSevIoMmu.c | 6 ++----
.../BaseMemEncryptSevLib/Ia32/MemEncryptSevLib.c | 10 ++--------
.../BaseMemEncryptSevLib/X64/MemEncryptSevLib.c | 16 ++++------------
.../X64/PeiDxeVirtualMemory.c | 14 ++++----------
.../BaseMemEncryptSevLib/X64/SecVirtualMemory.c | 8 ++------
.../SmmCpuFeaturesLib/SmmCpuFeaturesLib.c | 3 +--
OvmfPkg/PlatformPei/AmdSev.c | 3 +--
10 files changed, 21 insertions(+), 62 deletions(-)

Reviewed-by: Laszlo Ersek <lersek@...>

(v2 patches 10 through 12 are OK as well)

Thanks
Laszlo

diff --git a/OvmfPkg/Include/Library/MemEncryptSevLib.h b/OvmfPkg/Include/Library/MemEncryptSevLib.h
index b91490d5d44d..76d06c206c8b 100644
--- a/OvmfPkg/Include/Library/MemEncryptSevLib.h
+++ b/OvmfPkg/Include/Library/MemEncryptSevLib.h
@@ -100,8 +100,6 @@ MemEncryptSevIsEnabled (
address of a memory region.
@param[in] NumPages The number of pages from start memory
region.
- @param[in] Flush Flush the caches before clearing the bit
- (mostly TRUE except MMIO addresses)

@retval RETURN_SUCCESS The attributes were cleared for the
memory region.
@@ -114,8 +112,7 @@ EFIAPI
MemEncryptSevClearPageEncMask (
IN PHYSICAL_ADDRESS Cr3BaseAddress,
IN PHYSICAL_ADDRESS BaseAddress,
- IN UINTN NumPages,
- IN BOOLEAN Flush
+ IN UINTN NumPages
);

/**
@@ -128,8 +125,6 @@ MemEncryptSevClearPageEncMask (
address of a memory region.
@param[in] NumPages The number of pages from start memory
region.
- @param[in] Flush Flush the caches before setting the bit
- (mostly TRUE except MMIO addresses)

@retval RETURN_SUCCESS The attributes were set for the memory
region.
@@ -142,8 +137,7 @@ EFIAPI
MemEncryptSevSetPageEncMask (
IN PHYSICAL_ADDRESS Cr3BaseAddress,
IN PHYSICAL_ADDRESS BaseAddress,
- IN UINTN NumPages,
- IN BOOLEAN Flush
+ IN UINTN NumPages
);


diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/VirtualMemory.h b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/VirtualMemory.h
index 8dc39e647b90..21bbbd1c4f9c 100644
--- a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/VirtualMemory.h
+++ b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/VirtualMemory.h
@@ -58,8 +58,6 @@ InternalGetMemEncryptionAddressMask (
@param[in] PhysicalAddress The physical address that is the start
address of a memory region.
@param[in] Length The length of memory region
- @param[in] Flush Flush the caches before applying the
- encryption mask

@retval RETURN_SUCCESS The attributes were cleared for the
memory region.
@@ -72,8 +70,7 @@ EFIAPI
InternalMemEncryptSevSetMemoryDecrypted (
IN PHYSICAL_ADDRESS Cr3BaseAddress,
IN PHYSICAL_ADDRESS PhysicalAddress,
- IN UINTN Length,
- IN BOOLEAN Flush
+ IN UINTN Length
);

/**
@@ -85,8 +82,6 @@ InternalMemEncryptSevSetMemoryDecrypted (
@param[in] PhysicalAddress The physical address that is the start
address of a memory region.
@param[in] Length The length of memory region
- @param[in] Flush Flush the caches before applying the
- encryption mask

@retval RETURN_SUCCESS The attributes were set for the memory
region.
@@ -99,8 +94,7 @@ EFIAPI
InternalMemEncryptSevSetMemoryEncrypted (
IN PHYSICAL_ADDRESS Cr3BaseAddress,
IN PHYSICAL_ADDRESS PhysicalAddress,
- IN UINTN Length,
- IN BOOLEAN Flush
+ IN UINTN Length
);

/**
diff --git a/OvmfPkg/AmdSevDxe/AmdSevDxe.c b/OvmfPkg/AmdSevDxe/AmdSevDxe.c
index 80831b81facf..c66c4e9b9272 100644
--- a/OvmfPkg/AmdSevDxe/AmdSevDxe.c
+++ b/OvmfPkg/AmdSevDxe/AmdSevDxe.c
@@ -120,8 +120,7 @@ AmdSevDxeEntryPoint (
Status = MemEncryptSevClearPageEncMask (
0, // Cr3BaseAddress -- use current CR3
MapPagesBase, // BaseAddress
- MapPagesCount, // NumPages
- TRUE // Flush
+ MapPagesCount // NumPages
);
if (EFI_ERROR (Status)) {
DEBUG ((DEBUG_ERROR, "%a: MemEncryptSevClearPageEncMask(): %r\n",
diff --git a/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c b/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c
index 49ffa2448811..b30628078f73 100644
--- a/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c
+++ b/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c
@@ -252,8 +252,7 @@ IoMmuMap (
Status = MemEncryptSevClearPageEncMask (
0,
MapInfo->PlainTextAddress,
- MapInfo->NumberOfPages,
- TRUE
+ MapInfo->NumberOfPages
);
ASSERT_EFI_ERROR (Status);
if (EFI_ERROR (Status)) {
@@ -407,8 +406,7 @@ IoMmuUnmapWorker (
Status = MemEncryptSevSetPageEncMask (
0,
MapInfo->PlainTextAddress,
- MapInfo->NumberOfPages,
- TRUE
+ MapInfo->NumberOfPages
);
ASSERT_EFI_ERROR (Status);
if (EFI_ERROR (Status)) {
diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/Ia32/MemEncryptSevLib.c b/OvmfPkg/Library/BaseMemEncryptSevLib/Ia32/MemEncryptSevLib.c
index 169d3118e44f..be260e0d1014 100644
--- a/OvmfPkg/Library/BaseMemEncryptSevLib/Ia32/MemEncryptSevLib.c
+++ b/OvmfPkg/Library/BaseMemEncryptSevLib/Ia32/MemEncryptSevLib.c
@@ -25,8 +25,6 @@
address of a memory region.
@param[in] NumPages The number of pages from start memory
region.
- @param[in] Flush Flush the caches before clearing the bit
- (mostly TRUE except MMIO addresses)

@retval RETURN_SUCCESS The attributes were cleared for the
memory region.
@@ -39,8 +37,7 @@ EFIAPI
MemEncryptSevClearPageEncMask (
IN PHYSICAL_ADDRESS Cr3BaseAddress,
IN PHYSICAL_ADDRESS BaseAddress,
- IN UINTN NumPages,
- IN BOOLEAN Flush
+ IN UINTN NumPages
)
{
//
@@ -59,8 +56,6 @@ MemEncryptSevClearPageEncMask (
address of a memory region.
@param[in] NumPages The number of pages from start memory
region.
- @param[in] Flush Flush the caches before setting the bit
- (mostly TRUE except MMIO addresses)

@retval RETURN_SUCCESS The attributes were set for the memory
region.
@@ -73,8 +68,7 @@ EFIAPI
MemEncryptSevSetPageEncMask (
IN PHYSICAL_ADDRESS Cr3BaseAddress,
IN PHYSICAL_ADDRESS BaseAddress,
- IN UINTN NumPages,
- IN BOOLEAN Flush
+ IN UINTN NumPages
)
{
//
diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/MemEncryptSevLib.c b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/MemEncryptSevLib.c
index a2bf698bcde7..a57e8fd37fa7 100644
--- a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/MemEncryptSevLib.c
+++ b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/MemEncryptSevLib.c
@@ -27,8 +27,6 @@
address of a memory region.
@param[in] NumPages The number of pages from start memory
region.
- @param[in] Flush Flush the caches before clearing the bit
- (mostly TRUE except MMIO addresses)

@retval RETURN_SUCCESS The attributes were cleared for the
memory region.
@@ -41,15 +39,13 @@ EFIAPI
MemEncryptSevClearPageEncMask (
IN PHYSICAL_ADDRESS Cr3BaseAddress,
IN PHYSICAL_ADDRESS BaseAddress,
- IN UINTN NumPages,
- IN BOOLEAN Flush
+ IN UINTN NumPages
)
{
return InternalMemEncryptSevSetMemoryDecrypted (
Cr3BaseAddress,
BaseAddress,
- EFI_PAGES_TO_SIZE (NumPages),
- Flush
+ EFI_PAGES_TO_SIZE (NumPages)
);
}

@@ -63,8 +59,6 @@ MemEncryptSevClearPageEncMask (
address of a memory region.
@param[in] NumPages The number of pages from start memory
region.
- @param[in] Flush Flush the caches before setting the bit
- (mostly TRUE except MMIO addresses)

@retval RETURN_SUCCESS The attributes were set for the memory
region.
@@ -77,15 +71,13 @@ EFIAPI
MemEncryptSevSetPageEncMask (
IN PHYSICAL_ADDRESS Cr3BaseAddress,
IN PHYSICAL_ADDRESS BaseAddress,
- IN UINTN NumPages,
- IN BOOLEAN Flush
+ IN UINTN NumPages
)
{
return InternalMemEncryptSevSetMemoryEncrypted (
Cr3BaseAddress,
BaseAddress,
- EFI_PAGES_TO_SIZE (NumPages),
- Flush
+ EFI_PAGES_TO_SIZE (NumPages)
);
}

diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/PeiDxeVirtualMemory.c b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/PeiDxeVirtualMemory.c
index a18d336a8789..c696745f9d26 100644
--- a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/PeiDxeVirtualMemory.c
+++ b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/PeiDxeVirtualMemory.c
@@ -828,8 +828,6 @@ SetMemoryEncDec (
@param[in] PhysicalAddress The physical address that is the start
address of a memory region.
@param[in] Length The length of memory region
- @param[in] Flush Flush the caches before applying the
- encryption mask

@retval RETURN_SUCCESS The attributes were cleared for the
memory region.
@@ -842,8 +840,7 @@ EFIAPI
InternalMemEncryptSevSetMemoryDecrypted (
IN PHYSICAL_ADDRESS Cr3BaseAddress,
IN PHYSICAL_ADDRESS PhysicalAddress,
- IN UINTN Length,
- IN BOOLEAN Flush
+ IN UINTN Length
)
{

@@ -852,7 +849,7 @@ InternalMemEncryptSevSetMemoryDecrypted (
PhysicalAddress,
Length,
ClearCBit,
- Flush
+ TRUE
);
}

@@ -865,8 +862,6 @@ InternalMemEncryptSevSetMemoryDecrypted (
@param[in] PhysicalAddress The physical address that is the start
address of a memory region.
@param[in] Length The length of memory region
- @param[in] Flush Flush the caches before applying the
- encryption mask

@retval RETURN_SUCCESS The attributes were set for the memory
region.
@@ -879,8 +874,7 @@ EFIAPI
InternalMemEncryptSevSetMemoryEncrypted (
IN PHYSICAL_ADDRESS Cr3BaseAddress,
IN PHYSICAL_ADDRESS PhysicalAddress,
- IN UINTN Length,
- IN BOOLEAN Flush
+ IN UINTN Length
)
{
return SetMemoryEncDec (
@@ -888,7 +882,7 @@ InternalMemEncryptSevSetMemoryEncrypted (
PhysicalAddress,
Length,
SetCBit,
- Flush
+ TRUE
);
}

diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/SecVirtualMemory.c b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/SecVirtualMemory.c
index e0d3a15e8503..a155c8729bfb 100644
--- a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/SecVirtualMemory.c
+++ b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/SecVirtualMemory.c
@@ -42,8 +42,6 @@ InternalGetMemEncryptionAddressMask (
@param[in] PhysicalAddress The physical address that is the start
address of a memory region.
@param[in] Length The length of memory region
- @param[in] Flush Flush the caches before applying the
- encryption mask

@retval RETURN_SUCCESS The attributes were cleared for the
memory region.
@@ -56,8 +54,7 @@ EFIAPI
InternalMemEncryptSevSetMemoryDecrypted (
IN PHYSICAL_ADDRESS Cr3BaseAddress,
IN PHYSICAL_ADDRESS PhysicalAddress,
- IN UINTN Length,
- IN BOOLEAN Flush
+ IN UINTN Length
)
{
//
@@ -89,8 +86,7 @@ EFIAPI
InternalMemEncryptSevSetMemoryEncrypted (
IN PHYSICAL_ADDRESS Cr3BaseAddress,
IN PHYSICAL_ADDRESS PhysicalAddress,
- IN UINTN Length,
- IN BOOLEAN Flush
+ IN UINTN Length
)
{
//
diff --git a/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c b/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c
index fdf2380974fa..c7cc5b0389c8 100644
--- a/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c
+++ b/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c
@@ -283,8 +283,7 @@ SmmCpuFeaturesSmmRelocationComplete (
Status = MemEncryptSevSetPageEncMask (
0, // Cr3BaseAddress -- use current CR3
MapPagesBase, // BaseAddress
- MapPagesCount, // NumPages
- TRUE // Flush
+ MapPagesCount // NumPages
);
if (EFI_ERROR (Status)) {
DEBUG ((DEBUG_ERROR, "%a: MemEncryptSevSetPageEncMask(): %r\n",
diff --git a/OvmfPkg/PlatformPei/AmdSev.c b/OvmfPkg/PlatformPei/AmdSev.c
index dddffdebda4b..a8bf610022ba 100644
--- a/OvmfPkg/PlatformPei/AmdSev.c
+++ b/OvmfPkg/PlatformPei/AmdSev.c
@@ -72,8 +72,7 @@ AmdSevEsInitialize (
DecryptStatus = MemEncryptSevClearPageEncMask (
0,
GhcbBasePa + EFI_PAGES_TO_SIZE (PageCount),
- 1,
- TRUE
+ 1
);
ASSERT_RETURN_ERROR (DecryptStatus);
}

On 05/13/21 01:46, Brijesh Singh wrote:

BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3275

The MemEncryptSevClearMmioPageEncMask() helper can be used for clearing
the memory encryption mask for the Mmio region.

The MemEncryptSevClearMmioPageEncMask() is a simplified version of
MemEncryptSevClearPageEncMask() -- it does not flush the caches after
clearing the page encryption mask.

Cc: James Bottomley <jejb@...>
Cc: Min Xu <min.m.xu@...>
Cc: Jiewen Yao <jiewen.yao@...>
Cc: Tom Lendacky <thomas.lendacky@...>
Cc: Jordan Justen <jordan.l.justen@...>
Cc: Ard Biesheuvel <ardb+tianocore@...>
Cc: Laszlo Ersek <lersek@...>
Cc: Erdem Aktas <erdemaktas@...>
Reviewed-by: Laszlo Ersek <lersek@...>
Signed-off-by: Brijesh Singh <brijesh.singh@...>
---
OvmfPkg/Include/Library/MemEncryptSevLib.h | 25 ++++++++++++++
.../BaseMemEncryptSevLib/X64/VirtualMemory.h | 23 +++++++++++++
.../Ia32/MemEncryptSevLib.c | 31 +++++++++++++++++
.../X64/MemEncryptSevLib.c | 33 +++++++++++++++++++
.../X64/PeiDxeVirtualMemory.c | 33 +++++++++++++++++++
.../X64/SecVirtualMemory.c | 30 +++++++++++++++++
6 files changed, 175 insertions(+)

Sorry that I missed last time that the "SecMemEncryptSevLib.inf"
instance was not extended with a definition for the new function.

This update looks OK; my R-b stands.

Thanks
Laszlo

diff --git a/OvmfPkg/Include/Library/MemEncryptSevLib.h b/OvmfPkg/Include/Library/MemEncryptSevLib.h
index 99f15a7d1271..b91490d5d44d 100644
--- a/OvmfPkg/Include/Library/MemEncryptSevLib.h
+++ b/OvmfPkg/Include/Library/MemEncryptSevLib.h
@@ -203,4 +203,29 @@ MemEncryptSevGetAddressRangeState (
IN UINTN Length
);

+/**
+ This function clears memory encryption bit for the MMIO region specified by
+ BaseAddress and NumPages.
+
+ @param[in] Cr3BaseAddress Cr3 Base Address (if zero then use
+ current CR3)
+ @param[in] BaseAddress The physical address that is the start
+ address of a MMIO region.
+ @param[in] NumPages The number of pages from start memory
+ region.
+
+ @retval RETURN_SUCCESS The attributes were cleared for the
+ memory region.
+ @retval RETURN_INVALID_PARAMETER Number of pages is zero.
+ @retval RETURN_UNSUPPORTED Clearing the memory encryption attribute
+ is not supported
+**/
+RETURN_STATUS
+EFIAPI
+MemEncryptSevClearMmioPageEncMask (
+ IN PHYSICAL_ADDRESS Cr3BaseAddress,
+ IN PHYSICAL_ADDRESS BaseAddress,
+ IN UINTN NumPages
+ );
+
#endif // _MEM_ENCRYPT_SEV_LIB_H_
diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/VirtualMemory.h b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/VirtualMemory.h
index fe2a0b2826cd..8dc39e647b90 100644
--- a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/VirtualMemory.h
+++ b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/VirtualMemory.h
@@ -126,4 +126,27 @@ InternalMemEncryptSevGetAddressRangeState (
IN UINTN Length
);

+/**
+ This function clears memory encryption bit for the MMIO region specified by
+ PhysicalAddress and Length.
+
+ @param[in] Cr3BaseAddress Cr3 Base Address (if zero then use
+ current CR3)
+ @param[in] PhysicalAddress The physical address that is the start
+ address of a MMIO region.
+ @param[in] Length The length of memory region
+
+ @retval RETURN_SUCCESS The attributes were cleared for the
+ memory region.
+ @retval RETURN_INVALID_PARAMETER Length is zero.
+ @retval RETURN_UNSUPPORTED Clearing the memory encyrption attribute
+ is not supported
+**/
+RETURN_STATUS
+EFIAPI
+InternalMemEncryptSevClearMmioPageEncMask (
+ IN PHYSICAL_ADDRESS Cr3BaseAddress,
+ IN PHYSICAL_ADDRESS PhysicalAddress,
+ IN UINTN Length
+ );
#endif
diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/Ia32/MemEncryptSevLib.c b/OvmfPkg/Library/BaseMemEncryptSevLib/Ia32/MemEncryptSevLib.c
index 12a5bf495bd7..169d3118e44f 100644
--- a/OvmfPkg/Library/BaseMemEncryptSevLib/Ia32/MemEncryptSevLib.c
+++ b/OvmfPkg/Library/BaseMemEncryptSevLib/Ia32/MemEncryptSevLib.c
@@ -111,3 +111,34 @@ MemEncryptSevGetAddressRangeState (
//
return MemEncryptSevAddressRangeEncrypted;
}
+
+/**
+ This function clears memory encryption bit for the MMIO region specified by
+ BaseAddress and NumPages.
+
+ @param[in] Cr3BaseAddress Cr3 Base Address (if zero then use
+ current CR3)
+ @param[in] BaseAddress The physical address that is the start
+ address of a MMIO region.
+ @param[in] NumPages The number of pages from start memory
+ region.
+
+ @retval RETURN_SUCCESS The attributes were cleared for the
+ memory region.
+ @retval RETURN_INVALID_PARAMETER Number of pages is zero.
+ @retval RETURN_UNSUPPORTED Clearing the memory encryption attribute
+ is not supported
+**/
+RETURN_STATUS
+EFIAPI
+MemEncryptSevClearMmioPageEncMask (
+ IN PHYSICAL_ADDRESS Cr3BaseAddress,
+ IN PHYSICAL_ADDRESS BaseAddress,
+ IN UINTN NumPages
+ )
+{
+ //
+ // Memory encryption bit is not accessible in 32-bit mode
+ //
+ return RETURN_UNSUPPORTED;
+}
diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/MemEncryptSevLib.c b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/MemEncryptSevLib.c
index 4fea6a6be0ac..a2bf698bcde7 100644
--- a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/MemEncryptSevLib.c
+++ b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/MemEncryptSevLib.c
@@ -118,3 +118,36 @@ MemEncryptSevGetAddressRangeState (
Length
);
}
+
+/**
+ This function clears memory encryption bit for the mmio region specified by
+ BaseAddress and NumPages.
+
+ @param[in] Cr3BaseAddress Cr3 Base Address (if zero then use
+ current CR3)
+ @param[in] BaseAddress The physical address that is the start
+ address of a mmio region.
+ @param[in] NumPages The number of pages from start memory
+ region.
+
+ @retval RETURN_SUCCESS The attributes were cleared for the
+ memory region.
+ @retval RETURN_INVALID_PARAMETER Number of pages is zero.
+ @retval RETURN_UNSUPPORTED Clearing the memory encryption attribute
+ is not supported
+**/
+RETURN_STATUS
+EFIAPI
+MemEncryptSevClearMmioPageEncMask (
+ IN PHYSICAL_ADDRESS Cr3BaseAddress,
+ IN PHYSICAL_ADDRESS BaseAddress,
+ IN UINTN NumPages
+ )
+{
+ return InternalMemEncryptSevClearMmioPageEncMask (
+ Cr3BaseAddress,
+ BaseAddress,
+ EFI_PAGES_TO_SIZE (NumPages)
+ );
+
+}
diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/PeiDxeVirtualMemory.c b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/PeiDxeVirtualMemory.c
index d3455e812bd1..a18d336a8789 100644
--- a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/PeiDxeVirtualMemory.c
+++ b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/PeiDxeVirtualMemory.c
@@ -891,3 +891,36 @@ InternalMemEncryptSevSetMemoryEncrypted (
Flush
);
}
+
+/**
+ This function clears memory encryption bit for the MMIO region specified by
+ PhysicalAddress and Length.
+
+ @param[in] Cr3BaseAddress Cr3 Base Address (if zero then use
+ current CR3)
+ @param[in] PhysicalAddress The physical address that is the start
+ address of a MMIO region.
+ @param[in] Length The length of memory region
+
+ @retval RETURN_SUCCESS The attributes were cleared for the
+ memory region.
+ @retval RETURN_INVALID_PARAMETER Length is zero.
+ @retval RETURN_UNSUPPORTED Clearing the memory encyrption attribute
+ is not supported
+**/
+RETURN_STATUS
+EFIAPI
+InternalMemEncryptSevClearMmioPageEncMask (
+ IN PHYSICAL_ADDRESS Cr3BaseAddress,
+ IN PHYSICAL_ADDRESS PhysicalAddress,
+ IN UINTN Length
+ )
+{
+ return SetMemoryEncDec (
+ Cr3BaseAddress,
+ PhysicalAddress,
+ Length,
+ ClearCBit,
+ FALSE
+ );
+}
diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/SecVirtualMemory.c b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/SecVirtualMemory.c
index bca5e3febb1b..e0d3a15e8503 100644
--- a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/SecVirtualMemory.c
+++ b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/SecVirtualMemory.c
@@ -98,3 +98,33 @@ InternalMemEncryptSevSetMemoryEncrypted (
//
return RETURN_UNSUPPORTED;
}
+
+/**
+ This function clears memory encryption bit for the MMIO region specified by
+ PhysicalAddress and Length.
+
+ @param[in] Cr3BaseAddress Cr3 Base Address (if zero then use
+ current CR3)
+ @param[in] PhysicalAddress The physical address that is the start
+ address of a MMIO region.
+ @param[in] Length The length of memory region
+
+ @retval RETURN_SUCCESS The attributes were cleared for the
+ memory region.
+ @retval RETURN_INVALID_PARAMETER Length is zero.
+ @retval RETURN_UNSUPPORTED Clearing the memory encyrption attribute
+ is not supported
+**/
+RETURN_STATUS
+EFIAPI
+InternalMemEncryptSevClearMmioPageEncMask (
+ IN PHYSICAL_ADDRESS Cr3BaseAddress,
+ IN PHYSICAL_ADDRESS PhysicalAddress,
+ IN UINTN Length
+ )
+{
+ //
+ // This function is not available during SEC.
+ //
+ return RETURN_UNSUPPORTED;
+}

On 05/13/21 01:46, Brijesh Singh wrote:

From: Tom Lendacky <thomas.lendacky@...>

BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3275

The RMPADJUST instruction will be used by the SEV-SNP guest to modify the
RMP permissions for a guest page. See AMD APM volume 3 for further
details.

Cc: James Bottomley <jejb@...>
Cc: Min Xu <min.m.xu@...>
Cc: Jiewen Yao <jiewen.yao@...>
Cc: Tom Lendacky <thomas.lendacky@...>
Cc: Jordan Justen <jordan.l.justen@...>
Cc: Ard Biesheuvel <ardb+tianocore@...>
Cc: Laszlo Ersek <lersek@...>
Cc: Erdem Aktas <erdemaktas@...>
Cc: Michael D Kinney <michael.d.kinney@...>
Cc: Liming Gao <gaoliming@...>
Cc: Zhiguang Liu <zhiguang.liu@...>
Reviewed-by: Laszlo Ersek <lersek@...>
Reviewed-by: Liming Gao <gaoliming@...>
Signed-off-by: Tom Lendacky <thomas.lendacky@...>
Signed-off-by: Brijesh Singh <brijesh.singh@...>
---
MdePkg/Library/BaseLib/BaseLib.inf | 1 +
MdePkg/Include/Library/BaseLib.h | 35 ++++++++++++++++++++
MdePkg/Include/X64/Nasm.inc | 8 +++++
MdePkg/Library/BaseLib/X64/RmpAdjust.nasm | 40 +++++++++++++++++++++++
4 files changed, 84 insertions(+)
create mode 100644 MdePkg/Library/BaseLib/X64/RmpAdjust.nasm

diff --git a/MdePkg/Library/BaseLib/BaseLib.inf b/MdePkg/Library/BaseLib/BaseLib.inf
index 89a52f72c08a..6ccb8997b7e8 100644
--- a/MdePkg/Library/BaseLib/BaseLib.inf
+++ b/MdePkg/Library/BaseLib/BaseLib.inf
@@ -319,6 +319,7 @@ [Sources.X64]
X64/DisablePaging64.nasm
X64/Pvalidate.nasm
X64/RdRand.nasm
+ X64/RmpAdjust.nasm
X64/XGetBv.nasm
X64/XSetBv.nasm
X64/VmgExit.nasm
diff --git a/MdePkg/Include/Library/BaseLib.h b/MdePkg/Include/Library/BaseLib.h
index a2cd134bea9a..e8eff32716b8 100644
--- a/MdePkg/Include/Library/BaseLib.h
+++ b/MdePkg/Include/Library/BaseLib.h
@@ -4861,6 +4861,41 @@ AsmPvalidate (
IN BOOLEAN Validate,
IN PHYSICAL_ADDRESS Address
);
+
+//
+// RDX settings for RMPADJUST
+//
+#define RMPADJUST_VMPL_MAX 3
+#define RMPADJUST_VMPL_MASK 0xFF
+#define RMPADJUST_VMPL_SHIFT 0
+#define RMPADJUST_PERMISSION_MASK_MASK 0xFF
+#define RMPADJUST_PERMISSION_MASK_SHIFT 8
+#define RMPADJUST_VMSA_PAGE_BIT BIT16
+
+/**
+ Adjusts the permissions of an SEV-SNP guest page.
+
+ Executes a RMPADJUST instruction with the register state specified by Rax,
+ Rcx, and Rdx. Returns Rax. This function is only available on X64.
+
+ The instruction is available only when CPUID Fn8000_001F_EAX[SNP]=1.
+
+ @param[in] Rax The value to load into RAX before executing the RMPADJUST
+ instruction.
+ @param[in] Rcx The value to load into RCX before executing the RMPADJUST
+ instruction.
+ @param[in] Rdx The value to load into RDX before executing the RMPADJUST
+ instruction.
+
+ @return Rax
+**/
+UINT32
+EFIAPI
+AsmRmpAdjust (
+ IN UINT64 Rax,
+ IN UINT64 Rcx,
+ IN UINT64 Rdx
+ );
#endif

(1) Relative to v1, the updates are all good (thanks!), except for one:
"@return Rax" is actually a regression -- "@return Eax" was correct, in
my opinion.

This also affects the top of the leading comment, where Eax was also
replaced with Rax.

My R-b stands, but please fix the docs on this function again, because
now the documentation ("Rax", twice) is actually inconsistent with the
return type UINT32.

Thanks
Laszlo

diff --git a/MdePkg/Include/X64/Nasm.inc b/MdePkg/Include/X64/Nasm.inc
index 528bb3385609..cfb14edc9449 100644
--- a/MdePkg/Include/X64/Nasm.inc
+++ b/MdePkg/Include/X64/Nasm.inc
@@ -41,6 +41,14 @@
DB 0xF2, 0x0F, 0x01, 0xFF
%endmacro

+;
+; Macro for the RMPADJUST instruction, defined in AMD APM volume 3.
+; NASM feature request URL: https://bugzilla.nasm.us/show_bug.cgi?id=3392754
+;
+%macro RMPADJUST 0
+ DB 0xF3, 0x0F, 0x01, 0xFE
+%endmacro
+
; NASM provides built-in macros STRUC and ENDSTRUC for structure definition.
; For example, to define a structure called mytype containing a longword,
; a word, a byte and a string of bytes, you might code
diff --git a/MdePkg/Library/BaseLib/X64/RmpAdjust.nasm b/MdePkg/Library/BaseLib/X64/RmpAdjust.nasm
new file mode 100644
index 000000000000..c307f64b518a
--- /dev/null
+++ b/MdePkg/Library/BaseLib/X64/RmpAdjust.nasm
@@ -0,0 +1,40 @@
+;-----------------------------------------------------------------------------
+;
+; Copyright (c) 2021, Advanced Micro Devices, Inc. All rights reserved.<BR>
+; SPDX-License-Identifier: BSD-2-Clause-Patent
+;
+; Module Name:
+;
+; RmpAdjust.Asm
+;
+; Abstract:
+;
+; AsmRmpAdjust function
+;
+; Notes:
+;
+;-----------------------------------------------------------------------------
+
+%include "Nasm.inc"
+
+ SECTION .text
+
+;-----------------------------------------------------------------------------
+; UINT32
+; EFIAPI
+; AsmRmpAdjust (
+; IN UINT64 Rax,
+; IN UINT64 Rcx,
+; IN UINT64 Rdx
+; )
+;-----------------------------------------------------------------------------
+global ASM_PFX(AsmRmpAdjust)
+ASM_PFX(AsmRmpAdjust):
+ mov rax, rcx ; Input Rax is in RCX by calling convention
+ mov rcx, rdx ; Input Rcx is in RDX by calling convention
+ mov rdx, r8 ; Input Rdx is in R8 by calling convention
+
+ RMPADJUST
+
+ ; RMPADJUST returns the status in the EAX register.
+ ret

On 05/13/21 01:46, Brijesh Singh wrote:

BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3275

The PVALIDATE instruction validates or rescinds validation of a guest
page RMP entry. Upon completion, a return code is stored in EAX, rFLAGS
bits OF, ZF, AF, PF and SF are set based on this return code. If the
instruction completed succesfully, the rFLAGS bit CF indicates if the
contents of the RMP entry were changed or not.

For more information about the instruction see AMD APM volume 3.

Cc: James Bottomley <jejb@...>
Cc: Min Xu <min.m.xu@...>
Cc: Jiewen Yao <jiewen.yao@...>
Cc: Tom Lendacky <thomas.lendacky@...>
Cc: Jordan Justen <jordan.l.justen@...>
Cc: Ard Biesheuvel <ardb+tianocore@...>
Cc: Laszlo Ersek <lersek@...>
Cc: Erdem Aktas <erdemaktas@...>
Cc: Michael D Kinney <michael.d.kinney@...>
Cc: Liming Gao <gaoliming@...>
Cc: Zhiguang Liu <zhiguang.liu@...>
Reviewed-by: Liming Gao <gaoliming@...>
Signed-off-by: Brijesh Singh <brijesh.singh@...>
---
MdePkg/Library/BaseLib/BaseLib.inf | 1 +
MdePkg/Include/Library/BaseLib.h | 50 +++++++++++++++++++++++
MdePkg/Include/X64/Nasm.inc | 8 ++++
MdePkg/Library/BaseLib/X64/Pvalidate.nasm | 42 +++++++++++++++++++
4 files changed, 101 insertions(+)
create mode 100644 MdePkg/Library/BaseLib/X64/Pvalidate.nasm

Many thanks for the updates!

Reviewed-by: Laszlo Ersek <lersek@...>

Thanks
Laszlo

diff --git a/MdePkg/Library/BaseLib/BaseLib.inf b/MdePkg/Library/BaseLib/BaseLib.inf
index b76f3af380ea..89a52f72c08a 100644
--- a/MdePkg/Library/BaseLib/BaseLib.inf
+++ b/MdePkg/Library/BaseLib/BaseLib.inf
@@ -317,6 +317,7 @@ [Sources.X64]
X64/GccInlinePriv.c | GCC
X64/EnableDisableInterrupts.nasm
X64/DisablePaging64.nasm
+ X64/Pvalidate.nasm
X64/RdRand.nasm
X64/XGetBv.nasm
X64/XSetBv.nasm
diff --git a/MdePkg/Include/Library/BaseLib.h b/MdePkg/Include/Library/BaseLib.h
index 7253997a6f8c..a2cd134bea9a 100644
--- a/MdePkg/Include/Library/BaseLib.h
+++ b/MdePkg/Include/Library/BaseLib.h
@@ -4813,6 +4813,56 @@ SpeculationBarrier (
VOID
);

+#if defined (MDE_CPU_X64)
+//
+// The page size for the PVALIDATE instruction
+//
+typedef enum {
+ PvalidatePageSize4K = 0,
+ PvalidatePageSize2MB,
+} PVALIDATE_PAGE_SIZE;
+
+//
+// PVALIDATE Return Code.
+//
+#define PVALIDATE_RET_SUCCESS 0
+#define PVALIDATE_RET_FAIL_INPUT 1
+#define PVALIDATE_RET_SIZE_MISMATCH 6
+
+//
+// The PVALIDATE instruction did not make any changes to the RMP entry.
+//
+#define PVALIDATE_RET_NO_RMPUPDATE 255
+
+/**
+ Execute a PVALIDATE instruction to validate or to rescinds validation of a guest
+ page's RMP entry.
+
+ The instruction is available only when CPUID Fn8000_001F_EAX[SNP]=1.
+
+ The function is available on X64.
+
+ @param[in] PageSize The page size to use.
+ @param[in] Validate If TRUE, validate the guest virtual address
+ otherwise invalidate the guest virtual address.
+ @param[in] Address The guest virtual address.
+
+ @retval PVALIDATE_RET_SUCCESS The PVALIDATE instruction succeeded, and
+ updated the RMP entry.
+ @retval PVALIDATE_RET_NO_RMPUPDATE The PVALIDATE instruction succeeded, but
+ did not update the RMP entry.
+ @return Failure code from the PVALIDATE
+ instruction.
+**/
+UINT32
+EFIAPI
+AsmPvalidate (
+ IN PVALIDATE_PAGE_SIZE PageSize,
+ IN BOOLEAN Validate,
+ IN PHYSICAL_ADDRESS Address
+ );
+#endif
+

#if defined (MDE_CPU_IA32) || defined (MDE_CPU_X64)
///
diff --git a/MdePkg/Include/X64/Nasm.inc b/MdePkg/Include/X64/Nasm.inc
index 527f71e9eb4d..528bb3385609 100644
--- a/MdePkg/Include/X64/Nasm.inc
+++ b/MdePkg/Include/X64/Nasm.inc
@@ -33,6 +33,14 @@
DB 0xF3, 0x48, 0x0F, 0xAE, 0xE8
%endmacro

+;
+; Macro for the PVALIDATE instruction, defined in AMD APM volume 3.
+; NASM feature request URL: https://bugzilla.nasm.us/show_bug.cgi?id=3392753
+;
+%macro PVALIDATE 0
+ DB 0xF2, 0x0F, 0x01, 0xFF
+%endmacro
+
; NASM provides built-in macros STRUC and ENDSTRUC for structure definition.
; For example, to define a structure called mytype containing a longword,
; a word, a byte and a string of bytes, you might code
diff --git a/MdePkg/Library/BaseLib/X64/Pvalidate.nasm b/MdePkg/Library/BaseLib/X64/Pvalidate.nasm
new file mode 100644
index 000000000000..a7d177913405
--- /dev/null
+++ b/MdePkg/Library/BaseLib/X64/Pvalidate.nasm
@@ -0,0 +1,42 @@
+;-----------------------------------------------------------------------------
+;
+; Copyright (c) 2021, AMD. All rights reserved.<BR>
+; SPDX-License-Identifier: BSD-2-Clause-Patent
+;
+;-----------------------------------------------------------------------------
+
+%include "Nasm.inc"
+
+ SECTION .text
+
+;-----------------------------------------------------------------------------
+; UINT32
+; EFIAPI
+; AsmPvalidate (
+; IN UINT32 PageSize
+; IN UINT32 Validate,
+; IN UINT64 Address
+; )
+;-----------------------------------------------------------------------------
+global ASM_PFX(AsmPvalidate)
+ASM_PFX(AsmPvalidate):
+ mov rax, r8
+
+ PVALIDATE
+
+ ; Save the carry flag.
+ setc dl
+
+ ; The PVALIDATE instruction returns the status in rax register.
+ cmp rax, 0
+ jne PvalidateExit
+
+ ; Check the carry flag to determine if RMP entry was updated.
+ cmp dl, 0
+ je PvalidateExit
+
+ ; Return the PVALIDATE_RET_NO_RMPUPDATE.
+ mov rax, 255
+
+PvalidateExit:
+ ret

Patches v2 01-05 look good to me, thanks for the updates. Now on to this
one:

On 05/13/21 01:46, Brijesh Singh wrote:

From: Tom Lendacky <thomas.lendacky@...>

BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3D3275

(1) The "3D" seems like a typo in the bug ticket URL. (This crept into
v2 somehow; v1 didn't have it.)

Version 2 of GHCB introduces NAE for creating AP when SEV-SNP is enabled
in the guest VM. See the GHCB specification, Table 5 "List of Supported
Non-Automatic Events" and sections 4.1.9 and 4.3.2, for further details.

While at it, define the VMSA state save area that is required for creating
the AP. The save area format is defined in AMD APM volume 2, Table B-4
(there is a mistake in the table that defines the size of the reserved
area at offset 0xc8 as a dword, when it is actually a word). The format of
the save area segment registers is further defined in AMD APM volume 2,
sections 10 and 15.5.

Cc: James Bottomley <jejb@...>
Cc: Min Xu <min.m.xu@...>
Cc: Jiewen Yao <jiewen.yao@...>
Cc: Tom Lendacky <thomas.lendacky@...>
Cc: Jordan Justen <jordan.l.justen@...>
Cc: Ard Biesheuvel <ardb+tianocore@...>
Cc: Laszlo Ersek <lersek@...>
Cc: Erdem Aktas <erdemaktas@...>
Cc: Michael D Kinney <michael.d.kinney@...>
Cc: Liming Gao <gaoliming@...>
Cc: Zhiguang Liu <zhiguang.liu@...>
Reviewed-by: Liming Gao <gaoliming@...>
Signed-off-by: Tom Lendacky <thomas.lendacky@...>
Signed-off-by: Brijesh Singh <brijesh.singh@...>
---
MdePkg/Include/Register/Amd/Ghcb.h | 76 ++++++++++++++++++++++++++++++
1 file changed, 76 insertions(+)

diff --git a/MdePkg/Include/Register/Amd/Ghcb.h b/MdePkg/Include/Register/Amd/Ghcb.h
index 029904b1c63a..4d1ee29e0a5e 100644
--- a/MdePkg/Include/Register/Amd/Ghcb.h
+++ b/MdePkg/Include/Register/Amd/Ghcb.h
@@ -55,6 +55,7 @@
#define SVM_EXIT_AP_RESET_HOLD 0x80000004ULL
#define SVM_EXIT_AP_JUMP_TABLE 0x80000005ULL
#define SVM_EXIT_SNP_PAGE_STATE_CHANGE 0x80000010ULL
+#define SVM_EXIT_SNP_AP_CREATION 0x80000013ULL
#define SVM_EXIT_HYPERVISOR_FEATURES 0x8000FFFDULL
#define SVM_EXIT_UNSUPPORTED 0x8000FFFFULL

@@ -83,6 +84,12 @@
#define IOIO_SEG_ES 0
#define IOIO_SEG_DS (BIT11 | BIT10)

+//
+// AP Creation Information
+//
+#define SVM_VMGEXIT_SNP_AP_CREATE_ON_INIT 0
+#define SVM_VMGEXIT_SNP_AP_CREATE 1
+#define SVM_VMGEXIT_SNP_AP_DESTROY 2

typedef PACKED struct {
UINT8 Reserved1[203];
@@ -195,4 +202,73 @@ typedef struct {
SNP_PAGE_STATE_ENTRY Entry[SNP_PAGE_STATE_MAX_ENTRY];
} SNP_PAGE_STATE_CHANGE_INFO;

+//
+// SEV-ES save area mapping structures used for SEV-SNP AP Creation.
+// Only the fields required to be set to a non-zero value are defined.
+//
+#define SEV_ES_RESET_CODE_SEGMENT_TYPE 0xA
+#define SEV_ES_RESET_DATA_SEGMENT_TYPE 0x2
+
+#define SEV_ES_RESET_LDT_TYPE 0x2
+#define SEV_ES_RESET_TSS_TYPE 0x3
+
+#pragma pack (1)
+typedef union {
+ struct {
+ UINT16 Type:4;
+ UINT16 Sbit:1;
+ UINT16 Dpl:2;
+ UINT16 Present:1;
+ UINT16 Avl:1;
+ UINT16 Reserved1:1;
+ UINT16 Db:1;
+ UINT16 Granularity:1;
+ } Bits;
+ UINT16 Uint16;
+} SEV_ES_SEGMENT_REGISTER_ATTRIBUTES;
+
+typedef struct {
+ UINT16 Selector;
+ SEV_ES_SEGMENT_REGISTER_ATTRIBUTES Attributes;
+ UINT32 Limit;
+ UINT64 Base;
+} SEV_ES_SEGMENT_REGISTER;
+

I'm not saying anything is incorrect about this, but I *am* going to
rant about the APM.

It's simply impenetrable. I've been staring at it for ~50 minutes now,
and I still cannot fully connect it to your code.

[1] In sections "4.8.1 Code-Segment Descriptors" and "4.8.2 Data-Segment
Descriptors", the reader is introduced to the "normal" (not SEV-ES, not
virtualized, not SMM) segment descriptors. Why *these* are relevant
*here* is nothing short of mind-boggling, but please bear with me.

[2] In section "10.2.3 SMRAM State-Save Area", "Table 10-1. AMD64
Architecture SMM State-Save Area", the reader is introduced to the
2+2+4+8 segment register representation. The table only lists "Selector,
Attributes, Limit, Base" as fields, and nothing about the actual
contents. Way too little information. I guess this is covered by the
commit message reference "section 10".

[3] In section "15.5 VMRUN Instruction", "15.5.1 Basic Operation",
paragraph "Segment State in the VMCB", we're given a long-winded,
*textual* only -- no diagram! -- and *differential* (relative)
explanation, on top of the two, above-quoted, locations of the spec. I'm
sorry but this is just impossible to follow. Would it have been a
unaffordable to insert a self-contained diagram here, with
self-contained, absolute explanation?

So let me quote:

The segment registers are stored in the VMCB in a format similar to
that for SMM: both base and limit are fully expanded; segment
attributes are stored as 12-bit values formed by the concatenation
of bits 55:52 and 47:40 from the original 64-bit (in-memory) segment
descriptors; the descriptor “P” bit is used to signal NULL segments
(P=0) where permissible and/or relevant.

So, if we apply this to [1] and [2], the "Selector", "Limit" and "Base"
fields of the SMM and VMCB segment register representation are
explained. Further, we get the following for the Attributes field, by
manual reconstruction:

55 54 53 52 47 46 45 44 43 42 41 40

G D L AVL P [DPL] 1 1 C R A Code Segment Descriptor
* * * * * * ignored bits in 64-bit mode

G D/B - AVL P [DPL] 1 0 E W A Data Segment Descriptor
* * * * * * * * * * ignored bits in 64-bit mode

In other words, in the code segment descriptor, D, L, P, DPL, and C
matter. In the data segment descriptor, only P matters.

In particular, what [3] says, quoted above, about the "P" bit, seems
sensible -- "P" is indeed *not* ignored for either segment descriptor
type (code or data).

But then we continure reading [3], and we find (quote again):

The loading of segment attributes from the VMCB (which may have been
overwritten by software) may result in attribute bit values that are
otherwise not allowed. However, only some of the attribute bits are
actually observed by hardware, depending on the segment register in
question:
* CS—D, L, P, and R.
* SS—B, P, E, W, and Code/Data
* DS, ES, FS, GS —D, P, DPL, E, W, and Code/Data.
* LDTR—P, S, and Type (LDT)
* TR—P, S, and Type (32- or 16-bit TSS)

I'm going to ignore SS, LDTR, and TR, but let's check CS and DS.

For CS, the spec says, "D, L, P, and R" are observed by the hardware.
But we've just shown that R is ignored *in general* for code segments in
64-bit mode! In other words, { D, L, P, R } is *not a subset* of { D, L,
P, DPL, C }.

For DS, the spec says here, "D, P, DPL, E, W, and Code/Data" are
observed. I'm going to give "Code/Data" a pass (bit 43 in the original
representation), but the rest is {D, P, DPL, E, W}, which is *not a
subset* of { P }.

Again, from [1], section 4.8.2 specifically, E (expand-down) and W
(writeable) are ignored, DPL is ignored, and D isn't even called D but
"D/B", and it is ignored. So how can the spec say in [3] that the
hardware observes { D, DPL, E, W } when all those are ignored per prior
definition [1]?

- And then I see no reference that SEV-ES uses the same
(circumstantially-defined) segment register format.


So anyway, I'll just accept that I don't understand the spec -- I think
it has inconsistencies. Let's look at the code:

- Bits 43:40 are packed into the "Type" bit-field. That means [1,C,R,A]
for the code segment descriptor, and [0,E,W,A] for data segment descriptors

SEV_ES_RESET_CODE_SEGMENT_TYPE has value 0xA (binary 1010), which maps
to: 1=1, C=0, R=1, A=0. Problem: per [1], the R bit is ignored, so I
have no clue why we set it.

(2) Can you please explain that? Just in this discussion thread, no need
ot change the code or the commit message.

The C ("conforming") bit actually matters. It is documented in section
"4.7.2 Code-Segment Descriptors" (Code-Segment Descriptor—Legacy Mode).
It seems like "non-conforming" is not a problem here, as long as we
don't use multiple privilege levels, which I think we don't, in
firmware-land. OK.

Then, on to SEV_ES_RESET_DATA_SEGMENT_TYPE. Value 0x2 (binary 0010).
Maps to [0,E,W,A], meaning 0=0, E=0, W=1, A=0.

(3) Why is W (writeable) set to 1, when, according to [1], it is ignored
in 64-bit mode?


- "Sbit" seems to stand for bit 44 from the original representation --
constant 1. OK I think.

- "Dpl", "Present" and "Avl" look OK to me.

- Should "Reserved1" really be called that? It seems to map to bit 53 in
the original representation, and the L (long mode) bit actually matters
for the code segment. (It indeed appears undefined / reserved for data
segments.)

Am I *totally* mistaken here and we're not even talking long mode?

- "Db" and "Granularity" look OK.

- SEV_ES_RESET_LDT_TYPE (value 2) matches "64-bit LDT" in "4.8.3 System
Descriptors", "Table 4-6. System-Segment Descriptor Types—Long Mode". OK.

- SEV_ES_RESET_TSS_TYPE (value 3) seems wrong. In Table 4-6, value 3 is
associated with "Reserved (Illegal)". And, according to "12.2.2 TSS
Descriptor", "The TSS descriptor is a system-segment descriptor", which
makes me think that I'm looking at value 3 in the proper table (Table 4-6).


(4) Can you please explain why SEV_ES_RESET_TSS_TYPE is 3, and not (say)
0x9 ("Available 64-bit TSS") or 0xB ("Busy 64-bit TSS")?

(Please note that this is only superficial pattern matching from my side
("TSS"); I don't know the first thing about "hardware task management".)

+typedef struct {
+ SEV_ES_SEGMENT_REGISTER Es;
+ SEV_ES_SEGMENT_REGISTER Cs;
+ SEV_ES_SEGMENT_REGISTER Ss;
+ SEV_ES_SEGMENT_REGISTER Ds;
+ SEV_ES_SEGMENT_REGISTER Fs;
+ SEV_ES_SEGMENT_REGISTER Gs;
+ SEV_ES_SEGMENT_REGISTER Gdtr;
+ SEV_ES_SEGMENT_REGISTER Ldtr;
+ SEV_ES_SEGMENT_REGISTER Idtr;
+ SEV_ES_SEGMENT_REGISTER Tr;
+ UINT8 Reserved1[42];
+ UINT8 Vmpl;
+ UINT8 Reserved2[5];
+ UINT64 Efer;
+ UINT8 Reserved3[112];
+ UINT64 Cr4;
+ UINT8 Reserved4[8];
+ UINT64 Cr0;
+ UINT64 Dr7;
+ UINT64 Dr6;
+ UINT64 Rflags;
+ UINT64 Rip;
+ UINT8 Reserved5[232];
+ UINT64 GPat;
+ UINT8 Reserved6[320];
+ UINT64 SevFeatures;
+ UINT8 Reserved7[48];
+ UINT64 XCr0;
+ UINT8 Reserved8[24];
+ UINT32 Mxcsr;
+ UINT16 X87Ftw;
+ UINT8 Reserved9[2];
+ UINT16 X87Fcw;
+} SEV_ES_SAVE_AREA;
+#pragma pack ()
+
#endif

This part looks good to me.

(I spent almost two hours reviewing this patch.)

Thanks!
Laszlo

Pushed via:
PR: https://github.com/tianocore/edk2/pull/1647
Commit: https://github.com/tianocore/edk2/commit/e0cb5e1814a67bb12dd476a72d1698350633bcbb

Best Regards,
Hao Wu

How about below fix? I think it might be simpler to understand and doesn't introduce unnecessary logic to handle impossible case:
if (ResizableBarOp == PciResizableBarMax) {
Bit = HighBitSet64(Capabilities);
} else {
ASSERT (ResizableBarOp == PciResizableBarMin);
Bit = LowBitSet64(Capabilities);
}

Am 16.05.2021 um 03:17 schrieb Laszlo Ersek:

On 05/14/21 17:44, Marvin Häuser wrote:

On 14.05.21 17:23, Lendacky, Thomas wrote:

On 5/14/21 10:04 AM, Marvin Häuser wrote:

+      // 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);

Should the ASSERT not only catch the broken "allocate below" behaviour,
i.e. not trigger on failed allocation?

I think it's best to trigger on a failed allocation as well rather than
continuing and allowing a page fault or some other problem to occur.

Well, it should handle the error in a safe way, i.e. the deadloop below.
To not ASSERT on plausible conditions is a common design guideline in
most low-level projects including Linux kernel.

Best regards,
Marvin

Thanks,
Tom

+        CpuDeadLoop ();

"DEBUG + ASSERT(FALSE) + CpuDeadLoop()" is a pattern in edk2.
In RELEASE builds, it will lead to a CpuDeadLoop(). That's the main goal
-- don't continue execution if the condition controlling the whole block
fired.
In DEBUG and NOOPT builds, the pattern will lead to a debug message
(usually at the "error" level), followed by an assertion failure. The
error message of the assertion failure is irrelevant ("FALSE"). The
point of adding ASSERT ahead of CpuDeadLoop() is that the way ASSERT
hangs execution is customizable, via "PcdDebugPropertyMask", unlike
CpuDeadLoop(). In many cases, ASSERT() uses CpuDeadLoop() itself, so the
effect is the same -- the explicit CpuDeadLoop is not reached. In other
configs, ASSERT() can raise a debug exception (CpuBreakpoint()).

I absolutely do not *expect* Tom to change this, it was just a slight remark (as many places have this anyway). I'll still try to explain why I made that remark, but for whom it is of no interest, I do not expect it to be read. I'm fine with the patch as-is myself. Thank you a lot, Tom!



I know it, unfortunately, is a pattern in EDK II - taking this pattern too far is what caused the 8-revision patch regarding untrusted inputs we submitted previously. :)

There are many concerns about unconventional ASSERTs, though I must admit none but one (and that one barely) really apply here, which is why I have trouble explaining why I believe it should be changed. Here are some reasons outside the context of this patch:

1) Consistency between DEBUG and RELEASE builds: I think one can justify to have a breakpoint on a condition that may realistically occur. But a deadloop can give a wrong impression about how production code works. E.g. it also is a common pattern in EDK II to ASSERT on memory allocation failure but *not* have a proper check after, so DEBUG builds will nicely error or deadloop, while RELEASE goes ahead and causes a CPU exception or memory corruption depending on the context. Thus, real-world error handling cannot really be tested. This does not apply because there *is* a RELEASE deadloop.

2) Static analysis: Some static analysers use ASSERT information for their own analysis, and try to give hints about unsafe or unreachable code based on own annotations. This kind of applies, but only when substituting EDK II ASSERT with properly recognisable ASSERTs (e.g. __builtin_unreachable).

2) Dynamic analysis: ASSERTs can be useful when fuzzing for example. Enabled Sanitizers will only catch unsafe behaviour, but maybe you have some extra code in place to sanity-check the results further. An ASSERT yields an error dump (usually followed by the worker dying). However, as allocation failures are perfectly expected, this can cause a dramatic about of False Positives and testing interruption. This does not apply because deadloop'd code cannot really be fuzz-tested anyway.

ASSERTs really are designed as unbreakable conditions, i.e. 1) preconditions 2) invariants 3) postconditions. No allocator in early kernel-space or lower can really guarantee allocation success, thus it cannot be a postcondition of any such function. And while it might make debugging look a bit easier (but you will see from the backtrace anyway where you halted), it messes with all tools that assume proper usage.

Also, I just realised, you can of course see it from the address value when debugging, but you cannot see it from the ASSERT or DEBUG message *which* of the two logical error conditions failed (i.e. broken allocator or OOM). Changing the ASSERT would fix that. :)

Best regards,
Marvin

The required part of the pattern is CpuDeadLoop(); the DEBUG message
makes it more debugging-friendly, and the ASSERT(), with the tweakable
"hang method", makes it even more debugging-friendly.
Thanks
Laszlo

Add the SMBIOS type 32 table (System Boot Information) that includes
information about the System Boot Status.

Signed-off-by: Pranav Madhu <pranav.madhu@...>
---
Platform/ARM/SgiPkg/Drivers/SmbiosPlatformDxe/SmbiosPlatformDxe.inf =
| 1 +
Platform/ARM/SgiPkg/Drivers/SmbiosPlatformDxe/SmbiosPlatformDxe.h =
| 6 ++
Platform/ARM/SgiPkg/Drivers/SmbiosPlatformDxe/SmbiosPlatformDxe.c =
| 1 +
Platform/ARM/SgiPkg/Drivers/SmbiosPlatformDxe/Type32SystemBootInformatio=
n.c | 84 ++++++++++++++++++++
4 files changed, 92 insertions(+)

diff --git a/Platform/ARM/SgiPkg/Drivers/SmbiosPlatformDxe/SmbiosPlatform=
Dxe.inf b/Platform/ARM/SgiPkg/Drivers/SmbiosPlatformDxe/SmbiosPlatformDxe=
.inf
index f81494114188..4258eb9deadb 100644
--- a/Platform/ARM/SgiPkg/Drivers/SmbiosPlatformDxe/SmbiosPlatformDxe.inf
+++ b/Platform/ARM/SgiPkg/Drivers/SmbiosPlatformDxe/SmbiosPlatformDxe.inf
@@ -23,6 +23,7 @@
Type16PhysicalMemoryArray.c
Type17MemoryDevice.c
Type19MemoryArrayMappedAddress.c
+ Type32SystemBootInformation.c
=20
[Packages]
ArmPkg/ArmPkg.dec
diff --git a/Platform/ARM/SgiPkg/Drivers/SmbiosPlatformDxe/SmbiosPlatform=
Dxe.h b/Platform/ARM/SgiPkg/Drivers/SmbiosPlatformDxe/SmbiosPlatformDxe.h
index c6dd72cb6b99..0bbda4b4b45d 100644
--- a/Platform/ARM/SgiPkg/Drivers/SmbiosPlatformDxe/SmbiosPlatformDxe.h
+++ b/Platform/ARM/SgiPkg/Drivers/SmbiosPlatformDxe/SmbiosPlatformDxe.h
@@ -59,6 +59,12 @@ InstallMemoryArrayMappedAddress (
IN EFI_SMBIOS_PROTOCOL *Smbios
);
=20
+EFI_STATUS
+EFIAPI
+InstallSystemBootInformation (
+ IN EFI_SMBIOS_PROTOCOL *Smbios
+ );
+
enum SMBIOS_REFRENCE_HANDLES {
SMBIOS_HANDLE_ENCLOSURE =3D 0x1000,
SMBIOS_HANDLE_CLUSTER1,
diff --git a/Platform/ARM/SgiPkg/Drivers/SmbiosPlatformDxe/SmbiosPlatform=
Dxe.c b/Platform/ARM/SgiPkg/Drivers/SmbiosPlatformDxe/SmbiosPlatformDxe.c
index d5d1e6393184..77b22678f62a 100644
--- a/Platform/ARM/SgiPkg/Drivers/SmbiosPlatformDxe/SmbiosPlatformDxe.c
+++ b/Platform/ARM/SgiPkg/Drivers/SmbiosPlatformDxe/SmbiosPlatformDxe.c
@@ -34,6 +34,7 @@ ARM_RD_SMBIOS_TABLE_INSTALL_FPTR mSmbiosTableList[] =3D=
{
&InstallPhysicalMemoryArray,
&InstallMemoryDevice,
&InstallMemoryArrayMappedAddress,
+ &InstallSystemBootInformation,
};
=20
/**
diff --git a/Platform/ARM/SgiPkg/Drivers/SmbiosPlatformDxe/Type32SystemBo=
otInformation.c b/Platform/ARM/SgiPkg/Drivers/SmbiosPlatformDxe/Type32Sys=
temBootInformation.c
new file mode 100644
index 000000000000..1d3eaab810eb
--- /dev/null
+++ b/Platform/ARM/SgiPkg/Drivers/SmbiosPlatformDxe/Type32SystemBootInfor=
mation.c
@@ -0,0 +1,84 @@
+/** @file
+ SMBIOS Type 32 (System Boot Information) table for ARM RD platforms.
+
+ This file installs SMBIOS Type 32 (System Boot Information) table for =
Arm's
+ Reference Design platforms. It includes information about the System B=
oot
+ Status.
+
+ Copyright (c) 2021, ARM Limited. All rights reserved.
+ SPDX-License-Identifier: BSD-2-Clause-Patent
+
+ @par Specification Reference:
+ - SMBIOS Reference Specification 3.4.0, Chapter 7.33
+**/
+
+#include <Library/BaseMemoryLib.h>
+#include <Library/DebugLib.h>
+#include <Protocol/Smbios.h>
+
+#include "SmbiosPlatformDxe.h"
+
+#define TYPE32_STRINGS \
+ "\0" /* Null string */
+
+/* SMBIOS Type32 structure */
+#pragma pack(1)
+struct ArmRdSmbiosType32 {
+ SMBIOS_TABLE_TYPE32 Base;
+ UINT8 Strings[sizeof (TYPE32_STRINGS)];
+};
+#pragma pack()
+
+/* System Boot Information */
+static struct ArmRdSmbiosType32 mArmRdSmbiosType32 =3D {
+ {
+ {
+ // SMBIOS header
+ EFI_SMBIOS_TYPE_SYSTEM_BOOT_INFORMATION, // Type 32
+ sizeof (SMBIOS_TABLE_TYPE32), // Length
+ SMBIOS_HANDLE_PI_RESERVED
+ },
+ {0}, // Reserved field
+ BootInformationStatusNoError // Boot status
+ },
+ // Text strings (unformatted area)
+ TYPE32_STRINGS
+};
+
+/**
+ Install SMBIOS system boot information
+
+ Install the SMBIOS system boot information (type 32) table for RD plat=
forms.
+
+ @param[in] Smbios SMBIOS protocol.
+
+ @retval EFI_SUCCESS Record was added.
+ @retval EFI_OUT_OF_RESOURCES Record was not added.
+ @retval EFI_ALREADY_STARTED The SmbiosHandle passed in is already in=
use.
+**/
+EFI_STATUS
+InstallSystemBootInformation (
+ IN EFI_SMBIOS_PROTOCOL *Smbios
+ )
+{
+ EFI_STATUS Status;
+ EFI_SMBIOS_HANDLE SmbiosHandle;
+
+ SmbiosHandle =3D ((EFI_SMBIOS_TABLE_HEADER *)&mArmRdSmbiosType32)->Han=
dle;
+
+ /* Install type 32 table */
+ Status =3D Smbios->Add (
+ Smbios,
+ NULL,
+ &SmbiosHandle,
+ (EFI_SMBIOS_TABLE_HEADER *)&mArmRdSmbiosType32
+ );
+ if (Status !=3D EFI_SUCCESS) {
+ DEBUG ((
+ DEBUG_ERROR,
+ "SMBIOS: Failed to install Type32 SMBIOS table.\n"
+ ));
+ }
+
+ return Status;
+}
--=20
2.17.1