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

[Laszlo Ersek <lersek@redhat.com>]

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".)


With those addressed,

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Thanks
Laszlo