[RFC][PATCH] OpenPlatformPkg: HiKeyMem: Initialize the base system memory HOB before trying to create reservations
In the current edk2 code, as of 40a3f38f67cee ("ArmPlatformPkg/MemoryInitPei: Check if the main System Memory resource has been declared"), ArmPlatformGetVirtualMemoryMap() is now called before the base system memory HOB is created in MemoryPeim().
This causes the logic in HiKey's ArmPlatformGetVirtualMemoryMap() which tries to chop up that base HOB to fail, as there's nothing to chop up at that time.
The calling code is smart enough to check if a base system memory HOB is already present before creating one, so we can just set it up ourselves here before we try to make our reservations.
This avoids problems seen in the kernel with the current logic, where none of the reservations were actually being made.
Note: I'm a total newbie with this codebase (I just recently figured out what HOB stood for). So feedback would be greatly appreciated!
CC: Olivier Martin Olivier.Martin@arm.com Cc: Haojian Zhuang haojian.zhuang@linaro.org Cc: Vishal Bhoj vishal.bhoj@linaro.org Signed-off-by: John Stultz john.stultz@linaro.org --- Platforms/Hisilicon/HiKey/Library/HiKeyLib/HiKeyMem.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)
diff --git a/Platforms/Hisilicon/HiKey/Library/HiKeyLib/HiKeyMem.c b/Platforms/Hisilicon/HiKey/Library/HiKeyLib/HiKeyMem.c index a5ce05b..2d118e0 100644 --- a/Platforms/Hisilicon/HiKey/Library/HiKeyLib/HiKeyMem.c +++ b/Platforms/Hisilicon/HiKey/Library/HiKeyLib/HiKeyMem.c @@ -89,6 +89,24 @@ ArmPlatformGetVirtualMemoryMap (
MemorySize = HiKeyInitMemorySize ();
+ ResourceAttributes = ( + EFI_RESOURCE_ATTRIBUTE_PRESENT | + EFI_RESOURCE_ATTRIBUTE_INITIALIZED | + EFI_RESOURCE_ATTRIBUTE_UNCACHEABLE | + EFI_RESOURCE_ATTRIBUTE_WRITE_COMBINEABLE | + EFI_RESOURCE_ATTRIBUTE_WRITE_THROUGH_CACHEABLE | + EFI_RESOURCE_ATTRIBUTE_WRITE_BACK_CACHEABLE | + EFI_RESOURCE_ATTRIBUTE_TESTED + ); + + // Create initial Base Hob for system memory. + BuildResourceDescriptorHob ( + EFI_RESOURCE_SYSTEM_MEMORY, + ResourceAttributes, + PcdGet64 (PcdSystemMemoryBase), + PcdGet64 (PcdSystemMemorySize) + ); + NextHob.Raw = GetHobList (); Count = sizeof (HiKeyReservedMemoryBuffer) / sizeof (struct HiKeyReservedMemory); while ((NextHob.Raw = GetNextHob (EFI_HOB_TYPE_RESOURCE_DESCRIPTOR, NextHob.Raw)) != NULL)
On 6 May 2016 at 06:35, John Stultz john.stultz@linaro.org wrote:
In the current edk2 code, as of 40a3f38f67cee ("ArmPlatformPkg/MemoryInitPei: Check if the main System Memory resource has been declared"), ArmPlatformGetVirtualMemoryMap() is now called before the base system memory HOB is created in MemoryPeim().
This causes the logic in HiKey's ArmPlatformGetVirtualMemoryMap() which tries to chop up that base HOB to fail, as there's nothing to chop up at that time.
The calling code is smart enough to check if a base system memory HOB is already present before creating one, so we can just set it up ourselves here before we try to make our reservations.
This avoids problems seen in the kernel with the current logic, where none of the reservations were actually being made.
Note: I'm a total newbie with this codebase (I just recently figured out what HOB stood for). So feedback would be greatly appreciated!
Hi John,
Thanks for the patch. What tree is this commit based on?
CC: Olivier Martin Olivier.Martin@arm.com Cc: Haojian Zhuang haojian.zhuang@linaro.org Cc: Vishal Bhoj vishal.bhoj@linaro.org Signed-off-by: John Stultz john.stultz@linaro.org
Platforms/Hisilicon/HiKey/Library/HiKeyLib/HiKeyMem.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)
diff --git a/Platforms/Hisilicon/HiKey/Library/HiKeyLib/HiKeyMem.c b/Platforms/Hisilicon/HiKey/Library/HiKeyLib/HiKeyMem.c index a5ce05b..2d118e0 100644 --- a/Platforms/Hisilicon/HiKey/Library/HiKeyLib/HiKeyMem.c +++ b/Platforms/Hisilicon/HiKey/Library/HiKeyLib/HiKeyMem.c @@ -89,6 +89,24 @@ ArmPlatformGetVirtualMemoryMap (
MemorySize = HiKeyInitMemorySize ();
This suggests that the PcdSystemMemorySize below may be different from the real memory size, but I cannot tell from the context. On systems with ARM Trusted Firmware running at EL3, there is usually some ad-hoc logic to figure out how much memory has been taken up by the secure firmware, and that either needs to be reflected in the PCD (if it's a dynamic PCD), or perhaps we should be using the MemorySize below?
- ResourceAttributes = (
EFI_RESOURCE_ATTRIBUTE_PRESENT |EFI_RESOURCE_ATTRIBUTE_INITIALIZED |EFI_RESOURCE_ATTRIBUTE_UNCACHEABLE |EFI_RESOURCE_ATTRIBUTE_WRITE_COMBINEABLE |EFI_RESOURCE_ATTRIBUTE_WRITE_THROUGH_CACHEABLE |EFI_RESOURCE_ATTRIBUTE_WRITE_BACK_CACHEABLE |EFI_RESOURCE_ATTRIBUTE_TESTED- );
- // Create initial Base Hob for system memory.
- BuildResourceDescriptorHob (
EFI_RESOURCE_SYSTEM_MEMORY,ResourceAttributes,PcdGet64 (PcdSystemMemoryBase),PcdGet64 (PcdSystemMemorySize)- );
- NextHob.Raw = GetHobList (); Count = sizeof (HiKeyReservedMemoryBuffer) / sizeof (struct HiKeyReservedMemory); while ((NextHob.Raw = GetNextHob (EFI_HOB_TYPE_RESOURCE_DESCRIPTOR, NextHob.Raw)) != NULL)
-- 1.9.1
Linaro-uefi mailing list Linaro-uefi@lists.linaro.org https://lists.linaro.org/mailman/listinfo/linaro-uefi
On Wed, May 11, 2016 at 1:10 AM, Ard Biesheuvel ard.biesheuvel@linaro.org wrote:
On 6 May 2016 at 06:35, John Stultz john.stultz@linaro.org wrote:
In the current edk2 code, as of 40a3f38f67cee ("ArmPlatformPkg/MemoryInitPei: Check if the main System Memory resource has been declared"), ArmPlatformGetVirtualMemoryMap() is now called before the base system memory HOB is created in MemoryPeim().
This causes the logic in HiKey's ArmPlatformGetVirtualMemoryMap() which tries to chop up that base HOB to fail, as there's nothing to chop up at that time.
The calling code is smart enough to check if a base system memory HOB is already present before creating one, so we can just set it up ourselves here before we try to make our reservations.
This avoids problems seen in the kernel with the current logic, where none of the reservations were actually being made.
Note: I'm a total newbie with this codebase (I just recently figured out what HOB stood for). So feedback would be greatly appreciated!
Hi John,
Thanks for the patch. What tree is this commit based on?
Sorry. Its based on the 96boards-hikey tree: https://github.com/96boards-hikey/OpenPlatformPkg.git
I realize it may have out of tree dependencies, but part of sending this out was just to make sure I'm not "doing it wrong" and making trouble for folks eventually pushing the code upstream.
(Of course I now realize that not having any context of the file being changed doesn't really help you in reviewing. So apologies :)
CC: Olivier Martin Olivier.Martin@arm.com Cc: Haojian Zhuang haojian.zhuang@linaro.org Cc: Vishal Bhoj vishal.bhoj@linaro.org Signed-off-by: John Stultz john.stultz@linaro.org
Platforms/Hisilicon/HiKey/Library/HiKeyLib/HiKeyMem.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)
diff --git a/Platforms/Hisilicon/HiKey/Library/HiKeyLib/HiKeyMem.c b/Platforms/Hisilicon/HiKey/Library/HiKeyLib/HiKeyMem.c index a5ce05b..2d118e0 100644 --- a/Platforms/Hisilicon/HiKey/Library/HiKeyLib/HiKeyMem.c +++ b/Platforms/Hisilicon/HiKey/Library/HiKeyLib/HiKeyMem.c @@ -89,6 +89,24 @@ ArmPlatformGetVirtualMemoryMap (
MemorySize = HiKeyInitMemorySize ();
This suggests that the PcdSystemMemorySize below may be different from the real memory size, but I cannot tell from the context. On systems with ARM Trusted Firmware running at EL3, there is usually some ad-hoc logic to figure out how much memory has been taken up by the secure firmware, and that either needs to be reflected in the PCD (if it's a dynamic PCD), or perhaps we should be using the MemorySize below?
Yea. Looking at the code: https://github.com/96boards-hikey/OpenPlatformPkg/blob/hikey-aosp/Platforms/...
It generates a initial HOB for the base memory size (what all the boards have), but then uses that HiKeyInitMemorySize() to figure out if its a board with 2gigs of ram or not.
It then later uses that potentially larger MemorySize value to generate a new hob and virtualmemorytable entry if it is the larger size.
But yea, we probably need to get the base file out for review here so those sorts of comments can be considered more.
My main concern was just making sure the process of generating the base HOB and reservations in the platform specific code was proper here.
thanks -john
-
Ard Biesheuvel -
John Stultz