[edk2-devel] [PATCH v5 8/8] DynamicTablesPkg: IORT generator updates for Rev E.d spec
Sami Mujawar
sami.mujawar at arm.com
Wed Jul 13 17:07:38 UTC 2022
Hi Pierre,
Thank you for the feedback.
Please find my response inline marked [SAMI].
Regards,
Sami Mujawar
On 13/07/2022 01:18 pm, Pierre Gondois wrote:
> Hi Sami,
>
>> @@ -37,9 +37,11 @@ Requirements:
>> - EArmObjSmmuV1SmmuV2
>> - EArmObjSmmuV3
>> - EArmObjPmcg
>> + - EArmObjRmr
>> - EArmObjGicItsIdentifierArray
>> - EArmObjIdMappingArray
>> - - EArmObjGicItsIdentifierArray
>
> I think EArmObjGicItsIdentifierArray should be conserved.
[SAMI] This is present. I have removed the duplicate entry.
>
>> + - EArmObjSmmuInterruptArray
>> + - EArmObjMemoryRangeDescriptor
>> */
>> /** This macro expands to a function that retrieves the ITS
>> @@ -96,6 +98,24 @@ GET_OBJECT_LIST (
>> CM_ARM_PMCG_NODE
>> );
>> +/** This macro expands to a function that retrieves the
>> + RMR node information from the Configuration Manager.
>> +*/
>> +GET_OBJECT_LIST (
>> + EObjNameSpaceArm,
>> + EArmObjRmr,
>> + CM_ARM_RMR_NODE
>> + );
>> +
>> +/** This macro expands to a function that retrieves the
>> + Memory Range Descriptor Array information from the Configuration
>> Manager.
>> +*/
>> +GET_OBJECT_LIST (
>> + EObjNameSpaceArm,
>> + EArmObjMemoryRangeDescriptor,
>> + CM_ARM_MEMORY_RANGE_DESCRIPTOR
>> + );
>> +
>> /** This macro expands to a function that retrieves the
>> ITS Identifier Array information from the Configuration Manager.
>> */
>> @@ -174,16 +194,19 @@ GetSizeofItsGroupNodes (
>> Size = 0;
>> while (NodeCount-- != 0) {
>> - (*NodeIndexer)->Token = NodeList->Token;
>> - (*NodeIndexer)->Object = (VOID *)NodeList;
>> - (*NodeIndexer)->Offset = (UINT32)(Size + NodeStartOffset);
>> + (*NodeIndexer)->Token = NodeList->Token;
>> + (*NodeIndexer)->Object = (VOID *)NodeList;
>> + (*NodeIndexer)->Offset = (UINT32)(Size + NodeStartOffset);
>> + (*NodeIndexer)->Identifier = NodeList->Identifier;
>> DEBUG ((
>> DEBUG_INFO,
>> - "IORT: Node Indexer = %p, Token = %p, Object = %p, Offset =
>> 0x%x\n",
>> + "IORT: Node Indexer = %p, Token = %p, Object = %p,"
>> + " Offset = 0x%x, Identifier = 0x%x\n",
>> *NodeIndexer,
>> (*NodeIndexer)->Token,
>> (*NodeIndexer)->Object,
>> - (*NodeIndexer)->Offset
>> + (*NodeIndexer)->Offset,
>> + (*NodeIndexer)->Identifier
>
> I think all the GetSizeof...Nodes() functions should be merged as one
> and take a callback as an argument. This would help factorizing.
> For instance for GetSizeofItsGroupNodes():
>
> typedef UINT32 (*GET_NODE_SIZE_CB) (
> IN CONST VOID *Node
> );
>
> STATIC
> UINT64
> GetSizeofNodes (
> IN UINT32 NodeType,
> IN CONST UINT32 NodeStartOffset,
> IN CONST CM_ARM_ITS_GROUP_NODE *NodeList,
> IN UINT32 NodeCount,
> IN GET_NODE_SIZE *GetNodeSizeCb,
> IN OUT IORT_NODE_INDEXER **CONST NodeIndexer
> )
> {
> UINT64 Size;
>
> ASSERT (NodeList != NULL);
>
> Size = 0;
> while (NodeCount-- != 0) {
> (*NodeIndexer)->Token = NodeList->Token;
> (*NodeIndexer)->Object = (VOID *)NodeList;
> (*NodeIndexer)->Offset = (UINT32)(Size + NodeStartOffset);
> (*NodeIndexer)->Identifier = NodeList->Identifier;
> DEBUG ((
> DEBUG_INFO,
> "IORT: Node Indexer = %p, Token = %p, Object = %p,"
> " Offset = 0x%x, Identifier = 0x%x\n",
> *NodeIndexer,
> (*NodeIndexer)->Token,
> (*NodeIndexer)->Object,
> (*NodeIndexer)->Offset,
> (*NodeIndexer)->Identifier
> ));
>
> Size += GetNodeSizeCb (NodeList);
> (*NodeIndexer)++;
> NodeList++;
> }
>
> return Size;
> }
>
> And then:
> GetSizeofItsGroupNodes (...)
> becomes:
> GetSizeofNodes (..., GetItsGroupNodeSize, ...)
>
> (ideally done in a separate patch)
[SAMI] Agree. However, this is a different patch and not part of this
series.
>
>> ));
>
> [snip]
>
>> +/** Update the Memory Range Descriptor Array.
>> +
>> + This function retrieves the Memory Range Descriptor objects
>> referenced by
>> + MemRangeDescToken and updates the Memory Range Descriptor array.
>> +
>> + @param [in] This Pointer to the table Generator.
>> + @param [in] CfgMgrProtocol Pointer to the Configuration
>> Manager
>> + Protocol Interface.
>> + @param [in] DescArray Pointer to an array of Memory
>> Range
>> + Descriptors.
>> + @param [in] DescCount Number of Id Descriptors.
>> + @param [in] DescToken Reference Token for retrieving the
>> + Memory Range Descriptor Array.
>> +
>> + @retval EFI_SUCCESS Table generated successfully.
>> + @retval EFI_INVALID_PARAMETER A parameter is invalid.
>> + @retval EFI_NOT_FOUND The required object was not found.
>> +**/
>> +STATIC
>> +EFI_STATUS
>> +AddMemRangeDescArray (
>> + IN CONST ACPI_TABLE_GENERATOR *CONST This,
>> + IN CONST EDKII_CONFIGURATION_MANAGER_PROTOCOL *CONST
>> CfgMgrProtocol,
>> + IN EFI_ACPI_6_0_IO_REMAPPING_MEM_RANGE_DESC *DescArray,
>> + IN UINT32 DescCount,
>> + IN CONST CM_OBJECT_TOKEN DescToken
>> + )
>> +{
>> + EFI_STATUS Status;
>> + CM_ARM_MEMORY_RANGE_DESCRIPTOR *MemRangeDesc;
>> + UINT32 MemRangeDescCount;
>> +
>> + ASSERT (DescArray != NULL);
>> +
>> + // Get the Id Mapping Array
>> + Status = GetEArmObjMemoryRangeDescriptor (
>> + CfgMgrProtocol,
>> + DescToken,
>> + &MemRangeDesc,
>> + &MemRangeDescCount
>> + );
>> + if (EFI_ERROR (Status)) {
>> + DEBUG ((
>> + DEBUG_ERROR,
>> + "ERROR: IORT: Failed to get Memory Range Descriptor array.
>> Status = %r\n",
>> + Status
>> + ));
>> + return Status;
>> + }
>> +
>> + if (MemRangeDescCount < DescCount) {
>
> I think this should be '!='
[SAMI] This is not really required as we are using DescCount to add the
nodes. So even if the ConfigurationManager returns more than the
required number of nodes, we only use the first DescCount nodes. The
remaining are ignored.
>
>> + DEBUG ((
>> + DEBUG_ERROR,
>> + "ERROR: IORT: Failed to get the required number of Memory"
>> + " Range Descriptors.\n"
>> + ));
>> + return EFI_NOT_FOUND;
>> + }
>> +
>> + // Populate the Memory Range Descriptor array
>> + while (DescCount-- != 0) {
>> + DescArray->Base = MemRangeDesc->BaseAddress;
>> + DescArray->Length = MemRangeDesc->Length;
>> + DescArray->Reserved = EFI_ACPI_RESERVED_DWORD;
>> +
>> + DescArray++;
>> + MemRangeDesc++;
>> + }
>> +
>> + return EFI_SUCCESS;
>> +}
>> +
>
> [snip]
>
>> +
>> /** Construct the IORT ACPI table.
>> This function invokes the Configuration Manager protocol
>> interface
>> @@ -1632,6 +2078,7 @@ BuildIortTable (
>> UINT32 SmmuV1V2NodeCount;
>> UINT32 SmmuV3NodeCount;
>> UINT32 PmcgNodeCount;
>> + UINT32 RmrNodeCount;
>> UINT32 ItsGroupOffset;
>> UINT32 NamedComponentOffset;
>> @@ -1639,6 +2086,7 @@ BuildIortTable (
>> UINT32 SmmuV1V2Offset;
>> UINT32 SmmuV3Offset;
>> UINT32 PmcgOffset;
>> + UINT32 RmrOffset;
>> CM_ARM_ITS_GROUP_NODE *ItsGroupNodeList;
>> CM_ARM_NAMED_COMPONENT_NODE *NamedComponentNodeList;
>> @@ -1646,6 +2094,7 @@ BuildIortTable (
>> CM_ARM_SMMUV1_SMMUV2_NODE *SmmuV1V2NodeList;
>> CM_ARM_SMMUV3_NODE *SmmuV3NodeList;
>> CM_ARM_PMCG_NODE *PmcgNodeList;
>> + CM_ARM_RMR_NODE *RmrNodeList;
>> EFI_ACPI_6_0_IO_REMAPPING_TABLE *Iort;
>> IORT_NODE_INDEXER *NodeIndexer;
>> @@ -1789,6 +2238,29 @@ BuildIortTable (
>> // Add the PMCG node count
>> IortNodeCount += PmcgNodeCount;
>> + if (AcpiTableInfo->AcpiTableRevision >=
>> + EFI_ACPI_IO_REMAPPING_TABLE_REVISION_05)
>> + {
>> + // Get the RMR node info
>> + Status = GetEArmObjRmr (
>> + CfgMgrProtocol,
>> + CM_NULL_TOKEN,
>> + &RmrNodeList,
>> + &RmrNodeCount
>> + );
>> + if (EFI_ERROR (Status) && (Status != EFI_NOT_FOUND)) {
>> + DEBUG ((
>> + DEBUG_ERROR,
>> + "ERROR: IORT: Failed to get RMR Node Info. Status = %r\n",
>> + Status
>> + ));
>> + goto error_handler;
>> + }
>> +
>> + // Add the RMR node count
>> + IortNodeCount += RmrNodeCount;
>> + }
>> +
>> // Allocate Node Indexer array
>> NodeIndexer = (IORT_NODE_INDEXER *)AllocateZeroPool (
>> (sizeof (IORT_NODE_INDEXER) *
>> @@ -1998,6 +2470,40 @@ BuildIortTable (
>> ));
>> }
>> + // RMR Nodes
>> + if ((AcpiTableInfo->AcpiTableRevision >=
>> + EFI_ACPI_IO_REMAPPING_TABLE_REVISION_05) &&
>> + (RmrNodeCount > 0))
>> + {
>
> There are a few checks like this on the revision.
> It seems that the 'Identifier' field was added in rev E (=1)
> and its size was increased in E.a (=2). So the identifier
> field should theoretically be generated for a Revision >= 1.
>
> As we jump from rev D (=0) t5 E.d (=5), maybe we generation
> of IORT tables with a revision in between should be prevented.
[SAMI] Ack. I will add a check to say generation of IORT table for
revisions 1 to 4 is not supported.
>
> Regards,
> Pierre
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#91320): https://edk2.groups.io/g/devel/message/91320
Mute This Topic: https://groups.io/mt/92334083/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