[edk2-devel] [PATCH v5 6/8] ShellPkg: Acpiview: IORT parser update for IORT Rev E.d spec

[Sami Mujawar]

Hi Pierre,

Thank you for the feedback.

Please find my response inline marked [SAMI]

On 13/07/2022 01:10 pm, Pierre Gondois wrote:
> Hi Sami,
>
>> +
>>   /**
>>     Helper Macro for populating the IORT Node header in the 
>> ACPI_PARSER array.
>>   @@ -108,15 +160,15 @@ ValidateItsIdArrayReference (
>>     @param [out] ValidateIdArrayReference  Optional pointer to a 
>> function for
>>                                            validating the ID Array 
>> reference.
>>   **/
>> -#define PARSE_IORT_NODE_HEADER(ValidateIdMappingCount, \
>> - ValidateIdArrayReference)                 \
>> -  { L"Type", 1, 0, L"%d", NULL, (VOID**)&IortNodeType, NULL, NULL 
>> },     \
>> -  { L"Length", 2, 1, L"%d", NULL, (VOID**)&IortNodeLength, NULL, 
>> NULL }, \
>> -  { L"Revision", 1, 3, L"%d", NULL, NULL, NULL, NULL 
>> },                  \
>> -  { L"Reserved", 4, 4, L"0x%x", NULL, NULL, NULL, NULL 
>> },                \
>> -  { L"Number of ID mappings", 4, 8, L"%d", 
>> NULL,                         \
>> -    (VOID**)&IortIdMappingCount, ValidateIdMappingCount, NULL 
>> },         \
>> -  { L"Reference to ID Array", 4, 12, L"0x%x", 
>> NULL,                      \
>> +#define PARSE_IORT_NODE_HEADER(ValidateIdMappingCount, \
>> + ValidateIdArrayReference)                      \
>> +  { L"Type", 1, 0, L"%d", NULL, (VOID**)&IortNodeType, NULL, NULL 
>> },          \
>> +  { L"Length", 2, 1, L"%d", NULL, (VOID**)&IortNodeLength, NULL, 
>> NULL },      \
>> +  { L"Revision", 1, 3, L"%d", NULL, (VOID**)&IortNodeRevision, NULL, 
>> NULL },  \
>> +  { L"Identifier", 4, 4, L"0x%x", NULL, NULL, NULL, NULL 
>> },                   \
>> +  { L"Number of ID mappings", 4, 8, L"%d", 
>> NULL,                              \
>> +    (VOID**)&IortIdMappingCount, ValidateIdMappingCount, NULL 
>> },              \
>> +  { L"Reference to ID Array", 4, 12, L"0x%x", 
>> NULL,                           \
>>       (VOID**)&IortIdMappingOffset, ValidateIdArrayReference, NULL }
>>     /**
>> @@ -235,11 +287,13 @@ STATIC CONST ACPI_PARSER 
>> IortNodeNamedComponentParser[] = {
>>   **/
>>   STATIC CONST ACPI_PARSER  IortNodeRootComplexParser[] = {
>>     PARSE_IORT_NODE_HEADER (NULL, NULL),
>> -  { L"Memory access properties",8,    16,  L"0x%lx", NULL,       
>> NULL, NULL, NULL },
>> -  { L"ATS Attribute",           4,    24,  L"0x%x", NULL,       
>> NULL, NULL, NULL },
>> -  { L"PCI Segment number",      4,    28,  L"0x%x", NULL,       
>> NULL, NULL, NULL },
>> -  { L"Memory access size limit",1,    32,  L"0x%x", NULL,       
>> NULL, NULL, NULL },
>> -  { L"Reserved",                3,    33,  L"%x %x %x", Dump3Chars, 
>> NULL, NULL, NULL }
>> +  { L"Memory access properties",8,    16,  L"0x%lx", NULL, NULL, 
>> NULL, NULL },
>> +  { L"ATS Attribute",           4,    24,  L"0x%x",  NULL, NULL, 
>> NULL, NULL },
>> +  { L"PCI Segment number",      4,    28,  L"0x%x",  NULL, NULL, 
>> NULL, NULL },
>> +  { L"Memory access size limit",1,    32,  L"0x%x",  NULL, NULL, 
>> NULL, NULL },
>> +  { L"PASID capabilities",      2,    33,  L"0x%x",  NULL, NULL, 
>> NULL, NULL },
>> +  { L"Reserved",                1,    35,  L"%x",    NULL, NULL, 
>> NULL, NULL },
>> +  { L"Flags",                   4,    36,  L"%x",    NULL, NULL, 
>> NULL, NULL },
>
> It seems flags are usually printed as L"0x%x"
[SAMI] Ack.
>
>>   };
>>     /**
>> @@ -253,6 +307,29 @@ STATIC CONST ACPI_PARSER IortNodePmcgParser[] = {
>>     { L"Page 1 Base Address",                           8, 32,  
>> L"0x%lx", NULL, NULL, NULL, NULL }
>>   };
>>   +/**
>> +  An ACPI_PARSER array describing the IORT RMR node.
>> +**/
>> +STATIC CONST ACPI_PARSER  IortNodeRmrParser[] = {
>> +  PARSE_IORT_NODE_HEADER (NULL, NULL),
>> +  { L"Flags",                   4,                      16, L"0x%x", 
>> NULL, NULL, NULL, NULL },
>> +  { L"Memory Range Desc count", 4,                      20, L"%d",   
>> NULL,
>> +    (VOID **)&RmrMemDescCount, ValidateRmrMemDescCount,NULL },
>> +  { L"Memory Range Desc Ref",   4,                      24, L"0x%x", 
>> NULL,
>> +    (VOID **)&RmrMemDescOffset, NULL, NULL }
>> +};
>> +
>> +/**
>> +  An ACPI_PARSER array describing the IORT RMR Memory Range Descriptor.
>> +**/
>> +STATIC CONST ACPI_PARSER  IortNodeRmrMemRangeDescParser[] = {
>> +  { L"Physical Range offset", 8, 0,  L"0x%lx", NULL, NULL, 
>> ValidatePhysicalRange,
>> +    NULL },
>> +  { L"Physical Range length", 8, 8,  L"0x%lx", NULL, NULL, 
>> ValidatePhysicalRange,
>> +    NULL },
>> +  { L"Reserved",              4, 16, L"0x%x",  NULL, NULL, 
>> NULL,                 NULL}
>> +};
>> +
>>   /**
>
> [snip]
>
>>   /**
>>     This function parses the ACPI IORT table.
>> -  When trace is enabled this function parses the IORT table and 
>> traces the ACPI fields.
>> +  When trace is enabled this function parses the IORT table and 
>> traces the ACPI
>> +  fields.
>>       This function also parses the following nodes:
>>       - ITS Group
>> @@ -618,6 +788,7 @@ DumpIortNodePmcg (
>>       - SMMUv1/2
>>       - SMMUv3
>>       - PMCG
>> +    - RMR
>>       This function also performs validation of the ACPI table fields.
>>   @@ -643,6 +814,13 @@ ParseAcpiIort (
>>       return;
>>     }
>>   +  if (AcpiTableRevision == 4) {
>> +    IncrementErrorCount ();
>> +    Print (
>> +      L"ERROR: IORT tabe Revision E.c is deprecated and must not be 
>> used.\n"
>> +      );
>> +  }
>> +
>
> I think a macro corresponding to rev E.c should be added in MdePkg
> so we can identify the deprecated revision easily.
> Similar comment for the Rmr revision (=2) which is checked in 
> DumpIortNodeRmr().
>
[SAMI] Ack.
>
>>     ParseAcpi (
>>       TRUE,
>>       0,
>> @@ -765,7 +943,14 @@ ParseAcpiIort (
>>             *IortIdMappingOffset
>>             );
>>           break;
>> -
>> +      case EFI_ACPI_IORT_TYPE_RMR:
>> +        DumpIortNodeRmr (
>> +          NodePtr,
>> +          *IortNodeLength,
>> +          *IortIdMappingCount,
>> +          *IortIdMappingOffset
>> +          );
>> +        break;
>>         default:
>>           IncrementErrorCount ();
>>           Print (L"ERROR: Unsupported IORT Node type = %d\n", 
>> *IortNodeType);


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#91316): https://edk2.groups.io/g/devel/message/91316
Mute This Topic: https://groups.io/mt/92334076/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-