Re: [edk2-devel] [PATCH v2 7/8] DynamicTablesPkg: Update ArmNameSpaceObjects for IORT Rev E.b

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

Hi Sami,

With the small modifications:

Reviewed-by: Pierre Gondois <pierre.gondois@arm.com>


On 6/17/21 10:55, Sami Mujawar via groups.io wrote:
> Bugzilla: 3458 - Add support IORT Rev E.b specification updates
>           (https://bugzilla.tianocore.org/show_bug.cgi?id=3458)
>
> The IO Remapping Table, Platform Design Document, Revision E.b,
> Feb 2021 (https://developer.arm.com/documentation/den0049/)
> introduces the following updates, collectively including the
> updates and errata fixes to Rev E and Rev E.a:
>   - increments the IORT table revision to 3.
>   - updates the node definition to add an 'Identifier' field.
>   - adds definition of node type 6 - Reserved Memory Range node.
>   - adds definition for Memory Range Descriptors.
>   - adds flag to indicate PRI support for root complexes.
>   - adds flag to indicate if the root complex supports forwarding
>     of PASID information on translated transactions to the SMMU.
>
> Therefore, update the Arm namespace objects to:
>   - add Identifier field to IORT nodes.
>   - introduce enums to represent RMR nodes and Memory Range
>     descriptors.
>   - add definition of node type 6 - Reserved Memory Range node.
>   - add definition for Memory Range Descriptors.
>
> Signed-off-by: Sami Mujawar <sami.mujawar@arm.com>
> ---
>
> Notes:
>     v2:
>      - No code change since v1. Re-sending with v2 series.    [SAMI]
>
>  DynamicTablesPkg/Include/ArmNameSpaceObjects.h | 58 ++++++++++++++++++++
>  1 file changed, 58 insertions(+)
>
> diff --git a/DynamicTablesPkg/Include/ArmNameSpaceObjects.h b/DynamicTablesPkg/Include/ArmNameSpaceObjects.h
> index 19dcae13b2191e5f0b03ea85edec1191d2a406bf..98143cb5df127273cdd75452170fa651372b3896 100644
> --- a/DynamicTablesPkg/Include/ArmNameSpaceObjects.h
> +++ b/DynamicTablesPkg/Include/ArmNameSpaceObjects.h
> @@ -58,6 +58,8 @@ typedef enum ArmObjectID {
>    EArmObjGenericInitiatorAffinityInfo, ///< 34 - Generic Initiator Affinity
>    EArmObjSerialPortInfo,               ///< 35 - Generic Serial Port Info
>    EArmObjCmn600Info,                   ///< 36 - CMN-600 Info
> +  EArmObjRmr,                          ///< 37 - Reserved Memory Range Node
> +  EArmObjMemoryRangeDescriptor,        ///< 38 - Memory Range Descriptor
>    EArmObjMax
>  } EARM_OBJECT_ID;
>  
> @@ -466,6 +468,9 @@ typedef struct CmArmItsGroupNode {
>    UINT32            ItsIdCount;
>    /// Reference token for the ITS identifier array
>    CM_OBJECT_TOKEN   ItsIdToken;
> +
> +  /// Unique identifier for this node.
> +  UINT32            Identifier;

Would it be possible to say in the description that this is part of the
common node header ?

As we cannot modify the order of the fields anymore, I understand this
cannot be upper, but this would help as the order is a bit confusing.

>  } CM_ARM_ITS_GROUP_NODE;
>  
>  /** A structure that describes the
> @@ -497,6 +502,9 @@ typedef struct CmArmNamedComponentNode {
>        the entry in the namespace for this object.
>    */
>    CHAR8*            ObjectName;
> +
> +  /// Unique identifier for this node.
> +  UINT32            Identifier;
>  } CM_ARM_NAMED_COMPONENT_NODE;
>  
>  /** A structure that describes the
> @@ -525,6 +533,9 @@ typedef struct CmArmRootComplexNode {
>    UINT32            PciSegmentNumber;
>    /// Memory address size limit
>    UINT8             MemoryAddressSize;
> +
> +  /// Unique identifier for this node.
> +  UINT32            Identifier;
>  } CM_ARM_ROOT_COMPLEX_NODE;
>  
>  /** A structure that describes the
> @@ -567,6 +578,9 @@ typedef struct CmArmSmmuV1SmmuV2Node {
>    UINT32            SMMU_NSgCfgIrpt;
>    /// SMMU_NSgCfgIrpt interrupt flags
>    UINT32            SMMU_NSgCfgIrptFlags;
> +
> +  /// Unique identifier for this node.
> +  UINT32            Identifier;
>  } CM_ARM_SMMUV1_SMMUV2_NODE;
>  
>  /** A structure that describes the
> @@ -603,6 +617,9 @@ typedef struct CmArmSmmuV3Node {
>    UINT32            ProximityDomain;
>    /// Index into the array of ID mapping
>    UINT32            DeviceIdMappingIndex;
> +
> +  /// Unique identifier for this node.
> +  UINT32            Identifier;
>  } CM_ARM_SMMUV3_NODE;
>  
>  /** A structure that describes the
> @@ -627,6 +644,9 @@ typedef struct CmArmPmcgNode {
>  
>    /// Reference token for the IORT node associated with this node
>    CM_OBJECT_TOKEN   ReferenceToken;
> +
> +  /// Unique identifier for this node.
> +  UINT32            Identifier;
>  } CM_ARM_PMCG_NODE;
>  
>  /** A structure that describes the
> @@ -878,6 +898,44 @@ typedef struct CmArmCmn600Info {
>    CM_ARM_EXTENDED_INTERRUPT  DtcInterrupt[4];
>  } CM_ARM_CMN_600_INFO;
>  
> +/** A structure that describes the
> +    RMR node for the Platform.
> +
> +    ID: EArmObjRmr
> +*/
> +typedef struct CmArmRmrNode {
> +  /// An unique token used to identify this object
> +  CM_OBJECT_TOKEN   Token;
> +  /// Number of ID mappings
> +  UINT32            IdMappingCount;
> +  /// Reference token for the ID mapping array
> +  CM_OBJECT_TOKEN   IdMappingToken;
> +
> +  /// Reserved Memory Range flags.
> +  UINT32            Flags;
> +
> +  /// Memory range descriptor count.
> +  UINT32            MemRangeDescCount;
> +  /// Reference token for the Memory Range descriptor array
> +  CM_OBJECT_TOKEN   MemRangeDescToken;
> +
> +  /// Unique identifier for this node.
> +  UINT32            Identifier;
The 'Identifier' field should be added above the 'Flags' as the
structure is a new and to respect the order of the spec.
> +} CM_ARM_RMR_NODE;
> +
> +/** A structure that describes the
> +    Memory Range descriptor.
> +
> +    ID: EArmObjMemoryRangeDescriptor
> +*/
> +typedef struct CmArmRmrDescriptor {
> +  /// Base address.
> +  UINT64            BaseAddress;
> +
> +  /// Length.
> +  UINT64            Length;

The exact names of the fields in the spec are:
-Physical Range offset
-Physical Range length

Would it be possible to add these names in the description ?
> +} CM_ARM_MEMORY_RANGE_DESCRIPTOR;
> +
>  #pragma pack()
>  
>  #endif // ARM_NAMESPACE_OBJECTS_H_