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

["Aditya Angadi" <aditya.angadi@arm.com>]

Hi Ard,

> -----Original Message-----
> From: Ard Biesheuvel <ard.biesheuvel@arm.com>
> Sent: 28 April 2020 17:59
> To: devel@edk2.groups.io; Aditya Angadi <Aditya.Angadi@arm.com>
> Cc: Thomas Abraham <thomas.abraham@arm.com>; leif@nuviainc.com;
> Vijayenthiran Subramaniam <Vijayenthiran.Subramaniam@arm.com>
> Subject: Re: [edk2-devel] [edk2-platforms][PATCH v4 4/9] Platform/ARM/Sgi:
> Add support for remote numa memory nodes
>
> 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|0x0000
> 00
> > 09
> >
> > +  # Chip count on the platform
> > +  gArmSgiTokenSpaceGuid.PcdChipCount|1|UINT32|0x0000000C
> > +
>
> What does 'chip count' mean? Is it the number of sockets in use?

On RD-Daniel platform 'chip count' means the number of chips on the same die packaged together and connected over a coherent link. These are not separate sockets.

>
> > +  # Remote NUMA memory node base and size
> > +
> gArmSgiTokenSpaceGuid.PcdDramBlock1BaseRemote1|0|UINT64|0x000000
> 11
> > +
> gArmSgiTokenSpaceGuid.PcdDramBlock1SizeRemote1|0|UINT64|0x0000001
> 2
> > +
> gArmSgiTokenSpaceGuid.PcdDramBlock2BaseRemote1|0|UINT64|0x000000
> 13
> > +
> gArmSgiTokenSpaceGuid.PcdDramBlock2SizeRemote1|0|UINT64|0x0000001
> 4
> > +
> > +
> gArmSgiTokenSpaceGuid.PcdDramBlock1BaseRemote2|0|UINT64|0x000000
> 15
> > +
> gArmSgiTokenSpaceGuid.PcdDramBlock1SizeRemote2|0|UINT64|0x0000001
> 6
> > +
> gArmSgiTokenSpaceGuid.PcdDramBlock2BaseRemote2|0|UINT64|0x000000
> 17
> > +
> gArmSgiTokenSpaceGuid.PcdDramBlock2SizeRemote2|0|UINT64|0x0000001
> 8
> > +
> > +
> gArmSgiTokenSpaceGuid.PcdDramBlock1BaseRemote3|0|UINT64|0x000000
> 19
> > +
> gArmSgiTokenSpaceGuid.PcdDramBlock1SizeRemote3|0|UINT64|0x0000001
> A
> > +
> gArmSgiTokenSpaceGuid.PcdDramBlock2BaseRemote3|0|UINT64|0x000000
> 1B
> > +
> gArmSgiTokenSpaceGuid.PcdDramBlock2SizeRemote3|0|UINT64|0x0000001
> C
> > +
>
> 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.

Ok. These PCDs will be removed and replaced with dynamic assignments of the base and size of the remote memory.

Thanks
Aditya

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

IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.