Hi Sami,
With the small modification:
Reviewed-by: Pierre Gondois <pierre.gond...@arm.com>
On 6/17/21 10:55, Sami Mujawar via groups.io wrote:
> The IORT generator is populating the reference field for Context and
> PMU interrupts even if their count is zero.
>
> Update the IORT generator to set the references only if the interrupt
> count is not 0. Also add checks to ensure a valid reference token has
> been provided.
>
> Signed-off-by: Sami Mujawar <sami.muja...@arm.com>
> ---
>
> Notes:
> v2:
> - No code change since v1. Re-sending with v2 series. [SAMI]
>
> DynamicTablesPkg/Library/Acpi/Arm/AcpiIortLibArm/IortGenerator.c | 82
> +++++++++++++-------
> 1 file changed, 55 insertions(+), 27 deletions(-)
>
> diff --git a/DynamicTablesPkg/Library/Acpi/Arm/AcpiIortLibArm/IortGenerator.c
> b/DynamicTablesPkg/Library/Acpi/Arm/AcpiIortLibArm/IortGenerator.c
> index
> bdf839eab25e2b84b40c50da38f2bf961cdc5f42..9ccf72594db378878d4e3abbafe98e749d9963da
> 100644
> --- a/DynamicTablesPkg/Library/Acpi/Arm/AcpiIortLibArm/IortGenerator.c
> +++ b/DynamicTablesPkg/Library/Acpi/Arm/AcpiIortLibArm/IortGenerator.c
> @@ -1136,6 +1136,7 @@ AddSmmuV1V2Nodes (
> EFI_ACPI_6_0_IO_REMAPPING_SMMU_INT * ContextInterruptArray;
> EFI_ACPI_6_0_IO_REMAPPING_SMMU_INT * PmuInterruptArray;
> UINT64 NodeLength;
> + UINT32 Offset;
>
> ASSERT (Iort != NULL);
>
> @@ -1178,47 +1179,74 @@ AddSmmuV1V2Nodes (
> SmmuNode->GlobalInterruptArrayRef =
> OFFSET_OF (EFI_ACPI_6_0_IO_REMAPPING_SMMU_NODE, SMMU_NSgIrpt);
>
> + Offset = sizeof (EFI_ACPI_6_0_IO_REMAPPING_SMMU_NODE);
> // Context Interrupt
> SmmuNode->NumContextInterrupts = NodeList->ContextInterruptCount;
> - SmmuNode->ContextInterruptArrayRef =
> - sizeof (EFI_ACPI_6_0_IO_REMAPPING_SMMU_NODE);
> - ContextInterruptArray =
> - (EFI_ACPI_6_0_IO_REMAPPING_SMMU_INT*)((UINT8*)SmmuNode +
> - sizeof (EFI_ACPI_6_0_IO_REMAPPING_SMMU_NODE));
> + if (NodeList->ContextInterruptCount != 0) {
> + SmmuNode->ContextInterruptArrayRef = Offset;
> + ContextInterruptArray =
> + (EFI_ACPI_6_0_IO_REMAPPING_SMMU_INT*)((UINT8*)SmmuNode + Offset);
> + Offset += (NodeList->ContextInterruptCount *
> + sizeof (EFI_ACPI_6_0_IO_REMAPPING_SMMU_INT));
> + }
>
> // PMU Interrupt
> SmmuNode->NumPmuInterrupts = NodeList->PmuInterruptCount;
> - SmmuNode->PmuInterruptArrayRef = SmmuNode->ContextInterruptArrayRef +
> - (NodeList->ContextInterruptCount *
> - sizeof (EFI_ACPI_6_0_IO_REMAPPING_SMMU_INT));
> - PmuInterruptArray =
> - (EFI_ACPI_6_0_IO_REMAPPING_SMMU_INT*)((UINT8*)SmmuNode +
> - SmmuNode->PmuInterruptArrayRef);
> + if (NodeList->PmuInterruptCount != 0) {
> + SmmuNode->PmuInterruptArrayRef = Offset;
> + PmuInterruptArray =
> + (EFI_ACPI_6_0_IO_REMAPPING_SMMU_INT*)((UINT8*)SmmuNode + Offset);
> + }
>
> SmmuNode->SMMU_NSgIrpt = NodeList->SMMU_NSgIrpt;
> SmmuNode->SMMU_NSgIrptFlags = NodeList->SMMU_NSgIrptFlags;
> SmmuNode->SMMU_NSgCfgIrpt = NodeList->SMMU_NSgCfgIrpt;
> SmmuNode->SMMU_NSgCfgIrptFlags = NodeList->SMMU_NSgCfgIrptFlags;
>
> - // Add Context Interrupt Array
> - Status = AddSmmuInterruptArray (
> - CfgMgrProtocol,
> - ContextInterruptArray,
> - SmmuNode->NumContextInterrupts,
> - NodeList->ContextInterruptToken
> - );
> - if (EFI_ERROR (Status)) {
> - DEBUG ((
> - DEBUG_ERROR,
> - "ERROR: IORT: Failed to Context Interrupt Array. Status = %r\n",
> - Status
> - ));
> - return Status;
> + if (NodeList->ContextInterruptCount != 0) {
> + if (NodeList->ContextInterruptToken == CM_NULL_TOKEN) {
> + Status = EFI_INVALID_PARAMETER;
> + DEBUG ((
> + DEBUG_ERROR,
> + "ERROR: IORT: Invalid Context Interrupt token,"
> + " Token = 0x%x, Status =%r\n",
> + NodeList->ContextInterruptToken,
> + Status
> + ));
> + return Status;
> + }
> +
> + // Add Context Interrupt Array
> + Status = AddSmmuInterruptArray (
> + CfgMgrProtocol,
> + ContextInterruptArray,
> + SmmuNode->NumContextInterrupts,
> + NodeList->ContextInterruptToken
> + );
> + if (EFI_ERROR (Status)) {
> + DEBUG ((
> + DEBUG_ERROR,
> + "ERROR: IORT: Failed to Context Interrupt Array. Status = %r\n",
> + Status
> + ));
> + return Status;
> + }
> }
>
> // Add PMU Interrupt Array
> - if ((SmmuNode->NumPmuInterrupts > 0) &&
> - (NodeList->PmuInterruptToken != CM_NULL_TOKEN)) {
> + if (SmmuNode->NumPmuInterrupts != 0) {
> + if (NodeList->PmuInterruptToken == CM_NULL_TOKEN) {
> + Status = EFI_INVALID_PARAMETER;
> + DEBUG ((
> + DEBUG_ERROR,
> + "ERROR: IORT: Invalid PMU Interrupt token,"
> + " Token = 0x%x, Status =%r\n",
> + NodeList->PmuInterruptToken,
> + Status
> + ));
> + return Status;
> + }
> +
Some similar modifications are done in the last patch of the serie:
[PATCH v2 8/8] DynamicTablesPkg: IORT generator updates for Rev E.b spec
For instance, the same change is done in the AddPmcgNodes() function.
Would it be possible to group them ?
> Status = AddSmmuInterruptArray (
> CfgMgrProtocol,
> PmuInterruptArray,
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#82239): https://edk2.groups.io/g/devel/message/82239
Mute This Topic: https://groups.io/mt/83600728/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
