|
Bug report: serial timeout gets overwritten shortly after setting it
nicholasbishop@...
Hi, I have a bug to report with the serial I/O protocol. Using the SetAttributes function to change the timeout initially works, but after a short amount of time the timeout gets reset to 173 microseconds.
I believe this is likely due to the implementation of the simple text input protocol, which alters the serial timeout: https://github.com/tianocore/edk2/blob/e1e7306b54147e65cb7347b060e94f336d4a82d2/MdeModulePkg/Universal/Console/TerminalDxe/TerminalConIn.c#L549 I've created a full example here: https://github.com/nicholasbishop/edk2/commit/d6087f863a48f3c7812f47ea21d5d673adb8d754 The example sets the timeout to 1s which works correctly, then sleeps for 1s and reads the timeout value again. The timeout is now 173ms. |
|
|
|
Event: TianoCore Bug Triage - APAC / NAMO - 11/23/2021
#cal-reminder
devel@edk2.groups.io Calendar <noreply@...>
Reminder: TianoCore Bug Triage - APAC / NAMO When: Where: Organizer: Liming Gao gaoliming@... Description: TianoCore Bug Triage - APAC / NAMO Hosted by Liming Gao
________________________________________________________________________________ Microsoft Teams meeting Join on your computer or mobile app Click here to join the meeting Join with a video conferencing device Video Conference ID: 116 062 094 0 Alternate VTC dialing instructions Or call in (audio only) +1 916-245-6934,,77463821# United States, Sacramento Phone Conference ID: 774 638 21# |
|
|
|
Re: [PATCH] MdePkg: DebugLib: Compilation fix for clang-13.
Michael Brown
On 22/11/2021 16:42, Michael D Kinney wrote:
You are also modifying the DebugLib in the paths where ASSERT() macrosI would very strongly recommend having the non-debug version of the macro use something like: #define ASSERT(Expression) do { \ if (FALSE) { \ (VOID) (Expression); \ } \ } while (FALSE) Without the "if (FALSE)", you will find that an expression that may have side-effects (e.g. by calling an external function) will result in executable code being generated. Michael |
|
|
|
Re: [PATCH v3 0/2] ShellPkg/AcpiView: Adds ACPI_PARSER bitfield parser
Sami Mujawar
Hi Abdul,
toggle quoted message
Show quoted text
Thank you for this patch series. This series looks good to me. Reviewed-by: Sami Mujawar <sami.mujawar@...> Regards, Sami Mujawar On 22/11/2021 10:21 AM, Abdul Lateef Attar wrote:
Hi Sami, Zhichao, |
|
|
|
Re: [PATCH] MdePkg: DebugLib: Compilation fix for clang-13.
Michael D Kinney
Hi Mikhail,
toggle quoted message
Show quoted text
Are you able to provide a few examples of dead code this change uncovers? If there is additional issues then wouldn't applying this patch break all the open source package builds that contain these issues? Does this patch series pass EDK II CI? Wouldn't it also make sense to remove the disable of these warnings from all the toolchains if these updates are the only ones required to pass EDK II CI for all supported toolchains? You are also modifying the DebugLib in the paths where ASSERT() macros are disabled. When they are disabled, we want all code/data associated with ASSERT() to be removed by the optimizing compiler/linker. The source code change appears to force a reference to a variable/expression. Does this have any size impact to any of the toolchains when ASSERT() is disabled? Can you provide the size comparison before and after this change? I would like to add that I am in favor of using the compilers to help find issues in source code and increasing warning levels is great technique. We just need to make sure the changes do not generate false positives on issues and that the size and performance impacts of the changes are measured as part of the evaluation of the change. Especially components like DebugLib.h that can impact almost every component in the entire FW stack. Thanks, Mike -----Original Message----- |
|
|
|
Re: CdePkgBlog 2021-11-14
Maciej Rabeda
Hi Kilian,
toggle quoted message
Show quoted text
From my point of view, the main problem with adoption of CdePkg to EDK2 is that it relies on Torito C library.
Unless those problems are solved, I simply cannot use it. Thanks,Maciej On 14-Nov-21 20:51, Kilian Kegel wrote:
|
|
|
|
Re: [PATCH] OvmfPkg/AmdSev/SecretPei: Mark SEV launch secret area as reserved
Yao, Jiewen
Acked-by: Jiewen Yao <Jiewen.Yao@...>
toggle quoted message
Show quoted text
-----Original Message----- |
|
|
|
Re: [PATCH] OvmfPkg/AmdSev/SecretPei: Mark SEV launch secret area as reserved
Brijesh Singh
On 11/18/21 5:31 AM, Dov Murik wrote:
Mark the SEV launch secret MEMFD area as reserved, which will allow the Reviewed-by: Brijesh Singh <brijesh.singh@...> thanks |
|
|
|
Re: [edk2-platforms][PATCH 00/14] Revise U500 for the latest RISC-V packages.
Great! U500 confirmed working again and cleanup through generic code.
Reviewed-by: Daniel Schaefer <daniel.schaefer@...>
From: Chang, Abner (HPS SW/FW Technologist) <abner.chang@...>
Sent: Monday, November 15, 2021 10:56 To: devel@edk2.groups.io <devel@edk2.groups.io> Cc: Chang, Abner (HPS SW/FW Technologist) <abner.chang@...>; Schaefer, Daniel (ROM Janitor) <daniel.schaefer@...>; Sunil V L <sunilvl@...> Subject: [edk2-platforms][PATCH 00/14] Revise U500 for the latest RISC-V packages. Incorporate U500 platform with the latest RISC-V ProcessPkg and PlatformPkg.
Signed-off-by: Abner Chang <abner.chang@...> Cc: Daniel Schaefer <daniel.schaefer@...> Cc: Sunil V L <sunilvl@...> Abner Chang (14): RiscVPlatformPkg/U500: Pass DTB from PEI to DXE RiscVPlatformPkg/U500: Fix up FDT and install into config table RiscVPlatformPkg/U500: Use FirmwareContext library RiscVPlatformPkg/U500: Use generic platform library RiscVPlatformPkg/U500: Creates opensbi firmware domains RiscVPlatformPkg/U500: Uses RISC-V PeiCoreEntryPoint library RiscVPlatformPkg/U500: Use PlatormSecPpiLib RiscVPlatformPkg/U500: U500 uses mtime CSR library RiscVPlatformPkg/U500: Determines hart number from DTB RiscVPlatformPkg/U500: Use NULL instance of RiscVSpecialPlatformLib RiscVPlatformPkg/U500: Add device tree for U500 platform RiscVPlatformPkg/U500: Add device tree to build Platform/RISC-V: Add debug message to SecMain.c Platform/RISC-V: Initialize variable to zero .../FreedomU500VC707Board/U500.dsc | 18 +- .../FreedomU500VC707Board/U500.fdf | 8 + .../FreedomU500VC707Board/DeviceTree.fdf.inc | 33 +++ .../FreedomU500VC707Board/U500.fdf.inc | 84 ++++-- .../FreedomU500VC707Board/VarStore.fdf.inc | 6 +- .../DeviceTree/U500DeviceTree.inf | 25 ++ .../OpensbiPlatformLib/OpensbiPlatformLib.inf | 54 ---- .../FreedomU500VC707Board/DeviceTree/gpio.h | 45 +++ .../DeviceTree/sifive-fu500-prci.h | 19 ++ .../RiscVSpecialPlatformLib.c | 2 +- .../PlatformPkg/Universal/Sec/SecMain.c | 14 +- .../Library/OpensbiPlatformLib/Platform.c | 206 ------------- .../DeviceTree/fu500-c000.dtsi | 276 ++++++++++++++++++ .../DeviceTree/hifive-unleashed-a00.dts | 108 +++++++ 14 files changed, 611 insertions(+), 287 deletions(-) create mode 100644 Platform/SiFive/U5SeriesPkg/FreedomU500VC707Board/DeviceTree.fdf.inc create mode 100644 Platform/SiFive/U5SeriesPkg/FreedomU500VC707Board/DeviceTree/U500DeviceTree.inf delete mode 100644 Platform/SiFive/U5SeriesPkg/FreedomU500VC707Board/Library/OpensbiPlatformLib/OpensbiPlatformLib.inf create mode 100644 Platform/SiFive/U5SeriesPkg/FreedomU500VC707Board/DeviceTree/gpio.h create mode 100644 Platform/SiFive/U5SeriesPkg/FreedomU500VC707Board/DeviceTree/sifive-fu500-prci.h delete mode 100644 Platform/SiFive/U5SeriesPkg/FreedomU500VC707Board/Library/OpensbiPlatformLib/Platform.c create mode 100644 Platform/SiFive/U5SeriesPkg/FreedomU500VC707Board/DeviceTree/fu500-c000.dtsi create mode 100644 Platform/SiFive/U5SeriesPkg/FreedomU500VC707Board/DeviceTree/hifive-unleashed-a00.dts -- 2.31.1 |
|
|
|
[PATCH v3 2/2] ShellPkg/AcpiView: PrintFormatter for FADT Flags field
Abdul Lateef Attar
Adds PrintFormatter function to the FADT flags field.
Prints indivisual flag name along with flag value. Cc: Ray Ni <ray.ni@...> Cc: Zhichao Gao <zhichao.gao@...> Cc: Sami Mujawar <sami.mujawar@...> Signed-off-by: Abdul Lateef Attar <abdattar@...> --- ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Fadt/FadtParser.c | 59 +++++++++++++++++++- 1 file changed, 58 insertions(+), 1 deletion(-) diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Fadt/FadtParser.c b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Fadt/FadtParser.c index d86718bab67d..3b59864d2c7e 100644 --- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Fadt/FadtParser.c +++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Fadt/FadtParser.c @@ -2,6 +2,7 @@ FADT table parser Copyright (c) 2016 - 2020, ARM Limited. All rights reserved. + Copyright (c) 2021, AMD Incorporated. All rights reserved. SPDX-License-Identifier: BSD-2-Clause-Patent @par Reference(s): @@ -127,6 +128,62 @@ ValidateFlags ( #endif } +STATIC CONST ACPI_PARSER FadtFlagParser [] = { + {L"WBINVD", 1, 0, L"%d", NULL, NULL, NULL, NULL}, + {L"WBINVD_FLUSH", 1, 1, L"%d", NULL, NULL, NULL, NULL}, + {L"PROC_C1", 1, 2, L"%d", NULL, NULL, NULL, NULL}, + {L"P_LVL2_UP", 1, 3, L"%d", NULL, NULL, NULL, NULL}, + {L"PWR_BUTTON", 1, 4, L"%d", NULL, NULL, NULL, NULL}, + {L"SLP_BUTTON", 1, 5, L"%d", NULL, NULL, NULL, NULL}, + {L"FIX_RTC", 1, 6, L"%d", NULL, NULL, NULL, NULL}, + {L"RTC_S4", 1, 7, L"%d", NULL, NULL, NULL, NULL}, + {L"TMR_VAL_EXT", 1, 8, L"%d", NULL, NULL, NULL, NULL}, + {L"DCK_CAP", 1, 9, L"%d", NULL, NULL, NULL, NULL}, + {L"RESET_REG_SUP", 1, 10, L"%d", NULL, NULL, NULL, NULL}, + {L"SEALED_CASE", 1, 11, L"%d", NULL, NULL, NULL, NULL}, + {L"HEADLESS", 1, 12, L"%d", NULL, NULL, NULL, NULL}, + {L"CPU_SW_SLP", 1, 13, L"%d", NULL, NULL, NULL, NULL}, + {L"PCI_EXP_WAK", 1, 14, L"%d", NULL, NULL, NULL, NULL}, + {L"USE_PLATFORM_CLOCK", 1, 15, L"%d", NULL, NULL, NULL, NULL}, + {L"S4_RTC_STS_VALID", 1, 16, L"%d", NULL, NULL, NULL, NULL}, + {L"REMOTE_POWER_ON_CAPABLE", 1, 17, L"%d", NULL, NULL, NULL, NULL}, + {L"FORCE_APIC_CLUSTER_MODEL", 1, 18, L"%d", NULL, NULL, NULL, NULL}, + {L"FORCE_APIC_PHYSICAL_DESTINATION_MODE", 1, 19, L"%d", NULL, NULL, NULL, NULL}, + {L"HW_REDUCED_ACPI", 1, 20, L"%d", NULL, NULL, NULL, NULL}, + {L"LOW_POWER_S0_IDLE_CAPABLE", 1, 21, L"%d", NULL, NULL, NULL, NULL}, + {L"Reserved", 10, 22, L"%d", NULL, NULL, NULL, NULL} +}; + +/** + This function traces FADT Flags fields. + If no format string is specified the Format must be NULL. + + @param [in] Format Optional format string for tracing the data. + @param [in] Ptr Pointer to the start of the buffer. +**/ +VOID +EFIAPI +DumpFadtFlags ( + IN CONST CHAR16* Format OPTIONAL, + IN UINT8* Ptr + ) +{ + if (Format != NULL) { + Print (Format, *(UINT32*)Ptr); + return; + } + + Print (L"0x%X\n", *(UINT32*)Ptr); + ParseAcpiBitFields ( + TRUE, + 2, + NULL, + Ptr, + 4, + PARSER_PARAMS (FadtFlagParser) + ); +} + /** An ACPI_PARSER array describing the ACPI FADT Table. **/ @@ -170,7 +227,7 @@ STATIC CONST ACPI_PARSER FadtParser[] = { {L"CENTURY", 1, 108, L"0x%x", NULL, NULL, NULL, NULL}, {L"IAPC_BOOT_ARCH", 2, 109, L"0x%x", NULL, NULL, NULL, NULL}, {L"Reserved", 1, 111, L"0x%x", NULL, NULL, NULL, NULL}, - {L"Flags", 4, 112, L"0x%x", NULL, (VOID**)&Flags, ValidateFlags, NULL}, + {L"Flags", 4, 112, NULL, DumpFadtFlags, (VOID**)&Flags, ValidateFlags, NULL}, {L"RESET_REG", 12, 116, NULL, DumpGas, NULL, NULL, NULL}, {L"RESET_VALUE", 1, 128, L"0x%x", NULL, NULL, NULL, NULL}, {L"ARM_BOOT_ARCH", 2, 129, L"0x%x", NULL, NULL, NULL, NULL}, -- 2.25.1 |
|
|
|
[PATCH v3 1/2] ShellPkg/AcpiView: Adds ACPI_PARSER bitfield parser
Abdul Lateef Attar
Adds ParseAcpiBitFields() which is based on
ParseAcpi() and capable of parsing the bit fields. Supports parsing of UINT8, UINT16, UINT32 and UINT64 byte data. Cc: Ray Ni <ray.ni@...> Cc: Zhichao Gao <zhichao.gao@...> Cc: Sami Mujawar <sami.mujawar@...> Signed-off-by: Abdul Lateef Attar <abdattar@...> --- ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.h | 45 +++++ ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.c | 177 ++++++++++++++++++++ 2 files changed, 222 insertions(+) diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.h b/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.h index 5e34a70c8bae..d64eec533fab 100644 --- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.h +++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.h @@ -2,6 +2,7 @@ Header file for ACPI parser Copyright (c) 2016 - 2020, Arm Limited. All rights reserved. + Copyright (c) 2021, AMD Incorporated. All rights reserved. SPDX-License-Identifier: BSD-2-Clause-Patent **/ @@ -251,6 +252,11 @@ typedef VOID (EFIAPI *FNPTR_FIELD_VALIDATOR)(UINT8* Ptr, VOID* Context); the field data. If the field is more complex and requires additional processing for formatting and representation a print formatter function can be specified in 'PrintFormatter'. + + ParseAcpiBitFields() uses AcpiParser structure to parse the bit fields. + It considers Length as a number of bits that need to be parsed. + Also, the Offset field will be considered as starting offset of the bitfield. + The PrintFormatter function may choose to use the format string specified by 'Format' or use its own internal format string. @@ -265,10 +271,12 @@ typedef struct AcpiParser { /// The length of the field. /// (Byte Length column from ACPI table spec) + /// Length(in bits) of the bitfield if used with ParseAcpiBitFields(). UINT32 Length; /// The offset of the field from the start of the table. /// (Byte Offset column from ACPI table spec) + /// The Bit offset of the field if used with ParseAcpiBitFields(). UINT32 Offset; /// Optional Print() style format string for tracing the data. If not @@ -365,6 +373,43 @@ ParseAcpi ( IN UINT32 ParserItems ); +/** + This function is used to parse an ACPI table bitfield buffer. + + The ACPI table buffer is parsed using the ACPI table parser information + specified by a pointer to an array of ACPI_PARSER elements. This parser + function iterates through each item on the ACPI_PARSER array and logs the ACPI table bitfields. + + This function can optionally be used to parse ACPI tables and fetch specific + field values. The ItemPtr member of the ACPI_PARSER structure (where used) + is updated by this parser function to point to the selected field data + (e.g. useful for variable length nested fields). + + @param [in] Trace Trace the ACPI fields TRUE else only parse the + table. + @param [in] Indent Number of spaces to indent the output. + @param [in] AsciiName Optional pointer to an ASCII string that describes + the table being parsed. + @param [in] Ptr Pointer to the start of the buffer. + @param [in] Length Length of the buffer pointed by Ptr. + @param [in] Parser Pointer to an array of ACPI_PARSER structure that + describes the table being parsed. + @param [in] ParserItems Number of items in the ACPI_PARSER array. + + @retval Number of bits parsed. +**/ +UINT32 +EFIAPI +ParseAcpiBitFields ( + IN BOOLEAN Trace, + IN UINT32 Indent, + IN CONST CHAR8* AsciiName OPTIONAL, + IN UINT8* Ptr, + IN UINT32 Length, + IN CONST ACPI_PARSER* Parser, + IN UINT32 ParserItems +); + /** This is a helper macro to pass parameters to the Parser functions. diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.c b/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.c index 74056e72c359..d89c68ba3905 100644 --- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.c +++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.c @@ -2,12 +2,14 @@ ACPI parser Copyright (c) 2016 - 2021, Arm Limited. All rights reserved. + Copyright (c) 2021, AMD Incorporated. All rights reserved. SPDX-License-Identifier: BSD-2-Clause-Patent **/ #include <Uefi.h> #include <Library/UefiLib.h> #include <Library/UefiBootServicesTableLib.h> +#include <Library/BaseMemoryLib.h> #include "AcpiParser.h" #include "AcpiView.h" #include "AcpiViewConfig.h" @@ -738,3 +740,178 @@ ParseAcpiHeader ( return BytesParsed; } + +/** + This function is used to parse an ACPI table bitfield buffer. + + The ACPI table buffer is parsed using the ACPI table parser information + specified by a pointer to an array of ACPI_PARSER elements. This parser + function iterates through each item on the ACPI_PARSER array and logs the ACPI table bitfields. + + This function can optionally be used to parse ACPI tables and fetch specific + field values. The ItemPtr member of the ACPI_PARSER structure (where used) + is updated by this parser function to point to the selected field data + (e.g. useful for variable length nested fields). + + @param [in] Trace Trace the ACPI fields TRUE else only parse the + table. + @param [in] Indent Number of spaces to indent the output. + @param [in] AsciiName Optional pointer to an ASCII string that describes + the table being parsed. + @param [in] Ptr Pointer to the start of the buffer. + @param [in] Length Length in bytes of the buffer pointed by Ptr. + @param [in] Parser Pointer to an array of ACPI_PARSER structure that + describes the table being parsed. + @param [in] ParserItems Number of items in the ACPI_PARSER array. + + @retval Number of bits parsed. +**/ +UINT32 +EFIAPI +ParseAcpiBitFields ( + IN BOOLEAN Trace, + IN UINT32 Indent, + IN CONST CHAR8* AsciiName OPTIONAL, + IN UINT8* Ptr, + IN UINT32 Length, + IN CONST ACPI_PARSER* Parser, + IN UINT32 ParserItems +) +{ + UINT32 Index; + UINT32 Offset; + BOOLEAN HighLight; + UINTN OriginalAttribute; + + UINT64 Data; + UINT64 BitsData; + + if (Length == 0 || Length > 8) { + IncrementErrorCount (); + Print ( + L"\nERROR: Bitfield Length(%d) is zero or exceeding the 64 bit limit.\n", + Length + ); + return 0; + } + // + // set local variables to suppress incorrect compiler/analyzer warnings + // + OriginalAttribute = 0; + Offset = 0; + + // Increment the Indent + gIndent += Indent; + + CopyMem ((VOID *)&BitsData, (VOID *)Ptr, Length); + if (Trace && (AsciiName != NULL)){ + HighLight = GetColourHighlighting (); + + if (HighLight) { + OriginalAttribute = gST->ConOut->Mode->Attribute; + gST->ConOut->SetAttribute ( + gST->ConOut, + EFI_TEXT_ATTR(EFI_YELLOW, + ((OriginalAttribute&(BIT4|BIT5|BIT6))>>4)) + ); + } + Print ( + L"%*a%-*a :\n", + gIndent, + "", + (OUTPUT_FIELD_COLUMN_WIDTH - gIndent), + AsciiName + ); + if (HighLight) { + gST->ConOut->SetAttribute (gST->ConOut, OriginalAttribute); + } + } + + for (Index = 0; Index < ParserItems; Index++) { + if ((Offset + Parser[Index].Length) > (Length * 8)) { + // For fields outside the buffer length provided, reset any pointers + // which were supposed to be updated by this function call + if (Parser[Index].ItemPtr != NULL) { + *Parser[Index].ItemPtr = NULL; + } + + // We don't parse past the end of the max length specified + continue; + } + + if (Parser[Index].Length == 0) { + // don't parse the bitfield whose length is zero + continue; + } + + if (GetConsistencyChecking () && + (Offset != Parser[Index].Offset)) { + IncrementErrorCount (); + Print ( + L"\nERROR: %a: Offset Mismatch for %s\n" + L"CurrentOffset = %d FieldOffset = %d\n", + AsciiName, + Parser[Index].NameStr, + Offset, + Parser[Index].Offset + ); + } + + // extract Bitfield data for the current item + Data = (BitsData >> Parser[Index].Offset) & ~(~0ULL << Parser[Index].Length); + + if (Trace) { + // if there is a Formatter function let the function handle + // the printing else if a Format is specified in the table use + // the Format for printing + PrintFieldName (2, Parser[Index].NameStr); + if (Parser[Index].PrintFormatter != NULL) { + Parser[Index].PrintFormatter (Parser[Index].Format, (UINT8 *)&Data); + } else if (Parser[Index].Format != NULL) { + // convert bit length to byte length + switch ((Parser[Index].Length + 7) >> 3) { + // print the data depends on byte size + case 1: + DumpUint8 (Parser[Index].Format, (UINT8 *)&Data); + break; + case 2: + DumpUint16 (Parser[Index].Format, (UINT8 *)&Data); + break; + case 3: + case 4: + DumpUint32 (Parser[Index].Format, (UINT8 *)&Data); + break; + case 5: + case 6: + case 7: + case 8: + DumpUint64 (Parser[Index].Format, (UINT8 *)&Data); + break; + default: + Print ( + L"\nERROR: %a: CANNOT PARSE THIS FIELD, Field Length = %d\n", + AsciiName, + Parser[Index].Length + ); + } // switch + } + // Validating only makes sense if we are tracing + // the parsed table entries, to report by table name. + if (GetConsistencyChecking () && + (Parser[Index].FieldValidator != NULL)) { + Parser[Index].FieldValidator ((UINT8 *)&Data, Parser[Index].Context); + } + Print (L"\n"); + } // if (Trace) + + if (Parser[Index].ItemPtr != NULL) { + *Parser[Index].ItemPtr = (VOID*)(UINT8 *)&Data; + } + + Offset += Parser[Index].Length; + } // for + + // Decrement the Indent + gIndent -= Indent; + return Offset; +} -- 2.25.1 |
|
|
|
[PATCH v3 0/2] ShellPkg/AcpiView: Adds ACPI_PARSER bitfield parser
Abdul Lateef Attar
Hi Sami, Zhichao,
Addressed all review comments. Created two different patches, one for bitfield parser and another for FADT Flags parser. Thanks AbduL REF : https://github.com/abdattar/edk2/tree/FadtFlagsParser Cc: Ray Ni <ray.ni@...> Cc: Zhichao Gao <zhichao.gao@...> Cc: Sami Mujawar <sami.mujawar@...> Signed-off-by: Abdul Lateef Attar <abdattar@...> Abdul Lateef Attar (2): ShellPkg/AcpiView: Adds ACPI_PARSER bitfield parser ShellPkg/AcpiView: PrintFormatter for FADT Flags field ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.h | 45 +++++ ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.c | 177 ++++++++++++++++++++ ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Fadt/FadtParser.c | 59 ++++++- 3 files changed, 280 insertions(+), 1 deletion(-) -- 2.25.1 |
|
|
|
Re: [PATCH V3 3/3] OvmfPkg: Move LocalApicTimerDxe to UefiCpuPkg
Yao, Jiewen
Thanks Ray.
toggle quoted message
Show quoted text
I prefer to put the LocalApicTimerDxe to UefiCpuPkg, because it is hardware feature. There is no specific OVMF or QEMU related thing there. It does not make sense to keep it to OvmfPkg. Your quality concern is valid. I think the quality expectation for LocalApicTimerDxe should be same, no matter it is in OvmfPkg or UefiCpuPkg. I don't think the expectation is that the quality can be lower if it is in OvmfPkg and it must be higher if it is in UefiCpuPkg. All quality issues of LocalApicTimerDxe should be resolved, no matter where it is finally located. Thank you Yao Jiewen -----Original Message----- |
|
|
|
Re: [PATCH] UefiPayloadPkg/UefiPayloadPkg.dsc:Add BootManagerLib for BootManagerMenuApp
Ni, Ray
Reviewed-by: Ray Ni <ray.ni@...>
toggle quoted message
Show quoted text
-----Original Message-----
From: Feng, Ning <ning.feng@...> Sent: Thursday, November 18, 2021 6:10 PM To: devel@edk2.groups.io Cc: Feng, Ning <ning.feng@...>; Dong, Guo <guo.dong@...>; Ni, Ray <ray.ni@...>; Ma, Maurice <maurice.ma@...>; You, Benjamin <benjamin.you@...> Subject: [PATCH] UefiPayloadPkg/UefiPayloadPkg.dsc:Add BootManagerLib for BootManagerMenuApp Add PlatformBootManagerLibconstructor for BootManagerMenuApp, to get the value PcdBootManagefile overrided by platform side. Signed-off-by: Ning Feng <ning.feng@...> Cc: Guo Dong <guo.dong@...> Cc: Ray Ni <ray.ni@...> Cc: Maurice Ma <maurice.ma@...> Cc: Benjamin You <benjamin.you@...> --- UefiPayloadPkg/UefiPayloadPkg.dsc | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/UefiPayloadPkg/UefiPayloadPkg.dsc b/UefiPayloadPkg/UefiPayloadPkg.dsc index 0df5f827c9..3e05da9877 100644 --- a/UefiPayloadPkg/UefiPayloadPkg.dsc +++ b/UefiPayloadPkg/UefiPayloadPkg.dsc @@ -519,8 +519,10 @@ NULL|MdeModulePkg/Library/BootManagerUiLib/BootManagerUiLib.inf NULL|MdeModulePkg/Library/BootMaintenanceManagerUiLib/BootMaintenanceManagerUiLib.inf } - MdeModulePkg/Application/BootManagerMenuApp/BootManagerMenuApp.inf - + MdeModulePkg/Application/BootManagerMenuApp/BootManagerMenuApp.inf { + <LibraryClasses> + NULL|UefiPayloadPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf + } PcAtChipsetPkg/HpetTimerDxe/HpetTimerDxe.inf MdeModulePkg/Universal/Metronome/Metronome.inf MdeModulePkg/Universal/WatchdogTimerDxe/WatchdogTimer.inf -- 2.26.2.windows.1 |
|
|
|
Re: [PATCH V3 23/29] UefiCpuPkg: Update AddressEncMask in CpuPageTable
Ni, Ray
Gerd, thanks. I am about to raise the same comments...
toggle quoted message
Show quoted text
-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Gerd Hoffmann Sent: Wednesday, November 3, 2021 3:00 PM To: Xu, Min M <min.m.xu@...> Cc: devel@edk2.groups.io; Brijesh Singh <brijesh.singh@...>; Erdem Aktas <erdemaktas@...>; James Bottomley <jejb@...>; Yao, Jiewen <jiewen.yao@...>; Tom Lendacky <thomas.lendacky@...>; Dong, Eric <eric.dong@...>; Ni, Ray <ray.ni@...>; Kumar, Rahul1 <rahul1.kumar@...> Subject: Re: [edk2-devel] [PATCH V3 23/29] UefiCpuPkg: Update AddressEncMask in CpuPageTable Hi, + gEfiMdeModulePkgTokenSpaceGuid.PcdTdxSharedBitMask ## CONSUMES AddressEncMask = PcdGet64 (PcdPteMemoryEncryptionAddressOrMask) &Looks like two PCDs for basically the same thing. Should we create a common CC PCD here? take care, Gerd |
|
|
|
Re: [PATCH V3 3/3] OvmfPkg: Move LocalApicTimerDxe to UefiCpuPkg
Ni, Ray
Min,
toggle quoted message
Show quoted text
What's the reason of moving this driver to UefiCpuPkg? When the LocalApicTimerDxe is in OvmfPkg, it's clear that this driver is only used by OVMF/QEMU platform. Now since the patch moves the driver to UefiCpuPkg, it's possible that other platforms may choose this driver as the timer driver in DXE phase. So, we need to make sure the quality of this driver is good enough for a broad scope of platforms. 1. What's the issue when this driver still stays in OvmfPkg? If it's a must that this driver stays in UefiCpuPkg, please help to address following questions: 2. Can SourceLevelDebug (rely on Local APIC timer) work if this driver is chosen as DXE timer driver? 3. Can detailed comments be added for " @bug : This does not handle missed timer interrupts" in TimerInterruptHandler()? 4. Can detailed comments be added for " DisableInterrupts ();" in TimerInterruptHandler()? 5. In general what kinds of platforms are capable of using this driver as Timer driver? Thanks, Ray -----Original Message-----
From: Xu, Min M <min.m.xu@...> Sent: Monday, November 8, 2021 2:08 PM To: devel@edk2.groups.io Cc: Xu, Min M <min.m.xu@...>; Yao, Jiewen <jiewen.yao@...>; Gerd Hoffmann <kraxel@...>; Anthony Perard <anthony.perard@...>; Julien Grall <julien@...>; Dong, Eric <eric.dong@...>; Ni, Ray <ray.ni@...> Subject: [PATCH V3 3/3] OvmfPkg: Move LocalApicTimerDxe to UefiCpuPkg BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3711 OvmfPkg.dec is removed from [Packages] because it doesn't depend on OvmfPkg. LocalApicTimerDxe is moved to UefiCpuPkg. Cc: Jiewen Yao <jiewen.yao@...> Cc: Gerd Hoffmann <kraxel@...> Cc: Anthony Perard <anthony.perard@...> Cc: Julien Grall <julien@...> Cc: Eric Dong <eric.dong@...> Cc: Ray Ni <ray.ni@...> Signed-off-by: Min Xu <min.m.xu@...> --- OvmfPkg/AmdSev/AmdSevX64.dsc | 2 +- OvmfPkg/AmdSev/AmdSevX64.fdf | 2 +- OvmfPkg/Microvm/MicrovmX64.dsc | 2 +- OvmfPkg/Microvm/MicrovmX64.fdf | 2 +- OvmfPkg/OvmfPkgIa32.dsc | 2 +- OvmfPkg/OvmfPkgIa32.fdf | 2 +- OvmfPkg/OvmfPkgIa32X64.dsc | 2 +- OvmfPkg/OvmfPkgIa32X64.fdf | 2 +- OvmfPkg/OvmfPkgX64.dsc | 2 +- OvmfPkg/OvmfPkgX64.fdf | 2 +- OvmfPkg/OvmfXen.dsc | 2 +- OvmfPkg/OvmfXen.fdf | 2 +- .../LocalApicTimerDxe/LocalApicTimerDxe.c | 0 .../LocalApicTimerDxe/LocalApicTimerDxe.h | 0 .../LocalApicTimerDxe/LocalApicTimerDxe.inf | 6 ++++-- 15 files changed, 16 insertions(+), 14 deletions(-) rename {OvmfPkg => UefiCpuPkg}/LocalApicTimerDxe/LocalApicTimerDxe.c (100%) rename {OvmfPkg => UefiCpuPkg}/LocalApicTimerDxe/LocalApicTimerDxe.h (100%) rename {OvmfPkg => UefiCpuPkg}/LocalApicTimerDxe/LocalApicTimerDxe.inf (79%) diff --git a/OvmfPkg/AmdSev/AmdSevX64.dsc b/OvmfPkg/AmdSev/AmdSevX64.dsc index 88c51dfe8337..888fc24f1b58 100644 --- a/OvmfPkg/AmdSev/AmdSevX64.dsc +++ b/OvmfPkg/AmdSev/AmdSevX64.dsc @@ -674,7 +674,7 @@ MdeModulePkg/Universal/EbcDxe/EbcDxe.inf UefiCpuPkg/CpuIo2Dxe/CpuIo2Dxe.inf UefiCpuPkg/CpuDxe/CpuDxe.inf - OvmfPkg/LocalApicTimerDxe/LocalApicTimerDxe.inf + UefiCpuPkg/LocalApicTimerDxe/LocalApicTimerDxe.inf OvmfPkg/IncompatiblePciDeviceSupportDxe/IncompatiblePciDeviceSupport.inf OvmfPkg/PciHotPlugInitDxe/PciHotPlugInit.inf MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridgeDxe.inf { diff --git a/OvmfPkg/AmdSev/AmdSevX64.fdf b/OvmfPkg/AmdSev/AmdSevX64.fdf index 7489b04198fe..659810f96bec 100644 --- a/OvmfPkg/AmdSev/AmdSevX64.fdf +++ b/OvmfPkg/AmdSev/AmdSevX64.fdf @@ -208,7 +208,7 @@ INF MdeModulePkg/Universal/SecurityStubDxe/SecurityStubDxe.inf INF MdeModulePkg/Universal/EbcDxe/EbcDxe.inf INF UefiCpuPkg/CpuIo2Dxe/CpuIo2Dxe.inf INF UefiCpuPkg/CpuDxe/CpuDxe.inf -INF OvmfPkg/LocalApicTimerDxe/LocalApicTimerDxe.inf +INF UefiCpuPkg/LocalApicTimerDxe/LocalApicTimerDxe.inf INF OvmfPkg/IncompatiblePciDeviceSupportDxe/IncompatiblePciDeviceSupport.inf INF OvmfPkg/PciHotPlugInitDxe/PciHotPlugInit.inf INF MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridgeDxe.inf diff --git a/OvmfPkg/Microvm/MicrovmX64.dsc b/OvmfPkg/Microvm/MicrovmX64.dsc index 1a0e848f8356..fcb8b571a041 100644 --- a/OvmfPkg/Microvm/MicrovmX64.dsc +++ b/OvmfPkg/Microvm/MicrovmX64.dsc @@ -656,7 +656,7 @@ MdeModulePkg/Universal/SecurityStubDxe/SecurityStubDxe.inf MdeModulePkg/Universal/EbcDxe/EbcDxe.inf - OvmfPkg/LocalApicTimerDxe/LocalApicTimerDxe.inf + UefiCpuPkg/LocalApicTimerDxe/LocalApicTimerDxe.inf UefiCpuPkg/CpuIo2Dxe/CpuIo2Dxe.inf UefiCpuPkg/CpuDxe/CpuDxe.inf OvmfPkg/IncompatiblePciDeviceSupportDxe/IncompatiblePciDeviceSupport.inf diff --git a/OvmfPkg/Microvm/MicrovmX64.fdf b/OvmfPkg/Microvm/MicrovmX64.fdf index ac9efba26811..d02e88e2a48e 100644 --- a/OvmfPkg/Microvm/MicrovmX64.fdf +++ b/OvmfPkg/Microvm/MicrovmX64.fdf @@ -215,7 +215,7 @@ INF MdeModulePkg/Universal/PCD/Dxe/Pcd.inf INF MdeModulePkg/Core/RuntimeDxe/RuntimeDxe.inf INF MdeModulePkg/Universal/SecurityStubDxe/SecurityStubDxe.inf INF MdeModulePkg/Universal/EbcDxe/EbcDxe.inf -INF OvmfPkg/LocalApicTimerDxe/LocalApicTimerDxe.inf +INF UefiCpuPkg/LocalApicTimerDxe/LocalApicTimerDxe.inf INF UefiCpuPkg/CpuIo2Dxe/CpuIo2Dxe.inf INF UefiCpuPkg/CpuDxe/CpuDxe.inf INF OvmfPkg/IncompatiblePciDeviceSupportDxe/IncompatiblePciDeviceSupport.inf diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc index d0e9f3ca05f6..f8c8ef1e58be 100644 --- a/OvmfPkg/OvmfPkgIa32.dsc +++ b/OvmfPkg/OvmfPkgIa32.dsc @@ -763,7 +763,7 @@ OvmfPkg/8259InterruptControllerDxe/8259.inf OvmfPkg/8254TimerDxe/8254Timer.inf !else - OvmfPkg/LocalApicTimerDxe/LocalApicTimerDxe.inf + UefiCpuPkg/LocalApicTimerDxe/LocalApicTimerDxe.inf !endif OvmfPkg/IncompatiblePciDeviceSupportDxe/IncompatiblePciDeviceSupport.inf OvmfPkg/PciHotPlugInitDxe/PciHotPlugInit.inf diff --git a/OvmfPkg/OvmfPkgIa32.fdf b/OvmfPkg/OvmfPkgIa32.fdf index b7b35a3a490a..321d4a871afa 100644 --- a/OvmfPkg/OvmfPkgIa32.fdf +++ b/OvmfPkg/OvmfPkgIa32.fdf @@ -218,7 +218,7 @@ INF UefiCpuPkg/CpuDxe/CpuDxe.inf INF OvmfPkg/8259InterruptControllerDxe/8259.inf INF OvmfPkg/8254TimerDxe/8254Timer.inf !else - INF OvmfPkg/LocalApicTimerDxe/LocalApicTimerDxe.inf + INF UefiCpuPkg/LocalApicTimerDxe/LocalApicTimerDxe.inf !endif INF OvmfPkg/IncompatiblePciDeviceSupportDxe/IncompatiblePciDeviceSupport.inf INF OvmfPkg/PciHotPlugInitDxe/PciHotPlugInit.inf diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc index d3531d388e24..4de4ed21a5ca 100644 --- a/OvmfPkg/OvmfPkgIa32X64.dsc +++ b/OvmfPkg/OvmfPkgIa32X64.dsc @@ -777,7 +777,7 @@ OvmfPkg/8259InterruptControllerDxe/8259.inf OvmfPkg/8254TimerDxe/8254Timer.inf !else - OvmfPkg/LocalApicTimerDxe/LocalApicTimerDxe.inf + UefiCpuPkg/LocalApicTimerDxe/LocalApicTimerDxe.inf !endif OvmfPkg/IncompatiblePciDeviceSupportDxe/IncompatiblePciDeviceSupport.inf OvmfPkg/PciHotPlugInitDxe/PciHotPlugInit.inf diff --git a/OvmfPkg/OvmfPkgIa32X64.fdf b/OvmfPkg/OvmfPkgIa32X64.fdf index 986228a44c78..10e97c35001f 100644 --- a/OvmfPkg/OvmfPkgIa32X64.fdf +++ b/OvmfPkg/OvmfPkgIa32X64.fdf @@ -222,7 +222,7 @@ INF UefiCpuPkg/CpuDxe/CpuDxe.inf INF OvmfPkg/8259InterruptControllerDxe/8259.inf INF OvmfPkg/8254TimerDxe/8254Timer.inf !else - INF OvmfPkg/LocalApicTimerDxe/LocalApicTimerDxe.inf + INF UefiCpuPkg/LocalApicTimerDxe/LocalApicTimerDxe.inf !endif INF OvmfPkg/IncompatiblePciDeviceSupportDxe/IncompatiblePciDeviceSupport.inf INF OvmfPkg/PciHotPlugInitDxe/PciHotPlugInit.inf diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc index c6ee624fc738..57b0c3c10826 100644 --- a/OvmfPkg/OvmfPkgX64.dsc +++ b/OvmfPkg/OvmfPkgX64.dsc @@ -775,7 +775,7 @@ OvmfPkg/8259InterruptControllerDxe/8259.inf OvmfPkg/8254TimerDxe/8254Timer.inf !else - OvmfPkg/LocalApicTimerDxe/LocalApicTimerDxe.inf + UefiCpuPkg/LocalApicTimerDxe/LocalApicTimerDxe.inf !endif OvmfPkg/IncompatiblePciDeviceSupportDxe/IncompatiblePciDeviceSupport.inf OvmfPkg/PciHotPlugInitDxe/PciHotPlugInit.inf diff --git a/OvmfPkg/OvmfPkgX64.fdf b/OvmfPkg/OvmfPkgX64.fdf index 99339e73bb51..b52c43127845 100644 --- a/OvmfPkg/OvmfPkgX64.fdf +++ b/OvmfPkg/OvmfPkgX64.fdf @@ -238,7 +238,7 @@ INF UefiCpuPkg/CpuDxe/CpuDxe.inf INF OvmfPkg/8259InterruptControllerDxe/8259.inf INF OvmfPkg/8254TimerDxe/8254Timer.inf !else - INF OvmfPkg/LocalApicTimerDxe/LocalApicTimerDxe.inf + INF UefiCpuPkg/LocalApicTimerDxe/LocalApicTimerDxe.inf !endif INF OvmfPkg/IncompatiblePciDeviceSupportDxe/IncompatiblePciDeviceSupport.inf INF OvmfPkg/PciHotPlugInitDxe/PciHotPlugInit.inf diff --git a/OvmfPkg/OvmfXen.dsc b/OvmfPkg/OvmfXen.dsc index 7c4c5412d102..d4a9e8f87def 100644 --- a/OvmfPkg/OvmfXen.dsc +++ b/OvmfPkg/OvmfXen.dsc @@ -551,7 +551,7 @@ MdeModulePkg/Universal/SecurityStubDxe/SecurityStubDxe.inf MdeModulePkg/Universal/EbcDxe/EbcDxe.inf - OvmfPkg/LocalApicTimerDxe/LocalApicTimerDxe.inf + UefiCpuPkg/LocalApicTimerDxe/LocalApicTimerDxe.inf UefiCpuPkg/CpuIo2Dxe/CpuIo2Dxe.inf UefiCpuPkg/CpuDxe/CpuDxe.inf OvmfPkg/PciHotPlugInitDxe/PciHotPlugInit.inf diff --git a/OvmfPkg/OvmfXen.fdf b/OvmfPkg/OvmfXen.fdf index 196853740753..76934d354fa6 100644 --- a/OvmfPkg/OvmfXen.fdf +++ b/OvmfPkg/OvmfXen.fdf @@ -298,7 +298,7 @@ INF MdeModulePkg/Universal/PCD/Dxe/Pcd.inf INF MdeModulePkg/Core/RuntimeDxe/RuntimeDxe.inf INF MdeModulePkg/Universal/SecurityStubDxe/SecurityStubDxe.inf INF MdeModulePkg/Universal/EbcDxe/EbcDxe.inf -INF OvmfPkg/LocalApicTimerDxe/LocalApicTimerDxe.inf +INF UefiCpuPkg/LocalApicTimerDxe/LocalApicTimerDxe.inf INF UefiCpuPkg/CpuIo2Dxe/CpuIo2Dxe.inf INF UefiCpuPkg/CpuDxe/CpuDxe.inf INF OvmfPkg/PciHotPlugInitDxe/PciHotPlugInit.inf diff --git a/OvmfPkg/LocalApicTimerDxe/LocalApicTimerDxe.c b/UefiCpuPkg/LocalApicTimerDxe/LocalApicTimerDxe.c similarity index 100% rename from OvmfPkg/LocalApicTimerDxe/LocalApicTimerDxe.c rename to UefiCpuPkg/LocalApicTimerDxe/LocalApicTimerDxe.c diff --git a/OvmfPkg/LocalApicTimerDxe/LocalApicTimerDxe.h b/UefiCpuPkg/LocalApicTimerDxe/LocalApicTimerDxe.h similarity index 100% rename from OvmfPkg/LocalApicTimerDxe/LocalApicTimerDxe.h rename to UefiCpuPkg/LocalApicTimerDxe/LocalApicTimerDxe.h diff --git a/OvmfPkg/LocalApicTimerDxe/LocalApicTimerDxe.inf b/UefiCpuPkg/LocalApicTimerDxe/LocalApicTimerDxe.inf similarity index 79% rename from OvmfPkg/LocalApicTimerDxe/LocalApicTimerDxe.inf rename to UefiCpuPkg/LocalApicTimerDxe/LocalApicTimerDxe.inf index 3ad28a148c5b..4f2b4db9e5dc 100644 --- a/OvmfPkg/LocalApicTimerDxe/LocalApicTimerDxe.inf +++ b/UefiCpuPkg/LocalApicTimerDxe/LocalApicTimerDxe.inf @@ -1,6 +1,9 @@ ## @file # Local APIC timer driver that provides Timer Arch protocol. -# PcdFSBClock is defined in MdePkg and it should be set by the consumer. +# +# This driver is to support fixed frequency. If a real platform happens +# to have fixed frequency, then it can be used. In this case the consumer +# should set PcdFSBClock which is defined in MdePkg. # # Copyright (c) 2005 - 2019, Intel Corporation. All rights reserved.<BR> # Copyright (c) 2019, Citrix Systems, Inc. @@ -21,7 +24,6 @@ [Packages] MdePkg/MdePkg.dec UefiCpuPkg/UefiCpuPkg.dec - OvmfPkg/OvmfPkg.dec [LibraryClasses] UefiBootServicesTableLib -- 2.29.2.windows.2 |
|
|
|
Event: TianoCore Design Meeting - APAC/NAMO - 11/26/2021
#cal-reminder
devel@edk2.groups.io Calendar <noreply@...>
Reminder: TianoCore Design Meeting - APAC/NAMO When: Where: Organizer: Ray Ni ray.ni@... Description: TOPIC
For more info, see here: https://www.tianocore.org/design-meeting/ Microsoft Teams meetingJoin on your computer or mobile appClick here to join the meeting Join with a video conferencing deviceteams@... Video Conference ID: 119 715 416 0 |
|
|
|
Re: [PATCH V3 15/29] OvmfPkg: Update SecEntry.nasm to support Tdx
Yao, Jiewen
toggle quoted message
Show quoted text
-----Original Message-----[Jiewen] I don't think TDVF config-B will be like the AMD SEV is right statement. TDVF and SEV are two different platforms. Intel mainly focuses on TDVF and we will let AMD defines the feature set in SEV. They MAY be alike if possible. But difference is also acceptable if there is architecture difference or different decision in different company. No SMM support, no network stack, ...[Jiewen] I don't think this is right statement. In Tiano history, there were security bugs exposed in PEI phase, even the PEI Core on FV processing. I do see the value to skip PEI core. Again, I am very familiar with non-PEI flow. Back to 10 years ago, I was maintainer of a non-PEI platform (named DUET) and we jumped from SEC to DXE directly. I don't see any maintenance problem. [Jiewen] I think we are debating two different things.Config-A is to keep current architecture, to maximum compatible with Your statement that "config-B is similar to AmdSev" does not support the statement "config-B should be adopt what AmdSev chooses". TDVF config-B is proposed by Intel. AMD SEV is proposed by AMD. I respect SEV people and I *may* propose something, but I will ack their decision. I would not force them to accept the TDVF config-B. And vice versa. [Jiewen] My point is simple - Threat Model is different. That causes security objective difference and design difference. Each CC env owner should have freedom to choose what they want to enforce. IMHO, they are the policy decision maker. [Jiewen] If you look at how we state config-A and config-B again, you will find we made difference statement.s3, smm, tpm and maybe more depends on pei modules, so dropping the PEiJiewen argued this is a simplification. Which is not completely wrong,[Jiewen] I am not asking your to OVMF build to PEI-less. I copy it here again. 1) Config-A is to keep current architecture, to maximum compatible with OVMF. And we don't remove VMM out of TCB. 2) Config-B is to have a new TDVF design, to maximum satisfy the security requirement. And we remove VMM out of TCB. Because of the threat model difference, in config-A, we can safely make some assumption that the VMM is benign and VMM will not input malicious data. As such, we might not perform data validation. We just trust VMM input. However, in config-B, VMM is malicious, which means we need be careful to NOT trust VMM at any time. The code in config-A and config-B may do totally different thing to handle the difference situation. I don't think it is hidden assumption that if TDVF need do some check, then a generic OVMF need do this check. If OVMF trusts the VMM, the check might be totally unnecessary. I'm not going to open that discussion in this thread. But let me drop[Jiewen] This is how Intel views the "platform". In history, we call this one binary mode is "multiple-platform" or "multiple-SKU". That means we only have one binary, and this single binary can boot different platforms, which has similar CPU or silicon but different platform board design. And there will be platform specific code flow, such as Switch (PlatformId) { Case PlatformA: {do platformA init} Case PlatformB: {do platformB init} } If you treat CC_TYPE to be platformID, it perfectly matches. Also a platform may has extra module (a driver or an FV) to support the platform specific feature. Or a platform may much simpler design and skip some drivers. The actual multi-platform design is even more complicated, because we also have group concept. So I will stop the discussion here. [Jiewen] I think I have explained a lot above.I think a PEI-less TDVF is much easier to maintain, comparing withWell, we will have TDX support in the normal OVMF workflow anyway, The key difference between config-a and config-b is threat mode. I even propose config-a skip PEI phase. I am persuaded to let config-a adopt the OVMF way, because the threat model of config-A is same as the normal OVMF. But config-B is NOT. Different threat model drives different solution. I completely don't understand why they must be same. If you force me to add PEI to config-B. Finally, that causes TDVF config-B is compromised due to an issue in PEI. Who will take the responsibility? Will you or RedHat take that?
|
|
|
|
Re: [PATCH] MdePkg: DebugLib: Compilation fix for clang-13.
Michael D Kinney
Hi Mikhail,
toggle quoted message
Show quoted text
For RELEASE GCC5 toolchains in tools_def.txt, I see this warning is disabled. Likely for the same reason. RELEASE_GCC5_IA32_CC_FLAGS = DEF(GCC5_IA32_CC_FLAGS) -flto -Os -Wno-unused-but-set-variable -Wno-unused-const-variable RELEASE_GCC5_X64_CC_FLAGS = DEF(GCC5_X64_CC_FLAGS) -flto -DUSING_LTO -Os -Wno-unused-but-set-variable -Wno-unused-const-variable RELEASE_GCC5_ARM_CC_FLAGS = DEF(GCC5_ARM_CC_FLAGS) -flto -Wno-unused-but-set-variable -Wno-unused-const-variable RELEASE_GCC5_AARCH64_CC_FLAGS = DEF(GCC5_AARCH64_CC_FLAGS) -flto -Wno-unused-but-set-variable -Wno-unused-const-variable I think it would be better to ignore the same warning in RELEASE CLANG38 tool chains with an update to tools_def.txt instead of DebugLib.h. Unless you are thinking that disabling this warning is hiding some bad code practices that need to be cleaned up. Best regards, Mike -----Original Message----- |
|
|
|
Re: [PATCH V3 15/29] OvmfPkg: Update SecEntry.nasm to support Tdx
Gerd Hoffmann
Hi,
Comment on config-B. Sure.I'm sure I've asked this before: Why skip the PEI phase? So farSkipping PEI phase is valid architecture design. Second, the confidential computing changes the threat modelThat totally makes sense. I expect TDVF Config-B will look alot like the existing AmdSev configuration variant which is stripped down too. No SMM support, no network stack, ... There wouldn't be left much in PEI beside PeiCore and OvmfPkg/PlatformPei. But I don't see how dropping the PEI phase altogether helps much in stripping down the firmware image. The initialization currently handled by OvmfPkg/PlatformPei must happen somewhere else instead. Given SEC is a very restricted environment I don't expect the code can be shared easily, so we will probably end up with code duplication. Also two different boot workflows which I fear can easily introduce subtle bugs due to differences like a initialization order changes. This is what I see as maintenance problem. Config-A is to keep current architecture, to maximum compatible with Config-B is to have a new TDVF design, to maximum satisfy the securitySure. config-a is ovmf with tdx support added, all uefi features present, only basic tdx/sev support (basically support memory encryption). config-b is simliar to AmdSev (maybe we'll merge them some day), stripped down uefi feature set (no network etc), full tdx support including attestation etc. I don't want question all that. I still don't see the point in dropping the PEI phase and make config-b work different that all other ovmf variants though. s3, smm, tpm and maybe more depends on pei modules, so dropping the PEiJiewen argued this is a simplification. Which is not completely wrong,[Jiewen] I am not asking your to OVMF build to PEI-less. phase is not an option for a full-featured OVMF build. So I'd very much prefer all ovmf variants (including tdvf) use the PEI phase. On contrast, if we keep PEI for config B, it adds extra burden fromSame for all other modules used by tdvf. The list of affected PEI modules is rather short though. I think it's only PeiCore and DxeIpl. PlatformPei doesn't count as the code wouldn't go away but would be moved to SEC (and maybe parts of it to DXE). Comparing the effort to maintain the work flow and the effort toThe security workflow is a serious problem indeed. Not only for TDVF, also for OVMF in general, and other platforms too. We should certainly try to improve it. I'm not going to open that discussion in this thread. But let me drop two links two links to osfc talk and workshop (Not 30th + Dec 1st), titled "The firmware supply-chain security is broken: can we fix it?" https://talks.osfc.io/osfc2021/talk/D9X39Z/ https://talks.osfc.io/osfc2021/talk/E9YYJF/ Hmm? Seeing TDVF as "other platform" is a rather strange view givenI want TDVF be consistent with the rest of OVMF. Makes long-term[Jiewen] I am not convinced that TDVF be consist with rest of OVMF. that we are integrating tdx support into OVMF right now ... I think a PEI-less TDVF is much easier to maintain, comparing withWell, we will have TDX support in the normal OVMF workflow anyway, because we need that for config-a. Why use and maintain something different for config-b? That looks rather pointless to me ... take care, Gerd |
|
|