Re: [edk2-devel] [edk2-platforms][PATCH v4 4/9] Platform/ARM/Sgi: Add support for remote numa memory nodes

["Ard Biesheuvel" <ard.biesheuvel@arm.com>]

On 4/14/20 2:52 PM, Aditya Angadi via groups.io wrote:
> From: Vijayenthiran Subramaniam <vijayenthiran.subramaniam@arm.com>
> 
> Add new PCDs that define the base address and size of remote NUMA
> memory nodes on multi-chip platforms. Use these PCDs to setup
> system memory resource descriptor HOBs.
> 
> Signed-off-by: Aditya Angadi <aditya.angadi@arm.com>
> ---
>   Platform/ARM/SgiPkg/Library/PlatformLib/PlatformLib.inf  | 18 ++++
>   Platform/ARM/SgiPkg/Library/PlatformLib/PlatformLibMem.c | 87 +++++++++++++++++++-
>   Platform/ARM/SgiPkg/SgiPlatform.dec                      | 19 +++++
>   3 files changed, 123 insertions(+), 1 deletion(-)
> 
> diff --git a/Platform/ARM/SgiPkg/Library/PlatformLib/PlatformLib.inf b/Platform/ARM/SgiPkg/Library/PlatformLib/PlatformLib.inf
> index a918afef5fba..c3125d7e4e0f 100644
> --- a/Platform/ARM/SgiPkg/Library/PlatformLib/PlatformLib.inf
> +++ b/Platform/ARM/SgiPkg/Library/PlatformLib/PlatformLib.inf
> @@ -46,6 +46,24 @@ [FixedPcd]
>   
>     gArmTokenSpaceGuid.PcdSystemMemoryBase
>     gArmTokenSpaceGuid.PcdSystemMemorySize
> +
> +  gArmSgiTokenSpaceGuid.PcdChipCount
> +
> +  gArmSgiTokenSpaceGuid.PcdDramBlock1BaseRemote1
> +  gArmSgiTokenSpaceGuid.PcdDramBlock1SizeRemote1
> +  gArmSgiTokenSpaceGuid.PcdDramBlock2BaseRemote1
> +  gArmSgiTokenSpaceGuid.PcdDramBlock2SizeRemote1
> +
> +  gArmSgiTokenSpaceGuid.PcdDramBlock1BaseRemote2
> +  gArmSgiTokenSpaceGuid.PcdDramBlock1SizeRemote2
> +  gArmSgiTokenSpaceGuid.PcdDramBlock2BaseRemote2
> +  gArmSgiTokenSpaceGuid.PcdDramBlock2SizeRemote2
> +
> +  gArmSgiTokenSpaceGuid.PcdDramBlock1BaseRemote3
> +  gArmSgiTokenSpaceGuid.PcdDramBlock1SizeRemote3
> +  gArmSgiTokenSpaceGuid.PcdDramBlock2BaseRemote3
> +  gArmSgiTokenSpaceGuid.PcdDramBlock2SizeRemote3
> +
>     gArmTokenSpaceGuid.PcdGicDistributorBase
>     gArmTokenSpaceGuid.PcdGicRedistributorsBase
>     gArmTokenSpaceGuid.PcdFvBaseAddress
> diff --git a/Platform/ARM/SgiPkg/Library/PlatformLib/PlatformLibMem.c b/Platform/ARM/SgiPkg/Library/PlatformLib/PlatformLibMem.c
> index 8d0ad4ec9c84..d8d9a406cf91 100644
> --- a/Platform/ARM/SgiPkg/Library/PlatformLib/PlatformLibMem.c
> +++ b/Platform/ARM/SgiPkg/Library/PlatformLib/PlatformLibMem.c
> @@ -16,7 +16,8 @@
>   #include <SgiPlatform.h>
>   
>   // Total number of descriptors, including the final "end-of-table" descriptor.
> -#define MAX_VIRTUAL_MEMORY_MAP_DESCRIPTORS  13
> +#define MAX_VIRTUAL_MEMORY_MAP_DESCRIPTORS                 \
> +          (11 + (FixedPcdGet32 (PcdChipCount) * 2))
>   
>   /**
>     Returns the Virtual Memory Map of the platform.
> @@ -52,6 +53,48 @@ ArmPlatformGetVirtualMemoryMap (
>       FixedPcdGet64 (PcdDramBlock2Base),
>       FixedPcdGet64 (PcdDramBlock2Size));
>   
> +#if (FixedPcdGet32 (PcdChipCount) > 1)
> +  BuildResourceDescriptorHob (
> +     EFI_RESOURCE_SYSTEM_MEMORY,
> +     ResourceAttributes,
> +     FixedPcdGet64 (PcdDramBlock1BaseRemote1),
> +     FixedPcdGet64 (PcdDramBlock1SizeRemote1));
> +
> +   BuildResourceDescriptorHob (
> +     EFI_RESOURCE_SYSTEM_MEMORY,
> +     ResourceAttributes,
> +     FixedPcdGet64 (PcdDramBlock2BaseRemote1),
> +     FixedPcdGet64 (PcdDramBlock2SizeRemote1));
> +
> +#if (FixedPcdGet32 (PcdChipCount) > 2)
> +  BuildResourceDescriptorHob (
> +     EFI_RESOURCE_SYSTEM_MEMORY,
> +     ResourceAttributes,
> +     FixedPcdGet64 (PcdDramBlock1BaseRemote2),
> +     FixedPcdGet64 (PcdDramBlock1SizeRemote2));
> +
> +   BuildResourceDescriptorHob (
> +     EFI_RESOURCE_SYSTEM_MEMORY,
> +     ResourceAttributes,
> +     FixedPcdGet64 (PcdDramBlock2BaseRemote2),
> +     FixedPcdGet64 (PcdDramBlock2SizeRemote2));
> +
> +#if (FixedPcdGet32 (PcdChipCount) > 3)
> +  BuildResourceDescriptorHob (
> +     EFI_RESOURCE_SYSTEM_MEMORY,
> +     ResourceAttributes,
> +     FixedPcdGet64 (PcdDramBlock1BaseRemote3),
> +     FixedPcdGet64 (PcdDramBlock1SizeRemote3));
> +
> +   BuildResourceDescriptorHob (
> +     EFI_RESOURCE_SYSTEM_MEMORY,
> +     ResourceAttributes,
> +     FixedPcdGet64 (PcdDramBlock2BaseRemote3),
> +     FixedPcdGet64 (PcdDramBlock2SizeRemote3));
> +#endif
> +#endif
> +#endif
> +
>     ASSERT (VirtualMemoryMap != NULL);
>     Index = 0;
>   
> @@ -122,6 +165,48 @@ ArmPlatformGetVirtualMemoryMap (
>     VirtualMemoryTable[Index].Length          = PcdGet64 (PcdDramBlock2Size);
>     VirtualMemoryTable[Index].Attributes      = ARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK;
>   
> +#if (FixedPcdGet32 (PcdChipCount) > 1)
> +  // Chip 1 DDR Block 1 - (2GB)
> +  VirtualMemoryTable[++Index].PhysicalBase  = PcdGet64 (PcdDramBlock1BaseRemote1);
> +  VirtualMemoryTable[Index].VirtualBase     = PcdGet64 (PcdDramBlock1BaseRemote1);
> +  VirtualMemoryTable[Index].Length          = PcdGet64 (PcdDramBlock1BaseRemote1);
> +  VirtualMemoryTable[Index].Attributes      = ARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK;
> +
> +  // Chip 1 DDR Block 2 - (6GB)
> +  VirtualMemoryTable[++Index].PhysicalBase  = PcdGet64 (PcdDramBlock2BaseRemote1);
> +  VirtualMemoryTable[Index].VirtualBase     = PcdGet64 (PcdDramBlock2BaseRemote1);
> +  VirtualMemoryTable[Index].Length          = PcdGet64 (PcdDramBlock2BaseRemote1);
> +  VirtualMemoryTable[Index].Attributes      = ARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK;
> +
> +#if (FixedPcdGet32 (PcdChipCount) > 2)
> +  // Chip 2 DDR Block 1 - (2GB)
> +  VirtualMemoryTable[++Index].PhysicalBase  = PcdGet64 (PcdDramBlock1BaseRemote2);
> +  VirtualMemoryTable[Index].VirtualBase     = PcdGet64 (PcdDramBlock1BaseRemote2);
> +  VirtualMemoryTable[Index].Length          = PcdGet64 (PcdDramBlock1BaseRemote2);
> +  VirtualMemoryTable[Index].Attributes      = ARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK;
> +
> +  // Chip 2 DDR Block 2 - (6GB)
> +  VirtualMemoryTable[++Index].PhysicalBase  = PcdGet64 (PcdDramBlock2BaseRemote2);
> +  VirtualMemoryTable[Index].VirtualBase     = PcdGet64 (PcdDramBlock2BaseRemote2);
> +  VirtualMemoryTable[Index].Length          = PcdGet64 (PcdDramBlock2BaseRemote2);
> +  VirtualMemoryTable[Index].Attributes      = ARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK;
> +
> +#if (FixedPcdGet32 (PcdChipCount) > 3)
> +  // Chip 3 DDR Block 1 - (2GB)
> +  VirtualMemoryTable[++Index].PhysicalBase  = PcdGet64 (PcdDramBlock1BaseRemote3);
> +  VirtualMemoryTable[Index].VirtualBase     = PcdGet64 (PcdDramBlock1BaseRemote3);
> +  VirtualMemoryTable[Index].Length          = PcdGet64 (PcdDramBlock1BaseRemote3);
> +  VirtualMemoryTable[Index].Attributes      = ARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK;
> +
> +  // Chip 3 DDR Block 2 - (6GB)
> +  VirtualMemoryTable[++Index].PhysicalBase  = PcdGet64 (PcdDramBlock2BaseRemote3);
> +  VirtualMemoryTable[Index].VirtualBase     = PcdGet64 (PcdDramBlock2BaseRemote3);
> +  VirtualMemoryTable[Index].Length          = PcdGet64 (PcdDramBlock2BaseRemote3);
> +  VirtualMemoryTable[Index].Attributes      = ARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK;
> +#endif
> +#endif
> +#endif
> +
>     // PCI Configuration Space
>     VirtualMemoryTable[++Index].PhysicalBase  = PcdGet64 (PcdPciExpressBaseAddress);
>     VirtualMemoryTable[Index].VirtualBase     = PcdGet64 (PcdPciExpressBaseAddress);
> diff --git a/Platform/ARM/SgiPkg/SgiPlatform.dec b/Platform/ARM/SgiPkg/SgiPlatform.dec
> index 97c1e40349ea..28d738f982dd 100644
> --- a/Platform/ARM/SgiPkg/SgiPlatform.dec
> +++ b/Platform/ARM/SgiPkg/SgiPlatform.dec
> @@ -46,6 +46,25 @@ [PcdsFixedAtBuild]
>     gArmSgiTokenSpaceGuid.PcdVirtioNetSize|0x00000000|UINT32|0x00000008
>     gArmSgiTokenSpaceGuid.PcdVirtioNetInterrupt|0x00000000|UINT32|0x00000009
>   
> +  # Chip count on the platform
> +  gArmSgiTokenSpaceGuid.PcdChipCount|1|UINT32|0x0000000C
> +

What does 'chip count' mean? Is it the number of sockets in use?

> +  # Remote NUMA memory node base and size
> +  gArmSgiTokenSpaceGuid.PcdDramBlock1BaseRemote1|0|UINT64|0x00000011
> +  gArmSgiTokenSpaceGuid.PcdDramBlock1SizeRemote1|0|UINT64|0x00000012
> +  gArmSgiTokenSpaceGuid.PcdDramBlock2BaseRemote1|0|UINT64|0x00000013
> +  gArmSgiTokenSpaceGuid.PcdDramBlock2SizeRemote1|0|UINT64|0x00000014
> +
> +  gArmSgiTokenSpaceGuid.PcdDramBlock1BaseRemote2|0|UINT64|0x00000015
> +  gArmSgiTokenSpaceGuid.PcdDramBlock1SizeRemote2|0|UINT64|0x00000016
> +  gArmSgiTokenSpaceGuid.PcdDramBlock2BaseRemote2|0|UINT64|0x00000017
> +  gArmSgiTokenSpaceGuid.PcdDramBlock2SizeRemote2|0|UINT64|0x00000018
> +
> +  gArmSgiTokenSpaceGuid.PcdDramBlock1BaseRemote3|0|UINT64|0x00000019
> +  gArmSgiTokenSpaceGuid.PcdDramBlock1SizeRemote3|0|UINT64|0x0000001A
> +  gArmSgiTokenSpaceGuid.PcdDramBlock2BaseRemote3|0|UINT64|0x0000001B
> +  gArmSgiTokenSpaceGuid.PcdDramBlock2SizeRemote3|0|UINT64|0x0000001C
> +

Can we find a better way of representing this? Using PCDs this way does 
not scale at all. Also, it is entirely unclear what 'base' and 'remote' 
mean in this context.




>     # GIC
>     gArmSgiTokenSpaceGuid.PcdGicSize|0|UINT64|0x0000000A
>   
>