Re: [edk2-devel] [PATCH v1 2/2] ShellPkg: acpiview: Update SRAT parser to ACPI 6.3

["Krzysztof Koch" <krzysztof.koch@arm.com>]

Hi,

Is there any way I can help to get this patch merged upstream?

Kind regards,

Krzysztof

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Carsey, Jaben via Groups.Io
Sent: Wednesday, June 12, 2019 21:58
To: devel@edk2.groups.io; Krzysztof Koch <Krzysztof.Koch@arm.com>
Cc: Ni, Ray <ray.ni@intel.com>; Gao, Zhichao <zhichao.gao@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Liming <liming.gao@intel.com>; Sami Mujawar <Sami.Mujawar@arm.com>; Matteo Carlini <Matteo.Carlini@arm.com>; Stephanie Hughes-Fitt <Stephanie.Hughes-Fitt@arm.com>; nd <nd@arm.com>
Subject: Re: [edk2-devel] [PATCH v1 2/2] ShellPkg: acpiview: Update SRAT parser to ACPI 6.3

Reviewed-by: Jaben Carsey <jaben.carsey@intel.com>

I don't have access to a platform with ACPI 6.3 to do a test.  Can someone do a test and respond with tested-by if possible?

> -----Original Message-----
> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of 
> Krzysztof Koch
> Sent: Wednesday, June 12, 2019 7:11 AM
> To: devel@edk2.groups.io
> Cc: Carsey, Jaben <jaben.carsey@intel.com>; Ni, Ray 
> <ray.ni@intel.com>; Gao, Zhichao <zhichao.gao@intel.com>; Kinney, 
> Michael D <michael.d.kinney@intel.com>; Gao, Liming 
> <liming.gao@intel.com>; Sami.Mujawar@arm.com; Matteo.Carlini@arm.com; 
> Stephanie.Hughes- Fitt@arm.com; nd@arm.com
> Subject: [edk2-devel] [PATCH v1 2/2] ShellPkg: acpiview: Update SRAT 
> parser to ACPI 6.3
> Importance: High
> 
> Add support for parsing revision 3 of System Resource Affinity Table 
> (SRAT).
> 
> Decode and dump the new Generic Initiator Affinity Structure.
> 
> Validate the Device Handle Type field inside the Generic Initiator 
> Affinity Structure.
> 
> Signed-off-by: Krzysztof Koch <krzysztof.koch@arm.com>
> ---
> 
> Changes can be seen at:
> https://github.com/KrzysztofKoch1/edk2/tree/582_acpiview_6_3_srat_v1
> 
> Notes:
>     v1:
>     - dump and validate the Generic Initiator Affinity Struct 
> [Krzysztof]
> 
>  ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.c              |  35
> ++-
>  ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.h              |  16 ++
>
> ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Srat/SratParser.c 
> |
> 256 +++++++++++++++++++-
>  3 files changed, 299 insertions(+), 8 deletions(-)
> 
> diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.c
> b/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.c
> index
> b5965507b498b9fa9bc4d3124b2285f509004c4f..60523936732f901317ee93d03f
> e06df1403f3695 100644
> --- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.c
> +++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.c
> @@ -1,7 +1,7 @@
>  /** @file
>    ACPI parser
> 
> -  Copyright (c) 2016 - 2018, ARM Limited. All rights reserved.
> +  Copyright (c) 2016 - 2019, ARM Limited. All rights reserved.
>    SPDX-License-Identifier: BSD-2-Clause-Patent  **/
> 
> @@ -401,6 +401,39 @@ Dump8Chars (
>      );
>  }
> 
> +/**
> +  This function traces 12 characters which can be optionally
> +  formated using the format string if specified.
> +
> +  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
> +Dump12Chars (
> +  IN CONST CHAR16* Format OPTIONAL,
> +  IN       UINT8*  Ptr
> +  )
> +{
> +  Print (
> +    (Format != NULL) ? Format : L"%c%c%c%c%c%c%c%c%c%c%c%c",
> +    Ptr[0],
> +    Ptr[1],
> +    Ptr[2],
> +    Ptr[3],
> +    Ptr[4],
> +    Ptr[5],
> +    Ptr[6],
> +    Ptr[7],
> +    Ptr[8],
> +    Ptr[9],
> +    Ptr[10],
> +    Ptr[11]
> +    );
> +}
> +
>  /**
>    This function indents and prints the ACPI table Field Name.
> 
> diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.h
> b/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.h
> index
> 7657892d9fd2e2e14c6578611ff0cf1b6f6cd750..5b23ab6fa9bd2f87e034728787
> 2685a2f74622f3 100644
> --- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.h
> +++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.h
> @@ -184,6 +184,22 @@ Dump8Chars (
>    IN UINT8*        Ptr
>    );
> 
> +/**
> +  This function traces 12 characters which can be optionally
> +  formated using the format string if specified.
> +
> +  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
> +Dump12Chars (
> +  IN CONST CHAR16* Format OPTIONAL,
> +  IN       UINT8*  Ptr
> +  );
> +
>  /**
>    This function indents and prints the ACPI table Field Name.
> 
> diff --git
> a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Srat/SratParser
> .c 
> b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Srat/SratParser
> .c
> index
> 075ff2a141a82b522e8aaedb7ad79249aaf5eaac..8c268a11454d13c9e278691d6
> 19a580c4c14c08e 100644
> ---
> a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Srat/SratParser
> .c
> +++
> b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Srat/SratParser
> .c
> @@ -1,11 +1,11 @@
>  /** @file
>    SRAT table parser
> 
> -  Copyright (c) 2016 - 2018, ARM Limited. All rights reserved.
> +  Copyright (c) 2016 - 2019, ARM Limited. All rights reserved.
>    SPDX-License-Identifier: BSD-2-Clause-Patent
> 
>    @par Reference(s):
> -    - ACPI 6.2 Specification - Errata A, September 2017
> +    - ACPI 6.3 Specification - January 2019
>  **/
> 
>  #include <IndustryStandard/Acpi.h>
> @@ -17,6 +17,7 @@
>  // Local Variables
>  STATIC CONST UINT8* SratRAType;
>  STATIC CONST UINT8* SratRALength;
> +STATIC CONST UINT8* SratDeviceHandleType;
>  STATIC ACPI_DESCRIPTION_HEADER_INFO AcpiHdrInfo;
> 
>  /**
> @@ -34,6 +35,51 @@ ValidateSratReserved (
>    IN VOID*  Context
>    );
> 
> +/**
> +  This function validates the Device Handle Type field in the Generic 
> +Initiator
> +  Affinity Structure.
> +
> +  @param [in] Ptr     Pointer to the start of the field data.
> +  @param [in] Context Pointer to context specific information e.g. this
> +                      could be a pointer to the ACPI table header.
> +**/
> +STATIC
> +VOID
> +EFIAPI
> +ValidateSratDeviceHandleType (
> +  IN UINT8* Ptr,
> +  IN VOID*  Context
> +  );
> +
> +/**
> +  This function traces the Device Handle field inside Generic 
> +Initiator
> +  Affinity Structure.
> +
> +  @param [in] Format  Format string for tracing the data.
> +  @param [in] Ptr     Pointer to the start of the buffer.
> +**/
> +STATIC
> +VOID
> +EFIAPI
> +DumpSratDeviceHandle (
> +  IN CONST CHAR16* Format,
> +  IN       UINT8*  Ptr
> +  );
> +
> +/**
> +  This function traces the PCI BDF Number field inside Device Handle 
> +- PCI
> +
> +  @param [in] Format  Format string for tracing the data.
> +  @param [in] Ptr     Pointer to the start of the buffer.
> +**/
> +STATIC
> +VOID
> +EFIAPI
> +DumpSratPciBdfNumber (
> +  IN CONST CHAR16* Format,
> +  IN       UINT8*  Ptr
> +  );
> +
>  /**
>    This function traces the APIC Proximity Domain field.
> 
> @@ -90,6 +136,41 @@ STATIC CONST ACPI_PARSER 
> SratGicITSAffinityParser[] = {
>    {L"ITS Id", 4, 8, L"0x%x", NULL, NULL, NULL, NULL},  };
> 
> +/**
> +  An ACPI_PARSER array describing the Generic Initiator Affinity 
> +Structure **/ STATIC CONST ACPI_PARSER 
> +SratGenericInitiatorAffinityParser[] = {
> +  {L"Type", 1, 0, L"0x%x", NULL, NULL, NULL, NULL},
> +  {L"Length", 1, 1, L"0x%x", NULL, NULL, NULL, NULL},
> +
> +  {L"Reserved", 1, 2, L"0x%x", NULL, NULL, NULL, NULL},  {L"Device 
> + Handle Type", 1, 3, L"%d", NULL,
> (VOID**)&SratDeviceHandleType,
> +   ValidateSratDeviceHandleType, NULL},  {L"Proximity Domain", 4, 4, 
> + L"0x%x", NULL, NULL, NULL, NULL},  {L"Device Handle", 16, 8, L"%s", 
> + DumpSratDeviceHandle, NULL, NULL,
> NULL},
> +  {L"Flags", 4, 24, L"0x%x", NULL, NULL, NULL, NULL},
> +  {L"Reserved", 4, 28, L"0x%x", NULL, NULL, NULL, NULL} };
> +
> +/**
> +  An ACPI_PARSER array describing the Device Handle - ACPI **/ STATIC 
> +CONST ACPI_PARSER SratDeviceHandleAcpiParser[] = {
> +  {L"ACPI_HID", 8, 0, L"0x%x", NULL, NULL, NULL, NULL},
> +  {L"ACPI_UID", 4, 8, L"0x%x", NULL, NULL, NULL, NULL},
> +  {L"Reserved", 4, 12, L"0x%x", NULL, NULL, NULL, NULL} };
> +
> +/**
> +  An ACPI_PARSER array describing the Device Handle - PCI **/ STATIC 
> +CONST ACPI_PARSER SratDeviceHandlePciParser[] = {
> +  {L"PCI Segment", 2, 0, L"0x%x", NULL, NULL, NULL, NULL},
> +  {L"PCI BDF Number", 2, 2, NULL, DumpSratPciBdfNumber, NULL, NULL,
> NULL},
> +  {L"Reserved", 12, 4, L"%x %x %x %x - %x %x %x %x - %x %x %x %x",
> Dump12Chars,
> +   NULL, NULL, NULL}
> +};
> +
>  /**
>    An ACPI_PARSER array describing the Memory Affinity structure.
>  **/
> @@ -159,6 +240,148 @@ ValidateSratReserved (
>    }
>  }
> 
> +/**
> +  This function validates the Device Handle Type field in the Generic 
> +Initiator
> +  Affinity Structure.
> +
> +  @param [in] Ptr     Pointer to the start of the field data.
> +  @param [in] Context Pointer to context specific information e.g. this
> +                      could be a pointer to the ACPI table header.
> +**/
> +STATIC
> +VOID
> +EFIAPI
> +ValidateSratDeviceHandleType (
> +  IN UINT8* Ptr,
> +  IN VOID*  Context
> +  )
> +{
> +  UINT8   DeviceHandleType;
> +
> +  DeviceHandleType = *Ptr;
> +
> +  if (DeviceHandleType > EFI_ACPI_6_3_PCI_DEVICE_HANDLE) {
> +    IncrementErrorCount ();
> +    Print (
> +      L"\nERROR: Invalid Device Handle Type: %d. Must be between 0 
> + and
> %d.",
> +      DeviceHandleType,
> +      EFI_ACPI_6_3_PCI_DEVICE_HANDLE
> +      );
> +  }
> +}
> +
> +/**
> +  This function traces the Device Handle field inside Generic 
> +Initiator
> +  Affinity Structure.
> +
> +  @param [in] Format  Format string for tracing the data.
> +  @param [in] Ptr     Pointer to the start of the buffer.
> +**/
> +STATIC
> +VOID
> +EFIAPI
> +DumpSratDeviceHandle (
> +  IN CONST CHAR16* Format,
> +  IN UINT8*        Ptr
> + )
> +{
> +  if (SratDeviceHandleType == NULL) {
> +    IncrementErrorCount ();
> +    Print (L"\nERROR: Device Handle Type read incorrectly.\n");
> +    return;
> +  }
> +
> +  Print (L"\n");
> +
> +  if (*SratDeviceHandleType == EFI_ACPI_6_3_ACPI_DEVICE_HANDLE) {
> +    ParseAcpi (
> +      TRUE,
> +      2,
> +      NULL,
> +      Ptr,
> +      sizeof (EFI_ACPI_6_3_DEVICE_HANDLE_ACPI),
> +      PARSER_PARAMS (SratDeviceHandleAcpiParser)
> +      );
> +  } else if (*SratDeviceHandleType == EFI_ACPI_6_3_PCI_DEVICE_HANDLE) {
> +    ParseAcpi (
> +      TRUE,
> +      2,
> +      NULL,
> +      Ptr,
> +      sizeof (EFI_ACPI_6_3_DEVICE_HANDLE_PCI),
> +      PARSER_PARAMS (SratDeviceHandlePciParser)
> +      );
> +  }
> +}
> +
> +/**
> +  This function traces the PCI BDF Number field inside Device Handle 
> +- PCI
> +
> +  @param [in] Format  Format string for tracing the data.
> +  @param [in] Ptr     Pointer to the start of the buffer.
> +**/
> +STATIC
> +VOID
> +EFIAPI
> +DumpSratPciBdfNumber (
> +  IN CONST CHAR16* Format,
> +  IN UINT8*        Ptr
> +  )
> +{
> +  CHAR16 Buffer[OUTPUT_FIELD_COLUMN_WIDTH];
> +
> +  Print (L"\n");
> +
> +  /*
> +    The PCI BDF Number subfields are printed in the order specified 
> + in the
> ACPI
> +    specification. The format of the 16-bit PCI BDF Number field is as follows:
> +
> +    +-----+------+------+
> +    |DEV  | FUNC | BUS  |
> +    +-----+------+------+
> +    |15:11| 10:8 |  7:0 |
> +    +-----+------+------+
> +  */
> +
> +  // Print PCI Bus Number (Bits 7:0 of Byte 2)  UnicodeSPrint (
> +    Buffer,
> +    sizeof (Buffer),
> +    L"PCI Bus Number"
> +    );
> +  PrintFieldName (4, Buffer);
> +  Print (
> +    L"0x%x\n",
> +    *Ptr
> +    );
> +
> +  Ptr++;
> +
> +  // Print PCI Device Number (Bits 7:3 of Byte 3)  UnicodeSPrint (
> +    Buffer,
> +    sizeof (Buffer),
> +    L"PCI Device Number"
> +    );
> +  PrintFieldName (4, Buffer);
> +  Print (
> +    L"0x%x\n",
> +    (*Ptr & (BIT7 | BIT6 | BIT5 | BIT4 | BIT3)) >> 3
> +    );
> +
> +  // PCI Function Number (Bits 2:0 of Byte 3)
> +  UnicodeSPrint (
> +    Buffer,
> +    sizeof (Buffer),
> +    L"PCI Function Number"
> +    );
> +  PrintFieldName (4, Buffer);
> +  Print (
> +    L"0x%x\n",
> +    *Ptr & (BIT2 | BIT1 | BIT0)
> +    );
> +}
> +
>  /**
>    This function traces the APIC Proximity Domain field.
> 
> @@ -211,6 +434,7 @@ ParseAcpiSrat (
>    UINT8* ResourcePtr;
>    UINT32 GicCAffinityIndex;
>    UINT32 GicITSAffinityIndex;
> +  UINT32 GenericInitiatorAffinityIndex;
>    UINT32 MemoryAffinityIndex;
>    UINT32 ApicSapicAffinityIndex;
>    UINT32 X2ApicAffinityIndex;
> @@ -218,6 +442,7 @@ ParseAcpiSrat (
> 
>    GicCAffinityIndex = 0;
>    GicITSAffinityIndex = 0;
> +  GenericInitiatorAffinityIndex = 0;
>    MemoryAffinityIndex = 0;
>    ApicSapicAffinityIndex = 0;
>    X2ApicAffinityIndex = 0;
> @@ -247,7 +472,7 @@ ParseAcpiSrat (
>        );
> 
>      switch (*SratRAType) {
> -      case EFI_ACPI_6_2_GICC_AFFINITY:
> +      case EFI_ACPI_6_3_GICC_AFFINITY:
>          AsciiSPrint (
>            Buffer,
>            sizeof (Buffer),
> @@ -264,7 +489,7 @@ ParseAcpiSrat (
>            );
>          break;
> 
> -      case EFI_ACPI_6_2_GIC_ITS_AFFINITY:
> +      case EFI_ACPI_6_3_GIC_ITS_AFFINITY:
>          AsciiSPrint (
>            Buffer,
>            sizeof (Buffer),
> @@ -281,7 +506,24 @@ ParseAcpiSrat (
>          );
>          break;
> 
> -      case EFI_ACPI_6_2_MEMORY_AFFINITY:
> +      case EFI_ACPI_6_3_GENERIC_INITIATOR_AFFINITY:
> +        AsciiSPrint (
> +          Buffer,
> +          sizeof (Buffer),
> +          "Generic Initiator Affinity Structure [%d]",
> +          GenericInitiatorAffinityIndex++
> +        );
> +        ParseAcpi (
> +          TRUE,
> +          2,
> +          Buffer,
> +          ResourcePtr,
> +          *SratRALength,
> +          PARSER_PARAMS (SratGenericInitiatorAffinityParser)
> +        );
> +        break;
> +
> +      case EFI_ACPI_6_3_MEMORY_AFFINITY:
>          AsciiSPrint (
>            Buffer,
>            sizeof (Buffer),
> @@ -298,7 +540,7 @@ ParseAcpiSrat (
>            );
>          break;
> 
> -      case EFI_ACPI_6_2_PROCESSOR_LOCAL_APIC_SAPIC_AFFINITY:
> +      case EFI_ACPI_6_3_PROCESSOR_LOCAL_APIC_SAPIC_AFFINITY:
>          AsciiSPrint (
>            Buffer,
>            sizeof (Buffer),
> @@ -315,7 +557,7 @@ ParseAcpiSrat (
>            );
>          break;
> 
> -      case EFI_ACPI_6_2_PROCESSOR_LOCAL_X2APIC_AFFINITY:
> +      case EFI_ACPI_6_3_PROCESSOR_LOCAL_X2APIC_AFFINITY:
>          AsciiSPrint (
>            Buffer,
>            sizeof (Buffer),
> --
> 'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'
> 
> 
> 
>