Hi Pierre,
> -----Original Message-----
> From: Pierre Gondois <pierre.gond...@arm.com>
> Sent: Friday, August 4, 2023 12:20 PM
> To: Rohit Mathew <rohit.mat...@arm.com>; devel@edk2.groups.io
> Cc: Thomas Abraham <thomas.abra...@arm.com>; Sami Mujawar
> <sami.muja...@arm.com>; James Morse <james.mo...@arm.com>; Ray Ni
> <ray...@intel.com>; Zhichao Gao <zhichao....@intel.com>; nd
> <n...@arm.com>
> Subject: Re: [edk2-devel] [PATCH V3 3/3] ShellPkg/AcpiView: Add MPAM
> Parser
>
> Hello Rohit,
>
> On 7/31/23 22:14, Rohit Mathew wrote:
> > Hi Pierre,
> >
> > Apologies for the delay in response.
> >
> > ~~
> >
> >>>>>>> +
> >>>>>>> +/**
> >>>>>>> + 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 = Locator;
> >>>>>>> +
> >>>>>>> + switch (*LocatorType) {
> >>>>>>
> >>>>>> I think it would be simpler to define names as:
> >>>>>>
> >>>>>> STATIC CONST CHAR16 *MpamLocationNames[] = {
> >>>>>> 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[] = {
> >>>>>> { L"SMMU interface", 8, 0, L"%lu", NULL, NULL, NULL, NULL },
> >>>>>> { L"Reserved ID", 4, 8, L"%u", NULL, NULL,
> >>>>>> ValidateReservedGeneric,
> >>>> (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
> >> locator
> >>>> 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.
> >>>>
> >>>
> >>> [Rohit] PrintFomatter would only be called for a call with Trace =
> >>> TRUE. 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 != NULL) {
> >>> Parser[Index].PrintFormatter (Parser[Index].Format, Ptr);
> >>>
> >>
> >> IIUC, Trace = False for the MpamMscResourceLocatorParser struct
> >> because we just want to populate the 'Locator', and let
> >> ParseLocator() print/parse the fields.
> >>
> >> 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 ?
> >
> > [Rohit] The format for PrintFomatter is as follows - STATIC VOID
> > (*FN)(CONST CHAR16 *Format, UINT8 *Ptr).
> >
> > 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 this
> would be to have Offset and AcpiTableLength as globals, but this seems less
> 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 bit
> dodgy. Maybe another way to parse the locators would be as the
> 'GicITSParser'
> 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().
>
>
[Rohit] Agreed, parsing with ACPI_PARSER using different cases for LocatorType
would be cleaner. I will post V4 with this and other comments addressed.
> >
> >>
> >>>
> >>>>>
> >>>>>>
> >>>>>>> + 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.
> >>>>
> >>>
> >>> [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.
> >>>
> >>>>>
> >>>>> // 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 =
> %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
> >>>>>>> fields
> >>>>>>> + // 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 += 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 += 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
> >>>>> simple
> >>>> 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 locators, 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 not.
> >>>>
> >>>
> >>> [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 placed after the functional 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
> >> callback should be possible, unless I missed something ?
> >>
> >
> > [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.
>
> >
> >>>
> >>>>>
> >>>>>>> + 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
> >>>>>>> +}
> >>>>>>> +
> >
> > ~~
> >
> > Regard,
> > Rohit
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#107619): https://edk2.groups.io/g/devel/message/107619
Mute This Topic: https://groups.io/mt/99066188/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
