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

[Laszlo Ersek <lersek@redhat.com>]

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
Laszlo