On 11/21/17 17:23, Laszlo Ersek wrote:
> On 11/17/17 17:09, Ard Biesheuvel wrote:
>> As part of the effort to get rid of ArmPlatformLib (which incorporates
>> far too many duties in a single library), introduce ArmVirtMemInfoLib
>> which will be invoked by our ArmVirtMemoryInitPeiLib implementation to
>> get a description of the virtual address space. This will allow us to
>> remove this functionality from ArmPlatformLib later, or, in the case of
>> ArmVirtXen and ArmVirtQemuKernel, drop ArmPlatformLib altogether.
>>
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Ard Biesheuvel <ard.biesheu...@linaro.org>
>> ---
>> ArmVirtPkg/ArmVirtPkg.dec | 3 ++
>> ArmVirtPkg/Include/Library/ArmVirtMemInfoLib.h | 39 ++++++++++++++++++++
>> 2 files changed, 42 insertions(+)
>>
>> diff --git a/ArmVirtPkg/ArmVirtPkg.dec b/ArmVirtPkg/ArmVirtPkg.dec
>> index a8603e1b80e5..8f656fd2739d 100644
>> --- a/ArmVirtPkg/ArmVirtPkg.dec
>> +++ b/ArmVirtPkg/ArmVirtPkg.dec
>> @@ -30,6 +30,9 @@ [Defines]
>> [Includes.common]
>> Include # Root include for the package
>>
>> +[LibraryClasses]
>> + ArmVirtMemInfoLib|Include/Library/ArmVirtMemInfoLib.h
>> +
>> [Guids.common]
>> gArmVirtTokenSpaceGuid = { 0x0B6F5CA7, 0x4F53, 0x445A, { 0xB7, 0x6E,
>> 0x2E, 0x36, 0x5B, 0x80, 0x63, 0x66 } }
>> gEarlyPL011BaseAddressGuid = { 0xB199DEA9, 0xFD5C, 0x4A84, { 0x80,
>> 0x82, 0x2F, 0x41, 0x70, 0x78, 0x03, 0x05 } }
>> diff --git a/ArmVirtPkg/Include/Library/ArmVirtMemInfoLib.h
>> b/ArmVirtPkg/Include/Library/ArmVirtMemInfoLib.h
>> new file mode 100644
>> index 000000000000..65be2cbd8082
>> --- /dev/null
>> +++ b/ArmVirtPkg/Include/Library/ArmVirtMemInfoLib.h
>> @@ -0,0 +1,39 @@
>> +/** @file
>> +
>> + Copyright (c) 2011-2013, ARM Limited. All rights reserved.
>> + Copyright (c) 2017, Linaro, Ltd. All rights reserved.
>> +
>> + This program and the accompanying materials are licensed and made
>> available
>> + under the terms and conditions of the BSD License which accompanies this
>> + distribution. The full text of the license may be found at
>> + http://opensource.org/licenses/bsd-license.php
>> +
>> + THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
>> + WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR
>> IMPLIED.
>> +
>> +**/
>> +
>> +#ifndef _ARM_VIRT_MEMINFO_LIB_H_
>> +#define _ARM_VIRT_MEMINFO_LIB_H_
>> +
>> +#include <Base.h>
>> +#include <Library/ArmLib.h>
>> +
>> +/**
>> + Return the Virtual Memory Map of your platform
>> +
>> + This Virtual Memory Map is used by MemoryInitPei Module to initialize the
>> MMU
>> + on your platform.
>> +
>> + @param[out] VirtualMemoryMap Array of ARM_MEMORY_REGION_DESCRIPTOR
>> + describing a Physical-to-Virtual Memory
>> + mapping. This array must be ended by a
>> + zero-filled entry
>> +
>> +**/
>> +VOID
>> +ArmVirtGetMemoryMap (
>> + OUT ARM_MEMORY_REGION_DESCRIPTOR **VirtualMemoryMap
>> + );
>> +
>> +#endif
>>
>
> (1) Since this is a library API, please add EFIAPI to the declaration.
>
> (This will affect the instance(s) too.)
>
>
> (2) If it's not overly restrictive, then please mention in the
> "VirtualMemoryMap" param comment that the map is supposed to be
> allocated dynamically within the function, using the phase-matching
> MemoryAllocationLib instance.
>
> (Judged from the AllocatePages() call in
> "ArmVirtPkg/Library/ArmQemuRelocatablePlatformLib/QemuVirtMem.c".)
Looking at the patch right after this one, dynamic memory allocation
appears wrong to spell out in the library interface.
Then I guess the right thing to say would be, "the returned array is
never supposed to be freed; it is released at the latest when the OS
takes control".
> With those addressed,
>
> Reviewed-by: Laszlo Ersek <ler...@redhat.com>
My R-b stands, just please clarify the expected lifetime of the array
returned, one way or another.
Thanks
Laszlo
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
