Re: [PATCH v5 8/8] DynamicTablesPkg: IORT generator updates for Rev E.d spec

["PierreGondois" <pierre.gondois@arm.com>]

Hi Sami,

>   
> @@ -37,9 +37,11 @@ Requirements:
>     - EArmObjSmmuV1SmmuV2
>     - EArmObjSmmuV3
>     - EArmObjPmcg
> +  - EArmObjRmr
>     - EArmObjGicItsIdentifierArray
>     - EArmObjIdMappingArray
> -  - EArmObjGicItsIdentifierArray

I think EArmObjGicItsIdentifierArray should be conserved.

> +  - 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)

>         ));
>   

[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 '!='

> +    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.

Regards,
Pierre