Re: [edk2-devel] [PATCH V3 3/3] ShellPkg/AcpiView: Add MPAM Parser

["Rohit Mathew" <rohit.mathew@arm.com>]

Hi Pierre,

> -----Original Message-----
> From: Pierre Gondois <pierre.gondois@arm.com>
> Sent: Friday, August 4, 2023 12:20 PM
> To: Rohit Mathew <Rohit.Mathew@arm.com>; devel@edk2.groups.io
> Cc: Thomas Abraham <thomas.abraham@arm.com>; Sami Mujawar
> <Sami.Mujawar@arm.com>; James Morse <James.Morse@arm.com>; Ray Ni
> <ray.ni@intel.com>; Zhichao Gao <zhichao.gao@intel.com>; nd
> <nd@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/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-