[edk2-devel] [PATCH v4 1/8] MdePkg: IORT header update for IORT Rev E.d spec
Sami Mujawar
sami.mujawar at arm.com
Thu Jul 7 11:02:49 UTC 2022
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 at 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 at 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
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#91138): https://edk2.groups.io/g/devel/message/91138
Mute This Topic: https://groups.io/mt/92203096/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