Re: [edk2-devel] [edk2-platforms] [PATCH v1 2/3] Platform/ARM/SgiPkg: Add HMAT ACPI table for RdN1EdgeX2

["Vijayenthiran Subramaniam" <vijayenthiran.subramaniam@arm.com>]

Hi Jonathan,

On Fri, Jan 29, 2021 at 9:50 AM Jonathan Cameron via groups.io
<jonathan.cameron=huawei.com@groups.io> wrote:
>
> On Thu, 28 Jan 2021 15:58:48 +0000
> Jonathan Cameron via groups.io <jonathan.cameron=huawei.com@groups.io> wrote:
>
> > On Thu, 28 Jan 2021 19:12:30 +0530
> > Vijayenthiran Subramaniam <vijayenthiran.subramaniam@arm.com> wrote:
> >
> > > Add HMAT table support for RD-N1-Edge Dual-chip platform.
> > >
> > > Signed-off-by: Vijayenthiran Subramaniam <vijayenthiran.subramaniam@arm.com>
> > > ---
> > >  Platform/ARM/SgiPkg/AcpiTables/RdN1EdgeX2AcpiTables.inf |   1 +
> > >  Platform/ARM/SgiPkg/AcpiTables/RdN1EdgeX2/Hmat.aslc     | 108 ++++++++++++++++++++
> > >  2 files changed, 109 insertions(+)
> > >
> > > diff --git a/Platform/ARM/SgiPkg/AcpiTables/RdN1EdgeX2AcpiTables.inf b/Platform/ARM/SgiPkg/AcpiTables/RdN1EdgeX2AcpiTables.inf
> > > index d44f02ab0c16..36d41281439d 100644
> > > --- a/Platform/ARM/SgiPkg/AcpiTables/RdN1EdgeX2AcpiTables.inf
> > > +++ b/Platform/ARM/SgiPkg/AcpiTables/RdN1EdgeX2AcpiTables.inf
> > > @@ -22,6 +22,7 @@ [Sources]
> > >    Iort.aslc
> > >    Mcfg.aslc
> > >    RdN1Edge/Dsdt.asl
> > > +  RdN1EdgeX2/Hmat.aslc
> > >    RdN1EdgeX2/Madt.aslc
> > >    RdN1EdgeX2/Srat.aslc
> > >    Spcr.aslc
> > > diff --git a/Platform/ARM/SgiPkg/AcpiTables/RdN1EdgeX2/Hmat.aslc b/Platform/ARM/SgiPkg/AcpiTables/RdN1EdgeX2/Hmat.aslc
> > > new file mode 100644
> > > index 000000000000..29d089aed053
> > > --- /dev/null
> > > +++ b/Platform/ARM/SgiPkg/AcpiTables/RdN1EdgeX2/Hmat.aslc
> > > @@ -0,0 +1,108 @@
> > > +/** @file
> > > +*  Heterogeneous Memory Attribute Table (HMAT)
> > > +*
> > > +*  Copyright (c) 2020, ARM Limited. All rights reserved.
> > > +*
> > > +*  SPDX-License-Identifier: BSD-2-Clause-Patent
> > > +*
> > > +**/
> > > +
> > > +#include "SgiAcpiHeader.h"
> > > +#include "SgiPlatform.h"
> > > +#include <IndustryStandard/Acpi.h>
> > > +#include <Library/AcpiLib.h>
> > > +#include <Library/ArmLib.h>
> > > +
> > > +#define CHIP_CNT                      FixedPcdGet32 (PcdChipCount)
> > > +#define INITATOR_PROXIMITY_DOMAIN_CNT 2
> > > +#define TARGET_PROXIMITY_DOMIAIN_CNT  2
> >
> > DOMAIN

Ack, will fix in v2.

