Hi Jeremy, While we have had some discussions internally, I thought I would share these with the rest of the list for those interested.
No, this patch series does not break either build or runtime compatibility. If anyone tries to build an old table with PPTT ID type (type 2) structures then Acpiview will flag an error and not parse the structure however this only affects Acpiview and DynamicTablesPkg. Normally we would want a version check to ensure that old systems using the feature can still run and pass Acpiview validations however in this case that is not what we want. The type 2 structures were deprecated in an errata (ACPI 6.3A) and subsequently removed in ACPI 6.4. The original bugzilla ticket for removing the structure (https://bugzilla.tianocore.org/show_bug.cgi?id=2492) shows that the structure should not have been added in the first place hence it being removed before anyone had a chance to use it. If anyone is using PPTT type 2 structures then they are doing so mistakenly and should replace those structures with the alternative SMCCC API architectural call (SMCCC_ARCH_SOC_ID). Thanks, Chris ________________________________ From: Jeremy Linton <jeremy.lin...@arm.com> Sent: Monday, October 18, 2021 5:33 PM To: devel@edk2.groups.io <devel@edk2.groups.io>; Christopher Jones <christopher.jo...@arm.com> Cc: michael.d.kin...@intel.com <michael.d.kin...@intel.com>; gaolim...@byosoft.com.cn <gaolim...@byosoft.com.cn>; zhiguang....@intel.com <zhiguang....@intel.com>; ray...@intel.com <ray...@intel.com>; zhichao....@intel.com <zhichao....@intel.com>; Alexei Fedorov <alexei.fedo...@arm.com>; Sami Mujawar <sami.muja...@arm.com>; nd <n...@arm.com> Subject: Re: [edk2-devel] [PATCH v1 3/7] ShellPkg: Update Acpiview PPTT parser to ACPI 6.4 Hi, On 10/18/21 10:10 AM, Chris Jones via groups.io wrote: > Bugzilla: 3697 (https://bugzilla.tianocore.org/show_bug.cgi?id=3697) > > Update the Acpiview PPTT parser to use Acpi64.h. As part of the changes, > remove support for parsing PPTT type 2 ID structure. > > Signed-off-by: Chris Jones <christopher.jo...@arm.com> > --- > ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Pptt/PpttParser.c > | 61 ++++---------------- > ShellPkg/Library/UefiShellAcpiViewCommandLib/UefiShellAcpiViewCommandLib.c > | 2 +- > 2 files changed, 12 insertions(+), 51 deletions(-) > > diff --git > a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Pptt/PpttParser.c > b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Pptt/PpttParser.c > index > acd2b81bb3258c7322aa10d2c0e0d842d89e358b..bce9edcedde50e53035059e6da57b9449209a674 > 100644 > --- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Pptt/PpttParser.c > +++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Pptt/PpttParser.c > @@ -1,11 +1,11 @@ > /** @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): > - - ACPI 6.3 Specification - January 2019 > + - ACPI 6.4 Specification - January 2021 > - ARM Architecture Reference Manual ARMv8 (D.a) > **/ > > @@ -157,8 +157,8 @@ ValidateCacheAttributes ( > ) > { > // Reference: Advanced Configuration and Power Interface (ACPI) > Specification > - // Version 6.2 Errata A, September 2017 > - // Table 5-153: Cache Type Structure > + // Version 6.4, January 2021 > + // Table 5-140: Cache Type Structure > UINT8 Attributes; > Attributes = *(UINT8*)Ptr; > > @@ -222,22 +222,6 @@ STATIC CONST ACPI_PARSER CacheTypeStructureParser[] = { > {L"Line size", 2, 22, L"%d", NULL, NULL, ValidateCacheLineSize, NULL} > }; > > -/** > - An ACPI_PARSER array describing the ID Type Structure - Type 2. > -**/ > -STATIC CONST ACPI_PARSER IdStructureParser[] = { > - {L"Type", 1, 0, L"0x%x", NULL, NULL, NULL, NULL}, > - {L"Length", 1, 1, L"%d", NULL, NULL, NULL, NULL}, > - {L"Reserved", 2, 2, L"0x%x", NULL, NULL, NULL, NULL}, > - > - {L"VENDOR_ID", 4, 4, NULL, Dump4Chars, NULL, NULL, NULL}, > - {L"LEVEL_1_ID", 8, 8, L"0x%x", NULL, NULL, NULL, NULL}, > - {L"LEVEL_2_ID", 8, 16, L"0x%x", NULL, NULL, NULL, NULL}, > - {L"MAJOR_REV", 2, 24, L"0x%x", NULL, NULL, NULL, NULL}, > - {L"MINOR_REV", 2, 26, L"0x%x", NULL, NULL, NULL, NULL}, > - {L"SPIN_REV", 2, 28, L"0x%x", NULL, NULL, NULL, NULL}, > -}; > - Don't people compile shellpkg for out of tree machines? Are we 100% sure that there aren't any machines in the wild with older PPTT's using this structure? Although, this is less of a problem than below.. > /** > This function parses the Processor Hierarchy Node Structure (Type 0). > > @@ -335,29 +319,6 @@ 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. > -**/ > -STATIC > -VOID > -DumpIDStructure ( > - IN UINT8* Ptr, > - IN UINT8 Length > - ) > -{ > - ParseAcpi ( > - TRUE, > - 2, > - "ID Structure", > - Ptr, > - Length, > - PARSER_PARAMS (IdStructureParser) > - ); > -} > - > /** > This function parses the ACPI PPTT table. > When trace is enabled this function parses the PPTT table and > @@ -366,7 +327,6 @@ DumpIDStructure ( > This function parses the following processor topology structures: > - Processor hierarchy node structure (Type 0) > - Cache Type Structure (Type 1) > - - ID structure (Type 2) > > This function also performs validation of the ACPI table fields. > > @@ -444,22 +404,23 @@ ParseAcpiPptt ( > Print (L"0x%x\n", Offset); > > switch (*ProcessorTopologyStructureType) { > - case EFI_ACPI_6_2_PPTT_TYPE_PROCESSOR: > + case EFI_ACPI_6_4_PPTT_TYPE_PROCESSOR: I suspect this breaks every single other edk2-platforms machine in this tree not using the dynamic table generator. AKA all the hardcoded PPTTs used on synquacer/rpi/etc. I suspect this code path should be able to deal with multiple versions of the spec. Thanks, > DumpProcessorHierarchyNodeStructure ( > ProcessorTopologyStructurePtr, > *ProcessorTopologyStructureLength > ); > break; > - case EFI_ACPI_6_2_PPTT_TYPE_CACHE: > + case EFI_ACPI_6_4_PPTT_TYPE_CACHE: > DumpCacheTypeStructure ( > ProcessorTopologyStructurePtr, > *ProcessorTopologyStructureLength > ); > break; > - case EFI_ACPI_6_2_PPTT_TYPE_ID: > - DumpIDStructure ( > - ProcessorTopologyStructurePtr, > - *ProcessorTopologyStructureLength > + case EFI_ACPI_6_3_PPTT_TYPE_ID: > + IncrementErrorCount (); > + Print ( > + L"ERROR: PPTT Type 2 - Processor ID is deprecated and must not be" > + L"used.\n" > ); > break; > default: > diff --git > a/ShellPkg/Library/UefiShellAcpiViewCommandLib/UefiShellAcpiViewCommandLib.c > b/ShellPkg/Library/UefiShellAcpiViewCommandLib/UefiShellAcpiViewCommandLib.c > index > d725cad14c5d018e2004eb8e33c845aa9c719429..ab9e6c619d70df4f79d782416037d7bef62c92d5 > 100644 > --- > a/ShellPkg/Library/UefiShellAcpiViewCommandLib/UefiShellAcpiViewCommandLib.c > +++ > b/ShellPkg/Library/UefiShellAcpiViewCommandLib/UefiShellAcpiViewCommandLib.c > @@ -62,7 +62,7 @@ ACPI_TABLE_PARSER ParserList[] = { > ParseAcpiMcfg}, > {EFI_ACPI_6_4_PLATFORM_COMMUNICATIONS_CHANNEL_TABLE_SIGNATURE, > ParseAcpiPcct}, > - {EFI_ACPI_6_2_PROCESSOR_PROPERTIES_TOPOLOGY_TABLE_STRUCTURE_SIGNATURE, > + {EFI_ACPI_6_4_PROCESSOR_PROPERTIES_TOPOLOGY_TABLE_STRUCTURE_SIGNATURE, > ParseAcpiPptt}, > {RSDP_TABLE_INFO, ParseAcpiRsdp}, > {EFI_ACPI_6_2_SYSTEM_LOCALITY_INFORMATION_TABLE_SIGNATURE, ParseAcpiSlit}, > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#83283): https://edk2.groups.io/g/devel/message/83283 Mute This Topic: https://groups.io/mt/86416274/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
