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