From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail02.groups.io (mail02.groups.io [66.175.222.108]) by spool.mail.gandi.net (Postfix) with ESMTPS id D4954AC1326 for ; Mon, 24 Jul 2023 14:40:32 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=AT4qzNy1+KRCH7YaTlameGvwQOQvs0QR5Ns60lkfdJ0=; c=relaxed/simple; d=groups.io; h=X-Received:X-Received:X-Received:X-Received:Message-ID:Date:MIME-Version:User-Agent:Subject:To:Cc:References:From:In-Reply-To:Precedence:List-Subscribe:List-Help:Sender:List-Id:Mailing-List:Delivered-To:Reply-To:List-Unsubscribe-Post:List-Unsubscribe:X-Gm-Message-State:Content-Language:Content-Type:Content-Transfer-Encoding; s=20140610; t=1690209631; v=1; b=nwEM2FmuzkToXOo4Jp75SGOFDIg3fdbIx/clZTeZBdwOMHNu9UOaEKqi37bYbGRQZJqmJuud v5IbtS4gUWaDCfcaFtmVX8n3Qa6ONG2I5yMGiTqwQqrf5bR2YCX4c8z/4ep/ZTYQ4pAzj5k0a7M pTQTsbxWpZRaW5ctgjOuQ4sM= X-Received: by 127.0.0.2 with SMTP id rnimYY7687511xpD1kRDe2yv; Mon, 24 Jul 2023 07:40:31 -0700 X-Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by mx.groups.io with SMTP id smtpd.web11.49659.1690209630856629124 for ; Mon, 24 Jul 2023 07:40:31 -0700 X-Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id A6A8BFEC; Mon, 24 Jul 2023 07:41:13 -0700 (PDT) X-Received: from [10.34.100.101] (e126645.nice.arm.com [10.34.100.101]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 19CEE3F67D; Mon, 24 Jul 2023 07:40:28 -0700 (PDT) Message-ID: <3bf3780a-b064-1d60-78ed-0e323726adc7@arm.com> Date: Mon, 24 Jul 2023 16:40:24 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.13.0 Subject: Re: [edk2-devel] [PATCH V3 3/3] ShellPkg/AcpiView: Add MPAM Parser To: Rohit Mathew , "devel@edk2.groups.io" Cc: Thomas Abraham , Sami Mujawar , James Morse , Ray Ni , Zhichao Gao , nd References: <20230522144506.2799121-1-Rohit.Mathew@arm.com> From: "PierreGondois" In-Reply-To: Precedence: Bulk List-Subscribe: List-Help: Sender: devel@edk2.groups.io List-Id: Mailing-List: list devel@edk2.groups.io; contact devel+owner@edk2.groups.io Reply-To: devel@edk2.groups.io,pierre.gondois@arm.com List-Unsubscribe-Post: List-Unsubscribe=One-Click List-Unsubscribe: X-Gm-Message-State: yIQoMhYxUwF5hvn8MZRxM3Ubx7686176AA= Content-Language: en-US Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: quoted-printable X-GND-Status: LEGIT Authentication-Results: spool.mail.gandi.net; dkim=pass header.d=groups.io header.s=20140610 header.b=nwEM2Fmu; dmarc=fail reason="SPF not aligned (relaxed), DKIM not aligned (relaxed)" header.from=arm.com (policy=none); spf=pass (spool.mail.gandi.net: domain of bounce@groups.io designates 66.175.222.108 as permitted sender) smtp.mailfrom=bounce@groups.io Hello Rohit, Thanks for the answers, I should have also answered your questions, Regards, Pierre On 7/24/23 11:50, Rohit Mathew wrote: > Hi Pierre, >=20 > Thank you for the detailed review. Please find my response inline. >=20 >> On 5/22/23 16:45, Rohit Mathew via groups.io wrote: >>> Add a parser for the MPAM (Memory system resource partitioning and >>> monitoring) ACPI table. This parser would parse all MPAM related >>> structures embedded as part of the ACPI table. Necessary validations ar= e >>> also performed where and when required. >>> >>> Signed-off-by: Rohit Mathew >>> --- >>> ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.h = | >> 21 + >>> >> ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Mpam/MpamParser >> .c | 1331 ++++++++++++++++++++ >>> >> ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Mpam/MpamParser >> .h | 25 + >>> >> ShellPkg/Library/UefiShellAcpiViewCommandLib/UefiShellAcpiViewComman >> dLib.c | 3 +- >>> >> ShellPkg/Library/UefiShellAcpiViewCommandLib/UefiShellAcpiViewComman >> dLib.inf | 4 +- >>> >> ShellPkg/Library/UefiShellAcpiViewCommandLib/UefiShellAcpiViewComman >> dLib.uni | 3 +- >>> 6 files changed, 1384 insertions(+), 3 deletions(-) >>> >>> diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.h >> b/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.h >>> index c9f41650d9..fef08e714d 100644 >>> --- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.h >>> +++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.h >>> @@ -825,6 +825,27 @@ ParseAcpiMcfg ( >>> IN UINT8 AcpiTableRevision >>> ); >>> >>> +/** >>> + This function parses the ACPI MPAM table. >>> + When trace is enabled this function parses the MCFG table and >>> + traces the ACPI table fields. >>> + >>> + This function also performs validation of the ACPI table fields. >>> + >>> + @param [in] Trace If TRUE, trace the ACPI fields. >>> + @param [in] Ptr Pointer to the start of the buffer. >>> + @param [in] AcpiTableLength Length of the ACPI table. >>> + @param [in] AcpiTableRevision Revision of the ACPI table. >>> +**/ >>> +VOID >>> +EFIAPI >>> +ParseAcpiMpam ( >>> + IN BOOLEAN Trace, >>> + IN UINT8 *Ptr, >>> + IN UINT32 AcpiTableLength, >>> + IN UINT8 AcpiTableRevision >>> + ); >>> + >>> /** >>> This function parses the ACPI PCCT table including its sub-structu= res >>> of type 0 through 4. >>> diff --git >> a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Mpam/MpamPar >> ser.c >> b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Mpam/MpamPar >> ser.c >>> new file mode 100644 >>> index 0000000000..9352357318 >>> --- /dev/null >>> +++ >> b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Mpam/MpamPar >> ser.c >>> @@ -0,0 +1,1331 @@ >>> +/** @file >>> + MPAM table parser >>> + >>> + Copyright (c) 2023, ARM Limited. All rights reserved. >>> + SPDX-License-Identifier: BSD-2-Clause-Patent >>> + >>> + @par Specification Reference: >>> + - [1] ACPI for Memory System Resource Partitioning and Monitoring 2= .0 >>> + (https://developer.arm.com/documentation/den0065/latest) >>> + >>> + @par Glossary: >>> + - MPAM - Memory System Resource Partitioning And Monitoring >>> + - MSC - Memory System Component >>> + - PCC - Platform Communication Channel >>> + - RIS - Resource Instance Selection >>> + - SMMU - Arm System Memory Management Unit >>> + **/ >>> + >>> +#include >>> +#include >>> +#include >>> +#include "AcpiParser.h" >>> +#include "AcpiView.h" >>> +#include "AcpiViewConfig.h" >>> +#include "MpamParser.h" >>> + >>> +// Local variables >>> +STATIC UINT8 *Locator; >>> +STATIC CONST UINT8 *LocatorType; >>> +STATIC CONST UINT16 *MpamMscNodeLength; >>> +STATIC UINT32 MpamMscNodeLengthCumulative; >>> +STATIC UINT32 HeaderSize; >>> +STATIC CONST UINT32 *ErrorInterrupt; >>> +STATIC CONST UINT32 *InterfaceType; >>> +STATIC CONST UINT32 *NumberOfMscResources; >>> +STATIC CONST UINT32 *NumberOfFunctionalDependencies; >>> +STATIC CONST UINT32 *NumberOfInterconnectDescriptors; >>> +STATIC CONST UINT32 *OverflowInterrupt; >>> +STATIC ACPI_DESCRIPTION_HEADER_INFO AcpiHdrInfo; >>> + >>> +/** >>> + When the length of the table is insufficient to be parsed, this func= tion >> could >>> + be used to display an appropriate error message. >>> + >>> + @param [in] ErrorMsg Error message string that has to be append= ed to >> the >> >> (alignment) >=20 > [Rohit] Ack. >=20 >> >>> + main error log. This string could explain the reason >>> + why a insufficient length error was encountered in >>> + the first place. >>> +**/ >>> +STATIC >>> +VOID >>> +EFIAPI >> >> It seems this function is not used for length related issues. Maybe it s= hould be >> renamed >> and the 'Insufficient MPAM MSC Node length ...' message be removed. >=20 > [Rohit] - I responded to couple of comments addressing why we use this fu= nction, below. Let me know your thoughts. (Again, I moved bottom - up while= addressing comments as it was easier to walk the code that way) >=20 >> >>> +MpamLengthError ( >>> + IN CONST CHAR16 *ErrorMsg >>> + ) >>> +{ >>> + IncrementErrorCount (); >>> + Print (L"\nERROR : "); >>> + Print (ErrorMsg); >>> + Print ( >>> + L"\nError : Insufficient MPAM MSC Node length. Table length : %u.\= n", >>> + *(AcpiHdrInfo.Length) >>> + ); >>> +} >>> + >>> +/** >>> + This function validates the passed reserved parameter. Any reserved = field >>> + within the MPAM specification must be 0. >>> + >>> + @param [in] Reserved Reserved param that has to be validated. >>> +**/ >>> +STATIC >>> +VOID >>> +EFIAPI >> >> It seems the 'Reserved' fields is also checked in the SRAT table parsing= , cf. >> ValidateSratReserved(). Maybe it would be good to re-use your generic >> implementation >> there aswell (just a suggestion, maybe to be done in a later patch). >> >> >=20 > [Rohit] If it's okay, I could work out these changes once we get the MPAM= changes in? Yes sure, it was just a remark. >=20 >>> +ValidateReserved ( >>> + IN CONST UINT64 Reserved >>> + ) >>> +{ >>> + if (Reserved !=3D 0) { >>> + IncrementErrorCount (); >>> + Print (L"\nERROR : Reserved field should be 0\n"); >>> + } >>> +} >>> + >>> +/** >>> + This function validates reserved fields. Any reserved field within t= he >> MPAM >>> + specification must be 0. >>> + >>> + @param [in] Ptr Pointer to the start of the field data. >>> + @param [in] Context Pointer to context specific information. For t= his >>> + particular function, context holds the size of= the >>> + reserved field that needs to be validated. >>> +**/ >>> +STATIC >>> +VOID >>> +EFIAPI >>> +ValidateReservedGeneric ( >>> + IN UINT8 *Ptr, >>> + IN VOID *Context >>> + ) >>> +{ >>> + UINT64 Reserved; >>> + >>> + // If Context is not passed on, don't validate. without the length, = it is not >>> + // possible to decode the reserved field or plan out alignment >> requirements. >>> + if (Context =3D=3D NULL) { >>> + return; >>> + } >>> + >>> + // Only those cases which are handled currently have been implemente= d. >>> + switch ((UINT64)Context) { >>> + case 1: >>> + Reserved =3D *Ptr; >>> + break; >>> + case 2: >>> + if (((UINT64)Ptr) % 2 =3D=3D 0) { >>> + Reserved =3D *((UINT16 *)Ptr); >>> + } else { >>> + Reserved =3D (*(Ptr + 1) << 8); >>> + Reserved |=3D *Ptr; >>> + } >>> + >>> + break; >>> + case 4: >>> + if (((UINT64)Ptr) % 4 =3D=3D 0) { >>> + Reserved =3D *((UINT32 *)Ptr); >>> + } else { >>> + Reserved =3D (*(Ptr + 3) << 24); >>> + Reserved |=3D (*(Ptr + 2) << 16); >>> + Reserved |=3D (*(Ptr + 1) << 8); >>> + Reserved |=3D (*Ptr); >>> + } >>> + >>> + break; >>> + default: >>> + return; >>> + } >>> + >>> + ValidateReserved (Reserved); >>> +} >>> + >>> +/** >>> + Fields that need additional decoding could use this function to prin= t out >> the >>> + decoded value for a particular field. The decoded value handled in t= his >>> + function is a string. >>> + >>> + @param [in] Indent Number of spaces to add to the globa= l table >>> + indent. The global table indent is 0= by >>> + default; however this value is updat= ed on >>> + entry to the ParseAcpi() by adding t= he indent >>> + value provided to ParseAcpi() and re= stored >>> + back on exit. Therefore the total i= ndent in >>> + the output is dependent on from wher= e this >>> + function is called. >>> + @param [in] FieldStr Title string for the field of intere= st. >>> + @param [in] DecodedStr Decoded value to be printed for the = FieldStr. >>> +**/ >>> +STATIC >>> +VOID >>> +EFIAPI >>> +PrintDecodedField ( >>> + IN UINT32 Indent, >>> + IN CONST CHAR16 *FieldStr, >>> + IN CONST CHAR16 *DecodedStr >>> + ) >>> +{ >>> + Print (L"\n"); >>> + PrintFieldName (Indent, FieldStr); >>> + Print (DecodedStr); >>> +} >>> + >>> +/** >>> + This function validates the MMIO size within the MSC node body for >> MPAM ACPI >>> + table. MPAM ACPI specification states that the MMIO size for an MSC >> having PCC >>> + type interface should be zero. >>> + >>> + @param [in] Ptr Pointer to the start of the field data. >>> + @param [in] Context Pointer to context specific information. For = this >>> + function, context holds the parent/double poi= nter to a >>> + variable holding the interface type. Make sur= e to call >>> + the function accordingly. >>> +**/ >>> +STATIC >>> +VOID >>> +EFIAPI >>> +ValidateMmioSize ( >>> + IN UINT8 *Ptr, >>> + IN VOID *Context >>> + ) >>> +{ >>> + UINT8 InterfaceType; >>> + UINT8 **InterfaceTypeParentPtr; >>> + UINT32 MmioSize; >>> + >>> + InterfaceTypeParentPtr =3D (UINT8 **)Context; >> >> I don't think these comments are necessary as this is generic for acpivi= ew. >=20 > [Rohit] I added these comments as I could not find any other use-case of = *Context pointer within any parsers (or anywhere). Therefore, these comment= s were a way to show how 'MPAM parser' utilized context pointer for differe= nt validators. > Also, the way we use context within ValidateReservedGeneric is quite diff= erent from ValidateMmioSize due to compiler restrictions on using constants= for initializer lists. TODO >=20 > If they seem obvious, I could remove them. Let me know. >=20 >> >>> + // Context - double pointer to interface type. (Refer >> MpamMscNodeParse >>> + // table). >>> + // *Context - address of object holding interface type. >>> + // **Context - interface type. >>> + if (*InterfaceTypeParentPtr =3D=3D NULL) { >>> + MpamLengthError (L"Interface type not set!"); >>> + return; >>> + } >>> + >>> + InterfaceType =3D **InterfaceTypeParentPtr; >>> +> + if (InterfaceType =3D=3D EFI_ACPI_MPAM_INTERFACE_PCC) { >>> + MmioSize =3D *((UINT32 *)Ptr); >>> + >>> + if (MmioSize !=3D 0) { >>> + IncrementErrorCount (); >>> + Print ( >>> + L"\nERROR: MMIO size should be 0 for PCC interface type. Size = - %u\n", >>> + MmioSize >>> + ); >>> + } >>> + } >>> +} >>> + >>> +/** >>> + This function decodes and validates the link type for MPAM's interco= nnect >>> + descriptor. Valid links are of NUMA and PROC type. >>> + >>> + @param [in] Ptr Pointer to the start of the field data. >>> + @param [in] Context Pointer to context specific information. For= this >>> + function, context is ignored. >>> +**/ >>> +STATIC >>> +VOID >>> +EFIAPI >>> +DecodeLinkType ( >>> + IN UINT8 *Ptr, >>> + IN VOID *Context >>> + ) >>> +{ >>> + UINT8 LinkType; >>> + >>> + LinkType =3D *Ptr; >>> + >>> + if (LinkType =3D=3D EFI_ACPI_MPAM_LINK_TYPE_NUMA) { >>> + PrintDecodedField ( >>> + 2, >> >> The name of the field should have already been printed, >> I m not sure I see the use of printing: "* Link type decoded". >> Similar comment for the PrintDecodedField() in general, I think >> it would be better to print the value and the interpretation of >> the value, as: NUMA (0) for instance. >> >=20 > [Rohit] Ack - I see that I'm adding a \n to print {decoded string - decod= e value}. So, I could probably have the raw followed by decoded value on th= e same line by avoiding the newline. It would be the other way around I ass= ume - 0 (NUMA). ok yes, both should be fine. >=20 >>> + L"* Link type decoded", >>> + L"NUMA" >>> + ); >>> + } else if (LinkType =3D=3D EFI_ACPI_MPAM_LINK_TYPE_PROC) { >>> + PrintDecodedField ( >>> + 2, >>> + L"* Link type decoded", >>> + L"PROC" >>> + ); >>> + } else { >>> + IncrementErrorCount (); >>> + Print ( >>> + L"\nERROR: Invalid link type - %u\n", >>> + (UINT32)LinkType >>> + ); >>> + } >>> +} >>> + >>> +/** >>> + This function decodes the hardware ID field present within MPAM ACPI >> table. >>> + The specification states that the hardware ID has to be set to zero = if not >>> + being used. >>> + >>> + @param [in] Ptr Pointer to the start of the field data. >>> + @param [in] Context Pointer to context specific information. For= this >>> + function, context is ignored. >>> +**/ >>> +STATIC >>> +VOID >>> +EFIAPI >>> +DecodeHardwareId ( >>> + IN UINT8 *Ptr, >>> + IN VOID *Context >>> + ) >>> +{ >>> + UINT64 HardwareId; >>> + >> >> I don't think this comment is necessary. >=20 > [Rohit] Ack >=20 >> >>> + // Hardware Id fields within MPAM table always falls on offset divis= ible by >> 8. >>> + // Proceed to dereference 8 bytes in one go. >>> + HardwareId =3D *((UINT64 *)Ptr); >>> + >>> + if (HardwareId !=3D 0) { >>> + Print (L"\n"); >> >> Is it necessary to print '* Hardware ID type decoded' as the field name = should >> have already been printed ? >> >=20 > [Rohit] Ack. (responded to a similar comment, above) >=20 >>> + PrintFieldName (2, L"* Hardware ID type decoded"); >>> + Dump8Chars (NULL, Ptr); >>> + } >>> +} >>> + >>> +/** >>> + This function decodes and validates the interface type for MPAM. Val= id >>> + interfaces are of MMIO and PCC type. >>> + >>> + @param [in] Ptr Pointer to the start of the field data. >>> + @param [in] Context Pointer to context specific information. For= this >>> + function, context is ignored. >>> +**/ >>> +STATIC >>> +VOID >>> +EFIAPI >>> +DecodeInterfaceType ( >>> + IN UINT8 *Ptr, >>> + IN VOID *Context >>> + ) >>> +{ >>> + UINT8 InterfaceType; >>> + >>> + InterfaceType =3D *Ptr; >>> + >>> + if (InterfaceType =3D=3D EFI_ACPI_MPAM_INTERFACE_MMIO) { >>> + PrintDecodedField ( >>> + 2, >>> + L"* Interface type decoded", >>> + L"MMIO" >>> + ); >>> + } else if (InterfaceType =3D=3D EFI_ACPI_MPAM_INTERFACE_PCC) { >>> + PrintDecodedField ( >>> + 2, >>> + L"* Interface type decoded", >>> + L"PCC" >>> + ); >>> + } else { >>> + IncrementErrorCount (); >>> + Print ( >>> + L"\nERROR: I ype - %u\n", >>> + (UINT32)InterfaceType >>> + ); >>> + } >>> +} >>> + >>> +/** >>> + This function decodes and validates the interrupt flags present in t= he >> MPAM >>> + MSC node body. >>> + >>> + @param [in] Ptr Pointer to the start of the field data. >>> + @param [in] Context Pointer to context specific information. For = this >>> + function, context holds the parent/double poi= nter to a >>> + variable holding the interrupt gsiv. Make sur= e to call >>> + the function accordingly. >>> +**/ >>> +STATIC >>> +VOID >>> +EFIAPI >>> +DecodeInterruptFlags ( >>> + IN UINT8 *Ptr, >>> + IN VOID *Context >>> + ) >>> +{ >>> + UINT8 InterruptMode; >>> + UINT8 InterruptType; >>> + UINT8 InterruptAffinityType; >>> + UINT8 InterruptAffinityValid; >>> + UINT32 InterruptFlag; >>> + UINT32 InterruptGsiv; >>> + UINT32 **InterruptGsivParentPtr; >>> + UINT32 Reserved; >>> + >>> + InterruptGsivParentPtr =3D (UINT32 **)Context; >>> + // Context - double pointer to the gsiv object. (Refer >> MpamMscNodeParse >>> + // table). >>> + // *Context - pointer to gsiv object. >>> + // **Context - gsiv. >>> + if (*InterruptGsivParentPtr =3D=3D NULL) { >>> + MpamLengthError (L"Interrupt gsiv not set!"); >>> + return; >>> + } >>> + >>> + InterruptGsiv =3D **InterruptGsivParentPtr; >>> + >>> + // If Interrupts are absent for some reason, the flags wouldn't need= any >>> + // parsing. >>> + if (InterruptGsiv =3D=3D 0) { >>> + return; >>> + } >> >> I think there is something to parse bitfields in acpiview, cf: >> https://edk2.groups.io/g/devel/message/105029 >> >> Would it fit this field parsing aswell ? >> >=20 > [Rohit] Sorry, the link points to V2 of this patch. Was this the intended= link? Yes right, sorry for the wrong link, I meant: https://edk2.groups.io/g/devel/message/106957 >=20 >>> + >>> + InterruptFlag =3D *((UINT32 *)Ptr); >>> + >>> + // Decode interrupt mode. >>> + InterruptMode =3D (InterruptFlag & >> EFI_ACPI_MPAM_INTERRUPT_MODE_MASK) >>> + >> EFI_ACPI_MPAM_INTERRUPT_MODE_SHIFT; >>> + >>> + if (InterruptMode =3D=3D EFI_ACPI_MPAM_INTERRUPT_LEVEL_TRIGGERED) { >>> + PrintDecodedField ( >>> + 2, >>> + L"* Interrupt mode decoded", >>> + L"Level triggered" >>> + ); >>> + } else { >>> + PrintDecodedField ( >>> + 2, >>> + L"* Interrupt mode decoded", >>> + L"Edge triggered" >>> + ); >>> + } >>> + >>> + // Decode interrupt type. >>> + InterruptType =3D (InterruptFlag & >> EFI_ACPI_MPAM_INTERRUPT_TYPE_MASK) >>> + >> EFI_ACPI_MPAM_INTERRUPT_TYPE_SHIFT; >>> + >>> + if (InterruptType =3D=3D EFI_ACPI_MPAM_INTERRUPT_WIRED) { >>> + PrintDecodedField ( >>> + 2, >>> + L"* Interrupt type decoded", >>> + L"Wired interrupt" >>> + ); >>> + } else { >>> + IncrementErrorCount (); >>> + Print (L"\n"); >>> + PrintFieldName (2, L"* Interrupt type decoded"); >>> + Print (L"%u", InterruptType); >>> + Print (L"\nERROR : Interrupt type reserved\n"); >>> + } >>> + >>> + // Decode affinity valid. >>> + InterruptAffinityValid =3D (InterruptFlag >>> + & EFI_ACPI_MPAM_INTERRUPT_AFFINITY_VALID_M= ASK) >>> + >> EFI_ACPI_MPAM_INTERRUPT_AFFINITY_VALID_S= HIFT; >>> + >>> + if (InterruptAffinityValid !=3D >> EFI_ACPI_MPAM_INTERRUPT_AFFINITY_VALID) { >>> + PrintDecodedField ( >>> + 2, >>> + L"* Interrupt affinity valid decoded", >>> + L"Affinity not valid" >>> + ); >>> + } else { >>> + PrintDecodedField ( >>> + 2, >>> + L"* Interrupt affinity valid decoded", >>> + L"Affinity valid" >>> + ); >>> + >>> + // Decode affinity type. >>> + InterruptAffinityType =3D (InterruptFlag >>> + & EFI_ACPI_MPAM_INTERRUPT_AFFINITY_TYPE_M= ASK) >>> + >> EFI_ACPI_MPAM_INTERRUPT_AFFINITY_TYPE_S= HIFT; >>> + >>> + if (InterruptAffinityType =3D=3D >> EFI_ACPI_MPAM_INTERRUPT_PROCESSOR_AFFINITY) { >>> + PrintDecodedField ( >>> + 2, >>> + L"* Interrupt affinity type decoded", >>> + L"Processor affinity" >>> + ); >>> + } else { >>> + PrintDecodedField ( >>> + 2, >>> + L"* Interrupt affinity type decoded", >>> + L"Processor container affinity" >>> + ); >>> + } >>> + } >>> + >>> + // Decode reserved field. >>> + Reserved =3D (InterruptFlag & >> EFI_ACPI_MPAM_INTERRUPT_RESERVED_MASK) >>> + >> EFI_ACPI_MPAM_INTERRUPT_RESERVED_SHIFT; >>> + Print (L"\n"); >>> + PrintFieldName (2, L"* Reserved decoded"); >>> + Print (L"%u", Reserved); >>> + >>> + ValidateReserved (Reserved); >>> +} >>> + >>> +/** >>> + ACPI_PARSER array describing the Generic ACPI MPAM table header. >>> +**/ >>> +STATIC CONST ACPI_PARSER MpamParser[] =3D { >>> + PARSE_ACPI_HEADER (&AcpiHdrInfo) >>> +}; >>> + >>> +/** >>> + ACPI_PARSER array describing the MPAM MSC node object. >>> +**/ >>> +STATIC CONST ACPI_PARSER MpamMscNodeParser[] =3D { >>> + { L"Length", 2, 0, L"%u", NULL, >>> + (VOID **)&MpamMscNodeLength, NULL, NULL }, >>> + // Once Interface type is decoded, the address of interface type fie= ld is >>> + // captured into InterfaceType pointer so that it could be used to c= heck if >>> + // MMIO Size field is set as per the specification. >>> + { L"Interface type", 1, 2, L"0x%x", NULL, >>> + (VOID **)&InterfaceType, DecodeInterfaceType, NULL }, >>> + { L"Reserved", 1, 3, L"0x%x", NULL, >>> + NULL, ValidateReservedGeneric, (VOID *)1 }, >>> + { L"Identifier", 4, 4, L"%u", NULL, >>> + NULL, NULL, NULL }, >>> + { L"Base address", 8, 8, L"0x%lx", NULL, >>> + NULL, NULL, NULL }, >>> + { L"MMIO Size", 4, 16, L"0x%x", NULL, >>> + NULL, ValidateMmioSize, (VOID **)&InterfaceType }, >>> + { L"Overflow interrupt", 4, 20, L"%u", NULL, >>> + (VOID **)&OverflowInterrupt, NULL, NULL }, >>> + { L"Overflow interrupt flags", 4, 24, L"0x%x", NULL, >>> + // Initializer list has to have constants. Pass the address of >>> + // OverflowInterrupt to satisfy this. While using it within the va= lidator >>> + // make sure to dereference accordingly. >>> + NULL, DecodeInterruptFlags, (VOID *)&OverflowInterrupt }, >>> + { L"Reserved1", 4, 28, L"0x%x", NULL, >>> + NULL, ValidateReservedGeneric, (VOID *)4 }, >>> + { L"Overflow interrupt affinity", 4, 32, L"0x%x", NULL, >>> + NULL, NULL, NULL }, >>> + { L"Error interrupt", 4, 36, L"%u", NULL, >>> + (VOID **)&ErrorInterrupt, NULL, NULL }, >>> + { L"Error interrupt flags", 4, 40, L"0x%x", NULL, >>> + // Initializer list has to have constants. Pass the address of >>> + // OverflowInterrupt to satisfy this. While using it within the va= lidator >>> + // make sure to dereference accordingly. >>> + NULL, DecodeInterruptFlags, (VOID *)&ErrorInterrupt }, >>> + { L"Reserved2", 4, 44, L"0x%x", NULL, >>> + NULL, ValidateReservedGeneric, (VOID *)4 }, >>> + { L"Error interrupt affinity", 4, 48, L"0x%x", NULL, >>> + NULL, NULL, NULL }, >>> + { L"MAX_NRDY_USEC", 4, 52, L"0x%x", NULL, >>> + NULL, NULL, NULL }, >>> + { L"Hardware ID of linked device", 8, 56, L"0x%lx", NULL, >>> + NULL, DecodeHardwareId, NULL }, >>> + { L"Instance ID of linked device", 4, 64, L"0x%x", NULL, >>> + NULL, NULL, NULL }, >>> + { L"Number of resource nodes", 4, 68, L"%u", NULL, >>> + (VOID **)&NumberOfMscResources, NULL, NULL } >>> +}; >>> + >>> +/** >>> + ACPI_PARSER array describing the MPAM MSC resource fields till locat= or >> type. >>> +**/ >>> +STATIC CONST ACPI_PARSER MpamMscResourceParser[] =3D { >>> + { L"Identifier", 4, 0, L"%u", NULL, NULL, NULL, NULL }, >>> + { L"RIS index", 1, 4, L"%u", NULL, NULL, NULL, NULL }, >>> + { L"Reserved1", 2, 5, L"0x%x", NULL, >>> + NULL, ValidateReservedGeneric, (VOID *)2 }, >>> + { L"Locator type", 1, 7, L"0x%x", NULL, >>> + (VOID **)&LocatorType, >>> + NULL, NULL } >>> +}; >>> + >>> +/** >>> + ACPI_PARSER array describing the MPAM MSC resource locator field. >>> +**/ >>> +STATIC CONST ACPI_PARSER MpamMscResourceLocatorParser[] =3D { >>> + { L"Locator", 12, 0, NULL, NULL, (VOID **)&Locator, NULL, >>> + NULL } >>> +}; >>> + >>> +/** >>> + ACPI_PARSER array describing the MPAM MSC resource's functional >> dependencies >>> + count. >>> +**/ >>> +STATIC CONST ACPI_PARSER >> MpamMscFunctionalDependencyCountParser[] =3D { >>> + { L"Number of func dependencies", 4, 0, L"%u", NULL, >>> + (VOID **)&NumberOfFunctionalDependencies, NULL, NULL } >>> +}; >>> + >>> +/** >>> + ACPI_PARSER array describing the MPAM MSC resource's functional >> dependencies. >>> +**/ >>> +STATIC CONST ACPI_PARSER MpamMscFunctionalDependencyParser[] =3D { >>> + { L"Producer", 4, 0, L"0x%x", NULL, NULL, NULL, NULL }, >>> + { L"Reserved", 4, 4, L"0x%x", NULL, >>> + NULL, ValidateReservedGeneric, (VOID *)4 }, >>> +}; >>> + >>> +/** >>> + ACPI_PARSER array describing the interconnect descriptor table assoc= iated >> with >>> + the interconnect locator type. >>> +**/ >>> +STATIC CONST ACPI_PARSER MpamInterconnectDescriptorTableParser[] =3D = { >>> + { L"Signature", 16, 0, >>> + L"%x%x%x%x-%x%x-%x%x-%x%x-%x%x%x%x%x%x", Dump16Chars, >> NULL, NULL, NULL }, >>> + { L"Number of Interconnect desc", 4, 16,L"0x%x", NULL, >>> + (VOID **)&NumberOfInterconnectDescriptors, NULL, NULL } >>> +}; >>> + >>> +/** >>> + ACPI_PARSER array describing the interconnect descriptor associated = with >> the >>> + interconnect locator type. The specification includes an additional = reserved >>> + field also within the interconnect descriptor. This is not included = in the >>> + parser object on purpose. The function is of 3 bytes length which th= e >>> + ParseAcpi function can't parse. This has been handled separately. >>> +**/ >>> +STATIC CONST ACPI_PARSER MpamInterconnectDescriptorParser[] =3D { >>> + { L"Source ID", 4, 0, L"%u", NULL, NULL, NULL, NULL }, >>> + { L"Destination ID", 4, 4, L"%u", NULL, NULL, NULL, NULL }, >>> + { L"Link type", 1, 8, L"0x%x", NULL, >>> + NULL, DecodeLinkType, NULL } >>> +}; >>> + >>> +/** >>> + PrintLocatorTitle could be used to print the decoded locator title. >>> + >>> + @param [in] Indent Number of spaces to add to the globa= l table >>> + indent. The global table indent is 0= by >>> + default; however this value is updat= ed on >>> + entry to the ParseAcpi() by adding t= he indent >>> + value provided to ParseAcpi() and re= stored >>> + back on exit. Therefore the total i= ndent in >>> + the output is dependent on from wher= e this >>> + function is called. >>> + @param [in] LocatorTitle Title string to be used for the loca= tor. >>> +**/ >>> +STATIC >>> +VOID >>> +EFIAPI >>> +PrintLocatorTitle ( >>> + IN UINT32 Indent, >>> + IN CONST CHAR16 *LocatorTitle >>> + ) >>> +{ >>> + PrintFieldName (Indent, L"* Locator type decoded"); >>> + Print (LocatorTitle); >>> + Print (L"\n"); >>> + PrintFieldName (Indent, L"Locator"); >>> + Print (L"\n"); >>> +} >>> + >>> +/** >>> + PrintLocatorDescriptor64 could be used to print the 8 byte field >>> + within the 12 byte locator descriptor. >>> + >>> + @param [in] Indent Number of spaces to add to the globa= l table >>> + indent. The global table indent is 0= by >>> + default; however this value is updat= ed on >>> + entry to the ParseAcpi() by adding t= he indent >>> + value provided to ParseAcpi() and re= stored >>> + back on exit. Therefore the total i= ndent in >>> + the output is dependent on from wher= e this >>> + function is called. >>> + @param [in] DescriptorTitle Title string to be used for the desc= riptor. >>> + @param [in] Descriptor64 Descriptor to be printed. >>> + @param [in] Validate Boolean to indicate if the second 4 = byte field >>> + needs to be validated as a reserved = field. >>> +**/ >>> +STATIC >>> +VOID >>> +EFIAPI >>> +PrintLocatorDescriptor64 ( >>> + IN UINT32 Indent, >>> + IN CONST CHAR16 *DescriptorTitle, >>> + IN CONST UINT64 Descriptor64, >>> + IN BOOLEAN Validate >>> + ) >>> +{ >>> + PrintFieldName (Indent, DescriptorTitle); >>> + Print (L"%lu", Descriptor64); >>> + >>> + if (Validate) { >>> + ValidateReserved (Descriptor64); >>> + } >>> + >>> + Print (L"\n"); >>> +} >>> + >>> +/** >>> + PrintLocatorDescriptor32 could be used to print the 4 byte field >>> + within the 12 byte locator descriptor. >>> + >>> + @param [in] Indent Number of spaces to add to the globa= l table >>> + indent. The global table indent is 0= by >>> + default; however this value is updat= ed on >>> + entry to the ParseAcpi() by adding t= he indent >>> + value provided to ParseAcpi() and re= stored >>> + back on exit. Therefore the total i= ndent in >>> + the output is dependent on from wher= e this >>> + function is called. >>> + @param [in] DescriptorTitle Title string to be used for the desc= riptor. >>> + @param [in] Descriptor32 Descriptor to be printed. >>> + @param [in] Validate Boolean to indicate if the second 4 = byte field >>> + needs to be validated as a reserved = field. >>> +**/ >>> +STATIC >>> +VOID >>> +EFIAPI >>> +PrintLocatorDescriptor32 ( >>> + IN UINT32 Indent, >>> + IN CONST CHAR16 *DescriptorTitle, >>> + IN CONST UINT32 Descriptor32, >>> + IN BOOLEAN Validate >>> + ) >>> +{ >>> + PrintFieldName (Indent, DescriptorTitle); >>> + Print (L"0x%x", Descriptor32); >>> + >>> + if (Validate) { >>> + ValidateReserved (Descriptor32); >>> + } >>> + >>> + Print (L"\n"); >>> +} >>> + >>> +/** >>> + PrintGenericLocatorDescriptor should be used for decoding and printi= ng >> locator >>> + descriptor that could be split as two 8 and 4 byte fields. The Locat= orPtr >>> + field is casted to (UINT64 *) and decoded within the macro. From the >> MPAM ACPI >>> + specification, the offset of the locator descriptor field is divisib= le by 8. >>> + It is assumed that ACPI tables are placed at 8 byte aligned address = and >> thus >>> + unaligned access should not be a concern. >>> + >>> + Only locators which have its second field as reserved should use thi= s >>> + function. >>> + >>> + @param [in] Indent Number of spaces to add to the globa= l table >>> + indent. The global table indent is 0= by >>> + default; however this value is updat= ed on >>> + entry to the ParseAcpi() by adding t= he indent >>> + value provided to ParseAcpi() and re= stored >>> + back on exit. Therefore the total i= ndent in >>> + the output is dependent on from wher= e this >>> + function is called. >>> + @param [in] LocatorTitle Title string to be used for the loca= tor >>> + @param [in] Descriptor1Title Title string to be used for descript= or1 >>> + @param [in] Descriptor2Title Title string to be used for descript= or2 >>> + @param [in] LocatorPtr Ptr to the start of locator >>> + @param [in] Validate Boolean to indicate if the second 4 = byte field >>> + needs to be validated as a reserved = field. >>> +**/ >>> +STATIC >>> +VOID >>> +EFIAPI >>> +PrintGenericLocatorDescriptor ( >>> + IN UINT32 Indent, >>> + IN CONST CHAR16 *LocatorTitle, >>> + IN CONST CHAR16 *Descriptor1Title, >>> + IN CONST CHAR16 *Descriptor2Title, >>> + IN UINT8 *LocatorPtr, >>> + IN BOOLEAN Validate >>> + ) >>> +{ >>> + PrintLocatorTitle (Indent, LocatorTitle); >>> + PrintLocatorDescriptor64 ( >>> + Indent, >>> + Descriptor1Title, >>> + *((UINT64 *)LocatorPtr), >>> + FALSE >>> + ); >>> + >>> + // Move descriptor to point to next field. >>> + LocatorPtr +=3D sizeof (UINT64); >>> + >>> + PrintLocatorDescriptor32 ( >>> + Indent, >>> + Descriptor2Title, >>> + *((UINT64 *)LocatorPtr), >>> + Validate >>> + ); >>> +} >>> + >>> +/** >>> + This function parses the locator field within the resource node for = ACPI >> MPAM >>> + table. The parsing is based on the locator type field. >>> + >>> + This function also performs validation of the locator field. >>> + **/ >>> +STATIC >>> +VOID >>> +EFIAPI >>> +ParseLocator ( >>> + VOID >>> + ) >>> +{ >>> + UINT8 *LocatorPtr; >>> + >>> + LocatorPtr =3D Locator; >>> + >>> + switch (*LocatorType) { >> >> I think it would be simpler to define names as: >> >> STATIC CONST CHAR16 *MpamLocationNames[] =3D { >> L"Processor cache", >> L"Memory", >> ... >> >> and also to define ACPI_PARSER tables for the locator descriptors >> instead of using PrintGenericLocatorDescriptor(). >> Eg: >> STATIC CONST ACPI_PARSER SmmuLocatorDescriptorParser[] =3D { >> { L"SMMU interface", 8, 0, L"%lu", NULL, NULL, NULL, NULL }, >> { L"Reserved ID", 4, 8, L"%u", NULL, NULL, ValidateReservedGeneric, = (VOID >> *)2 }, >> >=20 > [Rohit] The only reason I did not want to do this was to avoid manually m= oving the offset back by x bytes to reparse the locator. We parse the locat= or using MpamMscResourceLocatorParser. If we would need to use ACPI_PARSER,= we would need to step back by 12 bytes (assuming offset is used right afte= r we parse the locator) and reparse the locator under the respective switch= case. We might not be able to skip MpamMscResourceLocatorParser as EFI_ACP= I_MPAM_LOCATION_MEMORY_CACHE can't be parsed by ACPI_PARSER. Would this be = cleaner, what are your thoughts? Ok right, I misread the structure the first time. In that case, would it be possible to use ParseLocator() (or a remake of th= e function) as a ACPI_PARSER's PrintFormatter() callback ? Cf. the comment below, I think this should be possible to parse a EFI_ACPI_= MPAM_LOCATION_MEMORY_CACHE struct using a ACPI_PARSER structure. >=20 >> >>> + case EFI_ACPI_MPAM_LOCATION_PROCESSOR_CACHE: >>> + PrintGenericLocatorDescriptor ( >>> + 4, >>> + L"Processor cache", >>> + L"* Cache reference", >>> + L"* Reserved", >>> + LocatorPtr, >>> + TRUE >>> + ); >>> + break; >>> + case EFI_ACPI_MPAM_LOCATION_MEMORY: >>> + PrintGenericLocatorDescriptor ( >>> + 4, >>> + L"Memory", >>> + L"* Proximity domain", >>> + L"* Reserved", >>> + LocatorPtr, >>> + TRUE >>> + ); >>> + break; >>> + case EFI_ACPI_MPAM_LOCATION_SMMU: >>> + PrintGenericLocatorDescriptor ( >>> + 4, >>> + L"SMMU", >>> + L"* SMMU interface", >>> + L"* Reserved", >>> + LocatorPtr, >>> + TRUE >>> + ); >>> + break; >>> + case EFI_ACPI_MPAM_LOCATION_MEMORY_CACHE: >> >> The code would be more generic with ACPI_PARSER structures I think >=20 > [Rohit] I believe ACPI_PARSER won't parse Memory-side cache locator descr= iptor due to its odd field length. I think there is a similar case for the Reserved field of the 'GT Block Tim= er Structure', Cf ACPI 6.5, Table 5.121: GT Block Timer Structure Format and in the GtdtParser for the GtBlockTimerParser structure. A Dump7Chars function would need to be added for our case I beleive. >=20 > // snippet from ACPI_PARSER's code >=20 > switch (Parser[Index].Length) { > case 1: > DumpUint8 (Parser[Index].Format, Ptr); > break; > case 2: > DumpUint16 (Parser[Index].Format, Ptr); > break; > case 4: > DumpUint32 (Parser[Index].Format, Ptr); > break; > case 8: > DumpUint64 (Parser[Index].Format, Ptr); > break; > default: > Print ( > L"\nERROR: %a: CANNOT PARSE THIS FIELD, Field Length =3D %= d\n", > AsciiName, > Parser[Index].Length > ); > } // switch > =09 > I have not tested this - but I do remember seeing such a behavior for Int= erconnect descriptor's reserved field. >=20 >> >>> + // PrintGenericLocatorDescriptor can't be used here as the field= s >>> + // For a memory cache locator descriptor don't fall in the 64bit= -32 bit >>> + // field length division. Parse these fields manually. >>> + PrintLocatorTitle (4, L"Memory cache"); >>> + >>> + // Parse field 1 >>> + PrintLocatorDescriptor64 ( >>> + 4, >>> + L"* Reserved", >>> + MPAM_MEMORY_LOCATOR_EXTRACT_RESERVED_FIELD ( >>> + *((UINT64 *)(LocatorPtr)) >>> + ), >>> + TRUE >>> + ); >>> + >>> + // Parse field 2 >>> + PrintLocatorDescriptor64 ( >>> + 4, >>> + L"* Level", >>> + MPAM_MEMORY_LOCATOR_EXTRACT_LEVEL_FIELD ( >>> + *((UINT64 *)(LocatorPtr)) >>> + ), >>> + FALSE >>> + ); >>> + >>> + LocatorPtr +=3D sizeof (UINT64); >>> + >>> + // Parse field 3 >>> + PrintLocatorDescriptor32 ( >>> + 4, >>> + L"* Reference", >>> + *((UINT32 *)(LocatorPtr)), >>> + FALSE >>> + ); >>> + break; >>> + case EFI_ACPI_MPAM_LOCATION_ACPI_DEVICE: >>> + // ACPI hardware ID would have to printed via Dump8Chars. >>> + PrintLocatorTitle (4, L"ACPI device"); >>> + PrintFieldName (4, L"* ACPI hardware ID"); >>> + Dump8Chars (NULL, LocatorPtr); >>> + Print (L"\n"); >>> + >>> + LocatorPtr +=3D sizeof (UINT64); >>> + >>> + // Parse field 2 >>> + PrintLocatorDescriptor32 ( >>> + 4, >>> + L"* ACPI unique ID", >>> + *((UINT32 *)(LocatorPtr)), >>> + FALSE >>> + ); >>> + break; >>> + case EFI_ACPI_MPAM_LOCATION_INTERCONNECT: >> >> I m not sure I understand why ParseInterconnectDescriptorTable () >> is not called from here as the pointer to the struct is available here. >> >=20 > [Rohit] All of the locators except interconnect locator have very simple = parsing for their fields. This would mean keeping the prototype for ParseLo= cator simple without any params related to ACPI parser pointer, offset etc = provided we offload the interconnect descriptor's internal parsing to a sco= pe where we have these fields available. We could very well do the parsing = internally - however this would mean changing the prototype just for interc= onnect descriptor locator. >=20 > The prototype change is twofold (return and params). Without ParseInterco= nnectDescriptorTable, we could get away without returning anything. However= , if we have ParseInterconnectDescriptorTable handled within ParseLocator, = we would need to handle the return as well. Since interconnect locator is a= n exception with respect to all other locators, it is handled as an excepti= on outside of ParseLocator. If we had quite a lot of locators with detailed= internal parsing, we should have handled this internally. Maybe there's something I don't understand correctly, but I think I m readi= ng the code while in the optic of having a ACPI_PARSER structure (and a PrintFormatter) for each locator. In this case, from withi= n the interconnect locator parser, the offset of the Interconnect descriptor is available and could be parsed using another ACPI= _PARSER structure. I m not sure what I say is really clear, please ping me in case it's not. >=20 >>> + PrintGenericLocatorDescriptor ( >>> + 4, >>> + L"Interconnect", >>> + L"* Interconnect desc tbl offset", >>> + L"* Reserved", >>> + LocatorPtr, >>> + TRUE >>> + ); >>> + break; >>> + case EFI_ACPI_MPAM_LOCATION_UNKNOWN: >>> + PrintGenericLocatorDescriptor ( >>> + 4, >>> + L"Unknown", >>> + L"* Descriptor1", >>> + L"* Descriptor2", >>> + LocatorPtr, >>> + FALSE >>> + ); >>> + break; >>> + default: >>> + Print (L"\nWARNING : Reserved locator type\n"); >>> + >>> + IncrementWarningCount (); >>> + break; >>> + } // switch >>> +} >>> + >>> +/** >>> + PrintBlockTitle could be used to print the title of blocks that >>> + appear more than once in the MPAM ACPI table. >>> + >>> + @param [in] Indent Number of spaces to add to the globa= l table >>> + indent. The global table indent is 0= by >>> + default; however this value is updat= ed on >>> + entry to the ParseAcpi() by adding t= he indent >>> + value provided to ParseAcpi() and re= stored >>> + back on exit. Therefore the total i= ndent in >>> + the output is dependent on from wher= e this >>> + function is called. >>> + @param [in] Title Title string to be used for the bloc= k. >>> + @param [in] Index Index of the block. >>> +**/ >>> +STATIC >>> +VOID >>> +EFIAPI >>> +PrintBlockTitle ( >>> + IN UINT32 Indent, >>> + IN CONST CHAR16 *Title, >>> + IN CONST UINT32 Index >>> + ) >>> +{ >>> + Print (L"\n"); >>> + PrintFieldName (Indent, Title); >>> + Print (L"%u\n\n", Index); >>> +} >>> + >>> +/** >> >> asssociated -> associated >> I think this might be caught by running the CI locally on the ShellPkg >> or by making a 'fake' pull request to trigger the edk2 upstream CI. >=20 >=20 > [Rohit] Ack. I did run Uncrustify and staurt CI. I very well could have m= issed this bit. Thanks for the hint. >=20 >> >>> + This function parses the interconnect descriptor(s) asssociated with >>> + an interconnect type locator object. >>> + >>> + @param [in] Ptr Pointer to the start of the buffer. >>> + @param [in] AcpiTableLength Length of the ACPI table. >>> + @param [in] Offset Pointer to current offset within Ptr. >>> + >>> +Returns: >>> + >>> + Status >>> + >>> + EFI_SUCCESS MPAM MSC Nodes were parsed properly. >>> + EFI_BAD_BUFFER_SIZE The buffer pointer provided as input = is not >>> + long enough to be parsed correctly. >>> +**/ >>> +STATIC >>> +EFI_STATUS >>> +EFIAPI >>> +ParseInterconnectDescriptors ( >>> + IN UINT8 *Ptr, >>> + IN UINT32 AcpiTableLength, >>> + IN UINT32 *Offset >>> + ) >>> +{ >>> + UINT32 InterconnectDescriptorIndex; >>> + UINT32 Reserved; >>> + >>> + InterconnectDescriptorIndex =3D 0; >>> + >>> + // Parse MPAM MSC resources within the MSC body >> >> I m not sure this case is possible as NumberOfInterconnectDescriptors is >> statically defined. >> >=20 > [Rohit] I have responded to comments for NumberOfFunctionalDependencies a= nd MpamMscNodeLength addressing this concern. Sorry, I had to start from th= e bottom while addressing comments as that helped with the code flow :) Ok yes no worries :) >=20 >>> + if (NumberOfInterconnectDescriptors =3D=3D NULL) { >>> + MpamLengthError (L"Number of interconnect descriptors not set!"); >>> + return EFI_BAD_BUFFER_SIZE; >>> + } >>> + >>> + while (InterconnectDescriptorIndex < *NumberOfInterconnectDescriptor= s) >> { >>> + PrintBlockTitle ( >>> + 6, >>> + L"* Interconnect descriptor *", >>> + InterconnectDescriptorIndex >>> + ); >>> + >>> + // Parse interconnect descriptor >>> + *Offset +=3D ParseAcpi ( >>> + TRUE, >>> + 4, >>> + NULL, >>> + Ptr + *Offset, >>> + AcpiTableLength - *Offset, >>> + PARSER_PARAMS (MpamInterconnectDescriptorParser) >>> + ); >>> + >>> + Reserved =3D *((UINT32 *)(Ptr + *Offset)) & 0x00FFFFFF; >>> + PrintFieldName (6, L"Reserved"); >>> + Print (L"%u\n", Reserved); >>> + ValidateReserved (Reserved); >>> + *Offset +=3D 3; >>> + >>> + InterconnectDescriptorIndex++; >>> + } >>> + >>> + return EFI_SUCCESS; >>> +} >>> + >>> +/** >>> + This function parses the interconnect descriptor table asssociated w= ith an >>> + interconnect type locator object. It also performs necessary validat= ion to >>> + make sure the interconnect descriptor is at a valid location. >>> + >>> + @param [in] Ptr Pointer to the start of the buffer. >>> + @param [in] AcpiTableLength Length of the ACPI table. >>> + @param [in] Offset Pointer to current offset within Ptr. >>> + @param [in] InterconnectOffset Offset to the interconnect descriptor >> table. >>> + >>> +Returns: >>> + >>> + Status >>> + >>> + EFI_SUCCESS MPAM MSC Nodes were parsed properly. >>> + EFI_BAD_BUFFER_SIZE The buffer pointer provided as input = is not >>> + long enough to be parsed correctly. >>> + EFI_INVALID_PARAMETER The Offset parameter encoded within t= he >> Ptr >>> + buffer is not valid. >>> +**/ >>> +STATIC >>> +EFI_STATUS >>> +EFIAPI >>> +ParseInterconnectDescriptorTable ( >>> + IN UINT8 *Ptr, >>> + IN UINT32 AcpiTableLength, >>> + IN UINT32 *Offset, >>> + IN UINT64 InterconnectOffset >>> + ) >>> +{ >>> + EFI_STATUS Status; >>> + >>> + // The specification doesn't strictly state that the interconnect ta= ble be >>> + // placed exactly after say, functional dependency table or the reso= urce >> node. >>> + // Instead, the interconnect locator's descriptor field 1 gives the = offset >>> + // from the start of the MSC node where the table could be found. >>> + if (*Offset > (MpamMscNodeLengthCumulative + >>> + InterconnectOffset + HeaderSize)) >>> + { >>> + IncrementErrorCount (); >>> + Print (L"\nERROR : Parsing Interconnect descriptor table failed!\n= "); >>> + Print ( >>> + L"ERROR : Offset overlaps with other objects within the MSC. Off= set >> %u.\n", >>> + InterconnectOffset >>> + ); >>> + >>> + return EFI_INVALID_PARAMETER; >>> + } >>> + >>> + if (InterconnectOffset > (*MpamMscNodeLength)) { >>> + IncrementErrorCount (); >>> + Print (L"\nERROR : Parsing Interconnect descriptor table failed!\n= "); >>> + Print ( >>> + L"ERROR : Offset falls outside MSC's space. Offset %u.\n", >>> + InterconnectOffset >>> + ); >>> + >>> + return EFI_INVALID_PARAMETER; >>> + } >>> + >>> + *Offset =3D HeaderSize + MpamMscNodeLengthCumulative + >> InterconnectOffset; >>> + >>> + Print (L"\n"); >>> + PrintFieldName (6, L"* Interconnect desc table *"); >>> + Print (L"\n\n"); >>> + >>> + // Parse interconnect descriptor table >>> + *Offset +=3D ParseAcpi ( >>> + TRUE, >>> + 4, >>> + NULL, >>> + Ptr + *Offset, >>> + AcpiTableLength - *Offset, >>> + PARSER_PARAMS (MpamInterconnectDescriptorTableParser) >>> + ); >>> + >>> + Status =3D ParseInterconnectDescriptors (Ptr, AcpiTableLength, Offse= t); >>> + return Status; >>> +} >>> + >>> +/** >>> + This function parses all the MPAM functional dependency nodes within= a >>> + single resource node. >>> + >>> + @param [in] Ptr Pointer to the start of the buffer. >>> + @param [in] AcpiTableLength Length of the ACPI table. >>> + @param [in] Offset Pointer to current offset within Ptr. >>> + >>> +Returns: >>> + >>> + Status >>> + >>> + EFI_SUCCESS MPAM MSC Nodes were parsed properly. >>> + EFI_BAD_BUFFER_SIZE The buffer pointer provided as input = is not >>> + long enough to be parsed correctly. >>> +**/ >>> +STATIC >>> +EFI_STATUS >>> +EFIAPI >>> +ParseMpamMscFunctionalDependencies ( >>> + IN UINT8 *Ptr, >>> + IN UINT32 AcpiTableLength, >>> + IN UINT32 *Offset >>> + ) >>> +{ >>> + UINT32 FunctionalDependencyIndex; >>> + >>> + FunctionalDependencyIndex =3D 0; >>> + >>> + // Parse MPAM MSC resources within the MSC body >> >> I think this should be (*NumberOfFunctionalDependencies =3D=3D 0) >=20 > [Rohit] This model of check is present in most of the parser functions. T= his is a check to catch the case where the populated MPAM table lacks Numbe= rOfFunctionalDependencies. If a resource has been defined, the table should= be long enough to have NumberOfFunctionalDependencies defined, whether zer= o or non-zreo. This is the reason why we throw MpamLengthError, which is an= error specifically indicating that the table length is not long enough to = make the table valid. I think it is ok to remove it as the Context can directly be read from the = ACPI_PARSER table. >=20 >> >>> + if (NumberOfFunctionalDependencies =3D=3D NULL) { >>> + MpamLengthError (L"Number of functional dependencies not set!"); >>> + return EFI_BAD_BUFFER_SIZE; >>> + } >>> + >>> + while (FunctionalDependencyIndex < >> *NumberOfFunctionalDependencies) { >>> + PrintBlockTitle ( >>> + 6, >>> + L"* Functional dependency *", >>> + FunctionalDependencyIndex >>> + ); >>> + >>> + // Parse functional dependency >>> + *Offset +=3D ParseAcpi ( >>> + TRUE, >>> + 4, >>> + NULL, >>> + Ptr + *Offset, >>> + AcpiTableLength - *Offset, >>> + PARSER_PARAMS (MpamMscFunctionalDependencyParser) >>> + ); >>> + >>> + FunctionalDependencyIndex++; >>> + } >>> + >>> + return EFI_SUCCESS; >>> +} >>> + >>> +/** >>> + This function parses all the MPAM resource nodes within a single MSC >>> + node within the MPAM ACPI table. It also invokes helper functions t= o >>> + validate and parse locators and functional dependency descriptors. >>> + >>> + @param [in] Ptr Pointer to the start of the buffer. >>> + @param [in] AcpiTableLength Length of the ACPI table. >>> + @param [in] Offset Pointer to current offset within Ptr. >>> + >>> +Returns: >>> + >>> + Status >>> + >>> + EFI_SUCCESS MPAM MSC Nodes were parsed properly. >>> + EFI_BAD_BUFFER_SIZE The buffer pointer provided as input = is not >>> + long enough to be parsed correctly. >>> +**/ >>> +STATIC >>> +EFI_STATUS >>> +EFIAPI >>> +ParseMpamMscResources ( >>> + IN UINT8 *Ptr, >>> + IN UINT32 AcpiTableLength, >>> + IN UINT32 *Offset >>> + ) >>> +{ >>> + EFI_STATUS Status; >>> + UINT64 *InterconnectOffsetPtr; >>> + UINT32 ResourceIndex; >>> + >>> + ResourceIndex =3D 0; >>> + >>> + if (NumberOfMscResources =3D=3D NULL) { >>> + MpamLengthError (L"Number of MSC resource not set!"); >>> + return EFI_BAD_BUFFER_SIZE; >>> + } >>> + >>> + while (ResourceIndex < *NumberOfMscResources) { >>> + PrintBlockTitle ( >>> + 4, >>> + L"* Resource *", >>> + ResourceIndex >>> + ); >>> + >>> + // Parse MPAM MSC resources within the MSC body. This could be >> traced. >>> + *Offset +=3D ParseAcpi ( >>> + TRUE, >>> + 2, >>> + NULL, >>> + Ptr + *Offset, >>> + AcpiTableLength - *Offset, >>> + PARSER_PARAMS (MpamMscResourceParser) >>> + ); >>> + >> >> I understand that the locator should be printed before the functional >> dependencies, >> but couldnt't the MpamMscResourceParser and >> MpamMscResourceLocatorParser structures >> be merged ? >> >=20 > [Rohit] The reason we parse it separately is because we don't want to dis= play MpamMscResourceLocatorParser fields (Trace =3D FALSE). ParseLocator wo= uld deal with parsing the 12 byte locator. Ok, it was written indeed ... >=20 >>> + // Parse MPAM MSC resources within the MSC body. These fields aren= 't >> traced. >>> + // ParseLocator would trace and display the fields. >>> + *Offset +=3D ParseAcpi ( >>> + FALSE, >>> + 2, >>> + NULL, >>> + Ptr + *Offset, >>> + AcpiTableLength - *Offset, >>> + PARSER_PARAMS (MpamMscResourceLocatorParser) >>> + ); >>> + >> >> Shouldn't it just be counted as an error and proceed ? >=20 > Shouldn't it just be counted as an error and proceed ? >=20 > [Rohit] The spec does talk about the case where a table could have no loc= ators (2.2.2 Empty MSC node). > --- An empty MSC node has no resource nodes, and its number of resource n= odes is set to 0. >=20 > However, since *NumberOfMscResources is non-zero, this seems to be a case= we should not let slip by. >=20 Ok right. >> >>> + // Proceed with parsing only if a valid locator has been set. >>> + if ((LocatorType =3D=3D NULL) || (Locator =3D=3D NULL)) { >>> + MpamLengthError (L"Locator type or Locator not set"); >>> + return EFI_BAD_BUFFER_SIZE; >>> + } >>> + >>> + ParseLocator (); >>> + >>> + // Parse the number of functional dependency descriptors. >>> + *Offset +=3D ParseAcpi ( >>> + TRUE, >>> + 2, >>> + NULL, >>> + Ptr + *Offset, >>> + AcpiTableLength - *Offset, >>> + PARSER_PARAMS (MpamMscFunctionalDependencyCountParser= ) >>> + ); >>> + >>> + Status =3D ParseMpamMscFunctionalDependencies (Ptr, AcpiTableLengt= h, >> Offset); >> >> Without empty line if possible (maybe the comment applies to other place= s >> too). >=20 > [Rohit] Ack. Is there a guideline on line spacing I could find somewhere? I m not sure there is, this was just to have the function call and result check in one small block. >=20 >> >>> + >>> + if (Status !=3D EFI_SUCCESS) { >>> + return Status; >>> + } >>> + >>> + // Reset locator to start of the locator descriptor. >>> + InterconnectOffsetPtr =3D (UINT64 *)Locator; >>> + >>> + // If offset field has been set, parse the interconnect descriptio= n table. >> >> if (( >> (without extra spaces) >=20 > [Rohit] Ack. >=20 >> >>> + if ( (*LocatorType =3D=3D EFI_ACPI_MPAM_LOCATION_INTERCONNECT) >>> + && (InterconnectOffsetPtr !=3D NULL)) >>> + { >>> + Status =3D ParseInterconnectDescriptorTable ( >>> + Ptr, >>> + AcpiTableLength, >>> + Offset, >>> + *InterconnectOffsetPtr >>> + ); >>> + >>> + if (Status !=3D EFI_SUCCESS) { >>> + return Status; >>> + } >>> + } >>> + >>> + ResourceIndex++; >>> + } >>> + >>> + return EFI_SUCCESS; >>> +} >>> + >>> +/** >>> + This function parses all the MPAM MSC nodes within the MPAM ACPI >> table. It >>> + also invokes a helper function to detect and parse resource nodes th= at >> maybe >>> + present. >>> + >>> + @param [in] Ptr Pointer to the start of the buffer. >>> + @param [in] AcpiTableLength Length of the ACPI table. >>> + @param [in] Offset Current offset within Ptr. >>> + >>> +Returns: >>> + >>> + Status >>> + >>> + EFI_SUCCESS MPAM MSC Nodes were parsed properly. >>> + EFI_BAD_BUFFER_SIZE The buffer pointer provided as input = is not >>> + long enough to be parsed correctly. >>> +**/ >>> +STATIC >>> +EFI_STATUS >>> +EFIAPI >>> +ParseMpamMscNodes ( >>> + IN UINT8 *Ptr, >>> + IN UINT32 AcpiTableLength, >>> + IN UINT32 Offset >>> + ) >>> +{ >>> + EFI_STATUS Status; >>> + UINT32 MscIndex; >>> + >>> + MscIndex =3D 0; >>> + >>> + while (Offset < AcpiTableLength) { >>> + PrintBlockTitle (2, L"* Msc *", MscIndex); >>> + // Parse MPAM msc node >>> + Offset +=3D ParseAcpi ( >>> + TRUE, >>> + 0, >>> + NULL, >>> + Ptr + Offset, >>> + AcpiTableLength - Offset, >>> + PARSER_PARAMS (MpamMscNodeParser) >>> + ); >>> + >>> + // Parse MPAM MSC resources within the MSC body >> >> I m not sure this case is possible as MpamMscNodeLength is statically >> defined. Maybe it should be (*MpamMscNodeLength =3D=3D 0). >=20 > [Rohit] This is a corner case check to see if someone has populated a MPA= M ACPI table without the MSC body. The statically defined pointer would be = NULL in this case. *MpamMscNodeLength's value is not restricted from the sp= ec but it certainly has to be at least sizeof(MpamMscNodebody) which we tal= ly using > MpamMscNodeLengthCumulative field. (similar explanation added for NumberO= fFunctionalDependencies) Ok right, sorry I misread. Thanks for the explanation. >=20 >> >>> + if (MpamMscNodeLength =3D=3D NULL) { >>> + MpamLengthError (L"MPAM MSC node length not set!"); >>> + return EFI_BAD_BUFFER_SIZE; >>> + } >>> + >>> + Status =3D ParseMpamMscResources (Ptr, AcpiTableLength, &Offset); >> >> Without empty line if possible. >=20 > [Rohit] Ack. >=20 >> >>> + >>> + if (Status !=3D EFI_SUCCESS) { >>> + return Status; >>> + } >>> + >>> + MpamMscNodeLengthCumulative +=3D (*MpamMscNodeLength); >>> + MscIndex++; >>> + } >>> + >>> + return EFI_SUCCESS; >>> +} >>> + >>> +/** >>> + This function parses the MPAM ACPI table's generic header. It also i= nvokes >> a >>> + sub routine that would help with parsing rest of the table. >>> + >>> + @param [in] Trace If TRUE, trace the ACPI fields. >>> + @param [in] Ptr Pointer to the start of the buffer. >>> + @param [in] AcpiTableLength Length of the ACPI table. >>> + @param [in] AcpiTableRevision Revision of the ACPI table. >>> +**/ >>> +VOID >>> +EFIAPI >>> +ParseAcpiMpam ( >>> + IN BOOLEAN Trace, >>> + IN UINT8 *Ptr, >>> + IN UINT32 AcpiTableLength, >>> + IN UINT8 AcpiTableRevision >>> + ) >>> +{ >>> + EFI_STATUS Status; >>> + >>> + if (!Trace) { >>> + return; >>> + } >>> + >>> + // Parse generic table header >>> + HeaderSize =3D ParseAcpi ( >>> + TRUE, >>> + 0, >>> + "MPAM", >>> + Ptr, >>> + AcpiTableLength, >>> + PARSER_PARAMS (MpamParser) >>> + ); >>> + >>> + Status =3D ParseMpamMscNodes (Ptr, AcpiTableLength, HeaderSize); >>> + >>> + if (Status =3D=3D EFI_SUCCESS) { >>> + // Check if the length of all MPAM MSCs with the header, matches w= ith >> the >>> + // ACPI table's length field. >>> + if (*(AcpiHdrInfo.Length) !=3D (MpamMscNodeLengthCumulative + >> HeaderSize)) { >>> + IncrementErrorCount (); >>> + Print (L"\nERROR: Length mismatch! : "); >>> + Print (L"Msc Length total !=3D MPAM table length."); >>> + Print ( >>> + L"table length : %u Msc total : %u\n", >>> + *(AcpiHdrInfo.Length), >>> + MpamMscNodeLengthCumulative >>> + ); >>> + } >>> + } >>> +} >=20 > ~~ >=20 > Regards, > Rohit -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#107173): https://edk2.groups.io/g/devel/message/107173 Mute This Topic: https://groups.io/mt/99066188/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io] -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-