Re: [edk2-devel] [PATCH v1 3/7] ShellPkg: Update Acpiview PPTT parser to ACPI 6.4

["Chris Jones" <christopher.jones@arm.com>]

[-- Attachment #1: Type: text/plain, Size: 8089 bytes --]

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.linton@arm.com>
Sent: Monday, October 18, 2021 5:33 PM
To: devel@edk2.groups.io <devel@edk2.groups.io>; Christopher Jones <Christopher.Jones@arm.com>
Cc: michael.d.kinney@intel.com <michael.d.kinney@intel.com>; gaoliming@byosoft.com.cn <gaoliming@byosoft.com.cn>; zhiguang.liu@intel.com <zhiguang.liu@intel.com>; ray.ni@intel.com <ray.ni@intel.com>; zhichao.gao@intel.com <zhichao.gao@intel.com>; Alexei Fedorov <Alexei.Fedorov@arm.com>; Sami Mujawar <Sami.Mujawar@arm.com>; nd <nd@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.jones@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},
>


[-- Attachment #2: Type: text/html, Size: 12825 bytes --]