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

["Gao, Zhichao" <zhichao.gao@intel.com>]

We can review for your code change. And for our personal point, the change is OK.
But we can't verify this patch on our platform. For the patch which is not, it is better to do a test and get a tested-by from someone.

Thanks,
Zhichao

> -----Original Message-----
> From: Krzysztof Koch [mailto:Krzysztof.Koch@arm.com]
> Sent: Thursday, June 20, 2019 10:54 PM
> To: devel@edk2.groups.io; Carsey, Jaben <jaben.carsey@intel.com>; Ni, Ray
> <ray.ni@intel.com>; Gao, Zhichao <zhichao.gao@intel.com>
> Cc: Sami Mujawar <Sami.Mujawar@arm.com>; nd <nd@arm.com>
> Subject: RE: [edk2-devel] [PATCH v1 2/2] ShellPkg: acpiview: Update SRAT
> parser to ACPI 6.3
> 
> 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)'
> >
> >
> >
> >
> 
> 
>