> >
> > > +
> > > +//
> > > +// HMAT Table
> > > +//
> > > +#pragma pack (1)
> > > +
> > > +typedef struct {
> > > +  UINT32  InitatorProximityDomain[INITATOR_PROXIMITY_DOMAIN_CNT];
> > > +  UINT32  TargetProximityDomiain[TARGET_PROXIMITY_DOMIAIN_CNT];
> > > +  UINT16  MatrixEntry[INITATOR_PROXIMITY_DOMAIN_CNT * TARGET_PROXIMITY_DOMIAIN_CNT];
> > > +} InitiatorTargetProximityMatrix;
> > > +
> > > +typedef struct {
> > > +  EFI_ACPI_6_3_HETEROGENEOUS_MEMORY_ATTRIBUTE_TABLE_HEADER                Header;
> > > +  EFI_ACPI_6_3_HMAT_STRUCTURE_MEMORY_PROXIMITY_DOMAIN_ATTRIBUTES          Proximity[CHIP_CNT];
> > > +  EFI_ACPI_6_3_HMAT_STRUCTURE_SYSTEM_LOCALITY_LATENCY_AND_BANDWIDTH_INFO  LatencyInfo;
> > > +  InitiatorTargetProximityMatrix                                          Matrix;
> > > +  EFI_ACPI_6_3_HMAT_STRUCTURE_MEMORY_SIDE_CACHE_INFO                      MemSideCache0;
> > > +  EFI_ACPI_6_3_HMAT_STRUCTURE_MEMORY_SIDE_CACHE_INFO                      MemSideCache1;
> > > +} EFI_ACPI_HETEROGENEOUS_MEMORY_ATTRIBUTE_TABLE;
> > > +
> > > +#pragma pack ()
> > > +
> > > +#define HMAT_STRUCTURE_MEMORY_SIDE_CACHE_INFO_CACHE_ATTRIBUTES_INIT(           \
> > > +  TotalCacheLevels, CacheLevel, CacheAssociativity, WritePolicy, CacheLineSize \
> > > +    )                                                                          \
> > > +{                                                                              \
> > > +  TotalCacheLevels, CacheLevel, CacheAssociativity, WritePolicy, CacheLineSize \
> > > +}
> > > +
> > > +EFI_ACPI_HETEROGENEOUS_MEMORY_ATTRIBUTE_TABLE Hmat = {
> > > +  // Header
> > > +  {
> > > +    ARM_ACPI_HEADER (
> > > +      EFI_ACPI_6_3_HETEROGENEOUS_MEMORY_ATTRIBUTE_TABLE_SIGNATURE,
> > > +      EFI_ACPI_HETEROGENEOUS_MEMORY_ATTRIBUTE_TABLE,
> > > +      EFI_ACPI_6_3_HETEROGENEOUS_MEMORY_ATTRIBUTE_TABLE_REVISION
> > > +    ),
> > > +    {
> > > +      EFI_ACPI_RESERVED_BYTE,
> > > +      EFI_ACPI_RESERVED_BYTE,
> > > +      EFI_ACPI_RESERVED_BYTE,
> > > +      EFI_ACPI_RESERVED_BYTE
> > > +    },
> > > +  },
> > > +
> > > +  // Memory Proximity Domain
> > > +  {
> > > +    EFI_ACPI_6_3_HMAT_STRUCTURE_MEMORY_PROXIMITY_DOMAIN_ATTRIBUTES_INIT (
> > > +      1, 0x0, 0x0),
> > > +    EFI_ACPI_6_3_HMAT_STRUCTURE_MEMORY_PROXIMITY_DOMAIN_ATTRIBUTES_INIT (
> > > +      1, 0x1, 0x1),
> > > +   },
> > > +
> > > +  // Latency Info
> > > +  EFI_ACPI_6_3_HMAT_STRUCTURE_SYSTEM_LOCALITY_LATENCY_AND_BANDWIDTH_INFO_INIT (
> > > +    0, 0, INITATOR_PROXIMITY_DOMAIN_CNT, TARGET_PROXIMITY_DOMIAIN_CNT, 100),
> > > +  {
> > > +    {0, 1}, {0, 1},
> > > +    {
> > > +      10, 20,
> > > +      20, 10,
> > > +    }
> > > +  },
>
> Hi,
>
> Now either you have a very odd machine in which latencies are round numbers that happen
> to look like the values in SLIT or ... ?
>
> HMAT has very carefully defined real world units to overcome the fact that there
> was no standard scaling etc for SLIT.  It is not a good idea to put tables out
> there which don't do this right as someone may copy this for a real system
> and we end up with HMAT being roughly as useless for performance prediction as
> SLIT is.  That is not something I would want to see.
>
> Jonathan

This patch is for a platform that is modelled and not for an actual board. So
the latencies are hypothetical but ensure that the typical latency between two
chips is represented. These tables are not expected to be copied for another
platform without understanding what these values mean. Such a copy, without
understanding what is being copied, would be an error in judgement on the part
of the person doing it. The commit message of this patch (in v2) can be updated
to exercise caution on reusing these values.

[1]: https://developer.arm.com/products/system-design/fixed-virtual-platforms

Best Regards,
Vijayenthiran

>
> > > +
> > > +  // Memory Side Cache
> > > +  EFI_ACPI_6_3_HMAT_STRUCTURE_MEMORY_SIDE_CACHE_INFO_INIT (
> > > +    0x0, SIZE_8MB,
> > > +    HMAT_STRUCTURE_MEMORY_SIDE_CACHE_INFO_CACHE_ATTRIBUTES_INIT (
> > > +      1,
> > > +      1,
> > > +      2,
> > > +      2,
> > > +      64 // 64 bytes cache line length
> > > +    ),
> > > +    0),
> > > +
> > > +  EFI_ACPI_6_3_HMAT_STRUCTURE_MEMORY_SIDE_CACHE_INFO_INIT (
> > > +    0x1, SIZE_8MB,
> > > +    HMAT_STRUCTURE_MEMORY_SIDE_CACHE_INFO_CACHE_ATTRIBUTES_INIT (
> > > +      1,
> > > +      1,
> > > +      2,
> > > +      2,
> > > +      64 // 64 bytes cache line length
> > > +    ),
> > > +    0),
> > > +};
> > > +
> > > +VOID* CONST ReferenceAcpiTable = &Hmat;
> >
> >
> >
> >
> >
> >