[edk2-devel] [PATCH V3 3/3] ShellPkg/AcpiView: Add MPAM Parser
Rohit Mathew
rohit.mathew at arm.com
Mon Jul 31 20:14:33 UTC 2023
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.
>
> >
> >>>
> >>>>
> >>>>> + 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.
> >
> >>>
> >>>>> + 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 (#107408): https://edk2.groups.io/g/devel/message/107408
Mute This Topic: https://groups.io/mt/99066188/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list