Re: [RFC PATCH edk2-platforms 2/2] SbsaQemu: AcpiTables: Add CEDT Table

[Jonathan Cameron <Jonathan.Cameron@Huawei.com>]

On Fri, 30 Aug 2024 11:15:45 +0800
Yuquan Wang <wangyuquan1236@phytium.com.cn> wrote:

> Provide CXL Early Discovery Table that describes the static CXL
> Platform Components of sbsa-ref.
> 
> This adds a static CXL Host Bridge structure and a CXL Fixed Memory
> Window structure which are implemented as two independent space on
> sbsa-ref: [SBSA_CXL_HOST] & [SBSA_CXL_FIXED_WINDOW].
> 
> Signed-off-by: Yuquan Wang <wangyuquan1236@phytium.com.cn>
A few superficial comments.

I'd love to see a dump of iasl -d for this table in the commit message.
That's much easier to sanity check for spec compliance than reading
the code that creates it.

Jonathan

> ---
>  .../Qemu/SbsaQemu/AcpiTables/AcpiTables.inf   |  6 +-
>  Silicon/Qemu/SbsaQemu/AcpiTables/Cedt.aslc    | 70 +++++++++++++++++++
>  Silicon/Qemu/SbsaQemu/SbsaQemu.dec            |  7 ++
>  3 files changed, 82 insertions(+), 1 deletion(-)
>  create mode 100644 Silicon/Qemu/SbsaQemu/AcpiTables/Cedt.aslc
> 
> diff --git a/Silicon/Qemu/SbsaQemu/AcpiTables/AcpiTables.inf b/Silicon/Qemu/SbsaQemu/AcpiTables/AcpiTables.inf
> index b4d5aa807bd9..f39b06d708d5 100644
> --- a/Silicon/Qemu/SbsaQemu/AcpiTables/AcpiTables.inf
> +++ b/Silicon/Qemu/SbsaQemu/AcpiTables/AcpiTables.inf
> @@ -21,7 +21,7 @@
>    Fadt.aslc
>    Mcfg.aslc
>    Spcr.aslc
> -
> +  Cedt.aslc
Fix up to keep the white space. Also this seems to be alphabetical
order so probably should stick to that.

