From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by mx.groups.io with SMTP id smtpd.web11.21117.1657714132647730014 for ; Wed, 13 Jul 2022 05:08:53 -0700 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: arm.com, ip: 217.140.110.172, mailfrom: pierre.gondois@arm.com) Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 57E521424; Wed, 13 Jul 2022 05:08:52 -0700 (PDT) Received: from [192.168.1.11] (unknown [172.31.20.19]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 51BCE3F73D; Wed, 13 Jul 2022 05:08:50 -0700 (PDT) Message-ID: <8b1466c6-07b8-34f2-4515-0a63fa57b1a4@arm.com> Date: Wed, 13 Jul 2022 14:08:13 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.9.1 Subject: Re: [PATCH v5 4/8] DynamicTablesPkg: IORT set reference to interrupt array if present To: Sami Mujawar , devel@edk2.groups.io Cc: Alexei.Fedorov@arm.com, Matteo.Carlini@arm.com, Akanksha.Jain2@arm.com, Ben.Adderson@arm.com, nd@arm.com References: <20220712143141.18516-1-sami.mujawar@arm.com> <20220712143141.18516-5-sami.mujawar@arm.com> From: "PierreGondois" In-Reply-To: <20220712143141.18516-5-sami.mujawar@arm.com> Content-Language: en-US Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit On 7/12/22 16:31, Sami Mujawar 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 > Reviewed-by: Pierre Gondois > --- > > Notes: > v5: > - No code change since v4. Re-sending with v5 series. [SAMI] > > v4: > - Minor reordering of code to group initialisation and [SAMI] > checks for the number of Context and PMU interrupts. > > v3: > - Move error handling for IdMappingToken. [PIERRE] > - Moved error handling for IdMappingToken in a separate [SAMI] > patch in v3 series. > Ref: https://edk2.groups.io/g/devel/topic/83600728#76662 > > v2: > - No code change since v1. Re-sending with v2 series. [SAMI] > > DynamicTablesPkg/Library/Acpi/Arm/AcpiIortLibArm/IortGenerator.c | 87 +++++++++++++------- > 1 file changed, 57 insertions(+), 30 deletions(-) > > diff --git a/DynamicTablesPkg/Library/Acpi/Arm/AcpiIortLibArm/IortGenerator.c b/DynamicTablesPkg/Library/Acpi/Arm/AcpiIortLibArm/IortGenerator.c > index a4dd3d4a895e0a1ae305c937d9a413665fb8e171..abef9e5a7f90a38e1d697227d3cd2c110db364a4 100644 > --- a/DynamicTablesPkg/Library/Acpi/Arm/AcpiIortLibArm/IortGenerator.c > +++ b/DynamicTablesPkg/Library/Acpi/Arm/AcpiIortLibArm/IortGenerator.c > @@ -1164,6 +1164,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); > > @@ -1206,48 +1207,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)); > + SmmuNode->NumContextInterrupts = NodeList->ContextInterruptCount; > + 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); > + SmmuNode->NumPmuInterrupts = NodeList->PmuInterruptCount; > + 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; > + } > + > Status = AddSmmuInterruptArray ( > CfgMgrProtocol, > PmuInterruptArray, Hi Sami, I think this bits belongs to the previous patch. Regards, Pierre