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/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
