[RFC] 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(+)

-- 
1.9.1

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/Hisilicon/HiKey/Library/HiKeyLib/HiKeyMem.c

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