Re: [edk2-devel] [PATCH v4 1/8] MdePkg: IORT header update for IORT Rev E.d spec

["Sami Mujawar" <sami.mujawar@arm.com>]

Hi Jon,

Thank you for your feedback. Please see my response inline marked [SAMI].

Regards,

Sami Mujawar

On 07/07/2022 07:05 am, Jon Nettleton wrote:
> On Wed, Jul 6, 2022 at 11:57 AM Sami Mujawar <sami.mujawar@arm.com> wrote:
>> Bugzilla: 3458 - Add support IORT Rev E.d specification updates
>>            (https://bugzilla.tianocore.org/show_bug.cgi?id=3458)
>>
>> The IO Remapping Table, Platform Design Document, Revision E.d,
>> Feb 2022 (https://developer.arm.com/documentation/den0049/)
>> introduces the following updates, collectively including the
>> updates and errata fixes to Rev E, Rev E.a, Rev E.b, Rev E.c:
>>    - increments the IORT table revision to 5.
>>    - 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.
>>    - adds flag to indicate if the root complex supports PASID.
>>    - adds flags to define access privilege and attributes for the
>>      memory ranges.
>>
>> Therefore, update the IORT header file to reflect these changes,
>> and also rename the EFI_ACPI_IO_REMAPPING_TABLE_REVISION macro to
>> EFI_ACPI_IO_REMAPPING_TABLE_REV0.
>>
>> Signed-off-by: Sami Mujawar <sami.mujawar@arm.com>
>> ---
>>
>> Notes:
>>      v4:
>>        - Updated patch series to IORT specification revision E.d.     [SAMI]
>>        - Add flag to indicate if the root complex supports PASID.     [SAMI]
>>        - Add flags to define access privilege and attributes for      [SAMI]
>>          the memory ranges.
>>      v3:
>>        - Submit patch series to update platform code to use the       [LIMING]
>>          EFI_ACPI_IO_REMAPPING_TABLE_REV0 macro.
>>          Ref: https://edk2.groups.io/g/devel/topic/83618423#76799
>>        - Removed definition of EFI_ACPI_IO_REMAPPING_TABLE_REVISION   [SAMI]
>>          as EFI_ACPI_IO_REMAPPING_TABLE_REV0 has been provided for
>>          representing Rev 0. Also, a corresponding patch series
>>          for updating the platforms in edk2-platforms repository
>>          shall be submitted to the edk2 mailing list.
>>        - Include r-b received from v2 series.                         [SAMI]
>>          Ref: https://edk2.groups.io/g/devel/topic/83600724#76660
>>
>>      v2:
>>        - Set EFI_ACPI_IO_REMAPPING_TABLE_REVISION to Rev 0 as     [SAMI]
>>          setting to Rev 3 will break existing platforms. The
>>          problem is that existing code would not be populating
>>          the Identifier field in the nodes. This can lead to
>>          non-unique values in the Identifier field.
>>
>>   MdePkg/Include/IndustryStandard/IoRemappingTable.h | 83 ++++++++++++++++++--
>>   1 file changed, 75 insertions(+), 8 deletions(-)
>>
>> diff --git a/MdePkg/Include/IndustryStandard/IoRemappingTable.h b/MdePkg/Include/IndustryStandard/IoRemappingTable.h
>> index 79a34678681d45b2982dc8573db6bd447f42e429..07cb7f49dc936fb00cc549113f1e62f988535e5d 100644
>> --- a/MdePkg/Include/IndustryStandard/IoRemappingTable.h
>> +++ b/MdePkg/Include/IndustryStandard/IoRemappingTable.h
>> @@ -1,12 +1,19 @@
>>   /** @file
>> -  ACPI IO Remapping Table (IORT) as specified in ARM spec DEN0049D
>> -
>> -  http://infocenter.arm.com/help/topic/com.arm.doc.den0049d/DEN0049D_IO_Remapping_Table.pdf
>> +  ACPI IO Remapping Table (IORT) definitions.
>>
>>     Copyright (c) 2017, Linaro Limited. All rights reserved.<BR>
>> -  Copyright (c) 2018, ARM Limited. All rights reserved.<BR>
>> +  Copyright (c) 2018 - 2022, Arm Limited. All rights reserved.<BR>
>>
>>     SPDX-License-Identifier: BSD-2-Clause-Patent
>> +
>> +  @par Reference(s):
>> +  - IO Remapping Table, Platform Design Document, Revision E.d, Feb 2022
>> +    (https://developer.arm.com/documentation/den0049/)
>> +
>> +  @par Glossary:
>> +  - Ref  : Reference
>> +  - Mem  : Memory
>> +  - Desc : Descriptor
>>   **/
>>
>>   #ifndef __IO_REMAPPING_TABLE_H__
>> @@ -14,7 +21,8 @@
>>
>>   #include <IndustryStandard/Acpi.h>
>>
>> -#define EFI_ACPI_IO_REMAPPING_TABLE_REVISION  0x0
>> +#define EFI_ACPI_IO_REMAPPING_TABLE_REV0  0x0
>> +#define EFI_ACPI_IO_REMAPPING_TABLE_REV5  0x5
>>
>>   #define EFI_ACPI_IORT_TYPE_ITS_GROUP     0x0
>>   #define EFI_ACPI_IORT_TYPE_NAMED_COMP    0x1
>> @@ -22,6 +30,7 @@
>>   #define EFI_ACPI_IORT_TYPE_SMMUv1v2      0x3
>>   #define EFI_ACPI_IORT_TYPE_SMMUv3        0x4
>>   #define EFI_ACPI_IORT_TYPE_PMCG          0x5
>> +#define EFI_ACPI_IORT_TYPE_RMR           0x6
>>
>>   #define EFI_ACPI_IORT_MEM_ACCESS_PROP_CCA  BIT0
>>
>> @@ -55,7 +64,29 @@
>>   #define EFI_ACPI_IORT_SMMUv3_MODEL_CAVIUM_CN99XX     0x2
>>
>>   #define EFI_ACPI_IORT_ROOT_COMPLEX_ATS_UNSUPPORTED  0x0
>> -#define EFI_ACPI_IORT_ROOT_COMPLEX_ATS_SUPPORTED    0x1
>> +#define EFI_ACPI_IORT_ROOT_COMPLEX_ATS_SUPPORTED    BIT0
>> +
>> +#define EFI_ACPI_IORT_ROOT_COMPLEX_PRI_UNSUPPORTED  0x0
>> +#define EFI_ACPI_IORT_ROOT_COMPLEX_PRI_SUPPORTED    BIT1
>> +
>> +#define EFI_ACPI_IORT_ROOT_COMPLEX_PASID_FWD_UNSUPPORTED  0x0
>> +#define EFI_ACPI_IORT_ROOT_COMPLEX_PASID_FWD_SUPPORTED    BIT2
>> +
>> +#define EFI_ACPI_IORT_ROOT_COMPLEX_PASID_UNSUPPORTED  0x0
>> +#define EFI_ACPI_IORT_ROOT_COMPLEX_PASID_SUPPORTED    BIT1
>> +
>> +#define EFI_ACPI_IORT_RMR_REMAP_NOT_PERMITTED  0x0
>> +#define EFI_ACPI_IORT_RMR_REMAP_PERMITTED      BIT0
>> +
>> +#define EFI_ACPI_IORT_RMR_ACCESS_REQ_NOT_PRIVILEGED  0x0
>> +#define EFI_ACPI_IORT_RMR_ACCESS_REQ_PRIVILEGED      BIT1
>> +
>> +#define EFI_ACPI_IORT_RMR_ACCESS_ATTRIB_DEV_NGNRNE             0x0
>> +#define EFI_ACPI_IORT_RMR_ACCESS_ATTRIB_DEV_NGNRE              0x1
>> +#define EFI_ACPI_IORT_RMR_ACCESS_ATTRIB_DEV_NGRE               0x2
>> +#define EFI_ACPI_IORT_RMR_ACCESS_ATTRIB_DEV_GRE                0x3
>> +#define EFI_ACPI_IORT_RMR_ACCESS_ATTRIB_NORM_IN_NC_OUT_NC      0x4
>> +#define EFI_ACPI_IORT_RMR_ACCESS_ATTRIB_NORM_IN_WB_OUT_WB_ISH  0x5
>>
>>   #define EFI_ACPI_IORT_ID_MAPPING_FLAGS_SINGLE  BIT0
>>
>> @@ -89,7 +120,7 @@ typedef struct {
>>     UINT8     Type;
>>     UINT16    Length;
>>     UINT8     Revision;
>> -  UINT32    Reserved;
>> +  UINT32    Identifier;
>>     UINT32    NumIdMappings;
>>     UINT32    IdReference;
>>   } EFI_ACPI_6_0_IO_REMAPPING_NODE;
>> @@ -118,7 +149,9 @@ typedef struct {
>>     UINT32                            AtsAttribute;
>>     UINT32                            PciSegmentNumber;
>>     UINT8                             MemoryAddressSize;
>> -  UINT8                             Reserved1[3];
>> +  UINT16                            PasidCapabilities;
>> +  UINT8                             Reserved1[1];
>> +  UINT32                            Flags;
>>   } EFI_ACPI_6_0_IO_REMAPPING_RC_NODE;
>>
>>   ///
>> @@ -198,6 +231,40 @@ typedef struct {
>>     // EFI_ACPI_6_0_IO_REMAPPING_ID_TABLE      OverflowInterruptMsiMapping[1];
>>   } EFI_ACPI_6_0_IO_REMAPPING_PMCG_NODE;
>>
>> +///
>> +/// Memory Range Descriptor.
>> +///
>> +typedef struct {
>> +  /// Base address of Reserved Memory Range,
>> +  /// aligned to a page size of 64K.
>> +  UINT64    Base;
>> +
>> +  /// Length of the Reserved Memory range.
>> +  /// Must be a multiple of the page size of 64K.
>> +  UINT64    Length;
>> +
>> +  /// Reserved, must be zero.
>> +  UINT32    Reserved;
>> +} EFI_ACPI_6_0_IO_REMAPPING_MEM_RANGE_DESC;
>> +
>> +///
>> +/// Node type 6: Reserved Memory Range (RMR) node
>> +///
>> +typedef struct {
>> +  EFI_ACPI_6_0_IO_REMAPPING_NODE    Node;
>> +
>> +  /// RMR flags
>> +  UINT32                            Flags;
>> +
>> +  /// Memory range descriptor count.
>> +  UINT32                            NumMemRangeDesc;
>> +
>> +  /// Offset of the memory range descriptor array.
>> +  UINT32                            MemRangeDescRef;
>> +  // EFI_ACPI_6_0_IO_REMAPPING_ID_TABLE         IdMapping[1];
>> +  // EFI_ACPI_6_0_IO_REMAPPING_MEM_RANGE_DESC   MemRangeDesc[1];
>> +} EFI_ACPI_6_0_IO_REMAPPING_RMR_NODE;
>> +
>>   #pragma pack()
>>
>>   #endif
>> --
>> 'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'
>>
>>
>>
> Please break out the Flags attribute to its own structure so it better
> reflects the documentation in the spec.
>
> I am current using something like this,
>
> +^M
> +///^M
> +/// Node header definition shared by all node types^M
> +///^M
> +typedef struct {^M
> +  UINT32                                  RemappingPermitted   : 1;
> +  UINT32                                  AccessPrivilege      : 1;
> +  UINT32                                  AccessAttributes     : 8;
> +  UINT32                                  Reserved10_31        : 22;
> +} EFI_ACPI_6_0_IO_REMAPPING_RMR_FLAGS;
> +^M
>   ///
>   /// Node type 6: Reserved Memory Range (RMR) node
>   ///
> @@ -240,7 +258,7 @@ typedef struct {
>     EFI_ACPI_6_0_IO_REMAPPING_NODE              Node;
>
>     /// RMR flags
> -  UINT32                                      Flags;
> +  EFI_ACPI_6_0_IO_REMAPPING_RMR_FLAGS         Flags;

[SAMI] I have followed the coding style as in rest of this file. My 
concern is that introducing the use of bit fields for RMR node would 
make it inconsistent with the rest of the file.

Also, updating the Flags field in other IORT nodes to use bit fileds, 
would result in having to make corresponding changes to the platform code.

Considering this, I don't think this is something we should do.

[/SAMI]
> -Jon