Re: [edk2-devel] [PATCH v2 14/17] Silicon/Ampere: Update ArmPlatformLib to work with changed ARM_CORE_INFO

["Nhi Pham" <nhi@os.amperecomputing.com>]

Hi Leif,

On 25/03/2022 23:30, Leif Lindholm wrote:
> Hi Nhi,
>
> On Sun, Dec 19, 2021 at 10:35:41 +0700, Nhi Pham via groups.io wrote:
>> Hi Rebecca,
>>
>> Leif is merging the rest of Altra port to the edk2-platforms which has SRAT
>> ACPI table consuming the CPU Core Info table. Therefore, we will need to fix
>> the SRAT too. I would defer the fix until the Altra port is fully merged.
>>
>> On 17/12/2021 05:07, Rebecca Cran wrote:
>>> The ARM_CORE_INFO struct has been updated so the MPIDR is now a single
>>> field instead of separate cluster/core fields. Update ArmPlatformLib.
>>>
>>> Signed-off-by: Rebecca Cran <rebecca@nuviainc.com>
>>> ---
>>>    Silicon/Ampere/AmpereAltraPkg/Library/ArmPlatformLib/ArmPlatformLib.c | 5 ++---
>>>    1 file changed, 2 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/Silicon/Ampere/AmpereAltraPkg/Library/ArmPlatformLib/ArmPlatformLib.c b/Silicon/Ampere/AmpereAltraPkg/Library/ArmPlatformLib/ArmPlatformLib.c
>>> index 5b4be0e55516..f2ec923d6f8d 100644
>>> --- a/Silicon/Ampere/AmpereAltraPkg/Library/ArmPlatformLib/ArmPlatformLib.c
>>> +++ b/Silicon/Ampere/AmpereAltraPkg/Library/ArmPlatformLib/ArmPlatformLib.c
>>> @@ -108,9 +108,8 @@ PrePeiCoreGetMpCoreInfo (
>>>        }
>>>        SocketId = SOCKET_ID (Index);
>>>        ClusterId = CLUSTER_ID (Index);
>>> -    mArmPlatformMpCoreInfoTable[mArmPlatformCoreCount].ClusterId = SocketId;
>>> -    mArmPlatformMpCoreInfoTable[mArmPlatformCoreCount].CoreId =
>>> -      (ClusterId << 8) | (Index % PLATFORM_CPU_NUM_CORES_PER_CPM);
>>> +    mArmPlatformMpCoreInfoTable[mArmPlatformCoreCount].Mpidr = GET_MPID (
>>> +      SocketId, (ClusterId << 8) | (Index % PLATFORM_CPU_NUM_CORES_PER_CPM));
>> For Ampere Altra, the correct MPIDR encoding is SocketId << 32 | ClusterId
>> << 16 | (Index % PLATFORM_CPU_NUM_CORES_PER_CPM) << 8
>>
>> It would be the same what
>> Platform/Ampere/JadePkg/Drivers/AcpiPlatformDxe/AcpiMadt.c (not available
>> yet - being merged in) is describing.
> This patch already got merged, so if you feel it is wrong, could you
> submit a fix please?
>
> The next patch for me to push from your set otherwise also requires
> some changes. My naïve attempt would look something like:
>
> diff --git a/Platform/Ampere/JadePkg/Drivers/AcpiPlatformDxe/AcpiSrat.c b/Platform/Ampere/JadePkg/Drivers/AcpiPlatformDxe/AcpiSrat.c
> index 906b771a250c..d5bc732b08bb 100644
> --- a/Platform/Ampere/JadePkg/Drivers/AcpiPlatformDxe/AcpiSrat.c
> +++ b/Platform/Ampere/JadePkg/Drivers/AcpiPlatformDxe/AcpiSrat.c
> @@ -7,6 +7,7 @@
>   **/
>   
>   #include <Guid/ArmMpCoreInfo.h>
> +#include <Library/ArmLib.h>
>   #include "AcpiPlatform.h"
>   
>   EFI_ACPI_6_3_SYSTEM_RESOURCE_AFFINITY_TABLE_HEADER SRATTableHeaderTemplate = {
> @@ -119,6 +120,7 @@ SratAddGiccAffinity (
>     UINTN               Count, NumNode, Idx;
>     UINT32              AcpiProcessorUid;
>     UINT8               Socket;
> +  UINT8               Core;
>     UINT8               Cpm;
>   
>     for (Idx = 0; Idx < gST->NumberOfTableEntries; Idx++) {
> @@ -137,14 +139,14 @@ SratAddGiccAffinity (
>     NumNode = 0;
>     while (Count != ArmProcessorTable->NumberOfEntries) {
>       for (Idx = 0; Idx < ArmProcessorTable->NumberOfEntries; Idx++ ) {
> -      Socket = ArmCoreInfoTable[Idx].ClusterId;
> -      Cpm = (ArmCoreInfoTable[Idx].CoreId >> PLATFORM_CPM_UID_BIT_OFFSET);
> +      Socket = GET_MPIDR_AFF1 (ArmCoreInfoTable[Idx].Mpidr);
> +      Core   = GET_MPIDR_AFF0 (ArmCoreInfoTable[Idx].Mpidr);
> +      Cpm = Core >> PLATFORM_CPM_UID_BIT_OFFSET;
>         if (CpuGetSubNumNode (Socket, Cpm) != NumNode) {
>           /* We add nodes based on ProximityDomain order */
>           continue;
>         }
> -      AcpiProcessorUid = (ArmCoreInfoTable[Idx].ClusterId << PLATFORM_SOCKET_UID_BIT_OFFSET) +
> -                         ArmCoreInfoTable[Idx].CoreId;
> +      AcpiProcessorUid = (Socket << PLATFORM_SOCKET_UID_BIT_OFFSET) + Core;
>         ZeroMem ((VOID *)&SratGiccAffinity[Count], sizeof (SratGiccAffinity[Count]));
>         SratGiccAffinity[Count].AcpiProcessorUid = AcpiProcessorUid;
>         SratGiccAffinity[Count].Flags = 1;
>
> Would you be happy for me to fold that into
> "AmpereAltraPkg, JadePkg: Add ACPI support", or would you be able to
> submit a v6 of that patch only?
>
> Best Regards,
>
> Leif

Thanks much for the patch. The MPIDR decoding matches with Rebecca's 
update for the ArmPlatformLib earlier and the ARM_CORE_INFO is just 
consumed in the AcpiSrat.c. So, that is good for now. Please help fold 
that into the ACPI patch when merging the rest of the Mt. Jade support 
patchset.

In the future, I will follow up with a patch that makes the 
representation of MPIDR in the ARM_CORE_INFO the same as in MADT table.

Thanks,

Nhi