Re: [edk2-devel] [PATCH] DynamicTablesPkg: IORT generator updates for Rev E.e spec

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

Hi Leif,

Please find my response inline marked [SAMI].

Regards,

Sami Mujawar

On 25/09/2023 10:40 am, Leif Lindholm wrote:
> On Fri, Sep 22, 2023 at 15:40:38 -0600, Swatisri Kantamsetti via groups.io wrote:
>> The IO Remapping Table, Platform Design Document, Revision E.e,
>> Sept 2022 (https://developer.arm.com/documentation/den0049/ee)
>> added flags in SMMUv3 node for validity of ID mappings for MSIs
>> related to control interrupts.
>> It makes one small addition to SMMUv3 nodes to
>> describe MSI support independently of wired GSIV support
>>
>> Therefore, update the IORT generator to:
>> - increment IORT table revision to 6
>> - increment SMMUV3 node revision to 5
>> - for SMMUV3 node revision >=5 check the DeviceID mapping index
>>    valid flag to populate DeviceIdMappingIndex field
>>
>> Signed-off-by: Swatisri Kantamsetti <swatisrik@nvidia.com>
>> ---
>>   .../Acpi/Arm/AcpiIortLibArm/IortGenerator.c   | 35 +++++++++++++++----
>>   1 file changed, 28 insertions(+), 7 deletions(-)
>>
>> diff --git a/DynamicTablesPkg/Library/Acpi/Arm/AcpiIortLibArm/IortGenerator.c b/DynamicTablesPkg/Library/Acpi/Arm/AcpiIortLibArm/IortGenerator.c
>> index f28973c1a8..a6e4b49cb1 100644
>> --- a/DynamicTablesPkg/Library/Acpi/Arm/AcpiIortLibArm/IortGenerator.c
>> +++ b/DynamicTablesPkg/Library/Acpi/Arm/AcpiIortLibArm/IortGenerator.c
>> @@ -5,7 +5,7 @@
>>     SPDX-License-Identifier: BSD-2-Clause-Patent
>>   
>>     @par Reference(s):
>> -  - IO Remapping Table, Platform Design Document, Revision E.d, Feb 2022
>> +  - IO Remapping Table, Platform Design Document, Revision E.e, Sept 2022
>>       (https://developer.arm.com/documentation/den0049/)
>>   
>>   **/
>> @@ -1554,9 +1554,14 @@ AddSmmuV3Nodes (
>>       {
>>         SmmuV3Node->Node.Revision   = 2;
>>         SmmuV3Node->Node.Identifier = EFI_ACPI_RESERVED_DWORD;
>> -    } else {
>> +    } else if (AcpiTableInfo->AcpiTableRevision <
>> +               EFI_ACPI_IO_REMAPPING_TABLE_REVISION_06)
>> +    {
>>         SmmuV3Node->Node.Revision   = 4;
>>         SmmuV3Node->Node.Identifier = NodeList->Identifier;
>> +    } else {
>> +      SmmuV3Node->Node.Revision   = 5;
>> +      SmmuV3Node->Node.Identifier = NodeList->Identifier;
> This is borderline, but I think it would be worth breaking out into a
> helper function. It feels like this may have further cases added in
> future.
>
>>       }
>>   
>>       // SMMUv3 specific data
>> @@ -1577,11 +1582,27 @@ AddSmmuV3Nodes (
>>         SmmuV3Node->ProximityDomain = 0;
>>       }
>>   
>> -    if ((SmmuV3Node->Event != 0) && (SmmuV3Node->Pri != 0) &&
>> -        (SmmuV3Node->Gerr != 0) && (SmmuV3Node->Sync != 0))
>> +    /* For older SMMUV3 nodes rev. < 5.
>> +       If all the SMMU control interrupts are GSIV based,
>> +       the DeviceID mapping index field is ignored.
>> +       DeviceID mapping valid flag was introduced in IORT rev E.e
>> +       for SMMUV3 nodes rev. > 5.
>> +       If the DeviceID mapping index valid flag is set to 0,
>> +       DeviceID mapping index field must be ignored.
>> +       Where the SMMU uses message signaled interrupts for
>> +       its control interrupts, DeviceId Mapping Index contains an
>> +       index into the array of ID mapping.
>> +     */
>> +    if (((SmmuV3Node->Node.Revision < 5) &&
>> +         (SmmuV3Node->Event != 0)        &&
>> +         (SmmuV3Node->Pri != 0)          &&
>> +         (SmmuV3Node->Gerr != 0)         &&
>> +         (SmmuV3Node->Sync != 0)
>> +         ) ||
>> +        ((SmmuV3Node->Node.Revision >= 5) &&
>> +         ((SmmuV3Node->Flags & EFI_ACPI_IORT_SMMUv3_FLAG_DEVICEID_VALID) == 0))
>> +        )
> This is not borderline. I'm sure all of this is correct, but it is no
> longer human readable. This type of information overload is better
> kept out of a top-level function.
> This also means we can give it an informative name.
>
> Best Regards,
>
> Leif
>
> p.s.
> Apologies Sami, but since I spotted this before it had been merged by
> CI, I dropped the "push" tag since I wanted to make a comment.
> You are the maintainer, and the above are my opinions only. If you
> disagree, feel free to add it back and push as is.
> (And feel free to do the same to push requests initiated by me in
> future.)

[SAMI] I agree, this is becoming complex to read. I think it would be 
good to have helper functions to make the code clearer.

I have closed the pull request and look forward to a new patch from 
Swatisri.

[/SAMI]

>>       {
>> -      // If all the SMMU control interrupts are GSIV based,
>> -      // the DeviceID mapping index field is ignored.
>>         SmmuV3Node->DeviceIdMappingIndex = 0;
>>       } else {
>>         SmmuV3Node->DeviceIdMappingIndex = NodeList->DeviceIdMappingIndex;
>> @@ -2819,7 +2840,7 @@ ACPI_IORT_GENERATOR  IortGenerator = {
>>       // ACPI Table Signature
>>       EFI_ACPI_6_4_IO_REMAPPING_TABLE_SIGNATURE,
>>       // ACPI Table Revision supported by this Generator
>> -    EFI_ACPI_IO_REMAPPING_TABLE_REVISION_05,
>> +    EFI_ACPI_IO_REMAPPING_TABLE_REVISION_06,
>>       // Minimum supported ACPI Table Revision
>>       EFI_ACPI_IO_REMAPPING_TABLE_REVISION_00,
>>       // Creator ID
>> -- 
>> 2.17.1
>>
>>
>>
>> 
>>
>>


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#109040): https://edk2.groups.io/g/devel/message/109040
Mute This Topic: https://groups.io/mt/101535844/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-