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 B1538740042 for ; Tue, 25 Jul 2023 13:55:27 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=xgRnDJqoDkAhBJD0dgsNdzRvsP3IN+pJ1M+YMCHGzQM=; 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=1690293326; v=1; b=qJEUrLwMpVpaxpjPRxHab0DcZlekCP9VTppIGDK2IPbSBpzE+Qj9FHjnFNQy8DvSbBn6Gb8u hucnsmrFE8nM7qdfDAzYfqbZxJ5MKpn1Cv8uaTqPNxQ9PRRABJNUHokhm0sswzVi1wZMLGyN+Kg VdJ52BXTZ9rJCVudsEoiB2jc= X-Received: by 127.0.0.2 with SMTP id wO6MYY7687511x4ccHxgfZtK; Tue, 25 Jul 2023 06:55:26 -0700 X-Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by mx.groups.io with SMTP id smtpd.web11.20976.1690293325288714169 for ; Tue, 25 Jul 2023 06:55:25 -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 A013F15BF; Tue, 25 Jul 2023 06:56:07 -0700 (PDT) X-Received: from [192.168.1.12] (unknown [172.31.20.19]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 347773F6C4; Tue, 25 Jul 2023 06:55:22 -0700 (PDT) Message-ID: <19e06d50-aa28-48e9-ace3-63b8640a1156@arm.com> Date: Tue, 25 Jul 2023 15:55:08 +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> <3bf3780a-b064-1d60-78ed-0e323726adc7@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: 0w2iIQOSAxLGYX6UMLYEm4xnx7686176AA= 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=qJEUrLwM; 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, I should have answered your comments, On 7/24/23 18:07, Rohit Mathew wrote: > Hi Pierre, >=20 > Thank you for the response. Few comments inline - >=20 >> -----Original Message----- >> From: Pierre Gondois >> Sent: Monday, July 24, 2023 3:40 PM >> To: Rohit Mathew ; devel@edk2.groups.io >> Cc: Thomas Abraham ; Sami Mujawar >> ; James Morse ; Ray Ni >> ; Zhichao Gao ; nd >> >> Subject: Re: [edk2-devel] [PATCH V3 3/3] ShellPkg/AcpiView: Add MPAM >> Parser >> >> 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, >>> >>> Thank you for the detailed review. Please find my response inline. >>> >>>> 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 = are >>>>> also performed where and when required. >>>>> >>>>> Signed-off-by: Rohit Mathew >>>>> --- >>>>> ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.h >> | >>>> 21 + >>>>> >>>> >> ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Mpam/MpamParse >> r >>>> .c | 1331 ++++++++++++++++++++ >>>>> >>>> >> ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Mpam/MpamParse >> r >>>> .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-stru= ctures >>>>> 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 *NumberOfInterconnectDescriptor= s; >>>>> +STATIC CONST UINT32 *OverflowInterrupt; >>>>> +STATIC ACPI_DESCRIPTION_HEADER_INFO AcpiHdrInfo; >>>>> + >>>>> +/** >>>>> + When the length of the table is insufficient to be parsed, this fu= nction >>>> could >>>>> + be used to display an appropriate error message. >>>>> + >>>>> + @param [in] ErrorMsg Error message string that has to be appe= nded >> to >>>> the >>>> >>>> (alignment) >>> >>> [Rohit] Ack. >>> >>>> >>>>> + 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= should >> be >>>> renamed >>>> and the 'Insufficient MPAM MSC Node length ...' message be removed. >>> >>> [Rohit] - I responded to couple of comments addressing why we use this >> function, below. Let me know your thoughts. (Again, I moved bottom - up >> while addressing comments as it was easier to walk the code that way) >>> >>>> >>>>> +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 reserve= d >> 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 parsi= ng, 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). >>>> >>>> >>> >>> [Rohit] If it's okay, I could work out these changes once we get the MP= AM >> changes in? >> >> Yes sure, it was just a remark. >> >>> >>>>> +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= the >>>> 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= this >>>>> + 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 >> implemented. >>>>> + 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 pr= int out >>>> the >>>>> + decoded value for a particular field. The decoded value handled in= this >>>>> + function is a string. >>>>> + >>>>> + @param [in] Indent Number of spaces to add to the glo= bal table >>>>> + indent. The global table indent is= 0 by >>>>> + default; however this value is upd= ated on >>>>> + entry to the ParseAcpi() by adding= the indent >>>>> + value provided to ParseAcpi() and = restored >>>>> + back on exit. Therefore the total= indent in >>>>> + the output is dependent on from wh= ere this >>>>> + function is called. >>>>> + @param [in] FieldStr Title string for the field of inte= rest. >>>>> + @param [in] DecodedStr Decoded value to be printed for th= e >> 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 MS= C >>>> 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. Fo= r this >>>>> + function, context holds the parent/double p= ointer to a >>>>> + variable holding the interface type. Make s= ure 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 acpi= view. >>> >>> [Rohit] I added these comments as I could not find any other use-case o= f >> *Context pointer within any parsers (or anywhere). Therefore, these >> comments were a way to show how 'MPAM parser' utilized context pointer >> for different validators. >>> Also, the way we use context within ValidateReservedGeneric is quite >> different from ValidateMmioSize due to compiler restrictions on using >> constants for initializer lists. >> >> TODO >=20 > [Rohit] I think the suggestion was to remove it from one of the comments = below. I'll remove it. Sorry this was a personnal note to rememeber to answer your comment. I think this is ok to remove the comment as it is straightforward to see what is past as in the 'Context' by just going to the ACPI_PARSER struct. >=20 >> >>> >>> If they seem obvious, I could remove them. Let me know. >>> >>>> >>>>> + // 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. Siz= e - >> %u\n", >>>>> + MmioSize >>>>> + ); >>>>> + } >>>>> + } >>>>> +} >>>>> + >>>>> +/** >>>>> + This function decodes and validates the link type for MPAM's >> interconnect >>>>> + 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. F= or 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. >>>> >>> >>> [Rohit] Ack - I see that I'm adding a \n to print {decoded string - dec= ode >> value}. So, I could probably have the raw followed by decoded value on t= he >> same line by avoiding the newline. It would be the other way around I as= sume >> - 0 (NUMA). >> >> ok yes, both should be fine. >> >>> >>>>> + 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 AC= PI >>>> table. >>>>> + The specification states that the hardware ID has to be set to zer= o if not >>>>> + being used. >>>>> + >>>>> + @param [in] Ptr Pointer to the start of the field data. >>>>> + @param [in] Context Pointer to context specific information. F= or 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. >>> >>> [Rohit] Ack >>> >>>> >>>>> + // Hardware Id fields within MPAM table always falls on offset div= isible >> 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 nam= e >> should >>>> have already been printed ? >>>> >>> >>> [Rohit] Ack. (responded to a similar comment, above) >>> >>>>> + PrintFieldName (2, L"* Hardware ID type decoded"); >>>>> + Dump8Chars (NULL, Ptr); >>>>> + } >>>>> +} >>>>> + >>>>> +/** >>>>> + This function decodes and validates the interface type for MPAM. V= alid >>>>> + 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. F= or 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= the >>>> MPAM >>>>> + MSC node body. >>>>> + >>>>> + @param [in] Ptr Pointer to the start of the field data. >>>>> + @param [in] Context Pointer to context specific information. Fo= r this >>>>> + function, context holds the parent/double p= ointer to a >>>>> + variable holding the interrupt gsiv. Make s= ure 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 ne= ed 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 ? >>>> >>> >>> [Rohit] Sorry, the link points to V2 of this patch. Was this the intend= ed link? >> >> Yes right, sorry for the wrong link, I meant: >> https://edk2.groups.io/g/devel/message/106957 >> >=20 > [Rohit] On a quick look, I feel this could be utilized. I'll get back If = I see any concerns. >=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= _MASK) >>>>> + >> EFI_ACPI_MPAM_INTERRUPT_AFFINITY_VALID= _SHIFT; >>>>> + >>>>> + 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= _MASK) >>>>> + >> EFI_ACPI_MPAM_INTERRUPT_AFFINITY_TYPE= _SHIFT; >>>>> + >>>>> + 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 f= ield is >>>>> + // captured into InterfaceType pointer so that it could be used to= check >> 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 = validator >>>>> + // 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 = validator >>>>> + // 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 loc= ator >>>> 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 >> associated >>>> 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 associate= d >> with >>>> the >>>>> + interconnect locator type. The specification includes an additiona= l >> reserved >>>>> + field also within the interconnect descriptor. This is not include= d in the >>>>> + parser object on purpose. The function is of 3 bytes length which = the >>>>> + 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 glo= bal table >>>>> + indent. The global table indent is= 0 by >>>>> + default; however this value is upd= ated on >>>>> + entry to the ParseAcpi() by adding= the indent >>>>> + value provided to ParseAcpi() and = restored >>>>> + back on exit. Therefore the total= indent in >>>>> + the output is dependent on from wh= ere this >>>>> + function is called. >>>>> + @param [in] LocatorTitle Title string to be used for the lo= cator. >>>>> +**/ >>>>> +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 glo= bal table >>>>> + indent. The global table indent is= 0 by >>>>> + default; however this value is upd= ated on >>>>> + entry to the ParseAcpi() by adding= the indent >>>>> + value provided to ParseAcpi() and = restored >>>>> + back on exit. Therefore the total= indent in >>>>> + the output is dependent on from wh= ere this >>>>> + function is called. >>>>> + @param [in] DescriptorTitle Title string to be used for the de= scriptor. >>>>> + @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 reserve= d 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 glo= bal table >>>>> + indent. The global table indent is= 0 by >>>>> + default; however this value is upd= ated on >>>>> + entry to the ParseAcpi() by adding= the indent >>>>> + value provided to ParseAcpi() and = restored >>>>> + back on exit. Therefore the total= indent in >>>>> + the output is dependent on from wh= ere this >>>>> + function is called. >>>>> + @param [in] DescriptorTitle Title string to be used for the de= scriptor. >>>>> + @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 reserve= d 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 prin= ting >>>> locator >>>>> + descriptor that could be split as two 8 and 4 byte fields. The Loc= atorPtr >>>>> + field is casted to (UINT64 *) and decoded within the macro. From t= he >>>> MPAM ACPI >>>>> + specification, the offset of the locator descriptor field is divis= ible by 8. >>>>> + It is assumed that ACPI tables are placed at 8 byte aligned addres= s and >>>> thus >>>>> + unaligned access should not be a concern. >>>>> + >>>>> + Only locators which have its second field as reserved should use t= his >>>>> + function. >>>>> + >>>>> + @param [in] Indent Number of spaces to add to the glo= bal table >>>>> + indent. The global table indent is= 0 by >>>>> + default; however this value is upd= ated on >>>>> + entry to the ParseAcpi() by adding= the indent >>>>> + value provided to ParseAcpi() and = restored >>>>> + back on exit. Therefore the total= indent in >>>>> + the output is dependent on from wh= ere this >>>>> + function is called. >>>>> + @param [in] LocatorTitle Title string to be used for the lo= cator >>>>> + @param [in] Descriptor1Title Title string to be used for descri= ptor1 >>>>> + @param [in] Descriptor2Title Title string to be used for descri= ptor2 >>>>> + @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 reserve= d 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 fo= r 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, ValidateReservedGeneri= c, >> (VOID >>>> *)2 }, >>>> >>> >>> [Rohit] The only reason I did not want to do this was to avoid manually >> moving the offset back by x bytes to reparse the locator. We parse the l= ocator >> using MpamMscResourceLocatorParser. If we would need to use >> ACPI_PARSER, we would need to step back by 12 bytes (assuming offset is >> used right after we parse the locator) and reparse the locator under the >> respective switch case. We might not be able to skip >> MpamMscResourceLocatorParser as >> EFI_ACPI_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= the >> 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 > [Rohit] PrintFomatter would only be called for a call with Trace =3D TRUE= . While parsing MpamMscResourceLocatorParser, Trace is set to FALSE. >=20 > // Snippet > if (Trace) { > // if there is a Formatter function let the function handle > // the printing else if a Format is specified in the table use > // the Format for printing > PrintFieldName (2, Parser[Index].NameStr); > if (Parser[Index].PrintFormatter !=3D NULL) { > Parser[Index].PrintFormatter (Parser[Index].Format, Ptr); >=20 IIUC, Trace =3D False for the MpamMscResourceLocatorParser struct because w= e just want to populate the 'Locator', and let ParseLocator() print/parse the fiel= ds. If a PrintFomatter() callback is implemented, the 'Locator' offset wouldn't need to be populated anymore, the printing/parsing would directly happen in the PrintFomatter(). Do you think it could work ? >=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 >>> >>> [Rohit] I believe ACPI_PARSER won't parse Memory-side cache locator >> descriptor due to its odd field length. >> >> I think there is a similar case for the Reserved field of the 'GT Block = Timer >> 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 > [Rohit] If we are to go with the ACPI_PARSERS, I can add this. I do have = a concern on using ACPI_PARSER which I have added above. >=20 >>> >>> // snippet from ACPI_PARSER's code >>> >>> 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 >>> >>> I have not tested this - but I do remember seeing such a behavior for >> Interconnect descriptor's reserved field. >>> >>>> >>>>> + // PrintGenericLocatorDescriptor can't be used here as the fie= lds >>>>> + // For a memory cache locator descriptor don't fall in the 64b= it-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= . >>>> >>> >>> [Rohit] All of the locators except interconnect locator have very simpl= e >> parsing for their fields. This would mean keeping the prototype for >> ParseLocator simple without any params related to ACPI parser pointer, o= ffset >> etc provided we offload the interconnect descriptor's internal parsing t= o a >> scope where we have these fields available. We could very well do the pa= rsing >> internally - however this would mean changing the prototype just for >> interconnect descriptor locator. >>> >>> The prototype change is twofold (return and params). Without >> ParseInterconnectDescriptorTable, 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 an exception with respect to all other locators,= it is >> handled as an exception outside of ParseLocator. If we had quite a lot o= f >> locators with detailed internal parsing, we should have handled this int= ernally. >> >> Maybe there's something I don't understand correctly, but I think I m re= ading >> the code while in the optic of having a ACPI_PARSER >> structure (and a PrintFormatter) for each locator. In this case, from wi= thin 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 > [Rohit] I missed another aspect for moving this out. We would need the fu= nctional dependencies parsed and printed before having interconnect descrip= tors parsed. The reason being MPAM Msc body is followed by MPAM MSC resourc= e. MPAM MSC resource is followed by MPAM resource functional dependency lis= t (from the spec). The interconnect descriptors are only placed after the f= unctional dependency list. Yes right, I assume this is ok as the offset of the Interconnect descriptor= is available in the interconnect locator struct. A call to ParseAcpi() from the interconnect locator's PrintFormatter callba= ck should be possible, unless I missed something ? >=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 glo= bal table >>>>> + indent. The global table indent is= 0 by >>>>> + default; however this value is upd= ated on >>>>> + entry to the ParseAcpi() by adding= the indent >>>>> + value provided to ParseAcpi() and = restored >>>>> + back on exit. Therefore the total= indent in >>>>> + the output is dependent on from wh= ere this >>>>> + function is called. >>>>> + @param [in] Title Title string to be used for the bl= ock. >>>>> + @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. >>> >>> >>> [Rohit] Ack. I did run Uncrustify and staurt CI. I very well could have= missed >> this bit. Thanks for the hint. >>> >>>> >>>>> + This function parses the interconnect descriptor(s) asssociated wi= th >>>>> + 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 Pt= r. >>>>> + >>>>> +Returns: >>>>> + >>>>> + Status >>>>> + >>>>> + EFI_SUCCESS MPAM MSC Nodes were parsed properly= . >>>>> + EFI_BAD_BUFFER_SIZE The buffer pointer provided as inpu= t 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. >>>> >>> >>> [Rohit] I have responded to comments for >> NumberOfFunctionalDependencies and MpamMscNodeLength addressing >> this concern. Sorry, I had to start from the bottom while addressing com= ments >> as that helped with the code flow :) >> >> Ok yes no worries :) >> >>> >>>>> + if (NumberOfInterconnectDescriptors =3D=3D NULL) { >>>>> + MpamLengthError (L"Number of interconnect descriptors not set!")= ; >>>>> + return EFI_BAD_BUFFER_SIZE; >>>>> + } >>>>> + >>>>> + while (InterconnectDescriptorIndex < >> *NumberOfInterconnectDescriptors) >>>> { >>>>> + 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= with >> an >>>>> + interconnect type locator object. It also performs necessary valid= ation 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 Pt= r. >>>>> + @param [in] InterconnectOffset Offset to the interconnect descript= or >>>> table. >>>>> + >>>>> +Returns: >>>>> + >>>>> + Status >>>>> + >>>>> + EFI_SUCCESS MPAM MSC Nodes were parsed properly= . >>>>> + EFI_BAD_BUFFER_SIZE The buffer pointer provided as inpu= t is not >>>>> + long enough to be parsed correctly. >>>>> + EFI_INVALID_PARAMETER The Offset parameter encoded within >> the >>>> 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 = table be >>>>> + // placed exactly after say, functional dependency table or the re= source >>>> node. >>>>> + // Instead, the interconnect locator's descriptor field 1 gives th= e 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. O= ffset >>>> %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, Off= set); >>>>> + return Status; >>>>> +} >>>>> + >>>>> +/** >>>>> + This function parses all the MPAM functional dependency nodes with= in >> 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 Pt= r. >>>>> + >>>>> +Returns: >>>>> + >>>>> + Status >>>>> + >>>>> + EFI_SUCCESS MPAM MSC Nodes were parsed properly= . >>>>> + EFI_BAD_BUFFER_SIZE The buffer pointer provided as inpu= t 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) >>> >>> [Rohit] This model of check is present in most of the parser functions.= This is >> a check to catch the case where the populated MPAM table lacks >> NumberOfFunctionalDependencies. If a resource has been defined, the tabl= e >> should be long enough to have NumberOfFunctionalDependencies defined, >> whether zero or non-zreo. This is the reason why we throw >> MpamLengthError, which is an error specifically indicating that the tabl= e 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 t= he >> ACPI_PARSER table. >> >=20 > [Rohit] I think this comment got copied unintentionally. I have responded= to it at the right place. yes right >=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 M= SC >>>>> + node within the MPAM ACPI table. It also invokes helper functions= to >>>>> + 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 Pt= r. >>>>> + >>>>> +Returns: >>>>> + >>>>> + Status >>>>> + >>>>> + EFI_SUCCESS MPAM MSC Nodes were parsed properly= . >>>>> + EFI_BAD_BUFFER_SIZE The buffer pointer provided as inpu= t 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 ? >>>> >>> >>> [Rohit] The reason we parse it separately is because we don't want to d= isplay >> MpamMscResourceLocatorParser fields (Trace =3D FALSE). ParseLocator woul= d >> deal with parsing the 12 byte locator. >> >> Ok, it was written indeed ... >> >>> >>>>> + // Parse MPAM MSC resources within the MSC body. These fields ar= en'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 ? >>> >>> Shouldn't it just be counted as an error and proceed ? >>> >>> [Rohit] The spec does talk about the case where a table could have no >> locators (2.2.2 Empty MSC node). >>> --- An empty MSC node has no resource nodes, and its number of resource >> nodes is set to 0. >>> >>> However, since *NumberOfMscResources is non-zero, this seems to be a >> case we should not let slip by. >>> >> >> 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, >> AcpiTableLength, >>>> Offset); >>>> >>>> Without empty line if possible (maybe the comment applies to other pla= ces >>>> too). >>> >>> [Rohit] Ack. Is there a guideline on line spacing I could find somewher= e? >> >> I m not sure there is, this was just to have the function call and resul= t >> check in one small block. >> >>> >>>> >>>>> + >>>>> + 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 descript= ion table. >>>> >>>> if (( >>>> (without extra spaces) >>> >>> [Rohit] Ack. >>> >>>> >>>>> + 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 = that >>>> 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 inpu= t 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). >>> >>> [Rohit] This is a corner case check to see if someone has populated a M= PAM >> ACPI table without the MSC body. The statically defined pointer would be >> NULL in this case. *MpamMscNodeLength's value is not restricted from the >> spec but it certainly has to be at least sizeof(MpamMscNodebody) which w= e >> tally using >>> MpamMscNodeLengthCumulative field. (similar explanation added for >> NumberOfFunctionalDependencies) >> >> Ok right, sorry I misread. >> Thanks for the explanation. >> >>> >>>> >>>>> + 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. >>> >>> [Rohit] Ack. >>> >>>> >>>>> + >>>>> + 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 >> invokes >>>> 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= with >>>> 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 >>>>> + ); >>>>> + } >>>>> + } >>>>> +} >>> >>> ~~ >>> >>> Regards, >>> Rohit Regards, Pierre -=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 (#107245): https://edk2.groups.io/g/devel/message/107245 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-