[edk2-devel] [PATCH V3 3/3] ShellPkg/AcpiView: Add MPAM Parser
PierreGondois
pierre.gondois at arm.com
Fri Aug 4 11:20:13 UTC 2023
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().
>
>>
>>>
>>>>>
>>>>>>
>>>>>>> + 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 (#107565): https://edk2.groups.io/g/devel/message/107565
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