|
[PATCH v1 0/5] Dot graph generator for PPTT
Joey Gouly
This series adds functionality to print a dot graph of a PPTT table.
This helps with understanding and debugging PPTT tables. The dot graph generator functionality is generic and could be used by other tables that would benefit from graph output. Bugzilla: 3378 (https://bugzilla.tianocore.org/show_bug.cgi?id=3378) The changes can be seen at https://github.com/jgouly/edk2/tree/1484_pptt_dot_graph_v1 Marc Moisson-Franckhauser (5): ShellPkg: Replace 'Trace' parameter with 'ParseFlags' ShellPkg: add a helper function for getting a new file name ShellPkg: add a Graph option to the Parser Flags ShellPkg: add dot file generator functions ShellPkg: add PPTT dot file genration .../UefiShellAcpiViewCommandLib.inf | 4 +- .../UefiShellAcpiViewCommandLib/AcpiParser.h | 79 +++-- .../AcpiTableParser.h | 6 +- .../UefiShellAcpiViewCommandLib/AcpiView.h | 25 +- .../AcpiViewConfig.h | 3 +- .../DotGenerator.h | 97 ++++++ .../AcpiTableParser.c | 20 +- .../UefiShellAcpiViewCommandLib/AcpiView.c | 117 +++++-- .../DotGenerator.c | 276 +++++++++++++++++ .../Parsers/Bgrt/BgrtParser.c | 10 +- .../Parsers/Dbg2/Dbg2Parser.c | 8 +- .../Parsers/Dsdt/DsdtParser.c | 8 +- .../Parsers/Facs/FacsParser.c | 10 +- .../Parsers/Fadt/FadtParser.c | 27 +- .../Parsers/Gtdt/GtdtParser.c | 8 +- .../Parsers/Iort/IortParser.c | 8 +- .../Parsers/Madt/MadtParser.c | 8 +- .../Parsers/Mcfg/McfgParser.c | 8 +- .../Parsers/Pptt/PpttParser.c | 285 +++++++++++++++--- .../Parsers/Rsdp/RsdpParser.c | 10 +- .../Parsers/Slit/SlitParser.c | 8 +- .../Parsers/Spcr/SpcrParser.c | 8 +- .../Parsers/Srat/SratParser.c | 8 +- .../Parsers/Ssdt/SsdtParser.c | 8 +- .../Parsers/Xsdt/XsdtParser.c | 10 +- .../UefiShellAcpiViewCommandLib.c | 24 +- .../UefiShellAcpiViewCommandLib.uni | 9 +- 27 files changed, 903 insertions(+), 189 deletions(-) create mode 100644 ShellPkg/Library/UefiShellAcpiViewCommandLib/DotGenerator.h create mode 100644 ShellPkg/Library/UefiShellAcpiViewCommandLib/DotGenerator.c -- Guid("CE165669-3EF3-493F-B85D-6190EE5B9759")
|
|
|
|
[PATCH v1 5/5] ShellPkg: add PPTT dot file genration
Joey Gouly
From: Marc Moisson-Franckhauser <marc.moisson-franckhauser@arm.com>
Bugzilla: 3378 (https://bugzilla.tianocore.org/show_bug.cgi?id=3378) This generates a dot file from the PPTT table that can be used to visualise the topology of the CPUs and Caches. Signed-off-by: Marc Moisson-Franckhauser <marc.moisson-franckhauser@arm.com> Signed-off-by: Joey Gouly <joey.gouly@arm.com> --- ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Pptt/PpttParser.c | 240 ++++++++++++++++++-- 1 file changed, 218 insertions(+), 22 deletions(-) diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Pptt/PpttParser.c b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Pptt/PpttParser.c index 538b6a69350d75ccbf36b86fff115255e77437c7..b52bd532b846b9cec0ca315b043beff95df40bd5 100644 --- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Pptt/PpttParser.c +++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Pptt/PpttParser.c @@ -15,11 +15,23 @@ #include "AcpiView.h" #include "AcpiViewConfig.h" #include "PpttParser.h" +#include "DotGenerator.h" // Local variables STATIC CONST UINT8* ProcessorTopologyStructureType; STATIC CONST UINT8* ProcessorTopologyStructureLength; + STATIC CONST UINT32* NumberOfPrivateResources; +STATIC CONST UINT32* ProcessorHierarchyParent; +STATIC CONST EFI_ACPI_6_3_PPTT_STRUCTURE_PROCESSOR_FLAGS* ProcStructFlags; +STATIC CONST UINT32* NextLevelOfCache; +STATIC CONST EFI_ACPI_6_3_PPTT_STRUCTURE_CACHE_ATTRIBUTES* CacheAttributes; +STATIC CONST UINT32* CacheSize; + +STATIC CONST UINT8* PpttStartPointer; + +STATIC SHELL_FILE_HANDLE mDotFileHandle; + STATIC ACPI_DESCRIPTION_HEADER_INFO AcpiHdrInfo; /** @@ -198,8 +210,9 @@ STATIC CONST ACPI_PARSER ProcessorHierarchyNodeStructureParser[] = { {L"Length", 1, 1, L"%d", NULL, NULL, NULL, NULL}, {L"Reserved", 2, 2, L"0x%x", NULL, NULL, NULL, NULL}, - {L"Flags", 4, 4, L"0x%x", NULL, NULL, NULL, NULL}, - {L"Parent", 4, 8, L"0x%x", NULL, NULL, NULL, NULL}, + {L"Flags", 4, 4, L"0x%x", NULL, (VOID**)&ProcStructFlags, NULL, NULL}, + {L"Parent", 4, 8, L"0x%x", NULL, + (VOID**)&ProcessorHierarchyParent, NULL, NULL}, {L"ACPI Processor ID", 4, 12, L"0x%x", NULL, NULL, NULL, NULL}, {L"Number of private resources", 4, 16, L"%d", NULL, (VOID**)&NumberOfPrivateResources, NULL, NULL} @@ -214,11 +227,13 @@ STATIC CONST ACPI_PARSER CacheTypeStructureParser[] = { {L"Reserved", 2, 2, L"0x%x", NULL, NULL, NULL, NULL}, {L"Flags", 4, 4, L"0x%x", NULL, NULL, NULL, NULL}, - {L"Next Level of Cache", 4, 8, L"0x%x", NULL, NULL, NULL, NULL}, - {L"Size", 4, 12, L"0x%x", NULL, NULL, NULL, NULL}, + {L"Next Level of Cache", 4, 8, L"0x%x", NULL, + (VOID**)&NextLevelOfCache, NULL, NULL}, + {L"Size", 4, 12, L"0x%x", NULL, (VOID**)&CacheSize, NULL, NULL}, {L"Number of sets", 4, 16, L"%d", NULL, NULL, ValidateCacheNumberOfSets, NULL}, {L"Associativity", 1, 20, L"%d", NULL, NULL, ValidateCacheAssociativity, NULL}, - {L"Attributes", 1, 21, L"0x%x", NULL, NULL, ValidateCacheAttributes, NULL}, + {L"Attributes", 1, 21, L"0x%x", NULL, (VOID**)&CacheAttributes, + ValidateCacheAttributes, NULL}, {L"Line size", 2, 22, L"%d", NULL, NULL, ValidateCacheLineSize, NULL} }; @@ -257,6 +272,7 @@ DumpProcessorHierarchyNodeStructure ( UINT32 Offset; UINT32 Index; CHAR16 Buffer[OUTPUT_FIELD_COLUMN_WIDTH]; + CONST UINT8* TypePtr; Offset = ParseAcpi ( IS_TRACE_FLAG_SET (ParseFlags), @@ -291,26 +307,67 @@ DumpProcessorHierarchyNodeStructure ( return; } - Index = 0; + if (IS_GRAPH_FLAG_SET (ParseFlags)) { + if (ProcStructFlags->ProcessorIsAThread) { + UnicodeSPrint(Buffer, sizeof (Buffer), L"Thread"); + } else if (ProcStructFlags->NodeIsALeaf) { + UnicodeSPrint(Buffer, sizeof (Buffer), L"Core"); + } else if (ProcStructFlags->PhysicalPackage) { + UnicodeSPrint(Buffer, sizeof (Buffer), L"Physical\\nPackage"); + } else { + UnicodeSPrint(Buffer, sizeof (Buffer), L"Cluster"); + } + + DotAddNode ( + mDotFileHandle, + (UINT32)(Ptr - PpttStartPointer), + DOT_BOX_SQUARE | DOT_COLOR_BLUE | DOT_BOX_ADD_ID_TO_LABEL, + Buffer + ); + + // Add link to parent node. + if (*ProcessorHierarchyParent != 0) { + DotAddLink ( + mDotFileHandle, + (UINT32)(Ptr - PpttStartPointer), + *ProcessorHierarchyParent, + 0x0 + ); + } + } // Parse the specified number of private resource references or the Processor // Hierarchy Node length. Whichever is minimum. - while (Index < *NumberOfPrivateResources) { - UnicodeSPrint ( - Buffer, - sizeof (Buffer), - L"Private resources [%d]", - Index - ); + for (Index = 0; Index < *NumberOfPrivateResources; Index++) { + if (IS_TRACE_FLAG_SET (ParseFlags)) { + UnicodeSPrint ( + Buffer, + sizeof (Buffer), + L"Private resources [%d]", + Index + ); - PrintFieldName (4, Buffer); - Print ( - L"0x%x\n", - *((UINT32*)(Ptr + Offset)) - ); + PrintFieldName (4, Buffer); + Print ( + L"0x%x\n", + *((UINT32*)(Ptr + Offset)) + ); + } + + if (IS_GRAPH_FLAG_SET (ParseFlags)) { + TypePtr = PpttStartPointer + *((UINT32*)(Ptr + Offset)); + if (*TypePtr == EFI_ACPI_6_2_PPTT_TYPE_ID) { + continue; + } + DotAddLink ( + mDotFileHandle, + *((UINT32*)(Ptr + Offset)), + (UINT32)(Ptr - PpttStartPointer), + DOT_ARROW_RANK_REVERSE + ); + } Offset += sizeof (UINT32); - Index++; } } @@ -329,6 +386,8 @@ DumpCacheTypeStructure ( IN UINT8 Length ) { + CHAR16 LabelBuffer[64]; + ParseAcpi ( IS_TRACE_FLAG_SET (ParseFlags), 2, @@ -337,6 +396,88 @@ DumpCacheTypeStructure ( Length, PARSER_PARAMS (CacheTypeStructureParser) ); + + if (IS_GRAPH_FLAG_SET (ParseFlags)) { + // Create cache node + + // Start node label with type of cache + switch (CacheAttributes->CacheType) { + case EFI_ACPI_6_3_CACHE_ATTRIBUTES_CACHE_TYPE_DATA: + UnicodeSPrint ( + LabelBuffer, + sizeof (LabelBuffer), + L"D-Cache\\n" + ); + break; + case EFI_ACPI_6_3_CACHE_ATTRIBUTES_CACHE_TYPE_INSTRUCTION: + UnicodeSPrint ( + LabelBuffer, + sizeof (LabelBuffer), + L"I-Cache\\n" + ); + break; + default: + UnicodeSPrint ( + LabelBuffer, + sizeof (LabelBuffer), + L"Unified Cache\\n" + ); + } + + // Add size of cache to node label + if (((*CacheSize) & 0xfff00000) != 0) { + UnicodeSPrint ( + LabelBuffer, + sizeof (LabelBuffer), + L"%s%dMiB", + LabelBuffer, + *CacheSize >> 20 + ); + } + if ((*CacheSize & 0xffc00) != 0) { + UnicodeSPrint ( + LabelBuffer, + sizeof (LabelBuffer), + L"%s%dkiB", + LabelBuffer, + (*CacheSize >> 10) & 0x3ff + ); + } + if ((*CacheSize & 0x3ff) != 0) { + UnicodeSPrint ( + LabelBuffer, + sizeof (LabelBuffer), + L"%s%dB", + LabelBuffer, + *CacheSize & 0x3ff + ); + } + if (*CacheSize == 0) { + UnicodeSPrint ( + LabelBuffer, + sizeof (LabelBuffer), + L"%s0B", + LabelBuffer + ); + } + + //Add node to dot file + DotAddNode ( + mDotFileHandle, + (UINT32)(Ptr - PpttStartPointer), + DOT_BOX_SQUARE | DOT_COLOR_YELLOW | DOT_BOX_ADD_ID_TO_LABEL, + LabelBuffer + ); + + if (*NextLevelOfCache != 0) { + DotAddLink ( + mDotFileHandle, + *NextLevelOfCache, + (UINT32)(Ptr - PpttStartPointer), + DOT_ARROW_RANK_REVERSE | DOT_COLOR_GRAY + ); + } + } } /** @@ -390,13 +531,46 @@ ParseAcpiPptt ( IN UINT8 AcpiTableRevision ) { - UINT32 Offset; - UINT8* ProcessorTopologyStructurePtr; + EFI_STATUS Status; + UINT32 Offset; + UINT8* ProcessorTopologyStructurePtr; + CHAR16 Buffer[128]; + CHAR16 FileNameBuffer[MAX_FILE_NAME_LEN]; - if (!IS_TRACE_FLAG_SET (ParseFlags)) { + if (!IS_TRACE_FLAG_SET (ParseFlags) && + !IS_GRAPH_FLAG_SET (ParseFlags)) { return; } + if (IS_GRAPH_FLAG_SET (ParseFlags)) { + Status = GetNewFileName ( + L"PPTT", + L"dot", + FileNameBuffer, + sizeof (FileNameBuffer) + ); + + if (EFI_ERROR (Status)) { + Print ( + L"Error: Could not open dot file for PPTT table:\n" + L"Could not get a file name." + ); + // Abandonning creation of dot graph by unsetting the flag. + // We continue parsing in case trace is set. + ParseFlags &= ~PARSE_FLAGS_GRAPH; + } else { + mDotFileHandle = DotOpenNewFile (FileNameBuffer); + if (mDotFileHandle == NULL) { + Print (L"ERROR: Could not open dot file for PPTT table.\n"); + // Abandonning creation of dot graph by unsetting the flag. + // We continue parsing in case trace is set. + ParseFlags &= ~PARSE_FLAGS_GRAPH; + } + } + } + + PpttStartPointer = Ptr; + Offset = ParseAcpi ( IS_TRACE_FLAG_SET (ParseFlags), 0, @@ -406,6 +580,24 @@ ParseAcpiPptt ( PARSER_PARAMS (PpttParser) ); + if (*(AcpiHdrInfo.Revision) < 2 && + IS_GRAPH_FLAG_SET (ParseFlags)) { + Print (L"\nWARNING: Dot output may not be consistent for PPTT revisions < 2\n"); + UnicodeSPrint ( + Buffer, + sizeof (Buffer), + L"WARNING: PPTT table revision is %u.\\n" \ + L"Revisions lower than 2 might lead to incorrect labelling", + *(AcpiHdrInfo.Revision) + ); + DotAddNode ( + mDotFileHandle, + 0, + DOT_COLOR_RED | DOT_BOX_SQUARE, + Buffer + ); + } + ProcessorTopologyStructurePtr = Ptr + Offset; while (Offset < AcpiTableLength) { @@ -486,4 +678,8 @@ ParseAcpiPptt ( ProcessorTopologyStructurePtr += *ProcessorTopologyStructureLength; Offset += *ProcessorTopologyStructureLength; } // while + + if (IS_GRAPH_FLAG_SET (ParseFlags)) { + DotCloseFile (mDotFileHandle); + } } -- Guid("CE165669-3EF3-493F-B85D-6190EE5B9759")
|
|
|
|
TianoCore Community Meeting - APAC/NAMO - Thu, 05/06/2021 7:30pm-8:30pm
#cal-reminder
devel@edk2.groups.io Calendar <devel@...>
Reminder: TianoCore Community Meeting - APAC/NAMO When: Thursday, 6 May 2021, 7:30pm to 8:30pm, (GMT-07:00) America/Los Angeles Where:https://teams.microsoft.com/l/meetup-join/19%3ameeting_Nzk4NmUzOTItNzIzNC00MTVkLTgxZDUtMDAwOTIxOGI2OWJk%40thread.v2/0?context=%7b%22Tid%22%3a%2246c98d88-e344-4ed4-8496-4ed7712e255d%22%2c%22Oid%22%3a%220d960867-aa36-403f-8446-f96a6cce5fa4%22%7d Organizer: Puja Pandya puja.pandya@... Description: Agenda:
________________________________________________________________________________ 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 ________________________________________________________________________________
|
|
|
|
TianoCore Community Meeting - APAC/NAMO - Thu, 05/06/2021 7:30pm-8:30pm
#cal-reminder
devel@edk2.groups.io Calendar <devel@...>
Reminder: TianoCore Community Meeting - APAC/NAMO When: Thursday, 6 May 2021, 7:30pm to 8:30pm, (GMT-07:00) America/Los Angeles Where:https://teams.microsoft.com/l/meetup-join/19%3ameeting_Nzk4NmUzOTItNzIzNC00MTVkLTgxZDUtMDAwOTIxOGI2OWJk%40thread.v2/0?context=%7b%22Tid%22%3a%2246c98d88-e344-4ed4-8496-4ed7712e255d%22%2c%22Oid%22%3a%220d960867-aa36-403f-8446-f96a6cce5fa4%22%7d Organizer: Puja Pandya puja.pandya@... Description: Agenda:
________________________________________________________________________________ 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 ________________________________________________________________________________
|
|
|
|
Re: [PATCH 2/3] MdePkg: Refactor BaseRngLib to support AARCH64 in addition to X86
Rebecca Cran <rebecca@...>
On 5/4/21 3:09 PM, Sami Mujawar wrote:
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].+EFIAPI[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 causes the X64 CryptoPkg host-based unit tests to fail because UnitTestHostBaseLibAsmCpuid just sets all the OUT parameters to 0, which causes RngDxe to think RDRAND isn't supported. Should the unit tests be using BaseRngLibTimerLib instead of BaseRngLib? Or should I leave the x86 code as it was, with the ASSERT in the constructor and no further checks at the time of use? -- Rebecca Cran
|
|
|
|
Re: [PATCH RFC v2 07/28] OvmfPkg: Use MemEncryptSevClearMmioPageEncMask() to clear EncMask from Mmio
Brijesh Singh
On 5/6/21 5:50 AM, Laszlo Ersek wrote:
On 04/30/21 13:51, Brijesh Singh wrote:Noted.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&sdata=CgY57XQGI9QBvj7vipJoJLVZLiEvpfySW17TLLx%2BZm8%3D&reserved=0The contents of this patch are sound, but they are incomplete, and (2) You missed the MemEncryptSevClearPageEncMask() call in I guess Tom's patches were not accepted when I rebased the the SNP patches. Will pick those changes in next rev.
|
|
|
|
Re: [PATCH RFC v2 06/28] OvmfPkg/BaseMemEncryptSevLib: Introduce MemEncryptSevClearMmioPageEncMask()
Brijesh Singh
On 5/6/21 5:39 AM, Laszlo Ersek via groups.io wrote:
On 04/30/21 13:51, Brijesh Singh wrote:Thanks Laszlo, I will go through the feedback on this patch and addressBZ: 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&sdata=YrgkAA3Kp6v2eJuovVho1I3kR%2FXAA5D%2BE2vaUKyFO68%3D&reserved=0The commit message, and five comments in the patch, say "current page them in next rev. -Brijesh Cc: James Bottomley <jejb@linux.ibm.com>(2) The parameter's name in the documentation ("IsMmio") does not match
|
|
|
|
Re: [PATCH RFC v2 11/28] OvmfPkg: Reserve Secrets page in MEMFD
James Bottomley
On Thu, 2021-05-06 at 13:57 +0300, Dov Murik wrote:
[...] It mirrors the TPM boot log behaviour: the log is stored in boot timeThe first use of the secret area was to hold the guest luks diskgit-blame fingers commit bff2811c6d99 ("OvmfPkg/AmdSev: assign andWould it make sense to always use EfiACPIMemoryNVS for theIdeally yes. Maybe James had some reasons for choosing the 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 thereI think forcing the kernel to consume the secret before exit boot services is still a good idea because 1. If the kernel can't consume the secret it gets freed 2. Not all secrets are consumed by the kernel, so it can pick the ones it wants out and discard the rest 3. If the kernel is using a secret protection mechanism, that may not work for the memfd page, so relocation of the secret might be a more secure mechanism. James
|
|
|
|
Re: [PATCH RFC v2 11/28] OvmfPkg: Reserve Secrets page in MEMFD
James Bottomley
On Wed, 2021-05-05 at 21:33 +0200, Laszlo Ersek wrote:
On 05/05/21 15:11, Brijesh Singh wrote:[...]On 5/5/21 1:42 AM, Dov Murik wrote: That's right: originally the design was not to have the boot secretsgit-blame fingers commit bff2811c6d99 ("OvmfPkg/AmdSev: assign andWould it make sense to always use EfiACPIMemoryNVS for theIdeally yes. Maybe James had some reasons for choosing the survive boot because they should already be copied into their correct, and presumably protected, locations by the time exit boot services comes. The grub code actually shreds the secret in the page once it consumes it, so the area for a simple disk secret should be empty anyway. James
|
|
|
|
TianoCore Community Meeting - EMEA / NAMO - Thu, 05/06/2021 9:00am-10:00am
#cal-reminder
devel@edk2.groups.io Calendar <devel@...>
Reminder: TianoCore Community Meeting - EMEA / NAMO When: Thursday, 6 May 2021, 9:00am to 10:00am, (GMT-07:00) America/Los Angeles Where:https://teams.microsoft.com/l/meetup-join/19%3ameeting_OTQ4NGExODMtOGRiOC00MmM0LWJhZTMtNzU1NDI2ZTc3MTNh%40thread.v2/0?context=%7b%22Tid%22%3a%2246c98d88-e344-4ed4-8496-4ed7712e255d%22%2c%22Oid%22%3a%220d960867-aa36-403f-8446-f96a6cce5fa4%22%7d Organizer: Puja Pandya puja.pandya@... Description: Agenda:
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
|
|
|
|
TianoCore Community Meeting - EMEA / NAMO - Thu, 05/06/2021 9:00am-10:00am
#cal-reminder
devel@edk2.groups.io Calendar <devel@...>
Reminder: TianoCore Community Meeting - EMEA / NAMO When: Thursday, 6 May 2021, 9:00am to 10:00am, (GMT-07:00) America/Los Angeles Where:https://teams.microsoft.com/l/meetup-join/19%3ameeting_OTQ4NGExODMtOGRiOC00MmM0LWJhZTMtNzU1NDI2ZTc3MTNh%40thread.v2/0?context=%7b%22Tid%22%3a%2246c98d88-e344-4ed4-8496-4ed7712e255d%22%2c%22Oid%22%3a%220d960867-aa36-403f-8446-f96a6cce5fa4%22%7d Organizer: Puja Pandya puja.pandya@... Description: Agenda:
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
|
|
|
|
Re: [PATCH RFC v2 11/28] OvmfPkg: Reserve Secrets page in MEMFD
Laszlo Ersek
Hi Dov,
On 05/06/21 12:57, Dov Murik wrote: 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
|
|
|
|
Re: [PATCH RFC v2 09/28] OvmfPkg/VmgExitLib: Allow PMBASE register access in Dxe phase
Laszlo Ersek
On 05/06/21 16:08, Laszlo Ersek wrote:
On 04/30/21 13:51, Brijesh Singh wrote:Actually, I've changed my mind, regarding the squashing.BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3275The above explanation is inexact. There are several typos ("APIC" is 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
|
|
|
|
Re: [PATCH RFC v2 09/28] OvmfPkg/VmgExitLib: Allow PMBASE register access in Dxe phase
Laszlo Ersek
On 04/30/21 13:51, Brijesh Singh wrote:
BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3275The 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( 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
|
|
|
|
Re: [PATCH RFC v2 08/28] OvmfPkg/BaseMemEncryptSevLib: Remove CacheFlush parameter
Laszlo Ersek
On 04/30/21 13:51, Brijesh Singh wrote:
BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3275The 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. Because we're modifying a library class header file, I agree we need to update multiple modules in a single patch. (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
|
|
|
|
Re: [PATCH RFC v2 11/28] OvmfPkg: Reserve Secrets page in MEMFD
Dov Murik <dovmurik@...>
On 05/05/2021 22:33, Laszlo Ersek wrote:
On 05/05/21 15:11, Brijesh Singh wrote: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.git-blame fingers commit bff2811c6d99 ("OvmfPkg/AmdSev: assign and 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). Thanks for this clarification.Since theYes. 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
|
|
|
|
Re: [PATCH RFC v2 07/28] OvmfPkg: Use MemEncryptSevClearMmioPageEncMask() to clear EncMask from Mmio
Laszlo Ersek
On 04/30/21 13:51, Brijesh Singh wrote:
BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3275The contents of this patch are sound, but they are incomplete, and incorrectly structured too. (1) Please provide a separate patch for each modified module. (2) You missed the MemEncryptSevClearPageEncMask() call in TpmMmioSevDecryptPeimEntryPoint() [OvmfPkg/Tcg/TpmMmioSevDecryptPei/TpmMmioSevDecryptPeim.c] -- probably because you worked on this series in parallel with Tom working on the SEV-ES TPM fixes. In the end, this patch should be split into three patches (because the change is needed for three modules). Thanks Laszlo
|
|
|
|
[PATCH v1 1/1] ArmPkg: Update SCMI Base Protocol version to 0x20000
PierreGondois
From: Nicola Mazzucato <nicola.mazzucato@arm.com>
The SCP-firmware has moved to full support for SCMIv2 which means that the base protocol can be either compliant with SCMI v1 or v2. Allow any version between SCMI v1.0 and SCMI v2.0 to be compatible with the current implementation. Signed-off-by: Nicola Mazzucato <nicola.mazzucato@arm.com> --- The changes can be seen at: https://github.com/PierreARM/edk2/tree/1732_Update_SCMI_version_v1 ArmPkg/Drivers/ArmScmiDxe/ScmiDxe.c | 10 ++++++---- ArmPkg/Include/Protocol/ArmScmiBaseProtocol.h | 10 +++++----- 2 files changed, 11 insertions(+), 9 deletions(-) diff --git a/ArmPkg/Drivers/ArmScmiDxe/ScmiDxe.c b/ArmPkg/Drivers/ArmScmiDxe/ScmiDxe.c index d5890a7633a2..fb4e79aa3610 100644 --- a/ArmPkg/Drivers/ArmScmiDxe/ScmiDxe.c +++ b/ArmPkg/Drivers/ArmScmiDxe/ScmiDxe.c @@ -4,9 +4,9 @@ SPDX-License-Identifier: BSD-2-Clause-Patent - System Control and Management Interface V1.0 - http://infocenter.arm.com/help/topic/com.arm.doc.den0056a/ - DEN0056A_System_Control_and_Management_Interface.pdf + @par Specification Reference: + - Arm System Control and Management Interface - Platform Design Document + (https://developer.arm.com/documentation/den0056/) **/ #include <Base.h> @@ -86,7 +86,9 @@ ArmScmiDxeEntryPoint ( return Status; } - if (Version != BASE_PROTOCOL_VERSION) { + // Accept any version between SCMI v1.0 and SCMI v2.0 + if ((Version < BASE_PROTOCOL_VERSION_V1) || + (Version > BASE_PROTOCOL_VERSION_V2)) { ASSERT (FALSE); return EFI_UNSUPPORTED; } diff --git a/ArmPkg/Include/Protocol/ArmScmiBaseProtocol.h b/ArmPkg/Include/Protocol/ArmScmiBaseProtocol.h index 73ad3e32a2f5..c4b81c0f56d3 100644 --- a/ArmPkg/Include/Protocol/ArmScmiBaseProtocol.h +++ b/ArmPkg/Include/Protocol/ArmScmiBaseProtocol.h @@ -4,9 +4,9 @@ SPDX-License-Identifier: BSD-2-Clause-Patent - System Control and Management Interface V1.0 - http://infocenter.arm.com/help/topic/com.arm.doc.den0056a/ - DEN0056A_System_Control_and_Management_Interface.pdf + @par Specification Reference: + - Arm System Control and Management Interface - Platform Design Document + (https://developer.arm.com/documentation/den0056/) **/ #ifndef ARM_SCMI_BASE_PROTOCOL_H_ @@ -14,7 +14,8 @@ #include <Protocol/ArmScmi.h> -#define BASE_PROTOCOL_VERSION 0x10000 +#define BASE_PROTOCOL_VERSION_V1 0x10000 +#define BASE_PROTOCOL_VERSION_V2 0x20000 #define NUM_PROTOCOL_MASK 0xFFU #define NUM_AGENT_MASK 0xFFU @@ -165,4 +166,3 @@ typedef enum { } SCMI_MESSAGE_ID_BASE; #endif /* ARM_SCMI_BASE_PROTOCOL_H_ */ - -- 2.17.1
|
|
|
|
Re: [PATCH RFC v2 06/28] OvmfPkg/BaseMemEncryptSevLib: Introduce MemEncryptSevClearMmioPageEncMask()
Laszlo Ersek
On 04/30/21 13:51, Brijesh Singh wrote:
BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3275The 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.) (2) The parameter's name in the documentation ("IsMmio") does not match the actual parameter name ("Mmio"). @@ -572,7 +573,8 @@ SetMemoryEncDec (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 (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 attributeThis 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 attributePlease carefully audit the rest of the series for comment blocks. Such issues render reviews inefficient. Thanks Laszlo
|
|
|
|
Re: [PATCH v4 1/7] IntelSiliconPkg/ReportCpuHobLib: Add ReportCpuHobLib
Ni, Ray
+/**1. "Build a HOB for the CPU." 2. PhysicalAddressBits = (UINT8) AddressSizeEax.Bits.PhysicalAddressBits
|
|
|