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

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

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?

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.

> +      __builtin_offsetof (ARM_EFI_ACPI_6_0_IO_REMAPPING_TABLE, ItsNode),  // OutputReference
> +      0,  // Flags
> +    }
> +  }
> +};
> +
> +VOID* CONST ReferenceAcpiTable = &Iort;
> diff --git a/Platform/ARM/SgiPkg/AcpiTables/Sgi575/Mcfg.aslc b/Platform/ARM/SgiPkg/AcpiTables/Sgi575/Mcfg.aslc
> new file mode 100644
> index 0000000..22a1018
> --- /dev/null
> +++ b/Platform/ARM/SgiPkg/AcpiTables/Sgi575/Mcfg.aslc
> @@ -0,0 +1,59 @@
> +/** @file
> +*  Memory mapped configuration space base address description table (MCFG)
> +*
> +*  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/Acpi61.h>
> +#include <IndustryStandard/MemoryMappedConfigurationSpaceAccessTable.h>
> +#include <Library/PcdLib.h>
> +#include "SgiAcpiHeader.h"
> +#include "SgiPlatform.h"
> +
> +#include <Library/AcpiLib.h>
> +#include <Library/ArmLib.h>
> +#include <IndustryStandard/Acpi.h>
> +
> +#pragma pack(1)
> +typedef struct
> +{
> +  EFI_ACPI_MEMORY_MAPPED_CONFIGURATION_BASE_ADDRESS_TABLE_HEADER Header;
> +  EFI_ACPI_MEMORY_MAPPED_ENHANCED_CONFIGURATION_SPACE_BASE_ADDRESS_ALLOCATION_STRUCTURE Structure[1];
> +} EFI_ACPI_PCI_MEMORY_MAPPED_CONFIGURATION_SPACE_ACCESS_DESCRIPTION_TABLE;
> +#pragma pack()
> +
> +EFI_ACPI_PCI_MEMORY_MAPPED_CONFIGURATION_SPACE_ACCESS_DESCRIPTION_TABLE Mcfg = {
> +  {
> +    ARM_ACPI_HEADER (
> +      EFI_ACPI_6_0_PCI_EXPRESS_MEMORY_MAPPED_CONFIGURATION_SPACE_BASE_ADDRESS_DESCRIPTION_TABLE_SIGNATURE,
> +      EFI_ACPI_PCI_MEMORY_MAPPED_CONFIGURATION_SPACE_ACCESS_DESCRIPTION_TABLE,
> +      EFI_ACPI_MEMORY_MAPPED_CONFIGURATION_SPACE_ACCESS_TABLE_REVISION
> +    ),
> +    EFI_ACPI_RESERVED_QWORD,  //Reserved
> +  },
> +  {
> +    // PCIe ECAM
> +    {
> +      FixedPcdGet32 (PcdPciConfigurationSpaceBaseAddress),  // Base Address
> +      0x0,                                                  // Segment Group Number
> +      0x0,                                                  // Start Bus Number
> +      0xff,                                                 // End Bus Number

Please use the BusMin and BusMax PCDs here.

> +      0x00000000,                                           // Reserved
> +    }
> +  }
> +};
> +
> +//
> +// Reference the table being generated to prevent the optimizer from removing the
> +// data structure from the executable
> +//
> +VOID* CONST ReferenceAcpiTable = &Mcfg;
> diff --git a/Platform/ARM/SgiPkg/AcpiTables/Sgi575/Ssdt.asl b/Platform/ARM/SgiPkg/AcpiTables/Sgi575/Ssdt.asl
> new file mode 100644
> index 0000000..bd418c7
> --- /dev/null
> +++ b/Platform/ARM/SgiPkg/AcpiTables/Sgi575/Ssdt.asl
> @@ -0,0 +1,91 @@
> +/** @file
> +*  Secondary System Description Table (SSDT)
> +*
> +*  Copyright (c) 2018, ARM Limited. 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 "SgiAcpiHeader.h"
> +
> +DefinitionBlock("SsdtPci.aml", "SSDT", 1, "ARMLTD", "ARM-SGI575",
> +                EFI_ACPI_ARM_OEM_REVISION)
> +{
> +  Scope (_SB) {
> +    // PCI Root Complex
> +    Device(PCI0) {
> +      Name (_HID, EISAID("PNP0A08")) // PCI Express Root Bridge
> +      Name (_CID, EISAID("PNP0A03")) // Compatible PCI Root Bridge
> +      Name (_SEG, Zero)              // PCI Segment Group number
> +      Name (_BBN, Zero)              // PCI Base Bus Number
> +      Name (_CCA, 1)                 // Cache Coherency Attribute
> +

Does this RC not support legacy INTx interrupts?

> +      // Root complex resources
> +      Method (_CRS, 0, Serialized) {
> +        Name (RBUF, ResourceTemplate () {
> +          WordBusNumber (      // Bus numbers assigned to this root
> +            ResourceProducer,
> +            MinFixed,
> +            MaxFixed,
> +            PosDecode,
> +            0,                 // AddressGranularity
> +            0,                 // AddressMinimum - Minimum Bus Number
> +            255,               // AddressMaximum - Maximum Bus Number
> +            0,                 // AddressTranslation - Set to 0
> +            256                // RangeLength - Number of Busses
> +          )
> +
> +          DWordMemory (        // 32-bit BAR Windows
> +            ResourceProducer,
> +            PosDecode,
> +            MinFixed,
> +            MaxFixed,
> +            Cacheable,
> +            ReadWrite,
> +            0x00000000,        // Granularity
> +            0x70000000,        // Min Base Address
> +            0x777FFFFF,        // Max Base Address
> +            0x00000000,        // Translate
> +            0x07800000         // Length
> +          )
> +
> +          QWordMemory (        // 64-bit BAR Windows
> +            ResourceProducer,
> +            PosDecode,
> +            MinFixed,
> +            MaxFixed,
> +            Cacheable,
> +            ReadWrite,
> +            0x00000000,        // Granularity
> +            0x500000000,       // Min Base Address
> +            0x7FFFFFFFF,       // Max Base Address
> +            0x00000000,        // Translate
> +            0x300000000        // Length
> +          )
> +
> +          DWordIo (             // IO window
> +            ResourceProducer,
> +            MinFixed,
> +            MaxFixed,
> +            PosDecode,
> +            EntireRange,
> +            0x00000000,         // Granularity
> +            0x00000000,         // Min Base Address
> +            0x007FFFFF,         // Max Base Address
> +            0x77800000,         // Translate
> +            0x00800000          // Length

You need to set the TypeTranslation attribute here.

> +          )
> +        }) // Name(RBUF)
> +
> +        Return (RBUF)
> +      } // Method (_CRS)
> +    }
> +  }
> +}
> --
> 2.7.4
>