Re: [PATCH edk2-platforms v4 9/9] Platform/ARM/Sgi: Add Ssdt, Iort and Mcfg tables

[Thomas Abraham <thomas.abraham@arm.com>]

On Mon, May 21, 2018 at 2:59 PM, Ard Biesheuvel
<ard.biesheuvel@linaro.org> wrote:
> On 21 May 2018 at 10:25, Thomas Abraham <thomas.abraham@arm.com> wrote:
>> SGI platforms support a AHCI controller which is attached to a PCIe
>> root complex and it can generate PCIe ITS-MSI transactions. So the
>> Ssdt, Iort and Mcfg ACPI tables to desribe this topology to the
>> linux kernel.
>>
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Thomas Abraham <thomas.abraham@arm.com>
>> ---
>>  .../ARM/SgiPkg/AcpiTables/Sgi575/AcpiTables.inf    |   5 +
>>  Platform/ARM/SgiPkg/AcpiTables/Sgi575/Iort.aslc    | 106 +++++++++++++++++++++
>>  Platform/ARM/SgiPkg/AcpiTables/Sgi575/Mcfg.aslc    |  59 ++++++++++++
>>  Platform/ARM/SgiPkg/AcpiTables/Sgi575/Ssdt.asl     |  91 ++++++++++++++++++
>>  4 files changed, 261 insertions(+)
>>  create mode 100644 Platform/ARM/SgiPkg/AcpiTables/Sgi575/Iort.aslc
>>  create mode 100644 Platform/ARM/SgiPkg/AcpiTables/Sgi575/Mcfg.aslc
>>  create mode 100644 Platform/ARM/SgiPkg/AcpiTables/Sgi575/Ssdt.asl
>>
>> diff --git a/Platform/ARM/SgiPkg/AcpiTables/Sgi575/AcpiTables.inf b/Platform/ARM/SgiPkg/AcpiTables/Sgi575/AcpiTables.inf
>> index 2c9bd98..485f819 100644
>> --- a/Platform/ARM/SgiPkg/AcpiTables/Sgi575/AcpiTables.inf
>> +++ b/Platform/ARM/SgiPkg/AcpiTables/Sgi575/AcpiTables.inf
>> @@ -25,8 +25,11 @@
>>    Dsdt.asl
>>    Fadt.aslc
>>    Gtdt.aslc
>> +  Iort.aslc
>>    Madt.aslc
>> +  Mcfg.aslc
>>    Spcr.aslc
>> +  Ssdt.asl
>>
>>  [Packages]
>>    ArmPkg/ArmPkg.dec
>> @@ -51,3 +54,5 @@
>>    gArmTokenSpaceGuid.PcdGenericWatchdogRefreshBase
>>    gArmPlatformTokenSpaceGuid.PcdSerialDbgRegisterBase
>>    gArmPlatformTokenSpaceGuid.PL011UartInterrupt
>> +
>> +  gArmSgiTokenSpaceGuid.PcdPciConfigurationSpaceBaseAddress
>> diff --git a/Platform/ARM/SgiPkg/AcpiTables/Sgi575/Iort.aslc b/Platform/ARM/SgiPkg/AcpiTables/Sgi575/Iort.aslc
>> new file mode 100644
>> index 0000000..f3b3105
>> --- /dev/null
>> +++ b/Platform/ARM/SgiPkg/AcpiTables/Sgi575/Iort.aslc
>> @@ -0,0 +1,106 @@
>> +/** @file
>> +*  I/O Remapping Table (Iort)
>> +*
>> +*  Copyright (c) 2018, ARM Ltd. All rights reserved.
>> +*
>> +*  This program and the accompanying materials are licensed and made available
>> +*  under the terms and conditions of the BSD License which accompanies this
>> +*  distribution.  The full text of the license may be found at
>> +*  http://opensource.org/licenses/bsd-license.php
>> +*
>> +*  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
>> +*  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
>> +*
>> +**/
>> +
>> +#include <IndustryStandard/Acpi.h>
>> +#include <IndustryStandard/Acpi60.h>
>> +#include <IndustryStandard/IoRemappingTable.h>
>> +#include "SgiAcpiHeader.h"
>> +
>> +#pragma pack(1)
>> +
>> +typedef struct
>> +{
>> +  EFI_ACPI_6_0_IO_REMAPPING_ITS_NODE       ItsNode;
>> +  UINT32                                   ItsIdentifiers;
>> +} ARM_EFI_ACPI_6_0_IO_REMAPPING_ITS_NODE;
>> +
>> +typedef struct
>> +{
>> +  EFI_ACPI_6_0_IO_REMAPPING_RC_NODE        RcNode;
>> +  EFI_ACPI_6_0_IO_REMAPPING_ID_TABLE       RcIdMap;
>> +} ARM_EFI_ACPI_6_0_IO_REMAPPING_RC_NODE;
>> +
>> +typedef struct
>> +{
>> +  EFI_ACPI_6_0_IO_REMAPPING_TABLE          Header;
>> +  ARM_EFI_ACPI_6_0_IO_REMAPPING_ITS_NODE   ItsNode;
>> +  ARM_EFI_ACPI_6_0_IO_REMAPPING_RC_NODE    RcNode;
>> +} ARM_EFI_ACPI_6_0_IO_REMAPPING_TABLE;
>> +
>> +#pragma pack ()
>> +
>> +ARM_EFI_ACPI_6_0_IO_REMAPPING_TABLE Iort =
>> +{
>> +  // EFI_ACPI_6_0_IO_REMAPPING_TABLE
>> +  {
>> +     ARM_ACPI_HEADER  // EFI_ACPI_DESCRIPTION_HEADER
>> +     (
>> +       EFI_ACPI_6_0_IO_REMAPPING_TABLE_SIGNATURE,
>> +       ARM_EFI_ACPI_6_0_IO_REMAPPING_TABLE,
>> +       EFI_ACPI_IO_REMAPPING_TABLE_REVISION
>> +     ),
>> +     2,  // NumNodes
>> +     sizeof (EFI_ACPI_6_0_IO_REMAPPING_TABLE),  // NodeOffset
>> +     0,  // Reserved
>> +  },
>> +  // ARM_EFI_ACPI_6_0_IO_REMAPPING_ITS_NODE
>> +  {
>> +    // EFI_ACPI_6_0_IO_REMAPPING_ITS_NODE
>> +    {
>> +      // EFI_ACPI_6_0_IO_REMAPPING_NODE
>> +      {
>> +        EFI_ACPI_IORT_TYPE_ITS_GROUP,  // Type
>> +        sizeof (ARM_EFI_ACPI_6_0_IO_REMAPPING_ITS_NODE), // Length
>> +        0,  // Revision
>> +        0,  // Reserved
>> +        0,  // NumIdMappings
>> +        0,  // IdReference
>> +      },
>> +      1,    // GIC ITS Identifiers
>> +    },
>> +    0,
>> +  },
>> +  // ARM_EFI_ACPI_6_0_IO_REMAPPING_RC_NODE
>> +  {
>> +    // EFI_ACPI_6_0_IO_REMAPPING_RC_NODE
>> +    {
>> +      // EFI_ACPI_6_0_IO_REMAPPING_NODE
>> +      {
>> +        EFI_ACPI_IORT_TYPE_ROOT_COMPLEX,  // Type
>> +        sizeof (ARM_EFI_ACPI_6_0_IO_REMAPPING_RC_NODE),  // Length
>> +        0,  // Revision
>> +        0,  // Reserved
>> +        1,  // NumIdMappings
>> +        __builtin_offsetof (ARM_EFI_ACPI_6_0_IO_REMAPPING_RC_NODE, RcIdMap),  // IdReference
>> +      },
>> +      1,  // CacheCoherent
>> +      0,  // AllocationHints
>> +      0,  // Reserved
>> +      0,  // MemoryAccessFlags
>> +      EFI_ACPI_IORT_ROOT_COMPLEX_ATS_UNSUPPORTED,  // AtsAttribute
>> +      0x0,                                         // PciSegmentNumber
>> +    },
>> +    // EFI_ACPI_6_0_IO_REMAPPING_ID_TABLE
>> +    {
>> +      0x0038,  // InputBase
>> +      0x0001,  // NumIds
>> +      0x0038,  // OutputBase
>
> This looks slightly dodgy to me. NumIds is zero based, so you are
> mapping device IDs 0x38 and 0x39 only. Is that what you intended?

Yes, that was the intent. I had tested it with these values. But
looking at again, it seems better to keep it zero based. Will fix it
in the next version.

>
> It is best to identity remap the entire RID range, even if the virtual
> AHCI is the only one that ever uses it. Alternatively, you can create
> a EFI_ACPI_IORT_ID_MAPPING_FLAGS_SINGLE mapping for the particular
> device ID that the AHCI uses.

Okay.

Thanks,
Thomas.

<snip>