>  [Packages]
>    ArmPlatformPkg/ArmPlatformPkg.dec
>    ArmPkg/ArmPkg.dec
> @@ -78,6 +78,10 @@
>    gArmVirtSbsaQemuPlatformTokenSpaceGuid.PcdCxlBarSize
>    gArmVirtSbsaQemuPlatformTokenSpaceGuid.PcdCxlBarLimit
>  
> +  gArmVirtSbsaQemuPlatformTokenSpaceGuid.PcdChbcrBase
> +  gArmVirtSbsaQemuPlatformTokenSpaceGuid.PcdCfmwsBase
> +  gArmVirtSbsaQemuPlatformTokenSpaceGuid.PcdCfmwsSize
> +
>    gEfiMdeModulePkgTokenSpaceGuid.PcdSerialRegisterBase
>  
>    gArmVirtSbsaQemuPlatformTokenSpaceGuid.PcdPlatformAhciBase
> diff --git a/Silicon/Qemu/SbsaQemu/AcpiTables/Cedt.aslc b/Silicon/Qemu/SbsaQemu/AcpiTables/Cedt.aslc
> new file mode 100644
> index 000000000000..66c9dc8858bc
> --- /dev/null
> +++ b/Silicon/Qemu/SbsaQemu/AcpiTables/Cedt.aslc
> @@ -0,0 +1,70 @@
> +/** @file
> +*  CXL Early Discovery Table (CEDT)
> +*
> +*  Copyright (c) 2024, Phytium Technology Co Ltd. All rights reserved.
> +*
> +**/
> +
> +#include <IndustryStandard/CXLEarlyDiscoveryTable.h>
> +#include <IndustryStandard/Acpi64.h>
> +#include <IndustryStandard/SbsaQemuAcpi.h>
> +
> +#pragma pack(1)
> +
> +typedef struct
> +{
> +  EFI_ACPI_6_4_CXL_Early_Discovery_TABLE           Header;
> +  EFI_ACPI_6_4_CXL_Host_Bridge_Structure           Chbs;
> +  EFI_ACPI_6_4_CXL_Fixed_Memory_Window_Structure   Cfmws;
> +} SBSA_REF_CEDT;
> +
> +
> +SBSA_REF_CEDT Cedt =
> +{
> +  // EFI_ACPI_6_4_CXL_Early_Discovery_TABLE(Header)
> +  {
> +     SBSAQEMU_ACPI_HEADER  // EFI_ACPI_DESCRIPTION_HEADER
> +     (
> +       EFI_ACPI_6_4_CXL_EARLY_DISCOVERY_TABLE_SIGNATURE,
> +       SBSA_REF_CEDT,
> +       EFI_ACPI_CXL_Early_Discovery_TABLE_REVISION_01
> +     ),
> +  },
> +  // EFI_ACPI_6_4_CXL_Host_Bridge_Structure
> +  {
> +    // EFI_ACPI_6_4_CEDT_Structure
> +    {
> +        EFI_ACPI_CEDT_TYPE_CHBS,                                 // Type
> +        0,                                                       // Reserved
> +        sizeof (EFI_ACPI_6_4_CXL_Host_Bridge_Structure),         // Length
> +    },
> +    FixedPcdGet32 (PcdCxlBusMin),          // UID
> +    0x1,                                   // CXLVersion
> +    0,                                     // Reserved
> +    FixedPcdGet32 (PcdChbcrBase),          // CHBCR Base
> +    0X10000,                               // Length
> +  },
> +  // EFI_ACPI_6_4_CXL_Fixed_Memory_Window_Structure
> +  {
> +    // EFI_ACPI_6_4_CEDT_Structure
> +    {
> +        EFI_ACPI_CEDT_TYPE_CFMWS,                                // Type
> +        0,                                                       // Reserved
> +        sizeof (EFI_ACPI_6_4_CXL_Fixed_Memory_Window_Structure), // Length
> +    },
> +    0,                                     // Reserved
> +    FixedPcdGet32 (PcdCfmwsBase),          // BaseHPA
> +    FixedPcdGet32 (PcdCfmwsSize),          // WindowSize
> +    0,                                     // InterleaveMembers
> +    0,                                     // InterleaveArithmetic
> +    0,                                     // Reserved1
> +    0,                                     // Granularity
> +    0xF,                                   // Restrictions
> +    0,                                     // QtgId

You'll need to implement the QTG DSM or I think the kernel will still moan at you.


> +    FixedPcdGet32 (PcdCxlBusMin),          // FirstTarget
> +  }
> +};
> +
> +#pragma pack ()
> +
> +VOID* CONST ReferenceAcpiTable = &Cedt;
> diff --git a/Silicon/Qemu/SbsaQemu/SbsaQemu.dec b/Silicon/Qemu/SbsaQemu/SbsaQemu.dec
> index 7d8c7997160b..dff838315d06 100644
> --- a/Silicon/Qemu/SbsaQemu/SbsaQemu.dec
> +++ b/Silicon/Qemu/SbsaQemu/SbsaQemu.dec
> @@ -65,6 +65,13 @@ HardwareInfoLib|Include/Library/HardwareInfoLib.h
>    gArmVirtSbsaQemuPlatformTokenSpaceGuid.PcdCxlBusMin|254|UINT32|0x00000019
>    gArmVirtSbsaQemuPlatformTokenSpaceGuid.PcdCxlBusMax|255|UINT32|0x00000020
>  
> +  # PCDs complementing base address for CXL CHBCR (CXL Host Bridge Component Registers)
> +  gArmVirtSbsaQemuPlatformTokenSpaceGuid.PcdChbcrBase|0x60120000|UINT64|0x00000021
> +
> +  # CXL Fixed Memory Window

I'd add an index from the start just to make this easier to extend.
PcdCFwms0Base perhaps?

> +  gArmVirtSbsaQemuPlatformTokenSpaceGuid.PcdCfmwsBase|0xA0000000000|UINT64|0x00000022
> +  gArmVirtSbsaQemuPlatformTokenSpaceGuid.PcdCfmwsSize|0x10000000000|UINT64|0x00000023
> +
>  [PcdsDynamic.common]
>    gArmVirtSbsaQemuPlatformTokenSpaceGuid.PcdSystemManufacturer|L""|VOID*|0x00000110
>    gArmVirtSbsaQemuPlatformTokenSpaceGuid.PcdSystemSerialNumber|L""|VOID*|0x00000111