[edk2-devel] [PATCH v2 6/8] DynamicTablesPkg: IORT set reference to interrupt array if present

[PierreGondois]

Hi Sami,

With the small modification:

Reviewed-by: Pierre Gondois <pierre.gondois at 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.mujawar at 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/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-