On 21 November 2017 at 16:27, Laszlo Ersek <ler...@redhat.com> wrote: > 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. Simply not freeing it is the current practice everywhere, given that PrePi and PEI MemoryAllocationLib implementations don't do FreePages() in the first place. But I agree it should be mentioned explicitly. _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel