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

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

This new helper will not overwrite existing files, by appending a number
to the end of the filename.

Signed-off-by: Marc Moisson-Franckhauser <marc.moisson-franckhauser@arm.com>
Signed-off-by: Joey Gouly <joey.gouly@arm.com>
---
ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiView.h | 25 +++++-
ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiView.c | 80 ++++++++++++++++----
2 files changed, 91 insertions(+), 14 deletions(-)

diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiView.h b/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiView.h
index d5b95f5ee707de18be1879b3cd235d6c5db11d9f..ae8a67b7681033d66d068341ae489ded67de8b44 100644
--- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiView.h
+++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiView.h
@@ -1,12 +1,13 @@
/** @file
Header file for AcpiView

- Copyright (c) 2016 - 2020, ARM Limited. All rights reserved.
+ Copyright (c) 2016 - 2021, Arm Limited. All rights reserved.
SPDX-License-Identifier: BSD-2-Clause-Patent
**/

#ifndef ACPIVIEW_H_
#define ACPIVIEW_H_
+#include <Library/ShellLib.h>

/**
A macro to define the max file name length
@@ -23,6 +24,28 @@
**/
#define RSDP_LENGTH_OFFSET 20

+/**
+ This function finds a filename not already used by adding a number in between
+ the BaseFileName and the extension.
+
+ Make sure the buffer FileName is big enough before calling the function. A
+ size of MAX_FILE_NAME_LEN is recommended.
+
+ @param [in] BaseFileName Start of the desired file name.
+ @param [in] Extension Extension of the desired file name
+ (without '.').
+ @param [in, out] FileName Preallocated buffer for the returned file
+ name.
+ @param [in] FileNameBufferLen Size of FileName buffer..
+**/
+EFI_STATUS
+GetNewFileName (
+ IN CONST CHAR16* BaseFileName,
+ IN CONST CHAR16* Extension,
+ IN OUT CHAR16* FileName,
+ IN UINT32 FileNameBufferLen
+ );
+
/**
This function resets the ACPI table error counter to Zero.
**/
diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiView.c b/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiView.c
index a4242ba9d99b05d07c829520c4011439445aadb0..db7b2e2a30525cc85a333b93f5eb97ec3a517b37 100644
--- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiView.c
+++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiView.c
@@ -27,8 +27,55 @@
#include "Arm/SbbrValidator.h"
#endif

-STATIC UINT32 mTableCount;
-STATIC UINT32 mBinTableCount;
+STATIC UINT32 mTableCount;
+
+/**
+ This function finds a filename not already used by adding a number in between
+ The BaseFileName and the extension.
+
+ Make sure the buffer FileName is big enough before calling the function. A
+ size of MAX_FILE_NAME_LEN is recommended.
+
+ @param [in] BaseFileName Start of the desired file name.
+ @param [in] Extension Extension of the desired file name
+ (without '.').
+ @param [in, out] FileName Preallocated buffer for the returned file
+ name.
+ @param [in] FileNameBufferLen Size of FileName buffer..
+**/
+EFI_STATUS
+GetNewFileName (
+ IN CONST CHAR16* BaseFileName,
+ IN CONST CHAR16* Extension,
+ IN OUT CHAR16* FileName,
+ IN UINT32 FileNameBufferLen
+ )
+{
+ UINT16 Index;
+ EFI_STATUS Status;
+ SHELL_FILE_HANDLE tmpFileHandle;
+ for (Index = 0; Index <= 99; Index++) {
+ UnicodeSPrint(
+ FileName,
+ FileNameBufferLen,
+ L"%s%02d.%s",
+ BaseFileName,
+ Index,
+ Extension
+ );
+ Status = ShellOpenFileByName (
+ FileName,
+ &tmpFileHandle,
+ EFI_FILE_MODE_READ,
+ 0
+ );
+ if (Status == EFI_NOT_FOUND) {
+ return EFI_SUCCESS;
+ }
+ ShellCloseFile (&tmpFileHandle);
+ }
+ return EFI_OUT_OF_RESOURCES;
+}

/**
This function dumps the ACPI table to a file.
@@ -46,19 +93,27 @@ DumpAcpiTableToFile (
IN CONST UINTN Length
)
{
- CHAR16 FileNameBuffer[MAX_FILE_NAME_LEN];
- UINTN TransferBytes;
- SELECTED_ACPI_TABLE *SelectedTable;
+ CHAR16 FileNameBuffer[MAX_FILE_NAME_LEN];
+ UINTN TransferBytes;
+ EFI_STATUS Status;
+ SELECTED_ACPI_TABLE* SelectedTable;

GetSelectedAcpiTable (&SelectedTable);

- UnicodeSPrint (
- FileNameBuffer,
- sizeof (FileNameBuffer),
- L".\\%s%04d.bin",
- SelectedTable->Name,
- mBinTableCount++
- );
+ Status = GetNewFileName (
+ SelectedTable->Name,
+ L"bin",
+ FileNameBuffer,
+ sizeof (FileNameBuffer)
+ );
+ if (EFI_ERROR (Status)) {
+ Print (
+ L"Error: Could not open bin file for %s table:\n"
+ L"Could not get a file name.",
+ SelectedTable->Name
+ );
+ return FALSE;
+ }

Print (L"Dumping ACPI table to : %s ... ", FileNameBuffer);

@@ -207,7 +262,6 @@ AcpiView (

// Reset Table counts
mTableCount = 0;
- mBinTableCount = 0;

// Reset The error/warning counters
ResetErrorCount ();
--
Guid("CE165669-3EF3-493F-B85D-6190EE5B9759")

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

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

This is preparation for adding a second flag to the parsers.

Signed-off-by: Marc Moisson-Franckhauser <marc.moisson-franckhauser@arm.com>
Signed-off-by: Joey Gouly <joey.gouly@arm.com>
---
ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.h | 77 +++++++++++---------
ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiTableParser.h | 6 +-
ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiTableParser.c | 20 ++---
ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiView.c | 28 ++++---
ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Bgrt/BgrtParser.c | 10 +--
ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Dbg2/Dbg2Parser.c | 8 +-
ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Dsdt/DsdtParser.c | 8 +-
ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Facs/FacsParser.c | 10 +--
ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Fadt/FadtParser.c | 27 ++++---
ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Gtdt/GtdtParser.c | 8 +-
ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Iort/IortParser.c | 8 +-
ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Madt/MadtParser.c | 8 +-
ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Mcfg/McfgParser.c | 8 +-
ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Pptt/PpttParser.c | 47 +++++++-----
ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Rsdp/RsdpParser.c | 10 +--
ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Slit/SlitParser.c | 8 +-
ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Spcr/SpcrParser.c | 8 +-
ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Srat/SratParser.c | 8 +-
ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Ssdt/SsdtParser.c | 8 +-
ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Xsdt/XsdtParser.c | 10 +--
20 files changed, 179 insertions(+), 146 deletions(-)

diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.h b/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.h
index 0b7726b9d5807ad2f5c5447408c4c5451718938b..b078c0b99335ba28f7589cac6b0a4190d9a6c3b5 100644
--- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.h
+++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.h
@@ -1,7 +1,7 @@
/** @file
Header file for ACPI parser

- Copyright (c) 2016 - 2020, Arm Limited. All rights reserved.
+ Copyright (c) 2016 - 2021, Arm Limited. All rights reserved.
SPDX-License-Identifier: BSD-2-Clause-Patent
**/

@@ -16,6 +16,16 @@
/// that allows us to process the log options.
#define RSDP_TABLE_INFO SIGNATURE_32('R', 'S', 'D', 'P')

+/**
+ Flags for the parser.
+*/
+#define PARSE_FLAGS_TRACE BIT0
+
+/**
+ Helper macros to test parser flags.
+*/
+#define IS_TRACE_FLAG_SET(Flags) (((Flags) & PARSE_FLAGS_TRACE) != 0)
+
/**
This function increments the ACPI table error counter.
**/
@@ -489,7 +499,7 @@ ParseAcpiAest (

This function also performs validation of the ACPI table fields.

- @param [in] Trace If TRUE, trace the ACPI fields.
+ @param [in] ParseFlags Flags describing what the parser needs to do.
@param [in] Ptr Pointer to the start of the buffer.
@param [in] AcpiTableLength Length of the ACPI table.
@param [in] AcpiTableRevision Revision of the ACPI table.
@@ -497,7 +507,7 @@ ParseAcpiAest (
VOID
EFIAPI
ParseAcpiBgrt (
- IN BOOLEAN Trace,
+ IN UINT8 ParseFlags,
IN UINT8* Ptr,
IN UINT32 AcpiTableLength,
IN UINT8 AcpiTableRevision
@@ -510,7 +520,7 @@ ParseAcpiBgrt (

This function also performs validation of the ACPI table fields.

- @param [in] Trace If TRUE, trace the ACPI fields.
+ @param [in] ParseFlags Flags describing what the parser needs to do.
@param [in] Ptr Pointer to the start of the buffer.
@param [in] AcpiTableLength Length of the ACPI table.
@param [in] AcpiTableRevision Revision of the ACPI table.
@@ -518,7 +528,7 @@ ParseAcpiBgrt (
VOID
EFIAPI
ParseAcpiDbg2 (
- IN BOOLEAN Trace,
+ IN UINT8 ParseFlags,
IN UINT8* Ptr,
IN UINT32 AcpiTableLength,
IN UINT8 AcpiTableRevision
@@ -531,7 +541,7 @@ ParseAcpiDbg2 (
For the DSDT table only the ACPI header fields are parsed and
traced.

- @param [in] Trace If TRUE, trace the ACPI fields.
+ @param [in] ParseFlags Flags describing what the parser needs to do.
@param [in] Ptr Pointer to the start of the buffer.
@param [in] AcpiTableLength Length of the ACPI table.
@param [in] AcpiTableRevision Revision of the ACPI table.
@@ -539,7 +549,7 @@ ParseAcpiDbg2 (
VOID
EFIAPI
ParseAcpiDsdt (
- IN BOOLEAN Trace,
+ IN UINT8 ParseFlags,
IN UINT8* Ptr,
IN UINT32 AcpiTableLength,
IN UINT8 AcpiTableRevision
@@ -552,7 +562,7 @@ ParseAcpiDsdt (

This function also performs validation of the ACPI table fields.

- @param [in] Trace If TRUE, trace the ACPI fields.
+ @param [in] ParseFlags Flags describing what the parser needs to do.
@param [in] Ptr Pointer to the start of the buffer.
@param [in] AcpiTableLength Length of the ACPI table.
@param [in] AcpiTableRevision Revision of the ACPI table.
@@ -560,7 +570,7 @@ ParseAcpiDsdt (
VOID
EFIAPI
ParseAcpiFacs (
- IN BOOLEAN Trace,
+ IN UINT8 ParseFlags,
IN UINT8* Ptr,
IN UINT32 AcpiTableLength,
IN UINT8 AcpiTableRevision
@@ -573,7 +583,7 @@ ParseAcpiFacs (

This function also performs validation of the ACPI table fields.

- @param [in] Trace If TRUE, trace the ACPI fields.
+ @param [in] ParseFlags Flags describing what the parser needs to do.
@param [in] Ptr Pointer to the start of the buffer.
@param [in] AcpiTableLength Length of the ACPI table.
@param [in] AcpiTableRevision Revision of the ACPI table.
@@ -581,7 +591,7 @@ ParseAcpiFacs (
VOID
EFIAPI
ParseAcpiFadt (
- IN BOOLEAN Trace,
+ IN UINT8 ParseFlags,
IN UINT8* Ptr,
IN UINT32 AcpiTableLength,
IN UINT8 AcpiTableRevision
@@ -598,7 +608,7 @@ ParseAcpiFadt (

This function also performs validation of the ACPI table fields.

- @param [in] Trace If TRUE, trace the ACPI fields.
+ @param [in] ParseFlags Flags describing what the parser needs to do.
@param [in] Ptr Pointer to the start of the buffer.
@param [in] AcpiTableLength Length of the ACPI table.
@param [in] AcpiTableRevision Revision of the ACPI table.
@@ -606,7 +616,7 @@ ParseAcpiFadt (
VOID
EFIAPI
ParseAcpiGtdt (
- IN BOOLEAN Trace,
+ IN UINT8 ParseFlags,
IN UINT8* Ptr,
IN UINT32 AcpiTableLength,
IN UINT8 AcpiTableRevision
@@ -653,7 +663,7 @@ ParseAcpiHmat (

This function also performs validation of the ACPI table fields.

- @param [in] Trace If TRUE, trace the ACPI fields.
+ @param [in] ParseFlags Flags describing what the parser needs to do.
@param [in] Ptr Pointer to the start of the buffer.
@param [in] AcpiTableLength Length of the ACPI table.
@param [in] AcpiTableRevision Revision of the ACPI table.
@@ -661,7 +671,7 @@ ParseAcpiHmat (
VOID
EFIAPI
ParseAcpiIort (
- IN BOOLEAN Trace,
+ IN UINT8 ParseFlags,
IN UINT8* Ptr,
IN UINT32 AcpiTableLength,
IN UINT8 AcpiTableRevision
@@ -682,7 +692,7 @@ ParseAcpiIort (

This function also performs validation of the ACPI table fields.

- @param [in] Trace If TRUE, trace the ACPI fields.
+ @param [in] ParseFlags Flags describing what the parser needs to do.
@param [in] Ptr Pointer to the start of the buffer.
@param [in] AcpiTableLength Length of the ACPI table.
@param [in] AcpiTableRevision Revision of the ACPI table.
@@ -690,7 +700,7 @@ ParseAcpiIort (
VOID
EFIAPI
ParseAcpiMadt (
- IN BOOLEAN Trace,
+ IN UINT8 ParseFlags,
IN UINT8* Ptr,
IN UINT32 AcpiTableLength,
IN UINT8 AcpiTableRevision
@@ -703,7 +713,7 @@ ParseAcpiMadt (

This function also performs validation of the ACPI table fields.

- @param [in] Trace If TRUE, trace the ACPI fields.
+ @param [in] ParseFlags Flags describing what the parser needs to do.
@param [in] Ptr Pointer to the start of the buffer.
@param [in] AcpiTableLength Length of the ACPI table.
@param [in] AcpiTableRevision Revision of the ACPI table.
@@ -711,7 +721,7 @@ ParseAcpiMadt (
VOID
EFIAPI
ParseAcpiMcfg (
- IN BOOLEAN Trace,
+ IN UINT8 ParseFlags,
IN UINT8* Ptr,
IN UINT32 AcpiTableLength,
IN UINT8 AcpiTableRevision
@@ -746,7 +756,7 @@ ParseAcpiPcct (

This function also performs validation of the ACPI table fields.

- @param [in] Trace If TRUE, trace the ACPI fields.
+ @param [in] ParseFlags Flags describing what the parser needs to do.
@param [in] Ptr Pointer to the start of the buffer.
@param [in] AcpiTableLength Length of the ACPI table.
@param [in] AcpiTableRevision Revision of the ACPI table.
@@ -754,7 +764,7 @@ ParseAcpiPcct (
VOID
EFIAPI
ParseAcpiPptt (
- IN BOOLEAN Trace,
+ IN UINT8 ParseFlags,
IN UINT8* Ptr,
IN UINT32 AcpiTableLength,
IN UINT8 AcpiTableRevision
@@ -769,7 +779,7 @@ ParseAcpiPptt (
This function also performs a RAW dump of the ACPI table and
validates the checksum.

- @param [in] Trace If TRUE, trace the ACPI fields.
+ @param [in] ParseFlags Flags describing what the parser needs to do.
@param [in] Ptr Pointer to the start of the buffer.
@param [in] AcpiTableLength Length of the ACPI table.
@param [in] AcpiTableRevision Revision of the ACPI table.
@@ -777,7 +787,7 @@ ParseAcpiPptt (
VOID
EFIAPI
ParseAcpiRsdp (
- IN BOOLEAN Trace,
+ IN UINT8 ParseFlags,
IN UINT8* Ptr,
IN UINT32 AcpiTableLength,
IN UINT8 AcpiTableRevision
@@ -793,7 +803,8 @@ ParseAcpiRsdp (
- Relative distance from System Locality at i*N+j is same as
j*N+i

- @param [in] Trace If TRUE, trace the ACPI fields.
+ @param [in] ParseFlags Flags describing what the parser needs to
+ do.
@param [in] Ptr Pointer to the start of the buffer.
@param [in] AcpiTableLength Length of the ACPI table.
@param [in] AcpiTableRevision Revision of the ACPI table.
@@ -801,7 +812,7 @@ ParseAcpiRsdp (
VOID
EFIAPI
ParseAcpiSlit (
- IN BOOLEAN Trace,
+ IN UINT8 ParseFlags,
IN UINT8* Ptr,
IN UINT32 AcpiTableLength,
IN UINT8 AcpiTableRevision
@@ -814,7 +825,7 @@ ParseAcpiSlit (

This function also performs validations of the ACPI table fields.

- @param [in] Trace If TRUE, trace the ACPI fields.
+ @param [in] ParseFlags Flags describing what the parser needs to do.
@param [in] Ptr Pointer to the start of the buffer.
@param [in] AcpiTableLength Length of the ACPI table.
@param [in] AcpiTableRevision Revision of the ACPI table.
@@ -822,7 +833,7 @@ ParseAcpiSlit (
VOID
EFIAPI
ParseAcpiSpcr (
- IN BOOLEAN Trace,
+ IN UINT8 ParseFlags,
IN UINT8* Ptr,
IN UINT32 AcpiTableLength,
IN UINT8 AcpiTableRevision
@@ -841,7 +852,7 @@ ParseAcpiSpcr (

This function also performs validation of the ACPI table fields.

- @param [in] Trace If TRUE, trace the ACPI fields.
+ @param [in] ParseFlags Flags describing what the parser needs to do.
@param [in] Ptr Pointer to the start of the buffer.
@param [in] AcpiTableLength Length of the ACPI table.
@param [in] AcpiTableRevision Revision of the ACPI table.
@@ -849,7 +860,7 @@ ParseAcpiSpcr (
VOID
EFIAPI
ParseAcpiSrat (
- IN BOOLEAN Trace,
+ IN UINT8 ParseFlags,
IN UINT8* Ptr,
IN UINT32 AcpiTableLength,
IN UINT8 AcpiTableRevision
@@ -862,7 +873,7 @@ ParseAcpiSrat (
For the SSDT table only the ACPI header fields are
parsed and traced.

- @param [in] Trace If TRUE, trace the ACPI fields.
+ @param [in] ParseFlags Flags describing what the parser needs to do.
@param [in] Ptr Pointer to the start of the buffer.
@param [in] AcpiTableLength Length of the ACPI table.
@param [in] AcpiTableRevision Revision of the ACPI table.
@@ -870,7 +881,7 @@ ParseAcpiSrat (
VOID
EFIAPI
ParseAcpiSsdt (
- IN BOOLEAN Trace,
+ IN UINT8 ParseFlags,
IN UINT8* Ptr,
IN UINT32 AcpiTableLength,
IN UINT8 AcpiTableRevision
@@ -882,7 +893,7 @@ ParseAcpiSsdt (

This function also performs validation of the XSDT table.

- @param [in] Trace If TRUE, trace the ACPI fields.
+ @param [in] ParseFlags Flags describing what the parser needs to do.
@param [in] Ptr Pointer to the start of the buffer.
@param [in] AcpiTableLength Length of the ACPI table.
@param [in] AcpiTableRevision Revision of the ACPI table.
@@ -890,7 +901,7 @@ ParseAcpiSsdt (
VOID
EFIAPI
ParseAcpiXsdt (
- IN BOOLEAN Trace,
+ IN UINT8 ParseFlags,
IN UINT8* Ptr,
IN UINT32 AcpiTableLength,
IN UINT8 AcpiTableRevision
diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiTableParser.h b/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiTableParser.h
index 0ebf79fb653ae3a8190273aee452723c6213eb58..94ce0a4860e5296d99d398480655a8013ab0f240 100644
--- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiTableParser.h
+++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiTableParser.h
@@ -1,7 +1,7 @@
/** @file
Header file for ACPI table parser

- Copyright (c) 2016 - 2020, Arm Limited. All rights reserved.
+ Copyright (c) 2016 - 2021, Arm Limited. All rights reserved.
SPDX-License-Identifier: BSD-2-Clause-Patent
**/

@@ -20,7 +20,7 @@
/**
A function that parses the ACPI table.

- @param [in] Trace If TRUE, trace the ACPI fields.
+ @param [in] ParseFlags Flags describing what the parser needs to do.
@param [in] Ptr Pointer to the start of the buffer.
@param [in] AcpiTableLength Length of the ACPI table.
@param [in] AcpiTableRevision Revision of the ACPI table.
@@ -28,7 +28,7 @@
typedef
VOID
(EFIAPI * PARSE_ACPI_TABLE_PROC) (
- IN BOOLEAN Trace,
+ IN UINT8 ParseFlags,
IN UINT8* Ptr,
IN UINT32 AcpiTableLength,
IN UINT8 AcpiTableRevision
diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiTableParser.c b/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiTableParser.c
index 4b618f131eac3957f4070a95e06c8cd157c3223c..ecb5d6339af37397c6ba1ba4c8f0d42a95811bf6 100644
--- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiTableParser.c
+++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiTableParser.c
@@ -1,7 +1,7 @@
/** @file
ACPI table parser

- Copyright (c) 2016 - 2020, ARM Limited. All rights reserved.
+ Copyright (c) 2016 - 2021, Arm Limited. All rights reserved.
SPDX-License-Identifier: BSD-2-Clause-Patent

@par Glossary:
@@ -183,7 +183,7 @@ ProcessAcpiTable (
)
{
EFI_STATUS Status;
- BOOLEAN Trace;
+ UINT8 ParseFlags;
CONST UINT32* AcpiTableSignature;
CONST UINT32* AcpiTableLength;
CONST UINT8* AcpiTableRevision;
@@ -197,13 +197,13 @@ ProcessAcpiTable (
&AcpiTableRevision
);

- Trace = ProcessTableReportOptions (
- *AcpiTableSignature,
- Ptr,
- *AcpiTableLength
- );
+ ParseFlags = ProcessTableReportOptions (
+ *AcpiTableSignature,
+ Ptr,
+ *AcpiTableLength
+ );

- if (Trace) {
+ if (IS_TRACE_FLAG_SET (ParseFlags)) {
DumpRaw (Ptr, *AcpiTableLength);

// Do not process the ACPI table any further if the table length read
@@ -236,14 +236,14 @@ ProcessAcpiTable (
Status = GetParser (*AcpiTableSignature, &ParserProc);
if (EFI_ERROR (Status)) {
// No registered parser found, do default handling.
- if (Trace) {
+ if (IS_TRACE_FLAG_SET (ParseFlags)) {
DumpAcpiHeader (Ptr);
}
return;
}

ParserProc (
- Trace,
+ ParseFlags,
Ptr,
*AcpiTableLength,
*AcpiTableRevision
diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiView.c b/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiView.c
index 9a5b013fb234e2a09a12a690607b5b871dffde72..a4242ba9d99b05d07c829520c4011439445aadb0 100644
--- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiView.c
+++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiView.c
@@ -1,6 +1,6 @@
/** @file

- Copyright (c) 2016 - 2020, ARM Limited. All rights reserved.
+ Copyright (c) 2016 - 2021, Arm Limited. All rights reserved.
SPDX-License-Identifier: BSD-2-Clause-Patent

@par Glossary:
@@ -73,9 +73,9 @@ DumpAcpiTableToFile (
@param [in] TablePtr Pointer to the ACPI table data.
@param [in] Length The length fo the ACPI table.

- @retval Returns TRUE if the ACPI table should be traced.
+ @retval Returns the ParseFlags for the ACPI table.
**/
-BOOLEAN
+UINT8
ProcessTableReportOptions (
IN CONST UINT32 Signature,
IN CONST UINT8* TablePtr,
@@ -84,7 +84,7 @@ ProcessTableReportOptions (
{
UINTN OriginalAttribute;
UINT8 *SignaturePtr;
- BOOLEAN Log;
+ UINT8 ParseFlags;
BOOLEAN HighLight;
SELECTED_ACPI_TABLE *SelectedTable;

@@ -93,17 +93,17 @@ ProcessTableReportOptions (
//
OriginalAttribute = 0;
SignaturePtr = (UINT8*)(UINTN)&Signature;
- Log = FALSE;
+ ParseFlags = 0;
HighLight = GetColourHighlighting ();
GetSelectedAcpiTable (&SelectedTable);

switch (GetReportOption ()) {
case ReportAll:
- Log = TRUE;
+ ParseFlags |= PARSE_FLAGS_TRACE;
break;
case ReportSelected:
if (Signature == SelectedTable->Type) {
- Log = TRUE;
+ ParseFlags |= PARSE_FLAGS_TRACE;
SelectedTable->Found = TRUE;
}
break;
@@ -143,7 +143,7 @@ ProcessTableReportOptions (
break;
} // switch

- if (Log) {
+ if (IS_TRACE_FLAG_SET (ParseFlags)) {
if (HighLight) {
OriginalAttribute = gST->ConOut->Mode->Attribute;
gST->ConOut->SetAttribute (
@@ -164,7 +164,7 @@ ProcessTableReportOptions (
}
}

- return Log;
+ return ParseFlags;
}


@@ -196,7 +196,7 @@ AcpiView (
UINT32 RsdpLength;
UINT8 RsdpRevision;
PARSE_ACPI_TABLE_PROC RsdpParserProc;
- BOOLEAN Trace;
+ UINT8 ParseFlags;
SELECTED_ACPI_TABLE *SelectedTable;

//
@@ -249,7 +249,11 @@ AcpiView (
// The RSDP length is 4 bytes starting at offset 20
RsdpLength = *(UINT32*)(RsdpPtr + RSDP_LENGTH_OFFSET);

- Trace = ProcessTableReportOptions (RSDP_TABLE_INFO, RsdpPtr, RsdpLength);
+ ParseFlags = ProcessTableReportOptions (
+ RSDP_TABLE_INFO,
+ RsdpPtr,
+ RsdpLength
+ );

Status = GetParser (RSDP_TABLE_INFO, &RsdpParserProc);
if (EFI_ERROR (Status)) {
@@ -260,7 +264,7 @@ AcpiView (
}

RsdpParserProc (
- Trace,
+ ParseFlags,
RsdpPtr,
RsdpLength,
RsdpRevision
diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Bgrt/BgrtParser.c b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Bgrt/BgrtParser.c
index 1a180271a4ebe47948b7f0b56d1cb6f81b5fdf13..6bd4c35bec85e038875ce3d9548c9139b1de2755 100644
--- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Bgrt/BgrtParser.c
+++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Bgrt/BgrtParser.c
@@ -1,7 +1,7 @@
/** @file
BGRT table parser

- Copyright (c) 2017 - 2018, ARM Limited. All rights reserved.
+ Copyright (c) 2017 - 2021, Arm Limited. All rights reserved.
SPDX-License-Identifier: BSD-2-Clause-Patent

@par Reference(s):
@@ -36,7 +36,7 @@ STATIC CONST ACPI_PARSER BgrtParser[] = {

This function also performs validation of the ACPI table fields.

- @param [in] Trace If TRUE, trace the ACPI fields.
+ @param [in] ParseFlags Flags describing what the parser needs to do.
@param [in] Ptr Pointer to the start of the buffer.
@param [in] AcpiTableLength Length of the ACPI table.
@param [in] AcpiTableRevision Revision of the ACPI table.
@@ -44,18 +44,18 @@ STATIC CONST ACPI_PARSER BgrtParser[] = {
VOID
EFIAPI
ParseAcpiBgrt (
- IN BOOLEAN Trace,
+ IN UINT8 ParseFlags,
IN UINT8* Ptr,
IN UINT32 AcpiTableLength,
IN UINT8 AcpiTableRevision
)
{
- if (!Trace) {
+ if (!IS_TRACE_FLAG_SET (ParseFlags)) {
return;
}

ParseAcpi (
- Trace,
+ TRUE,
0,
"BGRT",
Ptr,
diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Dbg2/Dbg2Parser.c b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Dbg2/Dbg2Parser.c
index 9df111ecaa7d7a703a13a39c243ed78b9f12ee97..a22f8ab76b84432b2073bea6ca6d8245ea9df7fb 100644
--- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Dbg2/Dbg2Parser.c
+++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Dbg2/Dbg2Parser.c
@@ -1,7 +1,7 @@
/** @file
DBG2 table parser

- Copyright (c) 2016 - 2020, ARM Limited. All rights reserved.
+ Copyright (c) 2016 - 2021, Arm Limited. All rights reserved.
SPDX-License-Identifier: BSD-2-Clause-Patent

@par Reference(s):
@@ -212,7 +212,7 @@ DumpDbgDeviceInfo (

This function also performs validation of the ACPI table fields.

- @param [in] Trace If TRUE, trace the ACPI fields.
+ @param [in] ParseFlags Flags describing what the parser needs to do.
@param [in] Ptr Pointer to the start of the buffer.
@param [in] AcpiTableLength Length of the ACPI table.
@param [in] AcpiTableRevision Revision of the ACPI table.
@@ -220,7 +220,7 @@ DumpDbgDeviceInfo (
VOID
EFIAPI
ParseAcpiDbg2 (
- IN BOOLEAN Trace,
+ IN UINT8 ParseFlags,
IN UINT8* Ptr,
IN UINT32 AcpiTableLength,
IN UINT8 AcpiTableRevision
@@ -229,7 +229,7 @@ ParseAcpiDbg2 (
UINT32 Offset;
UINT32 Index;

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

diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Dsdt/DsdtParser.c b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Dsdt/DsdtParser.c
index 6d43974f54d23fd9990fa7af721c96518bb96a36..609c17ef02d460be66f0c390203cae55ece653e3 100644
--- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Dsdt/DsdtParser.c
+++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Dsdt/DsdtParser.c
@@ -1,7 +1,7 @@
/** @file
DSDT table parser

- Copyright (c) 2016 - 2018, ARM Limited. All rights reserved.
+ Copyright (c) 2016 - 2021, Arm Limited. All rights reserved.
SPDX-License-Identifier: BSD-2-Clause-Patent

@par Reference(s):
@@ -20,7 +20,7 @@
For the DSDT table only the ACPI header fields are parsed and
traced.

- @param [in] Trace If TRUE, trace the ACPI fields.
+ @param [in] ParseFlags Flags describing what the parser needs to do.
@param [in] Ptr Pointer to the start of the buffer.
@param [in] AcpiTableLength Length of the ACPI table.
@param [in] AcpiTableRevision Revision of the ACPI table.
@@ -28,13 +28,13 @@
VOID
EFIAPI
ParseAcpiDsdt (
- IN BOOLEAN Trace,
+ IN UINT8 ParseFlags,
IN UINT8* Ptr,
IN UINT32 AcpiTableLength,
IN UINT8 AcpiTableRevision
)
{
- if (!Trace) {
+ if (!IS_TRACE_FLAG_SET (ParseFlags)) {
return;
}

diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Facs/FacsParser.c b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Facs/FacsParser.c
index d6bea86bdbaa79aa35b86840c809394b3c7a3bf6..d7545b6161eadd24e986a7828910662f2f52b2ec 100644
--- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Facs/FacsParser.c
+++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Facs/FacsParser.c
@@ -1,7 +1,7 @@
/** @file
FACS table parser

- Copyright (c) 2019, ARM Limited. All rights reserved.
+ Copyright (c) 2019 - 2021, Arm Limited. All rights reserved.
SPDX-License-Identifier: BSD-2-Clause-Patent

@par Reference(s):
@@ -42,7 +42,7 @@ STATIC CONST ACPI_PARSER FacsParser[] = {

This function also performs validation of the ACPI table fields.

- @param [in] Trace If TRUE, trace the ACPI fields.
+ @param [in] ParseFlags Flags describing what the parser needs to do.
@param [in] Ptr Pointer to the start of the buffer.
@param [in] AcpiTableLength Length of the ACPI table.
@param [in] AcpiTableRevision Revision of the ACPI table.
@@ -50,18 +50,18 @@ STATIC CONST ACPI_PARSER FacsParser[] = {
VOID
EFIAPI
ParseAcpiFacs (
- IN BOOLEAN Trace,
+ IN UINT8 ParseFlags,
IN UINT8* Ptr,
IN UINT32 AcpiTableLength,
IN UINT8 AcpiTableRevision
)
{
- if (!Trace) {
+ if (!IS_TRACE_FLAG_SET (ParseFlags)) {
return;
}

ParseAcpi (
- Trace,
+ TRUE,
0,
"FACS",
Ptr,
diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Fadt/FadtParser.c b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Fadt/FadtParser.c
index d86718bab67d45fd612bb7ac725b5eb3eeb7dfdc..8d0eb42ec3b361c1727184c542a757e39ef3da5c 100644
--- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Fadt/FadtParser.c
+++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Fadt/FadtParser.c
@@ -1,7 +1,7 @@
/** @file
FADT table parser

- Copyright (c) 2016 - 2020, ARM Limited. All rights reserved.
+ Copyright (c) 2016 - 2021, Arm Limited. All rights reserved.
SPDX-License-Identifier: BSD-2-Clause-Patent

@par Reference(s):
@@ -198,7 +198,7 @@ STATIC CONST ACPI_PARSER FadtParser[] = {

This function also performs validation of the ACPI table fields.

- @param [in] Trace If TRUE, trace the ACPI fields.
+ @param [in] ParseFlags Flags describing what the parser needs to do.
@param [in] Ptr Pointer to the start of the buffer.
@param [in] AcpiTableLength Length of the ACPI table.
@param [in] AcpiTableRevision Revision of the ACPI table.
@@ -206,7 +206,7 @@ STATIC CONST ACPI_PARSER FadtParser[] = {
VOID
EFIAPI
ParseAcpiFadt (
- IN BOOLEAN Trace,
+ IN UINT8 ParseFlags,
IN UINT8* Ptr,
IN UINT32 AcpiTableLength,
IN UINT8 AcpiTableRevision
@@ -219,6 +219,9 @@ ParseAcpiFadt (
UINT32 FacsLength;
UINT8 FacsRevision;
PARSE_ACPI_TABLE_PROC FacsParserProc;
+ BOOLEAN Trace;
+
+ Trace = IS_TRACE_FLAG_SET (ParseFlags);

ParseAcpi (
Trace,
@@ -253,7 +256,7 @@ ParseAcpiFadt (
// if HW_REDUCED_ACPI flag is not set, both FIRMWARE_CTRL and
// X_FIRMWARE_CTRL cannot be zero, and the FACS Table must be
// present.
- if ((Trace) &&
+ if (Trace &&
(Flags != NULL) &&
((*Flags & EFI_ACPI_6_3_HW_REDUCED_ACPI) != EFI_ACPI_6_3_HW_REDUCED_ACPI)) {
IncrementErrorCount ();
@@ -274,11 +277,15 @@ ParseAcpiFadt (
// The FACS version is 1 byte starting at offset 32.
FacsRevision = *(UINT8*)(FirmwareCtrlPtr + FACS_VERSION_OFFSET);

- Trace = ProcessTableReportOptions (
- FacsSignature,
- FirmwareCtrlPtr,
- FacsLength
- );
+ if (ProcessTableReportOptions (
+ FacsSignature,
+ FirmwareCtrlPtr,
+ FacsLength
+ )) {
+ ParseFlags |= PARSE_FLAGS_TRACE;
+ } else {
+ ParseFlags &= ~PARSE_FLAGS_TRACE;
+ }

Status = GetParser (FacsSignature, &FacsParserProc);
if (EFI_ERROR (Status)) {
@@ -289,7 +296,7 @@ ParseAcpiFadt (
}

FacsParserProc (
- Trace,
+ ParseFlags,
FirmwareCtrlPtr,
FacsLength,
FacsRevision
diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Gtdt/GtdtParser.c b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Gtdt/GtdtParser.c
index d02fc4929d6fa5e04672276810b19d3f4c62efd2..da93938771bfcf0da146fd46a025addeb05e71cc 100644
--- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Gtdt/GtdtParser.c
+++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Gtdt/GtdtParser.c
@@ -1,7 +1,7 @@
/** @file
GTDT table parser

- Copyright (c) 2016 - 2020, ARM Limited. All rights reserved.
+ Copyright (c) 2016 - 2021, Arm Limited. All rights reserved.
SPDX-License-Identifier: BSD-2-Clause-Patent

@par Reference(s):
@@ -254,7 +254,7 @@ DumpWatchdogTimer (

This function also performs validation of the ACPI table fields.

- @param [in] Trace If TRUE, trace the ACPI fields.
+ @param [in] ParseFlags Flags describing what the parser needs to do.
@param [in] Ptr Pointer to the start of the buffer.
@param [in] AcpiTableLength Length of the ACPI table.
@param [in] AcpiTableRevision Revision of the ACPI table.
@@ -262,7 +262,7 @@ DumpWatchdogTimer (
VOID
EFIAPI
ParseAcpiGtdt (
- IN BOOLEAN Trace,
+ IN UINT8 ParseFlags,
IN UINT8* Ptr,
IN UINT32 AcpiTableLength,
IN UINT8 AcpiTableRevision
@@ -272,7 +272,7 @@ ParseAcpiGtdt (
UINT32 Offset;
UINT8* TimerPtr;

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

diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Iort/IortParser.c b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Iort/IortParser.c
index f7447947b2308d35d4d2890373778f0fd2f97f9e..2f659349499a02175820ee4faf3a84034c8ced76 100644
--- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Iort/IortParser.c
+++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Iort/IortParser.c
@@ -1,7 +1,7 @@
/** @file
IORT table parser

- Copyright (c) 2016 - 2020, ARM Limited. All rights reserved.
+ Copyright (c) 2016 - 2021, Arm Limited. All rights reserved.
SPDX-License-Identifier: BSD-2-Clause-Patent

@par Reference(s):
@@ -612,7 +612,7 @@ DumpIortNodePmcg (

This function also performs validation of the ACPI table fields.

- @param [in] Trace If TRUE, trace the ACPI fields.
+ @param [in] ParseFlags Flags describing what the parser needs to do.
@param [in] Ptr Pointer to the start of the buffer.
@param [in] AcpiTableLength Length of the ACPI table.
@param [in] AcpiTableRevision Revision of the ACPI table.
@@ -620,7 +620,7 @@ DumpIortNodePmcg (
VOID
EFIAPI
ParseAcpiIort (
- IN BOOLEAN Trace,
+ IN UINT8 ParseFlags,
IN UINT8* Ptr,
IN UINT32 AcpiTableLength,
IN UINT8 AcpiTableRevision
@@ -630,7 +630,7 @@ ParseAcpiIort (
UINT32 Index;
UINT8* NodePtr;

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

diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Madt/MadtParser.c b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Madt/MadtParser.c
index 15aa2392b60cee9e3843c7c560b0ab84e0be4174..a29bf97a3985a74ff888f225eb0e5cfbcbea72b0 100644
--- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Madt/MadtParser.c
+++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Madt/MadtParser.c
@@ -1,7 +1,7 @@
/** @file
MADT table parser

- Copyright (c) 2016 - 2020, ARM Limited. All rights reserved.
+ Copyright (c) 2016 - 2021, Arm Limited. All rights reserved.
SPDX-License-Identifier: BSD-2-Clause-Patent

@par Reference(s):
@@ -216,7 +216,7 @@ STATIC CONST ACPI_PARSER MadtInterruptControllerHeaderParser[] = {

This function also performs validation of the ACPI table fields.

- @param [in] Trace If TRUE, trace the ACPI fields.
+ @param [in] ParseFlags Flags describing what the parser needs to do.
@param [in] Ptr Pointer to the start of the buffer.
@param [in] AcpiTableLength Length of the ACPI table.
@param [in] AcpiTableRevision Revision of the ACPI table.
@@ -224,7 +224,7 @@ STATIC CONST ACPI_PARSER MadtInterruptControllerHeaderParser[] = {
VOID
EFIAPI
ParseAcpiMadt (
- IN BOOLEAN Trace,
+ IN UINT8 ParseFlags,
IN UINT8* Ptr,
IN UINT32 AcpiTableLength,
IN UINT8 AcpiTableRevision
@@ -236,7 +236,7 @@ ParseAcpiMadt (

GICDCount = 0;

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

diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Mcfg/McfgParser.c b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Mcfg/McfgParser.c
index 9da4d60e849721ed3b635bfff8a3bd76728c0ade..febf8a2bd92d8e38bdc59a3f97a8c4485cdba5a5 100644
--- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Mcfg/McfgParser.c
+++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Mcfg/McfgParser.c
@@ -1,7 +1,7 @@
/** @file
MCFG table parser

- Copyright (c) 2016 - 2018, ARM Limited. All rights reserved.
+ Copyright (c) 2016 - 2021, Arm Limited. All rights reserved.
SPDX-License-Identifier: BSD-2-Clause-Patent

@par Reference(s):
@@ -42,7 +42,7 @@ STATIC CONST ACPI_PARSER PciCfgSpaceBaseAddrParser[] = {

This function also performs validation of the ACPI table fields.

- @param [in] Trace If TRUE, trace the ACPI fields.
+ @param [in] ParseFlags Flags describing what the parser needs to do.
@param [in] Ptr Pointer to the start of the buffer.
@param [in] AcpiTableLength Length of the ACPI table.
@param [in] AcpiTableRevision Revision of the ACPI table.
@@ -50,7 +50,7 @@ STATIC CONST ACPI_PARSER PciCfgSpaceBaseAddrParser[] = {
VOID
EFIAPI
ParseAcpiMcfg (
- IN BOOLEAN Trace,
+ IN UINT8 ParseFlags,
IN UINT8* Ptr,
IN UINT32 AcpiTableLength,
IN UINT8 AcpiTableRevision
@@ -60,7 +60,7 @@ ParseAcpiMcfg (
UINT32 PciCfgOffset;
UINT8* PciCfgSpacePtr;

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

diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Pptt/PpttParser.c b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Pptt/PpttParser.c
index acd2b81bb3258c7322aa10d2c0e0d842d89e358b..538b6a69350d75ccbf36b86fff115255e77437c7 100644
--- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Pptt/PpttParser.c
+++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Pptt/PpttParser.c
@@ -1,7 +1,7 @@
/** @file
PPTT table parser

- Copyright (c) 2019 - 2020, ARM Limited. All rights reserved.
+ Copyright (c) 2019 - 2021, Arm Limited. All rights reserved.
SPDX-License-Identifier: BSD-2-Clause-Patent

@par Reference(s):
@@ -241,13 +241,15 @@ STATIC CONST ACPI_PARSER IdStructureParser[] = {
/**
This function parses the Processor Hierarchy Node Structure (Type 0).

- @param [in] Ptr Pointer to the start of the Processor Hierarchy Node
- Structure data.
- @param [in] Length Length of the Processor Hierarchy Node Structure.
+ @param [in] ParseFlags Flags describing what the parser needs to do.
+ @param [in] Ptr Pointer to the start of the Processor Hierarchy Node
+ Structure data.
+ @param [in] Length Length of the Processor Hierarchy Node Structure.
**/
STATIC
VOID
DumpProcessorHierarchyNodeStructure (
+ IN UINT8 ParseFlags,
IN UINT8* Ptr,
IN UINT8 Length
)
@@ -257,7 +259,7 @@ DumpProcessorHierarchyNodeStructure (
CHAR16 Buffer[OUTPUT_FIELD_COLUMN_WIDTH];

Offset = ParseAcpi (
- TRUE,
+ IS_TRACE_FLAG_SET (ParseFlags),
2,
"Processor Hierarchy Node Structure",
Ptr,
@@ -315,18 +317,20 @@ DumpProcessorHierarchyNodeStructure (
/**
This function parses the Cache Type Structure (Type 1).

- @param [in] Ptr Pointer to the start of the Cache Type Structure data.
- @param [in] Length Length of the Cache Type Structure.
+ @param [in] ParseFlags Flags describing what the parser needs to do.
+ @param [in] Ptr Pointer to the start of the Cache Type Structure data.
+ @param [in] Length Length of the Cache Type Structure.
**/
STATIC
VOID
DumpCacheTypeStructure (
+ IN UINT8 ParseFlags,
IN UINT8* Ptr,
IN UINT8 Length
)
{
ParseAcpi (
- TRUE,
+ IS_TRACE_FLAG_SET (ParseFlags),
2,
"Cache Type Structure",
Ptr,
@@ -338,18 +342,20 @@ DumpCacheTypeStructure (
/**
This function parses the ID Structure (Type 2).

- @param [in] Ptr Pointer to the start of the ID Structure data.
- @param [in] Length Length of the ID Structure.
+ @param [in] ParseFlags Flags describing what the parser needs to do.
+ @param [in] Ptr Pointer to the start of the ID Structure data.
+ @param [in] Length Length of the ID Structure.
**/
STATIC
VOID
DumpIDStructure (
+ IN UINT8 ParseFlags,
IN UINT8* Ptr,
- IN UINT8 Length
+ IN UINT8 Length
)
{
ParseAcpi (
- TRUE,
+ IS_TRACE_FLAG_SET (ParseFlags),
2,
"ID Structure",
Ptr,
@@ -370,7 +376,7 @@ DumpIDStructure (

This function also performs validation of the ACPI table fields.

- @param [in] Trace If TRUE, trace the ACPI fields.
+ @param [in] ParseFlags Flags describing what the parser needs to do.
@param [in] Ptr Pointer to the start of the buffer.
@param [in] AcpiTableLength Length of the ACPI table.
@param [in] AcpiTableRevision Revision of the ACPI table.
@@ -378,7 +384,7 @@ DumpIDStructure (
VOID
EFIAPI
ParseAcpiPptt (
- IN BOOLEAN Trace,
+ IN UINT8 ParseFlags,
IN UINT8* Ptr,
IN UINT32 AcpiTableLength,
IN UINT8 AcpiTableRevision
@@ -387,12 +393,12 @@ ParseAcpiPptt (
UINT32 Offset;
UINT8* ProcessorTopologyStructurePtr;

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

Offset = ParseAcpi (
- TRUE,
+ IS_TRACE_FLAG_SET (ParseFlags),
0,
"PPTT",
Ptr,
@@ -440,24 +446,29 @@ ParseAcpiPptt (
return;
}

- PrintFieldName (2, L"* Structure Offset *");
- Print (L"0x%x\n", Offset);
+ if (IS_TRACE_FLAG_SET (ParseFlags)) {
+ PrintFieldName (2, L"* Structure Offset *");
+ Print (L"0x%x\n", Offset);
+ }

switch (*ProcessorTopologyStructureType) {
case EFI_ACPI_6_2_PPTT_TYPE_PROCESSOR:
DumpProcessorHierarchyNodeStructure (
+ ParseFlags,
ProcessorTopologyStructurePtr,
*ProcessorTopologyStructureLength
);
break;
case EFI_ACPI_6_2_PPTT_TYPE_CACHE:
DumpCacheTypeStructure (
+ ParseFlags,
ProcessorTopologyStructurePtr,
*ProcessorTopologyStructureLength
);
break;
case EFI_ACPI_6_2_PPTT_TYPE_ID:
DumpIDStructure (
+ ParseFlags,
ProcessorTopologyStructurePtr,
*ProcessorTopologyStructureLength
);
diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Rsdp/RsdpParser.c b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Rsdp/RsdpParser.c
index f4a8732a7db7c437031f2a3d2f266b80eff17b4b..c98bc62719c3aef3381152ff2436cb1a29f98362 100644
--- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Rsdp/RsdpParser.c
+++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Rsdp/RsdpParser.c
@@ -1,7 +1,7 @@
/** @file
RSDP table parser

- Copyright (c) 2016 - 2019, ARM Limited. All rights reserved.
+ Copyright (c) 2016 - 2021, Arm Limited. All rights reserved.
SPDX-License-Identifier: BSD-2-Clause-Patent

@par Reference(s):
@@ -110,7 +110,7 @@ STATIC CONST ACPI_PARSER RsdpParser[] = {
This function also performs a RAW dump of the ACPI table and
validates the checksum.

- @param [in] Trace If TRUE, trace the ACPI fields.
+ @param [in] ParseFlags Flags describing what the parser needs to do.
@param [in] Ptr Pointer to the start of the buffer.
@param [in] AcpiTableLength Length of the ACPI table.
@param [in] AcpiTableRevision Revision of the ACPI table.
@@ -118,19 +118,19 @@ STATIC CONST ACPI_PARSER RsdpParser[] = {
VOID
EFIAPI
ParseAcpiRsdp (
- IN BOOLEAN Trace,
+ IN UINT8 ParseFlags,
IN UINT8* Ptr,
IN UINT32 AcpiTableLength,
IN UINT8 AcpiTableRevision
)
{
- if (Trace) {
+ if (IS_TRACE_FLAG_SET (ParseFlags)) {
DumpRaw (Ptr, AcpiTableLength);
VerifyChecksum (TRUE, Ptr, AcpiTableLength);
}

ParseAcpi (
- Trace,
+ IS_TRACE_FLAG_SET (ParseFlags),
0,
"RSDP",
Ptr,
diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Slit/SlitParser.c b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Slit/SlitParser.c
index e4625ee8b13907893a9b6990ecb956baf91cc3b9..20d2fb79a8ea6251b0e31b5370cc400be9e1478f 100644
--- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Slit/SlitParser.c
+++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Slit/SlitParser.c
@@ -1,7 +1,7 @@
/** @file
SLIT table parser

- Copyright (c) 2016 - 2019, ARM Limited. All rights reserved.
+ Copyright (c) 2016 - 2021, Arm Limited. All rights reserved.
SPDX-License-Identifier: BSD-2-Clause-Patent

@par Reference(s):
@@ -42,7 +42,7 @@ STATIC CONST ACPI_PARSER SlitParser[] = {
- Relative distance from System Locality at i*N+j is same as
j*N+i

- @param [in] Trace If TRUE, trace the ACPI fields.
+ @param [in] ParseFlags Flags describing what the parser needs to do.
@param [in] Ptr Pointer to the start of the buffer.
@param [in] AcpiTableLength Length of the ACPI table.
@param [in] AcpiTableRevision Revision of the ACPI table.
@@ -50,7 +50,7 @@ STATIC CONST ACPI_PARSER SlitParser[] = {
VOID
EFIAPI
ParseAcpiSlit (
- IN BOOLEAN Trace,
+ IN UINT8 ParseFlags,
IN UINT8* Ptr,
IN UINT32 AcpiTableLength,
IN UINT8 AcpiTableRevision
@@ -63,7 +63,7 @@ ParseAcpiSlit (
UINT8* LocalityPtr;
CHAR16 Buffer[80]; // Used for AsciiName param of ParseAcpi

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

diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Spcr/SpcrParser.c b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Spcr/SpcrParser.c
index 3b06b05dee8c056c6e009b9e485ccd35d4194e95..99cffafa946979b79a06cd28f14257beaa6cafec 100644
--- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Spcr/SpcrParser.c
+++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Spcr/SpcrParser.c
@@ -1,7 +1,7 @@
/** @file
SPCR table parser

- Copyright (c) 2016 - 2019, ARM Limited. All rights reserved.
+ Copyright (c) 2016 - 2021, Arm Limited. All rights reserved.
SPDX-License-Identifier: BSD-2-Clause-Patent

@par Reference(s):
@@ -114,7 +114,7 @@ STATIC CONST ACPI_PARSER SpcrParser[] = {

This function also performs validations of the ACPI table fields.

- @param [in] Trace If TRUE, trace the ACPI fields.
+ @param [in] ParseFlags Flags describing what the parser needs to do.
@param [in] Ptr Pointer to the start of the buffer.
@param [in] AcpiTableLength Length of the ACPI table.
@param [in] AcpiTableRevision Revision of the ACPI table.
@@ -122,13 +122,13 @@ STATIC CONST ACPI_PARSER SpcrParser[] = {
VOID
EFIAPI
ParseAcpiSpcr (
- IN BOOLEAN Trace,
+ IN UINT8 ParseFlags,
IN UINT8* Ptr,
IN UINT32 AcpiTableLength,
IN UINT8 AcpiTableRevision
)
{
- if (!Trace) {
+ if (!IS_TRACE_FLAG_SET (ParseFlags)) {
return;
}

diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Srat/SratParser.c b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Srat/SratParser.c
index b9b67820b89f7fc5560a1022e976663db7d9df2d..907856368fbe0ad9f140d8f27e51bd9460f35b1a 100644
--- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Srat/SratParser.c
+++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Srat/SratParser.c
@@ -1,7 +1,7 @@
/** @file
SRAT table parser

- Copyright (c) 2016 - 2020, ARM Limited. All rights reserved.
+ Copyright (c) 2016 - 2021, Arm Limited. All rights reserved.
SPDX-License-Identifier: BSD-2-Clause-Patent

@par Reference(s):
@@ -344,7 +344,7 @@ STATIC CONST ACPI_PARSER SratX2ApciAffinityParser[] = {

This function also performs validation of the ACPI table fields.

- @param [in] Trace If TRUE, trace the ACPI fields.
+ @param [in] ParseFlags Flags describing what the parser needs to do.
@param [in] Ptr Pointer to the start of the buffer.
@param [in] AcpiTableLength Length of the ACPI table.
@param [in] AcpiTableRevision Revision of the ACPI table.
@@ -352,7 +352,7 @@ STATIC CONST ACPI_PARSER SratX2ApciAffinityParser[] = {
VOID
EFIAPI
ParseAcpiSrat (
- IN BOOLEAN Trace,
+ IN UINT8 ParseFlags,
IN UINT8* Ptr,
IN UINT32 AcpiTableLength,
IN UINT8 AcpiTableRevision
@@ -375,7 +375,7 @@ ParseAcpiSrat (
ApicSapicAffinityIndex = 0;
X2ApicAffinityIndex = 0;

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

diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Ssdt/SsdtParser.c b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Ssdt/SsdtParser.c
index f18664b8a6879b82dc9c55d9979ca21e01c4fc99..138a3159fb1a6121387f832621d23960c688ce30 100644
--- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Ssdt/SsdtParser.c
+++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Ssdt/SsdtParser.c
@@ -1,7 +1,7 @@
/** @file
SSDT table parser

- Copyright (c) 2016 - 2018, ARM Limited. All rights reserved.
+ Copyright (c) 2016 - 2021, Arm Limited. All rights reserved.
SPDX-License-Identifier: BSD-2-Clause-Patent

@par Reference(s):
@@ -20,7 +20,7 @@
For the SSDT table only the ACPI header fields are
parsed and traced.

- @param [in] Trace If TRUE, trace the ACPI fields.
+ @param [in] ParseFlags Flags describing what the parser needs to do.
@param [in] Ptr Pointer to the start of the buffer.
@param [in] AcpiTableLength Length of the ACPI table.
@param [in] AcpiTableRevision Revision of the ACPI table.
@@ -28,13 +28,13 @@
VOID
EFIAPI
ParseAcpiSsdt (
- IN BOOLEAN Trace,
+ IN UINT8 ParseFlags,
IN UINT8* Ptr,
IN UINT32 AcpiTableLength,
IN UINT8 AcpiTableRevision
)
{
- if (!Trace) {
+ if (!IS_TRACE_FLAG_SET (ParseFlags)) {
return;
}

diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Xsdt/XsdtParser.c b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Xsdt/XsdtParser.c
index e39061f8e2612f2cce4aebf51a511b63b703662b..bbd58d72ad9c676b4843d3b4f1d7c554a5abbd3f 100644
--- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Xsdt/XsdtParser.c
+++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Xsdt/XsdtParser.c
@@ -1,7 +1,7 @@
/** @file
XSDT table parser

- Copyright (c) 2016 - 2019, ARM Limited. All rights reserved.
+ Copyright (c) 2016 - 2021, Arm Limited. All rights reserved.
SPDX-License-Identifier: BSD-2-Clause-Patent

@par Reference(s):
@@ -40,7 +40,7 @@ GetAcpiXsdtHeaderInfo (

This function also performs validation of the XSDT table.

- @param [in] Trace If TRUE, trace the ACPI fields.
+ @param [in] ParseFlags Flags describing what the parser needs to do.
@param [in] Ptr Pointer to the start of the buffer.
@param [in] AcpiTableLength Length of the ACPI table.
@param [in] AcpiTableRevision Revision of the ACPI table.
@@ -48,7 +48,7 @@ GetAcpiXsdtHeaderInfo (
VOID
EFIAPI
ParseAcpiXsdt (
- IN BOOLEAN Trace,
+ IN UINT8 ParseFlags,
IN UINT8* Ptr,
IN UINT32 AcpiTableLength,
IN UINT8 AcpiTableRevision
@@ -61,7 +61,7 @@ ParseAcpiXsdt (
CHAR16 Buffer[32];

Offset = ParseAcpi (
- Trace,
+ IS_TRACE_FLAG_SET (ParseFlags),
0,
"XSDT",
Ptr,
@@ -71,7 +71,7 @@ ParseAcpiXsdt (

TableOffset = Offset;

- if (Trace) {
+ if (IS_TRACE_FLAG_SET (ParseFlags)) {
EntryIndex = 0;
TablePointer = (UINT64*)(Ptr + TableOffset);
while (Offset < AcpiTableLength) {
--
Guid("CE165669-3EF3-493F-B85D-6190EE5B9759")

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

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

This option informs the parser to generate a dot graph of a table.
This can be useful to understand or debug a table, such as the PPTT
table.

Signed-off-by: Marc Moisson-Franckhauser <marc.moisson-franckhauser@arm.com>
Signed-off-by: Joey Gouly <joey.gouly@arm.com>
---
ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.h | 2 ++
ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiViewConfig.h | 3 ++-
ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiView.c | 9 +++++++-
ShellPkg/Library/UefiShellAcpiViewCommandLib/UefiShellAcpiViewCommandLib.c | 24 +++++++++++++++++---
ShellPkg/Library/UefiShellAcpiViewCommandLib/UefiShellAcpiViewCommandLib.uni | 9 ++++++--
5 files changed, 40 insertions(+), 7 deletions(-)

diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.h b/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.h
index b078c0b99335ba28f7589cac6b0a4190d9a6c3b5..9a67fe084327434bf21b37b3089779468edfb0f1 100644
--- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.h
+++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.h
@@ -20,11 +20,13 @@
Flags for the parser.
*/
#define PARSE_FLAGS_TRACE BIT0
+#define PARSE_FLAGS_GRAPH BIT1

/**
Helper macros to test parser flags.
*/
#define IS_TRACE_FLAG_SET(Flags) (((Flags) & PARSE_FLAGS_TRACE) != 0)
+#define IS_GRAPH_FLAG_SET(Flags) (((Flags) & PARSE_FLAGS_GRAPH) != 0)

/**
This function increments the ACPI table error counter.
diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiViewConfig.h b/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiViewConfig.h
index 2db4a65415d8f9e70686cb2cc432862ab4e4c2dd..262302a15cbbe04a228fd55e523930fb76bcf6a8 100644
--- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiViewConfig.h
+++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiViewConfig.h
@@ -1,7 +1,7 @@
/** @file
Header file for 'acpiview' configuration.

- Copyright (c) 2016 - 2020, ARM Limited. All rights reserved.<BR>
+ Copyright (c) 2016 - 2021, Arm Limited. All rights reserved.<BR>
SPDX-License-Identifier: BSD-2-Clause-Patent
**/

@@ -106,6 +106,7 @@ typedef enum {
ReportSelected, ///< Report Selected table.
ReportTableList, ///< Report List of tables.
ReportDumpBinFile, ///< Dump selected table to a file.
+ ReportDotGraph, ///< Create Dot Graph for selected compatible table.
ReportMax,
} EREPORT_OPTION;

diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiView.c b/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiView.c
index db7b2e2a30525cc85a333b93f5eb97ec3a517b37..1155b2f3f411247c866f635fb666dd76455f18a4 100644
--- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiView.c
+++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiView.c
@@ -192,6 +192,12 @@ ProcessTableReportOptions (
DumpAcpiTableToFile (TablePtr, Length);
}
break;
+ case ReportDotGraph:
+ if (Signature == SelectedTable->Type) {
+ SelectedTable->Found = TRUE;
+ ParseFlags |= PARSE_FLAGS_GRAPH;
+ }
+ break;
case ReportMax:
// We should never be here.
// This case is only present to prevent compiler warning.
@@ -340,7 +346,8 @@ AcpiView (

ReportOption = GetReportOption ();
if (ReportTableList != ReportOption) {
- if (((ReportSelected == ReportOption) ||
+ if (((ReportSelected == ReportOption) ||
+ (ReportDotGraph == ReportOption) ||
(ReportDumpBinFile == ReportOption)) &&
(!SelectedTable->Found)) {
Print (L"\nRequested ACPI Table not found.\n");
diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/UefiShellAcpiViewCommandLib.c b/ShellPkg/Library/UefiShellAcpiViewCommandLib/UefiShellAcpiViewCommandLib.c
index b30ed3fc8597b229dd15b6ad4f2aab2e3d0ca583..d837b390938f3c3bc5cb90c1161e2feeb1ed6a6b 100644
--- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/UefiShellAcpiViewCommandLib.c
+++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/UefiShellAcpiViewCommandLib.c
@@ -1,7 +1,7 @@
/** @file
Main file for 'acpiview' Shell command function.

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

@@ -34,6 +34,7 @@ EFI_HII_HANDLE gShellAcpiViewHiiHandle = NULL;
STATIC CONST SHELL_PARAM_ITEM ParamList[] = {
{L"-q", TypeFlag},
{L"-d", TypeFlag},
+ {L"-g", TypeFlag},
{L"-h", TypeFlag},
{L"-l", TypeFlag},
{L"-s", TypeValue},
@@ -293,6 +294,18 @@ ShellCommandRunAcpiView (
L"-d"
);
ShellStatus = SHELL_INVALID_PARAMETER;
+ } else if (ShellCommandLineGetFlag (Package, L"-g") &&
+ !ShellCommandLineGetFlag (Package, L"-s")) {
+ ShellPrintHiiEx (
+ -1,
+ -1,
+ NULL,
+ STRING_TOKEN (STR_GEN_MISSING_OPTION),
+ gShellAcpiViewHiiHandle,
+ L"acpiview",
+ L"-s",
+ L"-g"
+ );
} else {
// Turn on colour highlighting if requested
SetColourHighlighting (ShellCommandLineGetFlag (Package, L"-h"));
@@ -316,10 +329,15 @@ ShellCommandRunAcpiView (
SelectAcpiTable (SelectedTableName);
SetReportOption (ReportSelected);

- if (ShellCommandLineGetFlag (Package, L"-d")) {
+ if (ShellCommandLineGetFlag (Package, L"-d") ||
+ ShellCommandLineGetFlag (Package, L"-g")) {
// Create a temporary file to check if the media is writable.
CHAR16 FileNameBuffer[MAX_FILE_NAME_LEN];
- SetReportOption (ReportDumpBinFile);
+ if (ShellCommandLineGetFlag (Package, L"-d")) {
+ SetReportOption (ReportDumpBinFile);
+ } else {
+ SetReportOption (ReportDotGraph);
+ }

UnicodeSPrint (
FileNameBuffer,
diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/UefiShellAcpiViewCommandLib.uni b/ShellPkg/Library/UefiShellAcpiViewCommandLib/UefiShellAcpiViewCommandLib.uni
index 393110e0ee98d54b3be0309c2d297a121c258570..51f2bea10f7b768e5e67f930237207193cba4246 100644
--- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/UefiShellAcpiViewCommandLib.uni
+++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/UefiShellAcpiViewCommandLib.uni
@@ -1,6 +1,6 @@
// /**
//
-// Copyright (c) 2016 - 2020, Arm Limited. All rights reserved.<BR>
+// Copyright (c) 2016 - 2021, Arm Limited. All rights reserved.<BR>
// SPDX-License-Identifier: BSD-2-Clause-Patent
//
// Module Name:
@@ -30,7 +30,7 @@
"Display ACPI Table information.\r\n"
".SH SYNOPSIS\r\n"
" \r\n"
-"ACPIVIEW [[-?] | [[[[-l] | [-s AcpiTable [-d]]] [-q] [-h]] [-r Spec]]]\r\n"
+"ACPIVIEW [[-?] | [[[[-l] | [-s AcpiTable [-d] [-g]]] [-q] [-h]] [-r Spec]]]\r\n"
" \r\n"
".SH OPTIONS\r\n"
" \r\n"
@@ -39,6 +39,7 @@
" invocation option.\r\n"
" AcpiTable : The required ACPI Table type.\r\n"
" -d - Generate a binary file dump of the specified AcpiTable.\r\n"
+" -g - Generate a dot graph of the specified AcpiTable.\r\n"
" -q - Quiet. Suppress errors and warnings. Disables consistency checks.\r\n"
" -h - Enable colour highlighting.\r\n"
" -r - Validate that all required ACPI tables are installed\r\n"
@@ -123,6 +124,10 @@
" in the current working directory:\r\n"
" fs0:\> acpiview -s DSDT -d\r\n"
" \r\n"
+" * To save a dot graph in the current working directory\r\n"
+" representing the processor architecture described in the PPTT table:\r\n"
+" fs0:\> acpiview -s PPTT -g\r\n"
+" \r\n"
" * To display contents of all ACPI tables:\r\n"
" fs0:\> acpiview\r\n"
" \r\n"
--
Guid("CE165669-3EF3-493F-B85D-6190EE5B9759")

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

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

These can be used to generate dot files, that can be used to visualise
graphs in tables, such as PPTT.

Signed-off-by: Marc Moisson-Franckhauser <marc.moisson-franckhauser@arm.com>
Signed-off-by: Joey Gouly <joey.gouly@arm.com>
---
ShellPkg/Library/UefiShellAcpiViewCommandLib/UefiShellAcpiViewCommandLib.inf | 4 +-
ShellPkg/Library/UefiShellAcpiViewCommandLib/DotGenerator.h | 97 +++++++
ShellPkg/Library/UefiShellAcpiViewCommandLib/DotGenerator.c | 276 ++++++++++++++++++++
3 files changed, 376 insertions(+), 1 deletion(-)

diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/UefiShellAcpiViewCommandLib.inf b/ShellPkg/Library/UefiShellAcpiViewCommandLib/UefiShellAcpiViewCommandLib.inf
index 63fc5a1281a894841dac704484c3d4f9481edb46..ffe4979b3ac5d0120bcf678cf7823afac6674e4f 100644
--- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/UefiShellAcpiViewCommandLib.inf
+++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/UefiShellAcpiViewCommandLib.inf
@@ -1,7 +1,7 @@
## @file
# Provides Shell 'acpiview' command functions
#
-# Copyright (c) 2016 - 2020, Arm Limited. All rights reserved.<BR>
+# Copyright (c) 2016 - 2021, Arm Limited. All rights reserved.<BR>
#
# SPDX-License-Identifier: BSD-2-Clause-Patent
#
@@ -27,6 +27,8 @@ [Sources.common]
AcpiView.h
AcpiViewConfig.c
AcpiViewConfig.h
+ DotGenerator.c
+ DotGenerator.h
Parsers/Aest/AestParser.c
Parsers/Bgrt/BgrtParser.c
Parsers/Dbg2/Dbg2Parser.c
diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/DotGenerator.h b/ShellPkg/Library/UefiShellAcpiViewCommandLib/DotGenerator.h
new file mode 100644
index 0000000000000000000000000000000000000000..7a8e2fdc2d2024ae54fdce963d751ab53a8a5758
--- /dev/null
+++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/DotGenerator.h
@@ -0,0 +1,97 @@
+/** @file
+ Header file for Dot File Generation
+
+ Copyright (c) 2021, Arm Limited. All rights reserved.
+ SPDX-License-Identifier: BSD-2-Clause-Patent
+**/
+
+#ifndef DOT_GENERATOR_H_
+#define DOT_GENERATOR_H_
+
+#include <Protocol/Shell.h>
+
+#define DOT_COLOR_MASK 0b111
+// Flags for color of arrow or node.
+#define DOT_COLOR_BLACK 0b000 // default
+#define DOT_COLOR_GRAY 0b001
+#define DOT_COLOR_BLUE 0b010
+#define DOT_COLOR_YELLOW 0b011
+#define DOT_COLOR_RED 0b100
+
+#define DOT_ARROW_TYPE_MASK 0b1000
+// Flags for style of arrow.
+#define DOT_ARROW_FULL 0b0000 // default
+#define DOT_ARROW_DOTTED 0b1000
+
+// Flag for reversing how the nodes will be ranked and displayed.
+#define DOT_ARROW_RANK_REVERSE 0b10000
+
+#define DOT_BOX_TYPE_MASK 0b1100000
+// Flag for shape of box
+#define DOT_BOX_SQUARE 0b0000000 // default
+#define DOT_BOX_DIAMOND 0b0100000
+
+// Flag for adding the node's ID to the end of the label.
+#define DOT_BOX_ADD_ID_TO_LABEL 0b10000000
+
+// Valid flags for DotAddNode.
+#define DOT_BOX_FLAGS_MASK (DOT_COLOR_MASK |\
+ DOT_BOX_TYPE_MASK |\
+ DOT_BOX_ADD_ID_TO_LABEL)
+// Valid flags for DotAddLink.
+#define DOT_ARROW_FLAGS_MASK (DOT_COLOR_MASK |\
+ DOT_ARROW_TYPE_MASK |\
+ DOT_ARROW_RANK_REVERSE)
+
+
+/**
+ Opens a new dot file and writes a dot directional graph.
+
+ @param [in] FileName Null terminated unicode string.
+**/
+SHELL_FILE_HANDLE
+DotOpenNewFile (
+ IN CHAR16* FileName
+ );
+
+/**
+ Writes a dot graph footer and closes the dot file.
+**/
+VOID
+DotCloseFile (
+ SHELL_FILE_HANDLE DotFile
+ );
+
+/**
+ Writes a line in the previously opened dot file describing a
+ new node.
+
+ @param [in] Id A unique identifier for the node.
+ @param [in] Flags Flags describing the node's characteristics.
+ @param [in] Label Label to be shown on the graph node.
+**/
+VOID
+DotAddNode (
+ SHELL_FILE_HANDLE DotFile,
+ IN UINT32 Id,
+ IN UINT16 Flags,
+ IN CONST CHAR16* Label
+ );
+
+/**
+ Writes a line in the previously opened dot file describing a
+ new link between two nodes.
+
+ @param [in] IdSource An identifier for the source node of the link.
+ @param [in] IdTarget An identifier for the target node of the link.
+ @param [in] Flags Flags describing the node's characteristics.
+**/
+VOID
+DotAddLink (
+ SHELL_FILE_HANDLE DotFile,
+ IN UINT32 IdSource,
+ IN UINT32 IdTarget,
+ IN UINT16 Flags
+ );
+
+#endif // DOT_GENERATOR_H_
diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/DotGenerator.c b/ShellPkg/Library/UefiShellAcpiViewCommandLib/DotGenerator.c
new file mode 100644
index 0000000000000000000000000000000000000000..8c644d96af30ca4b1160bc892fe391ae1ff5211b
--- /dev/null
+++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/DotGenerator.c
@@ -0,0 +1,276 @@
+/** @file
+ Dot File Generator
+
+ Copyright (c) 2021, Arm Limited. All rights reserved.
+ SPDX-License-Identifier: BSD-2-Clause-Patent
+**/
+
+#include <Library/BaseLib.h>
+#include <Library/UefiLib.h>
+#include <Library/DebugLib.h>
+#include <Library/PrintLib.h>
+#include "DotGenerator.h"
+#include "AcpiView.h"
+
+#define MAX_DOT_BUFFER_SIZE 128
+
+/**
+ Writes a Null terminated ASCII string to the dot file handle.
+
+ @param [in] String Null terminated ascii string.
+**/
+STATIC
+VOID
+DotWriteFile (
+ SHELL_FILE_HANDLE DotFileHandle,
+ IN CHAR8* String
+ )
+{
+ UINTN TransferBytes;
+ EFI_STATUS Status;
+
+ if (DotFileHandle == NULL) {
+ Print ("ERROR: Failed to write to dot file\n");
+ ASSERT (0);
+ return;
+ }
+
+ TransferBytes = AsciiStrLen (String);
+ Status = ShellWriteFile (
+ DotFileHandle,
+ &TransferBytes,
+ String
+ );
+ ASSERT_EFI_ERROR (Status);
+ ASSERT (AsciiStrLen (String) == TransferBytes);
+}
+
+/**
+ Writes a new parameter to a previously started parameter list.
+
+ @param [in] Name Null terminated string of the parameter's name.
+ @param [in] Value Null terminated string of the parameter's value.
+ @param [in] Quoted True if value needs to be quoted.
+**/
+STATIC
+VOID
+DotAddParameter (
+ SHELL_FILE_HANDLE DotFileHandle,
+ IN CHAR16* Name,
+ IN CHAR16* Value,
+ IN BOOLEAN Quoted
+ )
+{
+ CHAR8 StringBuffer[MAX_DOT_BUFFER_SIZE];
+
+ ASSERT(DotFileHandle != NULL);
+
+ if (Quoted) {
+ AsciiSPrint (
+ StringBuffer,
+ sizeof (StringBuffer),
+ "[%s=\"%s\"]",
+ Name,
+ Value
+ );
+ } else {
+ AsciiSPrint (
+ StringBuffer,
+ sizeof (StringBuffer),
+ "[%s=%s]",
+ Name,
+ Value
+ );
+ }
+
+ DotWriteFile (DotFileHandle, StringBuffer);
+}
+
+/**
+ Writes the color argument of nodes or links according to flags.
+
+ @param [in] Flags Flags describing the color (one of DOT_COLOR_...)
+**/
+STATIC
+VOID
+WriteColor (
+ SHELL_FILE_HANDLE DotFileHandle,
+ IN UINT16 Flags
+ )
+{
+ ASSERT(DotFileHandle != NULL);
+
+ switch (Flags & DOT_COLOR_MASK) {
+ case DOT_COLOR_GRAY:
+ DotAddParameter (DotFileHandle, L"color", L"gray", FALSE);
+ break;
+ case DOT_COLOR_YELLOW:
+ DotAddParameter (DotFileHandle, L"color", L"yellow", FALSE);
+ break;
+ case DOT_COLOR_BLUE:
+ DotAddParameter (DotFileHandle, L"color", L"blue", FALSE);
+ break;
+ case DOT_COLOR_RED:
+ DotAddParameter (DotFileHandle, L"color", L"red", FALSE);
+ break;
+ case DOT_COLOR_BLACK:
+ default:
+ DotAddParameter (DotFileHandle, L"color", L"black", FALSE);
+ break;
+ }
+}
+
+/**
+ Opens a new dot file and writes a dot directional graph.
+
+ @param [in] FileName Null terminated unicode string.
+**/
+SHELL_FILE_HANDLE
+DotOpenNewFile (
+ IN CHAR16* FileName
+ )
+{
+ SHELL_FILE_HANDLE DotFileHandle;
+ EFI_STATUS Status;
+
+ Status = ShellOpenFileByName (
+ FileName,
+ &DotFileHandle,
+ EFI_FILE_MODE_READ | EFI_FILE_MODE_WRITE | EFI_FILE_MODE_CREATE,
+ 0
+ );
+ if (EFI_ERROR (Status)) {
+ Print (L"ERROR: Couldn't open dot file");
+ return NULL;//Status;
+ }
+ Print (L"Creating DOT Graph in : %s... ", FileName);
+ DotWriteFile (DotFileHandle, "digraph {\n\trankdir=BT\n");
+ return DotFileHandle;
+}
+
+/**
+ Writes a dot graph footer and closes the dot file.
+**/
+VOID
+DotCloseFile (
+ SHELL_FILE_HANDLE DotFileHandle
+ )
+{
+ ASSERT(DotFileHandle != NULL);
+
+ DotWriteFile (DotFileHandle, "}\n");
+ ShellCloseFile (&DotFileHandle);
+ Print (L"Done.\n");
+}
+
+/**
+ Writes a line in the previously opened dot file describing a
+ new node.
+
+ @param [in] Id A unique identifier for the node.
+ @param [in] Flags Flags describing the node's characteristics.
+ @param [in] Label Label to be shown on the graph node.
+**/
+VOID
+DotAddNode (
+ SHELL_FILE_HANDLE DotFileHandle,
+ IN UINT32 Id,
+ IN UINT16 Flags,
+ IN CONST CHAR16* Label
+ )
+{
+ CHAR8 LineBuffer[64];
+ CHAR16 LabelBuffer[MAX_DOT_BUFFER_SIZE];
+
+ ASSERT ((Flags & ~DOT_BOX_FLAGS_MASK) == 0);
+ ASSERT(DotFileHandle != NULL);
+
+ AsciiSPrint (
+ LineBuffer,
+ sizeof (LineBuffer),
+ "\tx%x",
+ Id
+ );
+ DotWriteFile (DotFileHandle, LineBuffer);
+
+ switch (Flags & DOT_BOX_TYPE_MASK) {
+ case DOT_BOX_DIAMOND:
+ DotAddParameter (DotFileHandle, L"shape", L"diamond", FALSE);
+ break;
+ case DOT_BOX_SQUARE:
+ default:
+ DotAddParameter (DotFileHandle, L"shape", L"box", FALSE);
+ break;
+ }
+
+ if (Label != NULL) {
+ if ((Flags & DOT_BOX_ADD_ID_TO_LABEL) != 0) {
+ UnicodeSPrint (
+ LabelBuffer,
+ sizeof (LabelBuffer),
+ L"%s\\n0x%x",
+ Label,
+ Id
+ );
+ } else {
+ UnicodeSPrint (
+ LabelBuffer,
+ sizeof (LabelBuffer),
+ L"%s",
+ Label
+ );
+ }
+ DotAddParameter (DotFileHandle, L"label", LabelBuffer, TRUE);
+ }
+
+ WriteColor (DotFileHandle, Flags);
+ DotWriteFile (DotFileHandle, "\n");
+}
+
+/**
+ Writes a line in the previously opened dot file describing a
+ new link between two nodes.
+
+ @param [in] IdSource An identifier for the source node of the link.
+ @param [in] IdTarget An identifier for the target node of the link.
+ @param [in] Flags Flags describing the node's characteristics.
+**/
+VOID
+DotAddLink (
+ SHELL_FILE_HANDLE DotFileHandle,
+ IN UINT32 IdSource,
+ IN UINT32 IdTarget,
+ IN UINT16 Flags
+ )
+{
+ CHAR8 LineBuffer[64];
+
+ ASSERT(DotFileHandle != NULL);
+ ASSERT ((Flags & ~DOT_ARROW_FLAGS_MASK) == 0);
+
+ AsciiSPrint (
+ LineBuffer,
+ sizeof (LineBuffer),
+ "\tx%x -> x%x",
+ IdSource,
+ IdTarget
+ );
+ DotWriteFile (DotFileHandle, LineBuffer);
+
+ if ((Flags & DOT_ARROW_RANK_REVERSE) != 0) {
+ DotAddParameter (DotFileHandle, L"dir", L"back", FALSE);
+ }
+
+ switch (Flags & DOT_ARROW_TYPE_MASK) {
+ case DOT_ARROW_DOTTED:
+ DotAddParameter (DotFileHandle, L"style", L"dotted", FALSE);
+ break;
+ case DOT_ARROW_FULL:
+ default:
+ DotAddParameter (DotFileHandle, L"style", L"solid", FALSE);
+ break;
+ }
+
+ WriteColor (DotFileHandle, Flags);
+ DotWriteFile (DotFileHandle, "\n");
+}
--
Guid("CE165669-3EF3-493F-B85D-6190EE5B9759")

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")

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")

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

________________________________________________________________________________

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

________________________________________________________________________________

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

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

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

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

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

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

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

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

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.
//

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.
//

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);
}

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