From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by mx.groups.io with SMTP id smtpd.web11.6219.1588076943619525707 for ; Tue, 28 Apr 2020 05:29:04 -0700 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: arm.com, ip: 217.140.110.172, mailfrom: ard.biesheuvel@arm.com) Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 0FFD931B; Tue, 28 Apr 2020 05:29:02 -0700 (PDT) Received: from [192.168.1.81] (unknown [172.31.20.19]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 31CA13F73D; Tue, 28 Apr 2020 05:29:01 -0700 (PDT) Subject: Re: [edk2-devel] [edk2-platforms][PATCH v4 4/9] Platform/ARM/Sgi: Add support for remote numa memory nodes To: devel@edk2.groups.io, aditya.angadi@arm.com Cc: thomas.abraham@arm.com, leif@nuviainc.com, Vijayenthiran Subramaniam References: <20200414125208.2878-1-aditya.angadi@arm.com> <20200414125208.2878-5-aditya.angadi@arm.com> From: "Ard Biesheuvel" Message-ID: Date: Tue, 28 Apr 2020 14:28:59 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.7.0 MIME-Version: 1.0 In-Reply-To: <20200414125208.2878-5-aditya.angadi@arm.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit On 4/14/20 2:52 PM, Aditya Angadi via groups.io wrote: > From: Vijayenthiran Subramaniam > > 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 > --- > 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 > > // 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 > >