Re: [PATCH 10/15] ArmVirtPkg: introduce ArmVirtMemInfoLib library class

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

On 21 November 2017 at 16:27, Laszlo Ersek <lersek@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.biesheuvel@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 <lersek@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.