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 11DE6941442 for ; Fri, 4 Aug 2023 11:20:23 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=58jv8SG49gTMaI0MhvD/roJS8WNu47g7Chymn9j6kmU=; c=relaxed/simple; d=groups.io; h=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:Content-Language:Content-Type:Content-Transfer-Encoding; s=20140610; t=1691148022; v=1; b=f4wrGFjnDHAAlrFltBdUDL1LIMWjSoai2A412TmU3NiZZFdZZD+ap6XraxaRhTpa8Q0UhYzd 9hM5mfQpKSTotDlgQS+49BxBTyQ8RTix551kGf6DobCIXcWq+6FlaKMJ7xjMcuHL8cDrNonlWCI 2IevEXBJsWK0TjhlteEq2eSQ= X-Received: by 127.0.0.2 with SMTP id tYBGYY7687511xvLmqh4OVbn; Fri, 04 Aug 2023 04:20:22 -0700 X-Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by mx.groups.io with SMTP id smtpd.web10.8830.1691148021209279206 for ; Fri, 04 Aug 2023 04:20:21 -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 757561007; Fri, 4 Aug 2023 04:21:02 -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 62B573F5A1; Fri, 4 Aug 2023 04:20:18 -0700 (PDT) Message-ID: Date: Fri, 4 Aug 2023 13:20:13 +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> <19e06d50-aa28-48e9-ace3-63b8640a1156@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: CcFxweaGkmZrPi6fI2dVr89kx7686176AA= 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=f4wrGFjn; 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, On 7/31/23 22:14, Rohit Mathew wrote: > Hi Pierre, >=20 > Apologies for the delay in response. >=20 > ~~ >=20 >>>>>>> + >>>>>>> +/** >>>>>>> + 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, ValidateReservedGen= eric, >>>> (VOID >>>>>> *)2 }, >>>>>> >>>>> >>>>> [Rohit] The only reason I did not want to do this was to avoid manual= ly >>>> moving the offset back by x bytes to reparse the locator. We parse the >> locator >>>> using MpamMscResourceLocatorParser. If we would need to use >>>> ACPI_PARSER, we would need to step back by 12 bytes (assuming offset i= s >>>> used right after we parse the locator) and reparse the locator under t= he >>>> 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. >>>> >>> >>> [Rohit] PrintFomatter would only be called for a call with Trace =3D TR= UE. While >> parsing MpamMscResourceLocatorParser, Trace is set to FALSE. >>> >>> // 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); >>> >> >> IIUC, Trace =3D False for the MpamMscResourceLocatorParser struct becaus= e we >> just >> want to populate the 'Locator', and let ParseLocator() print/parse the f= ields. >> >> If a PrintFomatter() callback is implemented, the 'Locator' offset would= n't >> need to be populated anymore, the printing/parsing would directly happen >> in the PrintFomatter(). Do you think it could work ? >=20 > [Rohit] The format for PrintFomatter is as follows - > STATIC VOID (*FN)(CONST CHAR16 *Format, UINT8 *Ptr). >=20 > This would mean that we have access to Ptr to whatever field we implement= this callback for, but not the Offset and AcpiTableLength. A way around th= is would be to have Offset and AcpiTableLength as globals, but this seems l= ess clean in my opinion. Yes right, this would be an issue when parsing the interconnect locator. Doing the parsing inside PrintFromatter callbacks might effectively be a bi= t dodgy. Maybe another way to parse the locators would be as the 'GicITSParse= r' struct is parsed in the MadtParser, the Locator type allowing to decide how to parse the Locator descriptor. I think my point is that the locator structures could be represented as ACPI_PARSER structures, allowing to easily see a correlation between the spec and the code. This would allows to remove PrintGenericLocatorDescriptor(). >=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 Bloc= k 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. >>>> >>> >>> [Rohit] If we are to go with the ACPI_PARSERS, I can add this. I do hav= e a >> concern on using ACPI_PARSER which I have added above. >>> >>>>> >>>>> // 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 f= ields >>>>>>> + // For a memory cache locator descriptor don't fall in the 6= 4bit-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 he= re. >>>>>> >>>>> >>>>> [Rohit] All of the locators except interconnect locator have very sim= ple >>>> parsing for their fields. This would mean keeping the prototype for >>>> ParseLocator simple without any params related to ACPI parser pointer, >> offset >>>> etc provided we offload the interconnect descriptor's internal parsing= to a >>>> scope where we have these fields available. We could very well do the >> parsing >>>> 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 locator= s, it is >>>> handled as an exception 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 >> reading >>>> the code while in the optic of having a ACPI_PARSER >>>> structure (and a PrintFormatter) for each locator. In this case, from = within >> 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 n= ot. >>>> >>> >>> [Rohit] I missed another aspect for moving this out. We would need the >> functional dependencies parsed and printed before having interconnect >> descriptors parsed. The reason being MPAM Msc body is followed by MPAM >> MSC resource. MPAM MSC resource is followed by MPAM resource functional >> dependency list (from the spec). The interconnect descriptors are only p= laced >> after the functional dependency list. >> >> Yes right, I assume this is ok as the offset of the Interconnect descrip= tor is >> available in the interconnect locator struct. >> A call to ParseAcpi() from the interconnect locator's PrintFormatter cal= lback >> should be possible, >> unless I missed something ? >> >=20 > [Rohit] This is possible. However, the way the table is placed in memory = won't be aligned to how we display it. > This was a step to align the parser's view with how the table is defined = in the spec. Ok indeed. I assume using ACPI_PARSER structures for locators shouldn't impact this then. I.e. when parsing an interconnect descriptor, the InterconnectOffsetPtr could be populated just as the code does right now. >=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 >>>>>>> +} >>>>>>> + >=20 > ~~ >=20 > Regard, > 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 (#107565): https://edk2.groups.io/g/devel/message/107565 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-