Date   

[PATCH v1 0/5] Dot graph generator for PPTT

Joey Gouly
 

This series adds functionality to print a dot graph of a PPTT table.
This helps with understanding and debugging PPTT tables.
The dot graph generator functionality is generic and could be used by
other tables that would benefit from graph output.

Bugzilla: 3378 (https://bugzilla.tianocore.org/show_bug.cgi?id=3378)

The changes can be seen at https://github.com/jgouly/edk2/tree/1484_pptt_dot_graph_v1

Marc Moisson-Franckhauser (5):
ShellPkg: Replace 'Trace' parameter with 'ParseFlags'
ShellPkg: add a helper function for getting a new file name
ShellPkg: add a Graph option to the Parser Flags
ShellPkg: add dot file generator functions
ShellPkg: add PPTT dot file genration

.../UefiShellAcpiViewCommandLib.inf | 4 +-
.../UefiShellAcpiViewCommandLib/AcpiParser.h | 79 +++--
.../AcpiTableParser.h | 6 +-
.../UefiShellAcpiViewCommandLib/AcpiView.h | 25 +-
.../AcpiViewConfig.h | 3 +-
.../DotGenerator.h | 97 ++++++
.../AcpiTableParser.c | 20 +-
.../UefiShellAcpiViewCommandLib/AcpiView.c | 117 +++++--
.../DotGenerator.c | 276 +++++++++++++++++
.../Parsers/Bgrt/BgrtParser.c | 10 +-
.../Parsers/Dbg2/Dbg2Parser.c | 8 +-
.../Parsers/Dsdt/DsdtParser.c | 8 +-
.../Parsers/Facs/FacsParser.c | 10 +-
.../Parsers/Fadt/FadtParser.c | 27 +-
.../Parsers/Gtdt/GtdtParser.c | 8 +-
.../Parsers/Iort/IortParser.c | 8 +-
.../Parsers/Madt/MadtParser.c | 8 +-
.../Parsers/Mcfg/McfgParser.c | 8 +-
.../Parsers/Pptt/PpttParser.c | 285 +++++++++++++++---
.../Parsers/Rsdp/RsdpParser.c | 10 +-
.../Parsers/Slit/SlitParser.c | 8 +-
.../Parsers/Spcr/SpcrParser.c | 8 +-
.../Parsers/Srat/SratParser.c | 8 +-
.../Parsers/Ssdt/SsdtParser.c | 8 +-
.../Parsers/Xsdt/XsdtParser.c | 10 +-
.../UefiShellAcpiViewCommandLib.c | 24 +-
.../UefiShellAcpiViewCommandLib.uni | 9 +-
27 files changed, 903 insertions(+), 189 deletions(-)
create mode 100644 ShellPkg/Library/UefiShellAcpiViewCommandLib/DotGenerator.h
create mode 100644 ShellPkg/Library/UefiShellAcpiViewCommandLib/DotGenerator.c

--
Guid("CE165669-3EF3-493F-B85D-6190EE5B9759")


[PATCH v1 5/5] ShellPkg: add PPTT dot file genration

Joey Gouly
 

From: Marc Moisson-Franckhauser <marc.moisson-franckhauser@arm.com>

Bugzilla: 3378 (https://bugzilla.tianocore.org/show_bug.cgi?id=3378)

This generates a dot file from the PPTT table that can be used to
visualise the topology of the CPUs and Caches.

Signed-off-by: Marc Moisson-Franckhauser <marc.moisson-franckhauser@arm.com>
Signed-off-by: Joey Gouly <joey.gouly@arm.com>
---
ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Pptt/PpttParser.c | 240 ++++++++++++++++++--
1 file changed, 218 insertions(+), 22 deletions(-)

diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Pptt/PpttParser.c b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Pptt/PpttParser.c
index 538b6a69350d75ccbf36b86fff115255e77437c7..b52bd532b846b9cec0ca315b043beff95df40bd5 100644
--- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Pptt/PpttParser.c
+++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Pptt/PpttParser.c
@@ -15,11 +15,23 @@
#include "AcpiView.h"
#include "AcpiViewConfig.h"
#include "PpttParser.h"
+#include "DotGenerator.h"

// Local variables
STATIC CONST UINT8* ProcessorTopologyStructureType;
STATIC CONST UINT8* ProcessorTopologyStructureLength;
+
STATIC CONST UINT32* NumberOfPrivateResources;
+STATIC CONST UINT32* ProcessorHierarchyParent;
+STATIC CONST EFI_ACPI_6_3_PPTT_STRUCTURE_PROCESSOR_FLAGS* ProcStructFlags;
+STATIC CONST UINT32* NextLevelOfCache;
+STATIC CONST EFI_ACPI_6_3_PPTT_STRUCTURE_CACHE_ATTRIBUTES* CacheAttributes;
+STATIC CONST UINT32* CacheSize;
+
+STATIC CONST UINT8* PpttStartPointer;
+
+STATIC SHELL_FILE_HANDLE mDotFileHandle;
+
STATIC ACPI_DESCRIPTION_HEADER_INFO AcpiHdrInfo;

/**
@@ -198,8 +210,9 @@ STATIC CONST ACPI_PARSER ProcessorHierarchyNodeStructureParser[] = {
{L"Length", 1, 1, L"%d", NULL, NULL, NULL, NULL},
{L"Reserved", 2, 2, L"0x%x", NULL, NULL, NULL, NULL},

- {L"Flags", 4, 4, L"0x%x", NULL, NULL, NULL, NULL},
- {L"Parent", 4, 8, L"0x%x", NULL, NULL, NULL, NULL},
+ {L"Flags", 4, 4, L"0x%x", NULL, (VOID**)&ProcStructFlags, NULL, NULL},
+ {L"Parent", 4, 8, L"0x%x", NULL,
+ (VOID**)&ProcessorHierarchyParent, NULL, NULL},
{L"ACPI Processor ID", 4, 12, L"0x%x", NULL, NULL, NULL, NULL},
{L"Number of private resources", 4, 16, L"%d", NULL,
(VOID**)&NumberOfPrivateResources, NULL, NULL}
@@ -214,11 +227,13 @@ STATIC CONST ACPI_PARSER CacheTypeStructureParser[] = {
{L"Reserved", 2, 2, L"0x%x", NULL, NULL, NULL, NULL},

{L"Flags", 4, 4, L"0x%x", NULL, NULL, NULL, NULL},
- {L"Next Level of Cache", 4, 8, L"0x%x", NULL, NULL, NULL, NULL},
- {L"Size", 4, 12, L"0x%x", NULL, NULL, NULL, NULL},
+ {L"Next Level of Cache", 4, 8, L"0x%x", NULL,
+ (VOID**)&NextLevelOfCache, NULL, NULL},
+ {L"Size", 4, 12, L"0x%x", NULL, (VOID**)&CacheSize, NULL, NULL},
{L"Number of sets", 4, 16, L"%d", NULL, NULL, ValidateCacheNumberOfSets, NULL},
{L"Associativity", 1, 20, L"%d", NULL, NULL, ValidateCacheAssociativity, NULL},
- {L"Attributes", 1, 21, L"0x%x", NULL, NULL, ValidateCacheAttributes, NULL},
+ {L"Attributes", 1, 21, L"0x%x", NULL, (VOID**)&CacheAttributes,
+ ValidateCacheAttributes, NULL},
{L"Line size", 2, 22, L"%d", NULL, NULL, ValidateCacheLineSize, NULL}
};

@@ -257,6 +272,7 @@ DumpProcessorHierarchyNodeStructure (
UINT32 Offset;
UINT32 Index;
CHAR16 Buffer[OUTPUT_FIELD_COLUMN_WIDTH];
+ CONST UINT8* TypePtr;

Offset = ParseAcpi (
IS_TRACE_FLAG_SET (ParseFlags),
@@ -291,26 +307,67 @@ DumpProcessorHierarchyNodeStructure (
return;
}

- Index = 0;
+ if (IS_GRAPH_FLAG_SET (ParseFlags)) {
+ if (ProcStructFlags->ProcessorIsAThread) {
+ UnicodeSPrint(Buffer, sizeof (Buffer), L"Thread");
+ } else if (ProcStructFlags->NodeIsALeaf) {
+ UnicodeSPrint(Buffer, sizeof (Buffer), L"Core");
+ } else if (ProcStructFlags->PhysicalPackage) {
+ UnicodeSPrint(Buffer, sizeof (Buffer), L"Physical\\nPackage");
+ } else {
+ UnicodeSPrint(Buffer, sizeof (Buffer), L"Cluster");
+ }
+
+ DotAddNode (
+ mDotFileHandle,
+ (UINT32)(Ptr - PpttStartPointer),
+ DOT_BOX_SQUARE | DOT_COLOR_BLUE | DOT_BOX_ADD_ID_TO_LABEL,
+ Buffer
+ );
+
+ // Add link to parent node.
+ if (*ProcessorHierarchyParent != 0) {
+ DotAddLink (
+ mDotFileHandle,
+ (UINT32)(Ptr - PpttStartPointer),
+ *ProcessorHierarchyParent,
+ 0x0
+ );
+ }
+ }

// Parse the specified number of private resource references or the Processor
// Hierarchy Node length. Whichever is minimum.
- while (Index < *NumberOfPrivateResources) {
- UnicodeSPrint (
- Buffer,
- sizeof (Buffer),
- L"Private resources [%d]",
- Index
- );
+ for (Index = 0; Index < *NumberOfPrivateResources; Index++) {
+ if (IS_TRACE_FLAG_SET (ParseFlags)) {
+ UnicodeSPrint (
+ Buffer,
+ sizeof (Buffer),
+ L"Private resources [%d]",
+ Index
+ );

- PrintFieldName (4, Buffer);
- Print (
- L"0x%x\n",
- *((UINT32*)(Ptr + Offset))
- );
+ PrintFieldName (4, Buffer);
+ Print (
+ L"0x%x\n",
+ *((UINT32*)(Ptr + Offset))
+ );
+ }
+
+ if (IS_GRAPH_FLAG_SET (ParseFlags)) {
+ TypePtr = PpttStartPointer + *((UINT32*)(Ptr + Offset));
+ if (*TypePtr == EFI_ACPI_6_2_PPTT_TYPE_ID) {
+ continue;
+ }
+ DotAddLink (
+ mDotFileHandle,
+ *((UINT32*)(Ptr + Offset)),
+ (UINT32)(Ptr - PpttStartPointer),
+ DOT_ARROW_RANK_REVERSE
+ );
+ }

Offset += sizeof (UINT32);
- Index++;
}
}

@@ -329,6 +386,8 @@ DumpCacheTypeStructure (
IN UINT8 Length
)
{
+ CHAR16 LabelBuffer[64];
+
ParseAcpi (
IS_TRACE_FLAG_SET (ParseFlags),
2,
@@ -337,6 +396,88 @@ DumpCacheTypeStructure (
Length,
PARSER_PARAMS (CacheTypeStructureParser)
);
+
+ if (IS_GRAPH_FLAG_SET (ParseFlags)) {
+ // Create cache node
+
+ // Start node label with type of cache
+ switch (CacheAttributes->CacheType) {
+ case EFI_ACPI_6_3_CACHE_ATTRIBUTES_CACHE_TYPE_DATA:
+ UnicodeSPrint (
+ LabelBuffer,
+ sizeof (LabelBuffer),
+ L"D-Cache\\n"
+ );
+ break;
+ case EFI_ACPI_6_3_CACHE_ATTRIBUTES_CACHE_TYPE_INSTRUCTION:
+ UnicodeSPrint (
+ LabelBuffer,
+ sizeof (LabelBuffer),
+ L"I-Cache\\n"
+ );
+ break;
+ default:
+ UnicodeSPrint (
+ LabelBuffer,
+ sizeof (LabelBuffer),
+ L"Unified Cache\\n"
+ );
+ }
+
+ // Add size of cache to node label
+ if (((*CacheSize) & 0xfff00000) != 0) {
+ UnicodeSPrint (
+ LabelBuffer,
+ sizeof (LabelBuffer),
+ L"%s%dMiB",
+ LabelBuffer,
+ *CacheSize >> 20
+ );
+ }
+ if ((*CacheSize & 0xffc00) != 0) {
+ UnicodeSPrint (
+ LabelBuffer,
+ sizeof (LabelBuffer),
+ L"%s%dkiB",
+ LabelBuffer,
+ (*CacheSize >> 10) & 0x3ff
+ );
+ }
+ if ((*CacheSize & 0x3ff) != 0) {
+ UnicodeSPrint (
+ LabelBuffer,
+ sizeof (LabelBuffer),
+ L"%s%dB",
+ LabelBuffer,
+ *CacheSize & 0x3ff
+ );
+ }
+ if (*CacheSize == 0) {
+ UnicodeSPrint (
+ LabelBuffer,
+ sizeof (LabelBuffer),
+ L"%s0B",
+ LabelBuffer
+ );
+ }
+
+ //Add node to dot file
+ DotAddNode (
+ mDotFileHandle,
+ (UINT32)(Ptr - PpttStartPointer),
+ DOT_BOX_SQUARE | DOT_COLOR_YELLOW | DOT_BOX_ADD_ID_TO_LABEL,
+ LabelBuffer
+ );
+
+ if (*NextLevelOfCache != 0) {
+ DotAddLink (
+ mDotFileHandle,
+ *NextLevelOfCache,
+ (UINT32)(Ptr - PpttStartPointer),
+ DOT_ARROW_RANK_REVERSE | DOT_COLOR_GRAY
+ );
+ }
+ }
}

/**
@@ -390,13 +531,46 @@ ParseAcpiPptt (
IN UINT8 AcpiTableRevision
)
{
- UINT32 Offset;
- UINT8* ProcessorTopologyStructurePtr;
+ EFI_STATUS Status;
+ UINT32 Offset;
+ UINT8* ProcessorTopologyStructurePtr;
+ CHAR16 Buffer[128];
+ CHAR16 FileNameBuffer[MAX_FILE_NAME_LEN];

- if (!IS_TRACE_FLAG_SET (ParseFlags)) {
+ if (!IS_TRACE_FLAG_SET (ParseFlags) &&
+ !IS_GRAPH_FLAG_SET (ParseFlags)) {
return;
}

+ if (IS_GRAPH_FLAG_SET (ParseFlags)) {
+ Status = GetNewFileName (
+ L"PPTT",
+ L"dot",
+ FileNameBuffer,
+ sizeof (FileNameBuffer)
+ );
+
+ if (EFI_ERROR (Status)) {
+ Print (
+ L"Error: Could not open dot file for PPTT table:\n"
+ L"Could not get a file name."
+ );
+ // Abandonning creation of dot graph by unsetting the flag.
+ // We continue parsing in case trace is set.
+ ParseFlags &= ~PARSE_FLAGS_GRAPH;
+ } else {
+ mDotFileHandle = DotOpenNewFile (FileNameBuffer);
+ if (mDotFileHandle == NULL) {
+ Print (L"ERROR: Could not open dot file for PPTT table.\n");
+ // Abandonning creation of dot graph by unsetting the flag.
+ // We continue parsing in case trace is set.
+ ParseFlags &= ~PARSE_FLAGS_GRAPH;
+ }
+ }
+ }
+
+ PpttStartPointer = Ptr;
+
Offset = ParseAcpi (
IS_TRACE_FLAG_SET (ParseFlags),
0,
@@ -406,6 +580,24 @@ ParseAcpiPptt (
PARSER_PARAMS (PpttParser)
);

+ if (*(AcpiHdrInfo.Revision) < 2 &&
+ IS_GRAPH_FLAG_SET (ParseFlags)) {
+ Print (L"\nWARNING: Dot output may not be consistent for PPTT revisions < 2\n");
+ UnicodeSPrint (
+ Buffer,
+ sizeof (Buffer),
+ L"WARNING: PPTT table revision is %u.\\n" \
+ L"Revisions lower than 2 might lead to incorrect labelling",
+ *(AcpiHdrInfo.Revision)
+ );
+ DotAddNode (
+ mDotFileHandle,
+ 0,
+ DOT_COLOR_RED | DOT_BOX_SQUARE,
+ Buffer
+ );
+ }
+
ProcessorTopologyStructurePtr = Ptr + Offset;

while (Offset < AcpiTableLength) {
@@ -486,4 +678,8 @@ ParseAcpiPptt (
ProcessorTopologyStructurePtr += *ProcessorTopologyStructureLength;
Offset += *ProcessorTopologyStructureLength;
} // while
+
+ if (IS_GRAPH_FLAG_SET (ParseFlags)) {
+ DotCloseFile (mDotFileHandle);
+ }
}
--
Guid("CE165669-3EF3-493F-B85D-6190EE5B9759")


TianoCore Community Meeting - APAC/NAMO - Thu, 05/06/2021 7:30pm-8:30pm #cal-reminder

devel@edk2.groups.io Calendar <devel@...>
 

Reminder: TianoCore Community Meeting - APAC/NAMO

When: Thursday, 6 May 2021, 7:30pm to 8:30pm, (GMT-07:00) America/Los Angeles

Where:https://teams.microsoft.com/l/meetup-join/19%3ameeting_Nzk4NmUzOTItNzIzNC00MTVkLTgxZDUtMDAwOTIxOGI2OWJk%40thread.v2/0?context=%7b%22Tid%22%3a%2246c98d88-e344-4ed4-8496-4ed7712e255d%22%2c%22Oid%22%3a%220d960867-aa36-403f-8446-f96a6cce5fa4%22%7d

View Event

Organizer: Puja Pandya puja.pandya@...

Description: Agenda:

  • Events update
  • Stewards download
  • Stable tag
  • GSoC update
  • Opens

________________________________________________________________________________

Microsoft Teams meeting

Join on your computer or mobile app

Click here to join the meeting

Join with a video conferencing device

teams@...

Video Conference ID: 119 712 073 2

Alternate VTC dialing instructions

Learn More | Meeting options

________________________________________________________________________________


TianoCore Community Meeting - APAC/NAMO - Thu, 05/06/2021 7:30pm-8:30pm #cal-reminder

devel@edk2.groups.io Calendar <devel@...>
 

Reminder: TianoCore Community Meeting - APAC/NAMO

When: Thursday, 6 May 2021, 7:30pm to 8:30pm, (GMT-07:00) America/Los Angeles

Where:https://teams.microsoft.com/l/meetup-join/19%3ameeting_Nzk4NmUzOTItNzIzNC00MTVkLTgxZDUtMDAwOTIxOGI2OWJk%40thread.v2/0?context=%7b%22Tid%22%3a%2246c98d88-e344-4ed4-8496-4ed7712e255d%22%2c%22Oid%22%3a%220d960867-aa36-403f-8446-f96a6cce5fa4%22%7d

View Event

Organizer: Puja Pandya puja.pandya@...

Description: Agenda:

  • Events update
  • Stewards download
  • Stable tag
  • GSoC update
  • Opens

________________________________________________________________________________

Microsoft Teams meeting

Join on your computer or mobile app

Click here to join the meeting

Join with a video conferencing device

teams@...

Video Conference ID: 119 712 073 2

Alternate VTC dialing instructions

Learn More | Meeting options

________________________________________________________________________________


Re: [PATCH 2/3] MdePkg: Refactor BaseRngLib to support AARCH64 in addition to X86

Rebecca Cran <rebecca@...>
 

On 5/4/21 3:09 PM, Sami Mujawar wrote:

+EFIAPI
+BaseRngLibConstructor (
+ VOID
+ )
+{
+ UINT64 Isar0;
+ //
+ // Determine RNDR support by examining bits 63:60 of the ISAR0 register returned by
+ // MSR. A non-zero value indicates that the processor supports the RNDR instruction.
+ //
+ Isar0 = ArmReadIdIsar0 ();
+ ASSERT ((Isar0 & RNDR_MASK) != 0);
+ (void)Isar0;
[SAMI] ASSERTs will vanish in the release builds. So, I think this needs to be an if condition. If RNDR is not supported RETURN_UNSUPPORTED should be returned.
However, it appears thatthe auto generated function ProcessLibraryConstructorList() disregards the error code returned by the constructor (see Build\...\AutoGen.c files). So it looks like the loading operation would continue in release builds despite of an error.
I am not aware if this is the desired behavior or why the status code returned by the constructor is disregarded.
However, this would be a probem in the current case as subsequent calls to generate random numbers will result in an undefined instruction exception.
To prevent this, I think the above check should be done in either
   - ArmRndr()/ArmRndrrs()
  or
   - preferably in ArchGetRandomNumberXX(), which should return an error code EFI_UNSUPPORTED, EFI_NOT_READY or EFI_SUCCESS. However, the impact on IA32/x64 code needs to be evaluated.
I've updated both the x86 and aarch64 code to add checks that the RDRAND and RNDR instructions are supported before trying to use them in GetRandomNumber[16,32,64,128].
However, it causes the X64 CryptoPkg host-based unit tests to fail because UnitTestHostBaseLibAsmCpuid just sets all the OUT parameters to 0, which causes RngDxe to think RDRAND isn't supported.

Should the unit tests be using BaseRngLibTimerLib instead of BaseRngLib? Or should I leave the x86 code as it was, with the ASSERT in the constructor and no further checks at the time of use?

--
Rebecca Cran


Re: [PATCH RFC v2 07/28] OvmfPkg: Use MemEncryptSevClearMmioPageEncMask() to clear EncMask from Mmio

Brijesh Singh
 

On 5/6/21 5:50 AM, Laszlo Ersek wrote:
On 04/30/21 13:51, Brijesh Singh wrote:
BZ: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D3275&;data=04%7C01%7Cbrijesh.singh%40amd.com%7Cce4b852d83a14265d48f08d9107cd16f%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637558950553380286%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=CgY57XQGI9QBvj7vipJoJLVZLiEvpfySW17TLLx%2BZm8%3D&amp;reserved=0

Use the MemEncryptSevClearMmioPageEncMask() to clear memory encryption mask
for the Mmio address range from the current page table context.

Cc: James Bottomley <jejb@linux.ibm.com>
Cc: Min Xu <min.m.xu@intel.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Erdem Aktas <erdemaktas@google.com>
Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
---
OvmfPkg/AmdSevDxe/AmdSevDxe.c | 10 ++++------
OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceDxe.c | 5 ++---
OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlashSmm.c | 5 ++---
3 files changed, 8 insertions(+), 12 deletions(-)

diff --git a/OvmfPkg/AmdSevDxe/AmdSevDxe.c b/OvmfPkg/AmdSevDxe/AmdSevDxe.c
index 689bfb376d..80831b81fa 100644
--- a/OvmfPkg/AmdSevDxe/AmdSevDxe.c
+++ b/OvmfPkg/AmdSevDxe/AmdSevDxe.c
@@ -53,11 +53,10 @@ AmdSevDxeEntryPoint (
Desc = &AllDescMap[Index];
if (Desc->GcdMemoryType == EfiGcdMemoryTypeMemoryMappedIo ||
Desc->GcdMemoryType == EfiGcdMemoryTypeNonExistent) {
- Status = MemEncryptSevClearPageEncMask (
+ Status = MemEncryptSevClearMmioPageEncMask (
0,
Desc->BaseAddress,
- EFI_SIZE_TO_PAGES (Desc->Length),
- FALSE
+ EFI_SIZE_TO_PAGES (Desc->Length)
);
ASSERT_EFI_ERROR (Status);
}
@@ -73,11 +72,10 @@ AmdSevDxeEntryPoint (
// the range.
//
if (PcdGet16 (PcdOvmfHostBridgePciDevId) == INTEL_Q35_MCH_DEVICE_ID) {
- Status = MemEncryptSevClearPageEncMask (
+ Status = MemEncryptSevClearMmioPageEncMask (
0,
FixedPcdGet64 (PcdPciExpressBaseAddress),
- EFI_SIZE_TO_PAGES (SIZE_256MB),
- FALSE
+ EFI_SIZE_TO_PAGES (SIZE_256MB)
);

ASSERT_EFI_ERROR (Status);
diff --git a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceDxe.c b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceDxe.c
index 1f285e0083..ab40087a84 100644
--- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceDxe.c
+++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceDxe.c
@@ -205,11 +205,10 @@ MarkIoMemoryRangeForRuntimeAccess (
// memory range.
//
if (MemEncryptSevIsEnabled ()) {
- Status = MemEncryptSevClearPageEncMask (
+ Status = MemEncryptSevClearMmioPageEncMask (
0,
BaseAddress,
- EFI_SIZE_TO_PAGES (Length),
- FALSE
+ EFI_SIZE_TO_PAGES (Length)
);
ASSERT_EFI_ERROR (Status);
}
diff --git a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlashSmm.c b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlashSmm.c
index 7eb80bfeff..ea75b489c7 100644
--- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlashSmm.c
+++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlashSmm.c
@@ -38,11 +38,10 @@ QemuFlashBeforeProbe (
// C-bit on flash ranges from SMM page table.
//

- Status = MemEncryptSevClearPageEncMask (
+ Status = MemEncryptSevClearMmioPageEncMask (
0,
BaseAddress,
- EFI_SIZE_TO_PAGES (FdBlockSize * FdBlockCount),
- FALSE
+ EFI_SIZE_TO_PAGES (FdBlockSize * FdBlockCount)
);
ASSERT_EFI_ERROR (Status);
}
The contents of this patch are sound, but they are incomplete, and
incorrectly structured too.

(1) Please provide a separate patch for each modified module.
Noted.


(2) You missed the MemEncryptSevClearPageEncMask() call in
TpmMmioSevDecryptPeimEntryPoint()
[OvmfPkg/Tcg/TpmMmioSevDecryptPei/TpmMmioSevDecryptPeim.c] -- probably
because you worked on this series in parallel with Tom working on the
SEV-ES TPM fixes.

I guess Tom's patches were not accepted when I rebased the the SNP
patches. Will pick those changes in next rev.



In the end, this patch should be split into three patches (because the
change is needed for three modules).

Thanks
Laszlo


Re: [PATCH RFC v2 06/28] OvmfPkg/BaseMemEncryptSevLib: Introduce MemEncryptSevClearMmioPageEncMask()

Brijesh Singh
 

On 5/6/21 5:39 AM, Laszlo Ersek via groups.io wrote:
On 04/30/21 13:51, Brijesh Singh wrote:
BZ: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D3275&;data=04%7C01%7Cbrijesh.singh%40amd.com%7C2719bcebf85d4e5228e508d9107b449d%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637558943897655383%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=YrgkAA3Kp6v2eJuovVho1I3kR%2FXAA5D%2BE2vaUKyFO68%3D&amp;reserved=0

The MemEncryptSevClearMmioPageEncMask() helper can be used for clearing
the memory encryption mask for the Mmio region from the current page
table context.
The commit message, and five comments in the patch, say "current page
table context". However, Cr3BaseAddress is taken explicitly.

I realize the files being modified in this patch already make similarly
incorrect statements, but I'd like if we avoided adding more.

(1) Please just drop "current page table context" from the mentioned six
locations. (Explaining that Cr3BaseAddress=0 means "current CR3" is of
course valid.)

Thanks Laszlo, I will go through the feedback on this patch and address
them in next rev.

-Brijesh

Cc: James Bottomley <jejb@linux.ibm.com>
Cc: Min Xu <min.m.xu@intel.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Erdem Aktas <erdemaktas@google.com>
Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
---
OvmfPkg/Include/Library/MemEncryptSevLib.h | 25 +++++++++++
OvmfPkg/Library/BaseMemEncryptSevLib/Ia32/MemEncryptSevLib.c | 31 ++++++++++++++
OvmfPkg/Library/BaseMemEncryptSevLib/X64/MemEncryptSevLib.c | 33 +++++++++++++++
OvmfPkg/Library/BaseMemEncryptSevLib/X64/PeiDxeVirtualMemory.c | 44 ++++++++++++++++++--
OvmfPkg/Library/BaseMemEncryptSevLib/X64/VirtualMemory.h | 23 ++++++++++
5 files changed, 153 insertions(+), 3 deletions(-)

diff --git a/OvmfPkg/Include/Library/MemEncryptSevLib.h b/OvmfPkg/Include/Library/MemEncryptSevLib.h
index 99f15a7d12..c19f92afc6 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 from the current page table context.
+
+ @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/Ia32/MemEncryptSevLib.c b/OvmfPkg/Library/BaseMemEncryptSevLib/Ia32/MemEncryptSevLib.c
index 12a5bf495b..4e8a997d42 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 from the current page table context.
+
+ @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 4fea6a6be0..6786573aea 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 from the current page table context.
+
+ @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 d3455e812b..3bcc92f2e9 100644
--- a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/PeiDxeVirtualMemory.c
+++ b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/PeiDxeVirtualMemory.c
@@ -557,6 +557,7 @@ EnableReadOnlyPageWriteProtect (
@param[in] Mode Set or Clear mode
@param[in] CacheFlush Flush the caches before applying the
encryption mask
+ @param[in] IsMmio The address is Mmio address.

@retval RETURN_SUCCESS The attributes were cleared for the
memory region.
(2) The parameter's name in the documentation ("IsMmio") does not match
the actual parameter name ("Mmio").


@@ -572,7 +573,8 @@ SetMemoryEncDec (
IN PHYSICAL_ADDRESS PhysicalAddress,
IN UINTN Length,
IN MAP_RANGE_MODE Mode,
- IN BOOLEAN CacheFlush
+ IN BOOLEAN CacheFlush,
+ IN BOOLEAN Mmio
)
{
PAGE_MAP_AND_DIRECTORY_POINTER *PageMapLevel4Entry;
The "Mmio" parameter is not used for anything in this patch. It's very
confusing.

(3) Please remove the addition of the Mmio parameter from this patch.
Please introduce the Mmio parameter only when it is utilized -- as far
as I can tell from a quick git-blame, that's patch #26
("OvmfPkg/MemEncryptSevLib: Change the page state in the RMP table").

As a result, in this patch, InternalMemEncryptSevClearMmioPageEncMask()
will effectively become a slight simplification of
InternalMemEncryptSevSetMemoryDecrypted() -- rather than forwarding
"Flush" for "CacheFlush", it will pass constant FALSE for "CacheFlush".


(4) Please mention the above fact (= last paragraph above) in the commit
message.


@@ -852,7 +854,8 @@ InternalMemEncryptSevSetMemoryDecrypted (
PhysicalAddress,
Length,
ClearCBit,
- Flush
+ Flush,
+ FALSE
);
}

@@ -888,6 +891,41 @@ InternalMemEncryptSevSetMemoryEncrypted (
PhysicalAddress,
Length,
SetCBit,
- Flush
+ Flush,
+ FALSE
+ );
+}
+
+/**
+ This function clears memory encryption bit for the Mmio region specified by
+ PhysicalAddress and Length from the current page table context.
+
+ @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 Number of pages is zero.
This function does not take a NumPages parameter but a Length parameter.

(5) Please update the RETURN_INVALID_PARAMETER comment accordingly -- in
the header file as well.


+ @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,
+ TRUE
);
}
diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/VirtualMemory.h b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/VirtualMemory.h
index fe2a0b2826..99ee7ea0e8 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 from the current page table context.
+
+ @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 Number of pages is zero.
This function does not take a NumPages parameter but a Length parameter.

(6) Please update the RETURN_INVALID_PARAMETER comment accordingly --
same as in the C file.


+ @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
Please carefully audit the rest of the series for comment blocks. Such
issues render reviews inefficient.

Thanks
Laszlo






Re: [PATCH RFC v2 11/28] OvmfPkg: Reserve Secrets page in MEMFD

James Bottomley
 

On Thu, 2021-05-06 at 13:57 +0300, Dov Murik wrote:

On 05/05/2021 22:33, Laszlo Ersek wrote:
On 05/05/21 15:11, Brijesh Singh wrote:
On 5/5/21 1:42 AM, Dov Murik wrote:
[...]
Would it make sense to always use EfiACPIMemoryNVS for the
injected secret area, even for regular SEV (non-SNP)?
Ideally yes. Maybe James had some reasons for choosing the
EfiBootServicesData. If I had to guess, it was mainly because
there no guest kernel support which consumes the SEV secrets
page.
git-blame fingers commit bff2811c6d99 ("OvmfPkg/AmdSev: assign and
reserve the Sev Secret area", 2020-12-14).

Commit bff2811c6d99 makes it clear that the area in question lives
in MEMFD.

We're populating the area in the PEI phase. We don't want anything
in DXE to overwrite it.

Once the bootloader (and/or perhaps the kernel's EFI stub) fetched
the secret from that particular location, there is no need to
prevent later parts of the OS (the actual kernel) from repurposing
that area. That's why EfiBootServicesData was used.
The first use of the secret area was to hold the guest luks disk
passphrase; this is used in the grub-inside-OVMF (AmdSev package),
and there was no need to keep that page around for the guest kernel.

The reason I'm raising this whole point is that we're working now on
guest-kernel support for reading secrets from that injected page (for
plain SEV). We considered either (a) modifying the secrets page
memory type to reserved here, or (b) add code to the kernel EFI stub
that would copy this page somewhere else for kernel's later use
(which seems more work and not sure what's the advantage).
It mirrors the TPM boot log behaviour: the log is stored in boot time
only memory, so the kernel EFI stub has to copy it out. The reason the
TPM boot log behaves this way is that if the kernel didn't want to
collect it, it still gets freed. I imagine a similar rationale exists
for the boot secrets: if the kernel isn't interested in them for some
reason, we don't want them to persist.

Option (b) seems harder and more fragile, and I'm not sure if there
are any advantages (though I'm definitely not an expert in that
area).
I think forcing the kernel to consume the secret before exit boot
services is still a good idea because

1. If the kernel can't consume the secret it gets freed
2. Not all secrets are consumed by the kernel, so it can pick the ones
it wants out and discard the rest
3. If the kernel is using a secret protection mechanism, that may not
work for the memfd page, so relocation of the secret might be a more
secure mechanism.

James


Re: [PATCH RFC v2 11/28] OvmfPkg: Reserve Secrets page in MEMFD

James Bottomley
 

On Wed, 2021-05-05 at 21:33 +0200, Laszlo Ersek wrote:
On 05/05/21 15:11, Brijesh Singh wrote:
On 5/5/21 1:42 AM, Dov Murik wrote:
[...]
Would it make sense to always use EfiACPIMemoryNVS for the
injected secret area, even for regular SEV (non-SNP)?
Ideally yes. Maybe James had some reasons for choosing the
EfiBootServicesData. If I had to guess, it was mainly because there
no guest kernel support which consumes the SEV secrets page.
git-blame fingers commit bff2811c6d99 ("OvmfPkg/AmdSev: assign and
reserve the Sev Secret area", 2020-12-14).

Commit bff2811c6d99 makes it clear that the area in question lives in
MEMFD.

We're populating the area in the PEI phase. We don't want anything in
DXE to overwrite it.

Once the bootloader (and/or perhaps the kernel's EFI stub) fetched
the secret from that particular location, there is no need to prevent
later parts of the OS (the actual kernel) from repurposing that area.
That's why EfiBootServicesData was used.
That's right: originally the design was not to have the boot secrets
survive boot because they should already be copied into their correct,
and presumably protected, locations by the time exit boot services
comes. The grub code actually shreds the secret in the page once it
consumes it, so the area for a simple disk secret should be empty
anyway.

James


TianoCore Community Meeting - EMEA / NAMO - Thu, 05/06/2021 9:00am-10:00am #cal-reminder

devel@edk2.groups.io Calendar <devel@...>
 

Reminder: TianoCore Community Meeting - EMEA / NAMO

When: Thursday, 6 May 2021, 9:00am to 10:00am, (GMT-07:00) America/Los Angeles

Where:https://teams.microsoft.com/l/meetup-join/19%3ameeting_OTQ4NGExODMtOGRiOC00MmM0LWJhZTMtNzU1NDI2ZTc3MTNh%40thread.v2/0?context=%7b%22Tid%22%3a%2246c98d88-e344-4ed4-8496-4ed7712e255d%22%2c%22Oid%22%3a%220d960867-aa36-403f-8446-f96a6cce5fa4%22%7d

View Event

Organizer: Puja Pandya puja.pandya@...

Description: Agenda:

  • Events update
  • Stewards download
  • Stable tag
  • GSoC update
  • Opens

Microsoft Teams meeting

Join on your computer or mobile app

Click here to join the meeting

Join with a video conferencing device

teams@...

Video Conference ID: 119 993 291 6

Alternate VTC dialing instructions

Learn More | Meeting options


TianoCore Community Meeting - EMEA / NAMO - Thu, 05/06/2021 9:00am-10:00am #cal-reminder

devel@edk2.groups.io Calendar <devel@...>
 

Reminder: TianoCore Community Meeting - EMEA / NAMO

When: Thursday, 6 May 2021, 9:00am to 10:00am, (GMT-07:00) America/Los Angeles

Where:https://teams.microsoft.com/l/meetup-join/19%3ameeting_OTQ4NGExODMtOGRiOC00MmM0LWJhZTMtNzU1NDI2ZTc3MTNh%40thread.v2/0?context=%7b%22Tid%22%3a%2246c98d88-e344-4ed4-8496-4ed7712e255d%22%2c%22Oid%22%3a%220d960867-aa36-403f-8446-f96a6cce5fa4%22%7d

View Event

Organizer: Puja Pandya puja.pandya@...

Description: Agenda:

  • Events update
  • Stewards download
  • Stable tag
  • GSoC update
  • Opens

Microsoft Teams meeting

Join on your computer or mobile app

Click here to join the meeting

Join with a video conferencing device

teams@...

Video Conference ID: 119 993 291 6

Alternate VTC dialing instructions

Learn More | Meeting options


Re: [PATCH RFC v2 11/28] OvmfPkg: Reserve Secrets page in MEMFD

Laszlo Ersek
 

Hi Dov,

On 05/06/21 12:57, Dov Murik wrote:


On 05/05/2021 22:33, Laszlo Ersek wrote:
On 05/05/21 15:11, Brijesh Singh wrote:

On 5/5/21 1:42 AM, Dov Murik wrote:
[+cc: Tobin]

Hi Brijesh,

On 30/04/2021 14:51, Brijesh Singh wrote:
BZ: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D3275&;data=04%7C01%7Cbrijesh.singh%40amd.com%7C93168c94eb6d44ed08e608d90f910426%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637557937779907471%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=nLpmk3G%2BmXcZrzXxCmO3M9EDPiLRnP1IUmPqRQNbBuU%3D&amp;reserved=0

When AMD SEV is enabled in the guest VM, a hypervisor need to insert a
secrets page.

When SEV-SNP is enabled, the secrets page contains the VM platform
communication keys. The guest BIOS and OS can use this key to communicate
with the SEV firmware to get attesation report. See the SEV-SNP firmware
spec for more details for the content of the secrets page.

When SEV and SEV-ES is enabled, the secrets page contains the information
provided by the guest owner after the attestation. See the SEV
LAUNCH_SECRET command for more details.

Cc: James Bottomley <jejb@linux.ibm.com>
Cc: Min Xu <min.m.xu@intel.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Erdem Aktas <erdemaktas@google.com>
Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
---
OvmfPkg/AmdSev/SecretPei/SecretPei.c | 16 +++++++++++++++-
OvmfPkg/AmdSev/SecretPei/SecretPei.inf | 1 +
OvmfPkg/OvmfPkgX64.dsc | 2 ++
OvmfPkg/OvmfPkgX64.fdf | 5 +++++
4 files changed, 23 insertions(+), 1 deletion(-)

diff --git a/OvmfPkg/AmdSev/SecretPei/SecretPei.c b/OvmfPkg/AmdSev/SecretPei/SecretPei.c
index ad491515dd..92836c562c 100644
--- a/OvmfPkg/AmdSev/SecretPei/SecretPei.c
+++ b/OvmfPkg/AmdSev/SecretPei/SecretPei.c
@@ -7,6 +7,7 @@
#include <PiPei.h>
#include <Library/HobLib.h>
#include <Library/PcdLib.h>
+#include <Library/MemEncryptSevLib.h>

EFI_STATUS
EFIAPI
@@ -15,10 +16,23 @@ InitializeSecretPei (
IN CONST EFI_PEI_SERVICES **PeiServices
)
{
+ UINTN Type;
+
+ //
+ // The secret page should be mapped encrypted by the guest OS and must not
+ // be treated as a system RAM. Mark it as ACPI NVS so that guest OS maps it
+ // encrypted.
+ //
+ if (MemEncryptSevSnpIsEnabled ()) {
+ Type = EfiACPIMemoryNVS;
+ } else {
+ Type = EfiBootServicesData;
+ }
+
Would it make sense to always use EfiACPIMemoryNVS for the injected secret area, even for regular SEV (non-SNP)?
Ideally yes. Maybe James had some reasons for choosing the
EfiBootServicesData. If I had to guess, it was mainly because there no
guest kernel support which consumes the SEV secrets page.
git-blame fingers commit bff2811c6d99 ("OvmfPkg/AmdSev: assign and
reserve the Sev Secret area", 2020-12-14).

Commit bff2811c6d99 makes it clear that the area in question lives in MEMFD.

We're populating the area in the PEI phase. We don't want anything in
DXE to overwrite it.

Once the bootloader (and/or perhaps the kernel's EFI stub) fetched the
secret from that particular location, there is no need to prevent later
parts of the OS (the actual kernel) from repurposing that area. That's
why EfiBootServicesData was used.
The first use of the secret area was to hold the guest luks disk passphrase; this is used in the grub-inside-OVMF (AmdSev package), and there was no need to keep that page around for the guest kernel.

The reason I'm raising this whole point is that we're working now on guest-kernel support for reading secrets from that injected page (for plain SEV). We considered either (a) modifying the secrets page memory type to reserved here, or (b) add code to the kernel EFI stub that would copy this page somewhere else for kernel's later use (which seems more work and not sure what's the advantage).

Option (b) seems harder and more fragile, and I'm not sure if there are any advantages (though I'm definitely not an expert in that area).




Since the
memory is not marked ACPI NVS, so it can be used as a system RAM after
the ExitBootServices is called in the kernel.
Yes.

I don't think AcpiNVS would be a good fit. Linux saves and restores
AcpiNVS areas upon S3 suspend/resume. Regardless of whether S3 works, or
will work, in SEV* guests, if we don't want the guest kernel to touch
that area *at all*, Reserved is a better type.
Thanks for this clarification.


Please refer to "Table 7-6 Memory Type Usage after ExitBootServices()"
in the UEFI spec (v2.9).


I am fine with using ACPI NVS for both SEV and SEV-SNP. I was not able
to build and run AmdSev package in my setup, can you submit a prepatch
to change the memory type and verify that it works ?
NB: I've not yet reached this patch in my own review of the series, so
I'm likely missing some context. I do have a thought -- under SEV-SNP,
the secrets page apparently needs different (stronger) protection from
the host as under plain SEV. I don't think that hiding the different
protection requirements behind a single common memory type is helpful.
Not to mention the wasted memory in the plain SEV case -- it's not a lot
of memory, mind you, but the principle matters.
Like I said above, we have plans to have this small amount of memory available also to the guest OS; so maybe that shouldn't be the driving force in the decision here.
What you describe (= runtime guest OS access) seems to justify changing
the memory type of this allocation. However, that update doesn't look
tied to SEV-SNP -- it's basically a "change of use case" (change of
purpose) under plain SEV too. I think that deserves a separate
(stand-alone) patch; maybe even a separate TianoCore BZ ticket.

Thanks
Laszlo




-Dov



So ATM I would like to keep this patch in the SEV-SNP series, and to
preserve the different memory types between SEV and SEV-SNP.

Thanks
Laszlo






-Dov



BuildMemoryAllocationHob (
PcdGet32 (PcdSevLaunchSecretBase),
PcdGet32 (PcdSevLaunchSecretSize),
- EfiBootServicesData
+ Type
);

return EFI_SUCCESS;
diff --git a/OvmfPkg/AmdSev/SecretPei/SecretPei.inf b/OvmfPkg/AmdSev/SecretPei/SecretPei.inf
index 08be156c4b..9265f8adee 100644
--- a/OvmfPkg/AmdSev/SecretPei/SecretPei.inf
+++ b/OvmfPkg/AmdSev/SecretPei/SecretPei.inf
@@ -26,6 +26,7 @@
HobLib
PeimEntryPoint
PcdLib
+ MemEncryptSevLib

[FixedPcd]
gUefiOvmfPkgTokenSpaceGuid.PcdSevLaunchSecretBase
diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
index a7d747f6b4..593c0e69f6 100644
--- a/OvmfPkg/OvmfPkgX64.dsc
+++ b/OvmfPkg/OvmfPkgX64.dsc
@@ -716,6 +716,7 @@
OvmfPkg/SmmAccess/SmmAccessPei.inf
!endif
UefiCpuPkg/CpuMpPei/CpuMpPei.inf
+ OvmfPkg/AmdSev/SecretPei/SecretPei.inf

!if $(TPM_ENABLE) == TRUE
OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf
@@ -965,6 +966,7 @@
OvmfPkg/PlatformDxe/Platform.inf
OvmfPkg/AmdSevDxe/AmdSevDxe.inf
OvmfPkg/IoMmuDxe/IoMmuDxe.inf
+ OvmfPkg/AmdSev/SecretDxe/SecretDxe.inf

!if $(SMM_REQUIRE) == TRUE
OvmfPkg/SmmAccess/SmmAccess2Dxe.inf
diff --git a/OvmfPkg/OvmfPkgX64.fdf b/OvmfPkg/OvmfPkgX64.fdf
index d519f85328..b04175f77c 100644
--- a/OvmfPkg/OvmfPkgX64.fdf
+++ b/OvmfPkg/OvmfPkgX64.fdf
@@ -88,6 +88,9 @@ gUefiCpuPkgTokenSpaceGuid.PcdSevEsWorkAreaBase|gUefiCpuPkgTokenSpaceGuid.PcdSevE
0x00C000|0x001000
gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbBackupBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbBackupSize

+0x00D000|0x001000
+gUefiOvmfPkgTokenSpaceGuid.PcdSevLaunchSecretBase|gUefiOvmfPkgTokenSpaceGuid.PcdSevLaunchSecretSize
+
0x010000|0x010000
gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamSize

@@ -178,6 +181,7 @@ INF OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf
INF SecurityPkg/Tcg/TcgPei/TcgPei.inf
INF SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf
!endif
+INF OvmfPkg/AmdSev/SecretPei/SecretPei.inf

################################################################################

@@ -313,6 +317,7 @@ INF OvmfPkg/LinuxInitrdDynamicShellCommand/LinuxInitrdDynamicShellCommand.inf
INF ShellPkg/Application/Shell/Shell.inf

INF MdeModulePkg/Logo/LogoDxe.inf
+INF OvmfPkg/AmdSev/SecretDxe/SecretDxe.inf

#
# Network modules




Re: [PATCH RFC v2 09/28] OvmfPkg/VmgExitLib: Allow PMBASE register access in Dxe phase

Laszlo Ersek
 

On 05/06/21 16:08, Laszlo Ersek wrote:
On 04/30/21 13:51, Brijesh Singh wrote:
BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3275

Commit 85b8eac59b8c5bd9c7eb9afdb64357ce1aa2e803 added support to ensure
that MMIO is only performed against the un-encrypted memory. If MMIO
is performed against encrypted memory, a #GP is raised.

The VmgExitLib library depends on ApicTimerLib to get the APIC base
address so that it can exclude the APIC range from the un-encrypted
check. The OvmfPkg provides ApicTimerLib for the DXE phase. The
constructor AcpiTimerLibConstructor() used in the ApicTimerLib uses
the PciRead to get the PMBASE register. The PciRead() will cause an
MMIO access.

The AmdSevDxe driver clears the memory encryption attribute from the
MMIO ranges. However, if VmgExitLib is linked to AmdSevDxe driver then the
AcpiTimerLibConstructor() will be called before AmdSevDxe driver can
clear the encryption attributes for the MMIO regions.

Exclude the PMBASE register from the encrypted check so that we
can link VmgExitLib to the MemEncryptSevLib; which gets linked to
AmdSevDxe driver.
The above explanation is inexact. There are several typos ("APIC" is
incorrect, "ACPI" would be correct, for the TimerLib instance in
question), but that's really just a side observation.

The precise explanation is the following library instance dependency
chain:

OvmfPkg/AmdSevDxe/AmdSevDxe.inf
-----> MemEncryptSevLib class
-----> "OvmfPkg/Library/BaseMemEncryptSevLib/DxeMemEncryptSevLib.inf" instance
-[*]-> VmgExitLib class
-----> "OvmfPkg/Library/VmgExitLib/VmgExitLib.inf" instance
-----> LocalApicLib class
-----> "UefiCpuPkg/Library/BaseXApicX2ApicLib/BaseXApicX2ApicLib.inf" instance
-----> TimerLib class
-----> "OvmfPkg/Library/AcpiTimerLib/DxeAcpiTimerLib.inf" instance
-----> PciLib class
-----> "OvmfPkg/Library/DxePciLibI440FxQ35/DxePciLibI440FxQ35.inf" instance
-----> PciExpressLib class
-----> "MdePkg/Library/BasePciExpressLib/BasePciExpressLib.inf" instance

The link (or dependency) marked with [*] is introduced in patch #26
("OvmfPkg/MemEncryptSevLib: Change the page state in the RMP table").
That's the change that triggers the symptom. (In combination with you
testing on Q35, because on i440fx, the DxePciLibI440FxQ35 lib instance
accesses PCI config space via the 0xCF8, 0xCFC IO Ports, and those are
unaffected by SEV-ES.)

The symptom is somewhat "unjustified", because at the end of the series,
the AmdSevDxe driver makes no calls to actual TimerLib APIs (I checked
-- I disassembled the "AmdSevDxe.debug" file with "objdump -S", and
there is no call to any API declared in the "TimerLib.h" class header).
However, the ECAM (MMCONFIG) access is still triggered, because the
AcpiTimerLibConstructor() function, in
"OvmfPkg/Library/AcpiTimerLib/DxeAcpiTimerLib.c", is the constructor for
the "OvmfPkg/Library/AcpiTimerLib/DxeAcpiTimerLib.inf" instance, and
AcpiTimerLibConstructor() calls PciRead32().

If you check the "OvmfPkg/OvmfPkgX64.dsc" file, you'll find that the
PciLib class is resolved to
"MdePkg/Library/BasePciLibCf8/BasePciLibCf8.inf" by default, and to
"OvmfPkg/Library/DxePciLibI440FxQ35/DxePciLibI440FxQ35.inf" for the
following module types:

- DXE_DRIVER,
- DXE_RUNTIME_DRIVER,
- SMM_CORE,
- DXE_SMM_DRIVER,
- UEFI_DRIVER,
- UEFI_APPLICATION.

The consequence is that modules strictly after the DXE_CORE get
dynamically enabled extended config space access (ECAM) on Q35 via the
PciLib class, whereas all modules strictly before the DXE_CORE, and the
DXE_CORE itself, are restricted to normal config space (IO Ports 0xCF8 /
0xCFC) via the PciLib class.

AmdSevDxe is a DXE_DRIVER, so it used to get DxePciLibI440FxQ35 as well.

The solution should be simple. In the AmdSevDxe driver specifically, we
need no access to extended PCI config space. Accessing normal PCI config
space, via IO Ports 0xCF8 / 0xCFC, should suffice. That can be achieved
with the following module-scope override:

diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
index 8d9a0a077601..45a02b236633 100644
--- a/OvmfPkg/OvmfPkgX64.dsc
+++ b/OvmfPkg/OvmfPkgX64.dsc
@@ -966,7 +966,10 @@ [Components]
!endif

OvmfPkg/PlatformDxe/Platform.inf
- OvmfPkg/AmdSevDxe/AmdSevDxe.inf
+ OvmfPkg/AmdSevDxe/AmdSevDxe.inf {
+ <LibraryClasses>
+ PciLib|MdePkg/Library/BasePciLibCf8/BasePciLibCf8.inf
+ }
OvmfPkg/IoMmuDxe/IoMmuDxe.inf
OvmfPkg/AmdSev/SecretDxe/SecretDxe.inf
(

For consistency, all DSC files that include
"OvmfPkg/AmdSevDxe/AmdSevDxe.inf" should be modified similarly:

- OvmfPkg/AmdSev/AmdSevX64.dsc
- OvmfPkg/Bhyve/BhyveX64.dsc
- OvmfPkg/OvmfPkgIa32X64.dsc
- OvmfPkg/OvmfPkgX64.dsc
- OvmfPkg/OvmfXen.dsc

)

Therefore, please try dropping this patch, and modifying patch#26
instead -- the above module-scope override (for 5 DSC files) should be
squashed into patch#26, *and* the explanation I provided above should be
included in the commit message of patch#26.

... Correction: you have an independent bug in the series that affects
my above analysis. Namely, you *seem* to add the VmgExitLib dependency
to the "OvmfPkg/Library/BaseMemEncryptSevLib/DxeMemEncryptSevLib.inf"
library instance, in patch#26. That's where you modify the INF file. But
that's wrong: in patch#21 ("OvmfPkg/MemEncryptSevLib: Add support to
validate system RAM"), you add a VmgInit() call to the same library
instance, via the new file
"OvmfPkg/Library/BaseMemEncryptSevLib/X64/SnpPageStateChangeInternal.c".

The bug in that patch is clear from the fact that you introduce an
#include <Library/VmgExitLib.h> directive, but that's not mirrored by an
appropriate [LibraryClasses] change in the "DxeMemEncryptSevLib.inf"
file. (The other two lib instance INF files, "SecMemEncryptSevLib.inf"
and "PeiMemEncryptSevLib.inf" *are* modified as needed.)

So you even need to move some stuff from patch#26 to patch#21, and
*then* squash the above module-scope override (and explanation) into
patch#21.
Actually, I've changed my mind, regarding the squashing.

Patch#21 is already large, and the module-scope PciLib override requires
a fair bit of justification (commit message, 5 DSC files modified etc).
Therefore, please do implement the module-scope PciLib override in its
own dedicated patch; *replacing* this one.

The new patch with the module-scoped PciLib override should be inserted
in the series just before the patch that establishes the dependency
marked with [*], that is, just before patch#21.

Thanks
Laszlo


A significant amount of work is needed on this series. I'll stop
reviewing RFC v2 here, because I don't want to look at the remaining
patches deeply as long as code movements etc are going to affect them.
Please post the next version -- assuming no other reviewer would like to
finish reviewing this version first!

Thanks
Laszlo


Cc: James Bottomley <jejb@linux.ibm.com>
Cc: Min Xu <min.m.xu@intel.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Erdem Aktas <erdemaktas@google.com>
Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
---
OvmfPkg/Library/VmgExitLib/SecVmgExitLib.inf | 4 ++
OvmfPkg/Library/VmgExitLib/VmgExitLib.inf | 7 +++
OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c | 45 ++++++++++++++++++++
3 files changed, 56 insertions(+)

diff --git a/OvmfPkg/Library/VmgExitLib/SecVmgExitLib.inf b/OvmfPkg/Library/VmgExitLib/SecVmgExitLib.inf
index e6f6ea7972..22435a0590 100644
--- a/OvmfPkg/Library/VmgExitLib/SecVmgExitLib.inf
+++ b/OvmfPkg/Library/VmgExitLib/SecVmgExitLib.inf
@@ -27,6 +27,7 @@
SecVmgExitVcHandler.c

[Packages]
+ MdeModulePkg/MdeModulePkg.dec
MdePkg/MdePkg.dec
OvmfPkg/OvmfPkg.dec
UefiCpuPkg/UefiCpuPkg.dec
@@ -42,4 +43,7 @@
[FixedPcd]
gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbBackupBase
gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbBackupSize
+ gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress

+[Pcd]
+ gUefiOvmfPkgTokenSpaceGuid.PcdOvmfHostBridgePciDevId
diff --git a/OvmfPkg/Library/VmgExitLib/VmgExitLib.inf b/OvmfPkg/Library/VmgExitLib/VmgExitLib.inf
index c66c68726c..d3175c260e 100644
--- a/OvmfPkg/Library/VmgExitLib/VmgExitLib.inf
+++ b/OvmfPkg/Library/VmgExitLib/VmgExitLib.inf
@@ -27,6 +27,7 @@
PeiDxeVmgExitVcHandler.c

[Packages]
+ MdeModulePkg/MdeModulePkg.dec
MdePkg/MdePkg.dec
OvmfPkg/OvmfPkg.dec
UefiCpuPkg/UefiCpuPkg.dec
@@ -37,4 +38,10 @@
DebugLib
LocalApicLib
MemEncryptSevLib
+ PcdLib

+[FixedPcd]
+ gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress
+
+[Pcd]
+ gUefiOvmfPkgTokenSpaceGuid.PcdOvmfHostBridgePciDevId
diff --git a/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c b/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c
index 24259060fd..01ac5d8c19 100644
--- a/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c
+++ b/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c
@@ -14,7 +14,10 @@
#include <Library/VmgExitLib.h>
#include <Register/Amd/Msr.h>
#include <Register/Intel/Cpuid.h>
+#include <IndustryStandard/Q35MchIch9.h>
+#include <IndustryStandard/I440FxPiix4.h>
#include <IndustryStandard/InstructionParsing.h>
+#include <Library/PcdLib.h>

#include "VmgExitVcHandler.h"

@@ -596,6 +599,40 @@ UnsupportedExit (
return Status;
}

+STATIC
+BOOLEAN
+IsPmbaBaseAddress (
+ IN UINTN Address
+ )
+{
+ UINT16 HostBridgeDevId;
+ UINTN Pmba;
+
+ //
+ // Query Host Bridge DID to determine platform type
+ //
+ HostBridgeDevId = PcdGet16 (PcdOvmfHostBridgePciDevId);
+ switch (HostBridgeDevId) {
+ case INTEL_82441_DEVICE_ID:
+ Pmba = POWER_MGMT_REGISTER_PIIX4 (PIIX4_PMBA);
+ break;
+ case INTEL_Q35_MCH_DEVICE_ID:
+ Pmba = POWER_MGMT_REGISTER_Q35 (ICH9_PMBASE);
+ //
+ // Add the MMCONFIG base address to get the Pmba base access address
+ //
+ Pmba += FixedPcdGet64 (PcdPciExpressBaseAddress);
+ break;
+ default:
+ return FALSE;
+ }
+
+ // Round up the offset to page size
+ Pmba = Pmba & ~(SIZE_4KB - 1);
+
+ return (Address == Pmba);
+}
+
/**
Validate that the MMIO memory access is not to encrypted memory.

@@ -640,6 +677,14 @@ ValidateMmioMemory (
return 0;
}

+ //
+ // Allow PMBASE accesses (which will have the encryption bit set before
+ // AmdSevDxe runs in the DXE phase)
+ //
+ if (IsPmbaBaseAddress (Address)) {
+ return 0;
+ }
+
//
// Any state other than unencrypted is an error, issue a #GP.
//


Re: [PATCH RFC v2 09/28] OvmfPkg/VmgExitLib: Allow PMBASE register access in Dxe phase

Laszlo Ersek
 

On 04/30/21 13:51, Brijesh Singh wrote:
BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3275

Commit 85b8eac59b8c5bd9c7eb9afdb64357ce1aa2e803 added support to ensure
that MMIO is only performed against the un-encrypted memory. If MMIO
is performed against encrypted memory, a #GP is raised.

The VmgExitLib library depends on ApicTimerLib to get the APIC base
address so that it can exclude the APIC range from the un-encrypted
check. The OvmfPkg provides ApicTimerLib for the DXE phase. The
constructor AcpiTimerLibConstructor() used in the ApicTimerLib uses
the PciRead to get the PMBASE register. The PciRead() will cause an
MMIO access.

The AmdSevDxe driver clears the memory encryption attribute from the
MMIO ranges. However, if VmgExitLib is linked to AmdSevDxe driver then the
AcpiTimerLibConstructor() will be called before AmdSevDxe driver can
clear the encryption attributes for the MMIO regions.

Exclude the PMBASE register from the encrypted check so that we
can link VmgExitLib to the MemEncryptSevLib; which gets linked to
AmdSevDxe driver.
The above explanation is inexact. There are several typos ("APIC" is
incorrect, "ACPI" would be correct, for the TimerLib instance in
question), but that's really just a side observation.

The precise explanation is the following library instance dependency
chain:

OvmfPkg/AmdSevDxe/AmdSevDxe.inf
-----> MemEncryptSevLib class
-----> "OvmfPkg/Library/BaseMemEncryptSevLib/DxeMemEncryptSevLib.inf" instance
-[*]-> VmgExitLib class
-----> "OvmfPkg/Library/VmgExitLib/VmgExitLib.inf" instance
-----> LocalApicLib class
-----> "UefiCpuPkg/Library/BaseXApicX2ApicLib/BaseXApicX2ApicLib.inf" instance
-----> TimerLib class
-----> "OvmfPkg/Library/AcpiTimerLib/DxeAcpiTimerLib.inf" instance
-----> PciLib class
-----> "OvmfPkg/Library/DxePciLibI440FxQ35/DxePciLibI440FxQ35.inf" instance
-----> PciExpressLib class
-----> "MdePkg/Library/BasePciExpressLib/BasePciExpressLib.inf" instance

The link (or dependency) marked with [*] is introduced in patch #26
("OvmfPkg/MemEncryptSevLib: Change the page state in the RMP table").
That's the change that triggers the symptom. (In combination with you
testing on Q35, because on i440fx, the DxePciLibI440FxQ35 lib instance
accesses PCI config space via the 0xCF8, 0xCFC IO Ports, and those are
unaffected by SEV-ES.)

The symptom is somewhat "unjustified", because at the end of the series,
the AmdSevDxe driver makes no calls to actual TimerLib APIs (I checked
-- I disassembled the "AmdSevDxe.debug" file with "objdump -S", and
there is no call to any API declared in the "TimerLib.h" class header).
However, the ECAM (MMCONFIG) access is still triggered, because the
AcpiTimerLibConstructor() function, in
"OvmfPkg/Library/AcpiTimerLib/DxeAcpiTimerLib.c", is the constructor for
the "OvmfPkg/Library/AcpiTimerLib/DxeAcpiTimerLib.inf" instance, and
AcpiTimerLibConstructor() calls PciRead32().

If you check the "OvmfPkg/OvmfPkgX64.dsc" file, you'll find that the
PciLib class is resolved to
"MdePkg/Library/BasePciLibCf8/BasePciLibCf8.inf" by default, and to
"OvmfPkg/Library/DxePciLibI440FxQ35/DxePciLibI440FxQ35.inf" for the
following module types:

- DXE_DRIVER,
- DXE_RUNTIME_DRIVER,
- SMM_CORE,
- DXE_SMM_DRIVER,
- UEFI_DRIVER,
- UEFI_APPLICATION.

The consequence is that modules strictly after the DXE_CORE get
dynamically enabled extended config space access (ECAM) on Q35 via the
PciLib class, whereas all modules strictly before the DXE_CORE, and the
DXE_CORE itself, are restricted to normal config space (IO Ports 0xCF8 /
0xCFC) via the PciLib class.

AmdSevDxe is a DXE_DRIVER, so it used to get DxePciLibI440FxQ35 as well.

The solution should be simple. In the AmdSevDxe driver specifically, we
need no access to extended PCI config space. Accessing normal PCI config
space, via IO Ports 0xCF8 / 0xCFC, should suffice. That can be achieved
with the following module-scope override:

diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
index 8d9a0a077601..45a02b236633 100644
--- a/OvmfPkg/OvmfPkgX64.dsc
+++ b/OvmfPkg/OvmfPkgX64.dsc
@@ -966,7 +966,10 @@ [Components]
!endif

OvmfPkg/PlatformDxe/Platform.inf
- OvmfPkg/AmdSevDxe/AmdSevDxe.inf
+ OvmfPkg/AmdSevDxe/AmdSevDxe.inf {
+ <LibraryClasses>
+ PciLib|MdePkg/Library/BasePciLibCf8/BasePciLibCf8.inf
+ }
OvmfPkg/IoMmuDxe/IoMmuDxe.inf
OvmfPkg/AmdSev/SecretDxe/SecretDxe.inf
(

For consistency, all DSC files that include
"OvmfPkg/AmdSevDxe/AmdSevDxe.inf" should be modified similarly:

- OvmfPkg/AmdSev/AmdSevX64.dsc
- OvmfPkg/Bhyve/BhyveX64.dsc
- OvmfPkg/OvmfPkgIa32X64.dsc
- OvmfPkg/OvmfPkgX64.dsc
- OvmfPkg/OvmfXen.dsc

)

Therefore, please try dropping this patch, and modifying patch#26
instead -- the above module-scope override (for 5 DSC files) should be
squashed into patch#26, *and* the explanation I provided above should be
included in the commit message of patch#26.

... Correction: you have an independent bug in the series that affects
my above analysis. Namely, you *seem* to add the VmgExitLib dependency
to the "OvmfPkg/Library/BaseMemEncryptSevLib/DxeMemEncryptSevLib.inf"
library instance, in patch#26. That's where you modify the INF file. But
that's wrong: in patch#21 ("OvmfPkg/MemEncryptSevLib: Add support to
validate system RAM"), you add a VmgInit() call to the same library
instance, via the new file
"OvmfPkg/Library/BaseMemEncryptSevLib/X64/SnpPageStateChangeInternal.c".

The bug in that patch is clear from the fact that you introduce an
#include <Library/VmgExitLib.h> directive, but that's not mirrored by an
appropriate [LibraryClasses] change in the "DxeMemEncryptSevLib.inf"
file. (The other two lib instance INF files, "SecMemEncryptSevLib.inf"
and "PeiMemEncryptSevLib.inf" *are* modified as needed.)

So you even need to move some stuff from patch#26 to patch#21, and
*then* squash the above module-scope override (and explanation) into
patch#21.

A significant amount of work is needed on this series. I'll stop
reviewing RFC v2 here, because I don't want to look at the remaining
patches deeply as long as code movements etc are going to affect them.
Please post the next version -- assuming no other reviewer would like to
finish reviewing this version first!

Thanks
Laszlo


Cc: James Bottomley <jejb@linux.ibm.com>
Cc: Min Xu <min.m.xu@intel.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Erdem Aktas <erdemaktas@google.com>
Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
---
OvmfPkg/Library/VmgExitLib/SecVmgExitLib.inf | 4 ++
OvmfPkg/Library/VmgExitLib/VmgExitLib.inf | 7 +++
OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c | 45 ++++++++++++++++++++
3 files changed, 56 insertions(+)

diff --git a/OvmfPkg/Library/VmgExitLib/SecVmgExitLib.inf b/OvmfPkg/Library/VmgExitLib/SecVmgExitLib.inf
index e6f6ea7972..22435a0590 100644
--- a/OvmfPkg/Library/VmgExitLib/SecVmgExitLib.inf
+++ b/OvmfPkg/Library/VmgExitLib/SecVmgExitLib.inf
@@ -27,6 +27,7 @@
SecVmgExitVcHandler.c

[Packages]
+ MdeModulePkg/MdeModulePkg.dec
MdePkg/MdePkg.dec
OvmfPkg/OvmfPkg.dec
UefiCpuPkg/UefiCpuPkg.dec
@@ -42,4 +43,7 @@
[FixedPcd]
gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbBackupBase
gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbBackupSize
+ gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress

+[Pcd]
+ gUefiOvmfPkgTokenSpaceGuid.PcdOvmfHostBridgePciDevId
diff --git a/OvmfPkg/Library/VmgExitLib/VmgExitLib.inf b/OvmfPkg/Library/VmgExitLib/VmgExitLib.inf
index c66c68726c..d3175c260e 100644
--- a/OvmfPkg/Library/VmgExitLib/VmgExitLib.inf
+++ b/OvmfPkg/Library/VmgExitLib/VmgExitLib.inf
@@ -27,6 +27,7 @@
PeiDxeVmgExitVcHandler.c

[Packages]
+ MdeModulePkg/MdeModulePkg.dec
MdePkg/MdePkg.dec
OvmfPkg/OvmfPkg.dec
UefiCpuPkg/UefiCpuPkg.dec
@@ -37,4 +38,10 @@
DebugLib
LocalApicLib
MemEncryptSevLib
+ PcdLib

+[FixedPcd]
+ gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress
+
+[Pcd]
+ gUefiOvmfPkgTokenSpaceGuid.PcdOvmfHostBridgePciDevId
diff --git a/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c b/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c
index 24259060fd..01ac5d8c19 100644
--- a/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c
+++ b/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c
@@ -14,7 +14,10 @@
#include <Library/VmgExitLib.h>
#include <Register/Amd/Msr.h>
#include <Register/Intel/Cpuid.h>
+#include <IndustryStandard/Q35MchIch9.h>
+#include <IndustryStandard/I440FxPiix4.h>
#include <IndustryStandard/InstructionParsing.h>
+#include <Library/PcdLib.h>

#include "VmgExitVcHandler.h"

@@ -596,6 +599,40 @@ UnsupportedExit (
return Status;
}

+STATIC
+BOOLEAN
+IsPmbaBaseAddress (
+ IN UINTN Address
+ )
+{
+ UINT16 HostBridgeDevId;
+ UINTN Pmba;
+
+ //
+ // Query Host Bridge DID to determine platform type
+ //
+ HostBridgeDevId = PcdGet16 (PcdOvmfHostBridgePciDevId);
+ switch (HostBridgeDevId) {
+ case INTEL_82441_DEVICE_ID:
+ Pmba = POWER_MGMT_REGISTER_PIIX4 (PIIX4_PMBA);
+ break;
+ case INTEL_Q35_MCH_DEVICE_ID:
+ Pmba = POWER_MGMT_REGISTER_Q35 (ICH9_PMBASE);
+ //
+ // Add the MMCONFIG base address to get the Pmba base access address
+ //
+ Pmba += FixedPcdGet64 (PcdPciExpressBaseAddress);
+ break;
+ default:
+ return FALSE;
+ }
+
+ // Round up the offset to page size
+ Pmba = Pmba & ~(SIZE_4KB - 1);
+
+ return (Address == Pmba);
+}
+
/**
Validate that the MMIO memory access is not to encrypted memory.

@@ -640,6 +677,14 @@ ValidateMmioMemory (
return 0;
}

+ //
+ // Allow PMBASE accesses (which will have the encryption bit set before
+ // AmdSevDxe runs in the DXE phase)
+ //
+ if (IsPmbaBaseAddress (Address)) {
+ return 0;
+ }
+
//
// Any state other than unencrypted is an error, issue a #GP.
//


Re: [PATCH RFC v2 08/28] OvmfPkg/BaseMemEncryptSevLib: Remove CacheFlush parameter

Laszlo Ersek
 

On 04/30/21 13:51, Brijesh Singh wrote:
BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3275

The CacheFlush parameter is used to provide 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
CacheFlush parameter from MemEncryptSev{Set,Clear}PageEncMask().
The subject and the commit message body refer to "CacheFlush", but the
parameter (at the library class level) is actually called "Flush".

(1) Please update the subject and the commit message body.



Cc: James Bottomley <jejb@linux.ibm.com>
Cc: Min Xu <min.m.xu@intel.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Erdem Aktas <erdemaktas@google.com>
Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
---
OvmfPkg/AmdSevDxe/AmdSevDxe.c | 3 +--
OvmfPkg/Include/Library/MemEncryptSevLib.h | 10 ++--------
OvmfPkg/IoMmuDxe/AmdSevIoMmu.c | 6 ++----
OvmfPkg/Library/BaseMemEncryptSevLib/Ia32/MemEncryptSevLib.c | 10 ++--------
OvmfPkg/Library/BaseMemEncryptSevLib/X64/MemEncryptSevLib.c | 16 ++++------------
OvmfPkg/Library/BaseMemEncryptSevLib/X64/PeiDxeVirtualMemory.c | 14 ++++----------
OvmfPkg/Library/BaseMemEncryptSevLib/X64/SecVirtualMemory.c | 8 ++------
OvmfPkg/Library/BaseMemEncryptSevLib/X64/VirtualMemory.h | 10 ++--------
OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c | 3 +--
OvmfPkg/PlatformPei/AmdSev.c | 3 +--
10 files changed, 21 insertions(+), 62 deletions(-)
Because we're modifying a library class header file, I agree we need to
update multiple modules in a single patch.



diff --git a/OvmfPkg/AmdSevDxe/AmdSevDxe.c b/OvmfPkg/AmdSevDxe/AmdSevDxe.c
index 80831b81fa..41e4b291d0 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",
(2) By the removal of the comma, the comments are no longer nicely
aligned. Please insert a space character.

The rest looks OK to me.

Thanks
Laszlo

diff --git a/OvmfPkg/Include/Library/MemEncryptSevLib.h b/OvmfPkg/Include/Library/MemEncryptSevLib.h
index c19f92afc6..9b15d80931 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/IoMmuDxe/AmdSevIoMmu.c b/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c
index 49ffa24488..b30628078f 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 4e8a997d42..34e7c59e2c 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 6786573aea..5c260c546e 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 3bcc92f2e9..707db5a74a 100644
--- a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/PeiDxeVirtualMemory.c
+++ b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/PeiDxeVirtualMemory.c
@@ -830,8 +830,6 @@ Done:
@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.
@@ -844,8 +842,7 @@ EFIAPI
InternalMemEncryptSevSetMemoryDecrypted (
IN PHYSICAL_ADDRESS Cr3BaseAddress,
IN PHYSICAL_ADDRESS PhysicalAddress,
- IN UINTN Length,
- IN BOOLEAN Flush
+ IN UINTN Length
)
{

@@ -854,7 +851,7 @@ InternalMemEncryptSevSetMemoryDecrypted (
PhysicalAddress,
Length,
ClearCBit,
- Flush,
+ TRUE,
FALSE
);
}
@@ -868,8 +865,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.
@@ -882,8 +877,7 @@ EFIAPI
InternalMemEncryptSevSetMemoryEncrypted (
IN PHYSICAL_ADDRESS Cr3BaseAddress,
IN PHYSICAL_ADDRESS PhysicalAddress,
- IN UINTN Length,
- IN BOOLEAN Flush
+ IN UINTN Length
)
{
return SetMemoryEncDec (
@@ -891,7 +885,7 @@ InternalMemEncryptSevSetMemoryEncrypted (
PhysicalAddress,
Length,
SetCBit,
- Flush,
+ TRUE,
FALSE
);
}
diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/SecVirtualMemory.c b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/SecVirtualMemory.c
index bca5e3febb..24d19d3ca1 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/BaseMemEncryptSevLib/X64/VirtualMemory.h b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/VirtualMemory.h
index 99ee7ea0e8..832ff10a33 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/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c b/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c
index fdf2380974..c7cc5b0389 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 dddffdebda..a8bf610022 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);
}


Re: [PATCH RFC v2 11/28] OvmfPkg: Reserve Secrets page in MEMFD

Dov Murik <dovmurik@...>
 

On 05/05/2021 22:33, Laszlo Ersek wrote:
On 05/05/21 15:11, Brijesh Singh wrote:

On 5/5/21 1:42 AM, Dov Murik wrote:
[+cc: Tobin]

Hi Brijesh,

On 30/04/2021 14:51, Brijesh Singh wrote:
BZ: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D3275&;data=04%7C01%7Cbrijesh.singh%40amd.com%7C93168c94eb6d44ed08e608d90f910426%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637557937779907471%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=nLpmk3G%2BmXcZrzXxCmO3M9EDPiLRnP1IUmPqRQNbBuU%3D&amp;reserved=0

When AMD SEV is enabled in the guest VM, a hypervisor need to insert a
secrets page.

When SEV-SNP is enabled, the secrets page contains the VM platform
communication keys. The guest BIOS and OS can use this key to communicate
with the SEV firmware to get attesation report. See the SEV-SNP firmware
spec for more details for the content of the secrets page.

When SEV and SEV-ES is enabled, the secrets page contains the information
provided by the guest owner after the attestation. See the SEV
LAUNCH_SECRET command for more details.

Cc: James Bottomley <jejb@linux.ibm.com>
Cc: Min Xu <min.m.xu@intel.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Erdem Aktas <erdemaktas@google.com>
Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
---
OvmfPkg/AmdSev/SecretPei/SecretPei.c | 16 +++++++++++++++-
OvmfPkg/AmdSev/SecretPei/SecretPei.inf | 1 +
OvmfPkg/OvmfPkgX64.dsc | 2 ++
OvmfPkg/OvmfPkgX64.fdf | 5 +++++
4 files changed, 23 insertions(+), 1 deletion(-)

diff --git a/OvmfPkg/AmdSev/SecretPei/SecretPei.c b/OvmfPkg/AmdSev/SecretPei/SecretPei.c
index ad491515dd..92836c562c 100644
--- a/OvmfPkg/AmdSev/SecretPei/SecretPei.c
+++ b/OvmfPkg/AmdSev/SecretPei/SecretPei.c
@@ -7,6 +7,7 @@
#include <PiPei.h>
#include <Library/HobLib.h>
#include <Library/PcdLib.h>
+#include <Library/MemEncryptSevLib.h>

EFI_STATUS
EFIAPI
@@ -15,10 +16,23 @@ InitializeSecretPei (
IN CONST EFI_PEI_SERVICES **PeiServices
)
{
+ UINTN Type;
+
+ //
+ // The secret page should be mapped encrypted by the guest OS and must not
+ // be treated as a system RAM. Mark it as ACPI NVS so that guest OS maps it
+ // encrypted.
+ //
+ if (MemEncryptSevSnpIsEnabled ()) {
+ Type = EfiACPIMemoryNVS;
+ } else {
+ Type = EfiBootServicesData;
+ }
+
Would it make sense to always use EfiACPIMemoryNVS for the injected secret area, even for regular SEV (non-SNP)?
Ideally yes. Maybe James had some reasons for choosing the
EfiBootServicesData. If I had to guess, it was mainly because there no
guest kernel support which consumes the SEV secrets page.
git-blame fingers commit bff2811c6d99 ("OvmfPkg/AmdSev: assign and
reserve the Sev Secret area", 2020-12-14).

Commit bff2811c6d99 makes it clear that the area in question lives in MEMFD.

We're populating the area in the PEI phase. We don't want anything in
DXE to overwrite it.

Once the bootloader (and/or perhaps the kernel's EFI stub) fetched the
secret from that particular location, there is no need to prevent later
parts of the OS (the actual kernel) from repurposing that area. That's
why EfiBootServicesData was used.
The first use of the secret area was to hold the guest luks disk passphrase; this is used in the grub-inside-OVMF (AmdSev package), and there was no need to keep that page around for the guest kernel.

The reason I'm raising this whole point is that we're working now on guest-kernel support for reading secrets from that injected page (for plain SEV). We considered either (a) modifying the secrets page memory type to reserved here, or (b) add code to the kernel EFI stub that would copy this page somewhere else for kernel's later use (which seems more work and not sure what's the advantage).

Option (b) seems harder and more fragile, and I'm not sure if there are any advantages (though I'm definitely not an expert in that area).




Since the
memory is not marked ACPI NVS, so it can be used as a system RAM after
the ExitBootServices is called in the kernel.
Yes.

I don't think AcpiNVS would be a good fit. Linux saves and restores
AcpiNVS areas upon S3 suspend/resume. Regardless of whether S3 works, or
will work, in SEV* guests, if we don't want the guest kernel to touch
that area *at all*, Reserved is a better type.
Thanks for this clarification.


Please refer to "Table 7-6 Memory Type Usage after ExitBootServices()"
in the UEFI spec (v2.9).


I am fine with using ACPI NVS for both SEV and SEV-SNP. I was not able
to build and run AmdSev package in my setup, can you submit a prepatch
to change the memory type and verify that it works ?
NB: I've not yet reached this patch in my own review of the series, so
I'm likely missing some context. I do have a thought -- under SEV-SNP,
the secrets page apparently needs different (stronger) protection from
the host as under plain SEV. I don't think that hiding the different
protection requirements behind a single common memory type is helpful.
Not to mention the wasted memory in the plain SEV case -- it's not a lot
of memory, mind you, but the principle matters.
Like I said above, we have plans to have this small amount of memory available also to the guest OS; so maybe that shouldn't be the driving force in the decision here.

-Dov



So ATM I would like to keep this patch in the SEV-SNP series, and to
preserve the different memory types between SEV and SEV-SNP.

Thanks
Laszlo






-Dov



BuildMemoryAllocationHob (
PcdGet32 (PcdSevLaunchSecretBase),
PcdGet32 (PcdSevLaunchSecretSize),
- EfiBootServicesData
+ Type
);

return EFI_SUCCESS;
diff --git a/OvmfPkg/AmdSev/SecretPei/SecretPei.inf b/OvmfPkg/AmdSev/SecretPei/SecretPei.inf
index 08be156c4b..9265f8adee 100644
--- a/OvmfPkg/AmdSev/SecretPei/SecretPei.inf
+++ b/OvmfPkg/AmdSev/SecretPei/SecretPei.inf
@@ -26,6 +26,7 @@
HobLib
PeimEntryPoint
PcdLib
+ MemEncryptSevLib

[FixedPcd]
gUefiOvmfPkgTokenSpaceGuid.PcdSevLaunchSecretBase
diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
index a7d747f6b4..593c0e69f6 100644
--- a/OvmfPkg/OvmfPkgX64.dsc
+++ b/OvmfPkg/OvmfPkgX64.dsc
@@ -716,6 +716,7 @@
OvmfPkg/SmmAccess/SmmAccessPei.inf
!endif
UefiCpuPkg/CpuMpPei/CpuMpPei.inf
+ OvmfPkg/AmdSev/SecretPei/SecretPei.inf

!if $(TPM_ENABLE) == TRUE
OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf
@@ -965,6 +966,7 @@
OvmfPkg/PlatformDxe/Platform.inf
OvmfPkg/AmdSevDxe/AmdSevDxe.inf
OvmfPkg/IoMmuDxe/IoMmuDxe.inf
+ OvmfPkg/AmdSev/SecretDxe/SecretDxe.inf

!if $(SMM_REQUIRE) == TRUE
OvmfPkg/SmmAccess/SmmAccess2Dxe.inf
diff --git a/OvmfPkg/OvmfPkgX64.fdf b/OvmfPkg/OvmfPkgX64.fdf
index d519f85328..b04175f77c 100644
--- a/OvmfPkg/OvmfPkgX64.fdf
+++ b/OvmfPkg/OvmfPkgX64.fdf
@@ -88,6 +88,9 @@ gUefiCpuPkgTokenSpaceGuid.PcdSevEsWorkAreaBase|gUefiCpuPkgTokenSpaceGuid.PcdSevE
0x00C000|0x001000
gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbBackupBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbBackupSize

+0x00D000|0x001000
+gUefiOvmfPkgTokenSpaceGuid.PcdSevLaunchSecretBase|gUefiOvmfPkgTokenSpaceGuid.PcdSevLaunchSecretSize
+
0x010000|0x010000
gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamSize

@@ -178,6 +181,7 @@ INF OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf
INF SecurityPkg/Tcg/TcgPei/TcgPei.inf
INF SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf
!endif
+INF OvmfPkg/AmdSev/SecretPei/SecretPei.inf

################################################################################

@@ -313,6 +317,7 @@ INF OvmfPkg/LinuxInitrdDynamicShellCommand/LinuxInitrdDynamicShellCommand.inf
INF ShellPkg/Application/Shell/Shell.inf

INF MdeModulePkg/Logo/LogoDxe.inf
+INF OvmfPkg/AmdSev/SecretDxe/SecretDxe.inf

#
# Network modules




Re: [PATCH RFC v2 07/28] OvmfPkg: Use MemEncryptSevClearMmioPageEncMask() to clear EncMask from Mmio

Laszlo Ersek
 

On 04/30/21 13:51, Brijesh Singh wrote:
BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3275

Use the MemEncryptSevClearMmioPageEncMask() to clear memory encryption mask
for the Mmio address range from the current page table context.

Cc: James Bottomley <jejb@linux.ibm.com>
Cc: Min Xu <min.m.xu@intel.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Erdem Aktas <erdemaktas@google.com>
Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
---
OvmfPkg/AmdSevDxe/AmdSevDxe.c | 10 ++++------
OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceDxe.c | 5 ++---
OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlashSmm.c | 5 ++---
3 files changed, 8 insertions(+), 12 deletions(-)

diff --git a/OvmfPkg/AmdSevDxe/AmdSevDxe.c b/OvmfPkg/AmdSevDxe/AmdSevDxe.c
index 689bfb376d..80831b81fa 100644
--- a/OvmfPkg/AmdSevDxe/AmdSevDxe.c
+++ b/OvmfPkg/AmdSevDxe/AmdSevDxe.c
@@ -53,11 +53,10 @@ AmdSevDxeEntryPoint (
Desc = &AllDescMap[Index];
if (Desc->GcdMemoryType == EfiGcdMemoryTypeMemoryMappedIo ||
Desc->GcdMemoryType == EfiGcdMemoryTypeNonExistent) {
- Status = MemEncryptSevClearPageEncMask (
+ Status = MemEncryptSevClearMmioPageEncMask (
0,
Desc->BaseAddress,
- EFI_SIZE_TO_PAGES (Desc->Length),
- FALSE
+ EFI_SIZE_TO_PAGES (Desc->Length)
);
ASSERT_EFI_ERROR (Status);
}
@@ -73,11 +72,10 @@ AmdSevDxeEntryPoint (
// the range.
//
if (PcdGet16 (PcdOvmfHostBridgePciDevId) == INTEL_Q35_MCH_DEVICE_ID) {
- Status = MemEncryptSevClearPageEncMask (
+ Status = MemEncryptSevClearMmioPageEncMask (
0,
FixedPcdGet64 (PcdPciExpressBaseAddress),
- EFI_SIZE_TO_PAGES (SIZE_256MB),
- FALSE
+ EFI_SIZE_TO_PAGES (SIZE_256MB)
);

ASSERT_EFI_ERROR (Status);
diff --git a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceDxe.c b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceDxe.c
index 1f285e0083..ab40087a84 100644
--- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceDxe.c
+++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceDxe.c
@@ -205,11 +205,10 @@ MarkIoMemoryRangeForRuntimeAccess (
// memory range.
//
if (MemEncryptSevIsEnabled ()) {
- Status = MemEncryptSevClearPageEncMask (
+ Status = MemEncryptSevClearMmioPageEncMask (
0,
BaseAddress,
- EFI_SIZE_TO_PAGES (Length),
- FALSE
+ EFI_SIZE_TO_PAGES (Length)
);
ASSERT_EFI_ERROR (Status);
}
diff --git a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlashSmm.c b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlashSmm.c
index 7eb80bfeff..ea75b489c7 100644
--- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlashSmm.c
+++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlashSmm.c
@@ -38,11 +38,10 @@ QemuFlashBeforeProbe (
// C-bit on flash ranges from SMM page table.
//

- Status = MemEncryptSevClearPageEncMask (
+ Status = MemEncryptSevClearMmioPageEncMask (
0,
BaseAddress,
- EFI_SIZE_TO_PAGES (FdBlockSize * FdBlockCount),
- FALSE
+ EFI_SIZE_TO_PAGES (FdBlockSize * FdBlockCount)
);
ASSERT_EFI_ERROR (Status);
}
The contents of this patch are sound, but they are incomplete, and
incorrectly structured too.

(1) Please provide a separate patch for each modified module.

(2) You missed the MemEncryptSevClearPageEncMask() call in
TpmMmioSevDecryptPeimEntryPoint()
[OvmfPkg/Tcg/TpmMmioSevDecryptPei/TpmMmioSevDecryptPeim.c] -- probably
because you worked on this series in parallel with Tom working on the
SEV-ES TPM fixes.

In the end, this patch should be split into three patches (because the
change is needed for three modules).

Thanks
Laszlo


[PATCH v1 1/1] ArmPkg: Update SCMI Base Protocol version to 0x20000

PierreGondois
 

From: Nicola Mazzucato <nicola.mazzucato@arm.com>

The SCP-firmware has moved to full support for SCMIv2 which means that
the base protocol can be either compliant with SCMI v1 or v2.

Allow any version between SCMI v1.0 and SCMI v2.0 to be compatible
with the current implementation.

Signed-off-by: Nicola Mazzucato <nicola.mazzucato@arm.com>
---
The changes can be seen at: https://github.com/PierreARM/edk2/tree/1732_Update_SCMI_version_v1

ArmPkg/Drivers/ArmScmiDxe/ScmiDxe.c | 10 ++++++----
ArmPkg/Include/Protocol/ArmScmiBaseProtocol.h | 10 +++++-----
2 files changed, 11 insertions(+), 9 deletions(-)

diff --git a/ArmPkg/Drivers/ArmScmiDxe/ScmiDxe.c b/ArmPkg/Drivers/ArmScmiDxe/ScmiDxe.c
index d5890a7633a2..fb4e79aa3610 100644
--- a/ArmPkg/Drivers/ArmScmiDxe/ScmiDxe.c
+++ b/ArmPkg/Drivers/ArmScmiDxe/ScmiDxe.c
@@ -4,9 +4,9 @@

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

- System Control and Management Interface V1.0
- http://infocenter.arm.com/help/topic/com.arm.doc.den0056a/
- DEN0056A_System_Control_and_Management_Interface.pdf
+ @par Specification Reference:
+ - Arm System Control and Management Interface - Platform Design Document
+ (https://developer.arm.com/documentation/den0056/)
**/

#include <Base.h>
@@ -86,7 +86,9 @@ ArmScmiDxeEntryPoint (
return Status;
}

- if (Version != BASE_PROTOCOL_VERSION) {
+ // Accept any version between SCMI v1.0 and SCMI v2.0
+ if ((Version < BASE_PROTOCOL_VERSION_V1) ||
+ (Version > BASE_PROTOCOL_VERSION_V2)) {
ASSERT (FALSE);
return EFI_UNSUPPORTED;
}
diff --git a/ArmPkg/Include/Protocol/ArmScmiBaseProtocol.h b/ArmPkg/Include/Protocol/ArmScmiBaseProtocol.h
index 73ad3e32a2f5..c4b81c0f56d3 100644
--- a/ArmPkg/Include/Protocol/ArmScmiBaseProtocol.h
+++ b/ArmPkg/Include/Protocol/ArmScmiBaseProtocol.h
@@ -4,9 +4,9 @@

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

- System Control and Management Interface V1.0
- http://infocenter.arm.com/help/topic/com.arm.doc.den0056a/
- DEN0056A_System_Control_and_Management_Interface.pdf
+ @par Specification Reference:
+ - Arm System Control and Management Interface - Platform Design Document
+ (https://developer.arm.com/documentation/den0056/)
**/

#ifndef ARM_SCMI_BASE_PROTOCOL_H_
@@ -14,7 +14,8 @@

#include <Protocol/ArmScmi.h>

-#define BASE_PROTOCOL_VERSION 0x10000
+#define BASE_PROTOCOL_VERSION_V1 0x10000
+#define BASE_PROTOCOL_VERSION_V2 0x20000

#define NUM_PROTOCOL_MASK 0xFFU
#define NUM_AGENT_MASK 0xFFU
@@ -165,4 +166,3 @@ typedef enum {
} SCMI_MESSAGE_ID_BASE;

#endif /* ARM_SCMI_BASE_PROTOCOL_H_ */
-
--
2.17.1


Re: [PATCH RFC v2 06/28] OvmfPkg/BaseMemEncryptSevLib: Introduce MemEncryptSevClearMmioPageEncMask()

Laszlo Ersek
 

On 04/30/21 13:51, 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 from the current page
table context.
The commit message, and five comments in the patch, say "current page
table context". However, Cr3BaseAddress is taken explicitly.

I realize the files being modified in this patch already make similarly
incorrect statements, but I'd like if we avoided adding more.

(1) Please just drop "current page table context" from the mentioned six
locations. (Explaining that Cr3BaseAddress=0 means "current CR3" is of
course valid.)



Cc: James Bottomley <jejb@linux.ibm.com>
Cc: Min Xu <min.m.xu@intel.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Erdem Aktas <erdemaktas@google.com>
Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
---
OvmfPkg/Include/Library/MemEncryptSevLib.h | 25 +++++++++++
OvmfPkg/Library/BaseMemEncryptSevLib/Ia32/MemEncryptSevLib.c | 31 ++++++++++++++
OvmfPkg/Library/BaseMemEncryptSevLib/X64/MemEncryptSevLib.c | 33 +++++++++++++++
OvmfPkg/Library/BaseMemEncryptSevLib/X64/PeiDxeVirtualMemory.c | 44 ++++++++++++++++++--
OvmfPkg/Library/BaseMemEncryptSevLib/X64/VirtualMemory.h | 23 ++++++++++
5 files changed, 153 insertions(+), 3 deletions(-)

diff --git a/OvmfPkg/Include/Library/MemEncryptSevLib.h b/OvmfPkg/Include/Library/MemEncryptSevLib.h
index 99f15a7d12..c19f92afc6 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 from the current page table context.
+
+ @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/Ia32/MemEncryptSevLib.c b/OvmfPkg/Library/BaseMemEncryptSevLib/Ia32/MemEncryptSevLib.c
index 12a5bf495b..4e8a997d42 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 from the current page table context.
+
+ @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 4fea6a6be0..6786573aea 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 from the current page table context.
+
+ @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 d3455e812b..3bcc92f2e9 100644
--- a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/PeiDxeVirtualMemory.c
+++ b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/PeiDxeVirtualMemory.c
@@ -557,6 +557,7 @@ EnableReadOnlyPageWriteProtect (
@param[in] Mode Set or Clear mode
@param[in] CacheFlush Flush the caches before applying the
encryption mask
+ @param[in] IsMmio The address is Mmio address.

@retval RETURN_SUCCESS The attributes were cleared for the
memory region.
(2) The parameter's name in the documentation ("IsMmio") does not match
the actual parameter name ("Mmio").


@@ -572,7 +573,8 @@ SetMemoryEncDec (
IN PHYSICAL_ADDRESS PhysicalAddress,
IN UINTN Length,
IN MAP_RANGE_MODE Mode,
- IN BOOLEAN CacheFlush
+ IN BOOLEAN CacheFlush,
+ IN BOOLEAN Mmio
)
{
PAGE_MAP_AND_DIRECTORY_POINTER *PageMapLevel4Entry;
The "Mmio" parameter is not used for anything in this patch. It's very
confusing.

(3) Please remove the addition of the Mmio parameter from this patch.
Please introduce the Mmio parameter only when it is utilized -- as far
as I can tell from a quick git-blame, that's patch #26
("OvmfPkg/MemEncryptSevLib: Change the page state in the RMP table").

As a result, in this patch, InternalMemEncryptSevClearMmioPageEncMask()
will effectively become a slight simplification of
InternalMemEncryptSevSetMemoryDecrypted() -- rather than forwarding
"Flush" for "CacheFlush", it will pass constant FALSE for "CacheFlush".


(4) Please mention the above fact (= last paragraph above) in the commit
message.


@@ -852,7 +854,8 @@ InternalMemEncryptSevSetMemoryDecrypted (
PhysicalAddress,
Length,
ClearCBit,
- Flush
+ Flush,
+ FALSE
);
}

@@ -888,6 +891,41 @@ InternalMemEncryptSevSetMemoryEncrypted (
PhysicalAddress,
Length,
SetCBit,
- Flush
+ Flush,
+ FALSE
+ );
+}
+
+/**
+ This function clears memory encryption bit for the Mmio region specified by
+ PhysicalAddress and Length from the current page table context.
+
+ @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 Number of pages is zero.
This function does not take a NumPages parameter but a Length parameter.

(5) Please update the RETURN_INVALID_PARAMETER comment accordingly -- in
the header file as well.


+ @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,
+ TRUE
);
}
diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/VirtualMemory.h b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/VirtualMemory.h
index fe2a0b2826..99ee7ea0e8 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 from the current page table context.
+
+ @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 Number of pages is zero.
This function does not take a NumPages parameter but a Length parameter.

(6) Please update the RETURN_INVALID_PARAMETER comment accordingly --
same as in the C file.


+ @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
Please carefully audit the rest of the series for comment blocks. Such
issues render reviews inefficient.

Thanks
Laszlo


Re: [PATCH v4 1/7] IntelSiliconPkg/ReportCpuHobLib: Add ReportCpuHobLib

Ni, Ray
 

+/**

+ Function for Report CPU HOB library
+ This library report the CPU HOB with Physical Address bits.
1. "Build a HOB for the CPU."

+ UINT8 PhysicalAddressBits;

+ CPUID_VIR_PHY_ADDRESS_SIZE_EAX AddressSizeEax;

+

+ AsmCpuid (CPUID_VIR_PHY_ADDRESS_SIZE, &AddressSizeEax.Uint32, NULL, NULL, NULL);

+ if (AddressSizeEax.Uint32 >= CPUID_VIR_PHY_ADDRESS_SIZE) {

+ AsmCpuid (CPUID_VIR_PHY_ADDRESS_SIZE, &AddressSizeEax.Uint32, NULL, NULL, NULL);

+ PhysicalAddressBits = (UINT8) AddressSizeEax.Uint32;
2. PhysicalAddressBits = (UINT8) AddressSizeEax.Bits.PhysicalAddressBits

15121 - 15140 of 89854