Re: [edk2-devel] [edk2-platforms][PATCH V1 1/8] Platform/Sgi: Helper macros for PPTT Table

["Pranav Madhu" <pranav.madhu@arm.com>]

Hi Pierre,

Thanks for reviewing this patch. Please find my response inline.

> -----Original Message-----
> From: Pierre Gondois <pierre.gondois@arm.com>
> Sent: Tuesday, April 13, 2021 2:48 PM
> To: devel@edk2.groups.io; Pranav Madhu <Pranav.Madhu@arm.com>
> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>; Leif Lindholm
> <leif@nuviainc.com>; Sami Mujawar <Sami.Mujawar@arm.com>
> Subject: Re: [edk2-devel] [edk2-platforms][PATCH V1 1/8] Platform/Sgi:
> Helper macros for PPTT Table
> 
> Hi Pranav,
> 
> > diff --git a/Platform/ARM/SgiPkg/Include/SgiAcpiHeader.h
> > b/Platform/ARM/SgiPkg/Include/SgiAcpiHeader.h
> > index 8d715de173c9..7ceb090a78e9 100644
> > --- a/Platform/ARM/SgiPkg/Include/SgiAcpiHeader.h
> > +++ b/Platform/ARM/SgiPkg/Include/SgiAcpiHeader.h
> > @@ -1,6 +1,6 @@
> > �/** @file
> > �*
> > -*� Copyright (c) 2018-2020, ARM Limited. All rights reserved.
> > +*� Copyright (c) 2018-2021, ARM Limited. All rights reserved.
> > �*
> > �*� SPDX-License-Identifier: BSD-2-Clause-Patent
> > �*
> > @@ -20,6 +20,132 @@
> > �#define EFI_ACPI_ARM_CREATOR_ID SIGNATURE_32('A','R','M',' ')
> > �#define EFI_ACPI_ARM_CREATOR_REVISION 0x00000099
> >
> > +#define CORE_COUNT����� FixedPcdGet32 (PcdCoreCount)
> > +#define CLUSTER_COUNT�� FixedPcdGet32 (PcdClusterCount)
> > +
> > +#pragma pack(1)
> > +// PPTT processor core structure
> > +typedef struct {
> > +� EFI_ACPI_6_3_PPTT_STRUCTURE_PROCESSOR� Core; �
> >
> +UINT32��������������������ï¿
> ½
> > +����������� Offset[2];
> 
> I think there should be 3 entries (DCache, ICache, L2Cache). Updating this will
> require updating the other PPTT tables written.

As per ACPI spec 6.4, chapter '5.2.29.2 Cache Type Structure - Type 1', " Only
the head of the list needs to be listed as a resource by a processor node (and
counted toward Number of Private Resources), as the cache node itself
contains a link to the next level of cache."
Here L2 cache is represented as next level of L1, so no need to count it.

> 
> Would it be also possible to rename the field 'PrivateResources' as in the
> spec ?

Yes, but in actual, it is not the private resource count.

> Another question: what does 'RD_' stands for ?

RD Stands for Reference Design, it is the convention we follow.

> 
> > +� EFI_ACPI_6_3_PPTT_STRUCTURE_CACHE����� DCache; �
> > +EFI_ACPI_6_3_PPTT_STRUCTURE_CACHE����� ICache; �
> > +EFI_ACPI_6_3_PPTT_STRUCTURE_CACHE����� L2Cache; }
> > +RD_PPTT_CORE;
> > +
> > +// PPTT processor cluster structure
> > +typedef struct {
> > +� EFI_ACPI_6_3_PPTT_STRUCTURE_PROCESSOR� Cluster; �
> >
> +UINT32��������������������ï¿
> ½
> > +����������� Offset; �
> > +EFI_ACPI_6_3_PPTT_STRUCTURE_CACHE����� L3Cache; �
> > +RD_PPTT_CORE Core[CORE_COUNT]; } RD_PPTT_CLUSTER;
> > +
> > +// PPTT processor cluster structure without cache typedef struct {
> > +� EFI_ACPI_6_3_PPTT_STRUCTURE_PROCESSOR� Cluster; �
> >
> +UINT32��������������������ï¿
> ½
> > +����������� Offset;
> I think there is no need for an offset here. Updating this will require updating
> the other PPTT tables written.

Right. Will update.

> > +� RD_PPTT_CORE Core[CORE_COUNT];
> > +} RD_PPTT_MINIMAL_CLUSTER;
> > +
> > +// PPTT processor package structure
> > +typedef struct {
> > +� EFI_ACPI_6_3_PPTT_STRUCTURE_PROCESSOR� Package; �
> >
> +UINT32��������������������ï¿
> ½
> > +����������� Offset; �
> > +EFI_ACPI_6_3_PPTT_STRUCTURE_CACHE����� Slc; �
> > +RD_PPTT_MINIMAL_CLUSTER Cluster[CLUSTER_COUNT]; }
> > +RD_PPTT_SLC_PACKAGE; #pragma pack ()
> > +
> > +//
> > +// PPTT processor structure flags for different SoC components as
> > defined in
> > +// ACPI 6.3 specification
> > +//
> > +
> [...]
> >
> > +// EFI_ACPI_6_3_PPTT_STRUCTURE_PROCESSOR
> > +#define EFI_ACPI_6_3_PPTT_STRUCTURE_PROCESSOR_INIT(Length, Flag,
> > Parent,������ \
> > +� ACPIProcessorID, NumberOfPrivateResource) \
> 
> I think it should be possible to remove the 'Length' parameter and compute it
> as:
> sizeof (EFI_ACPI_6_3_PPTT_STRUCTURE_PROCESSOR) +
> NumberOfPrivateResource * sizeof
> (EFI_ACPI_6_3_PPTT_STRUCTURE_CACHE) + NumberOfPrivateResource *
> sizeof (UINT32)
> 

As per 6.4 specification, table 5.138, the Length is "Length of the local processor
structure in bytes" It is just the length of local processor, not the entire structure.

> > + { \
> > +���
> >
> +EFI_ACPI_6_3_PPTT_TYPE_PROCESSOR,����������ï¿
> ½ï¿½
> > +���� /* Type 0
> > */������������ \
> > +���
> >
> +Length,��������������������ï¿
> > +½ï¿½ï¿½ï¿½ï¿½ï¿½ï¿½ï¿½ï¿½ï¿½ï¿½ï¿½ï¿½ï¿½ï¿½ï¿½ï¿½ï¿½ï¿½ï¿½ï¿½ï¿½ /*
> > +Length
> > */������������ \
> > + { \
> > + EFI_ACPI_RESERVED_BYTE, \
> > + EFI_ACPI_RESERVED_BYTE, \
> > + }, \
> > +���
> > +Flag,���������������������ï
> >
> +¿½ï¿½ï¿½ï¿½ï¿½ï¿½ï¿½ï¿½ï¿½ï¿½ï¿½ï¿½ï¿½ï¿½ï¿½ï¿½ï¿½ï¿½ï¿½ï¿½ï¿½ï¿½ï¿½
> > +/* Processor
> > flags */��� \
> > +���
> >
> +Parent,��������������������ï¿
> > +½ï¿½ï¿½ï¿½ï¿½ï¿½ï¿½ï¿½ï¿½ï¿½ï¿½ï¿½ï¿½ï¿½ï¿½ï¿½ï¿½ï¿½ï¿½ï¿½ï¿½ï¿½ /*
> > +Ref to
> > parent node */ \
> > +���
> >
> +ACPIProcessorID,�����������������ï¿
> > +½ï¿½ï¿½ï¿½ï¿½ï¿½ï¿½ï¿½ï¿½ï¿½ï¿½ï¿½ï¿½ï¿½ï¿½ï¿½ /* UID, as per
> > MADT */�� \
> > +���
> >
> +NumberOfPrivateResource��������������ï¿
> ½ï
> > +¿½ï¿½ï¿½ï¿½ï¿½ï¿½ï¿½ï¿½ï¿½ï¿½ï¿½ /* Resource
> > count */���� \
> > +� }
> > +
> > +// EFI_ACPI_6_3_PPTT_STRUCTURE_CACHE
> > +#define EFI_ACPI_6_3_PPTT_STRUCTURE_CACHE_INIT(Flag,
> NextLevelCache,
> > Size,���� \
> > +� NoOfSets, Associativity, Attributes,
> > LineSize)��������������������ï
> > ¿½ï¿½ï¿½ï¿½ï¿½ï¿½ï¿½ï¿½ï¿½ï¿½ \
> > + { \
> > +���
> >
> +EFI_ACPI_6_3_PPTT_TYPE_CACHE,������������ï
> ¿½ï
> > +¿½ï¿½ï¿½ï¿½ï¿½ï¿½ï¿½ /* Type 1
> > */������������ \
> > +��� sizeof
> > +(EFI_ACPI_6_3_PPTT_STRUCTURE_CACHE),������ /* Length
> > */������������ \
> > + { \
> > + EFI_ACPI_RESERVED_BYTE, \
> > + EFI_ACPI_RESERVED_BYTE, \
> > + }, \
> > +���
> > +Flag,���������������������ï
> >
> +¿½ï¿½ï¿½ï¿½ï¿½ï¿½ï¿½ï¿½ï¿½ï¿½ï¿½ï¿½ï¿½ï¿½ï¿½ï¿½ï¿½ï¿½ï¿½ï¿½ï¿½ï¿½ï¿½
> > +/* Cache flags
> > */������� \
> > +���
> >
> +NextLevelCache,�����������������ï¿
> ½
> > +���������������� /* Ref to next
> > level */� \
> > +���
> > +Size,���������������������ï
> >
> +¿½ï¿½ï¿½ï¿½ï¿½ï¿½ï¿½ï¿½ï¿½ï¿½ï¿½ï¿½ï¿½ï¿½ï¿½ï¿½ï¿½ï¿½ï¿½ï¿½ï¿½ï¿½ï¿½
> > +/* Size in
> > bytes */����� \
> > +���
> >
> +NoOfSets,�������������������ï¿
> ½
> > +�������������������� /* Num
> > +of sets
> > */������� \
> > +���
> > +Associativity,������������������ï
> > +¿½ï¿½ï¿½ï¿½ï¿½ï¿½ï¿½ï¿½ï¿½ï¿½ï¿½ï¿½ï¿½ï¿½ï¿½ï¿½ï¿½ /* Num of ways
> > */������� \
> > +���
> > +Attributes,�������������������ï
> > +¿½ï¿½ï¿½ï¿½ï¿½ï¿½ï¿½ï¿½ï¿½ï¿½ï¿½ï¿½ï¿½ï¿½ï¿½ï¿½ï¿½ï¿½ï¿½ /* Cache
> > attributes */�� \
> > +���
> >
> +LineSize��������������������ï
> > +¿½ï¿½ï¿½ï¿½ï¿½ï¿½ï¿½ï¿½ï¿½ï¿½ï¿½ï¿½ï¿½ï¿½ï¿½ï¿½ï¿½ï¿½ï¿½ï¿½ï¿½ /*
> > +Line size in
> > bytes */ \
> > +� }
> > +
> > �#endif /* __SGI_ACPI_HEADER__ */
> > --
> > 2.17.1
> 
> Regards,
> 
> Pierre
> 

Regards,
Pranav.