Re: [PATCH 12/15] ArmVirtPkg/ArmVirtQemu: add ArmVirtMemInfoLib implementation

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

On 21 November 2017 at 16:56, Laszlo Ersek <lersek@redhat.com> wrote:
> On 11/17/17 17:09, Ard Biesheuvel wrote:
>> Clone the existing ArmPlatformGetVirtualMemoryMap () for this platform,
>> clean it up slightly (by removing the support for uncached DRAM mappings),
>> and turn it into a new ArmVirtMemInfoLib implementation.
>
> I've looked at this patch with "git show --find-copies-harder". It looks
> OK, but the commit message could be improved:
>
> (1) the support for uncached DRAM mappings is removed in the copy-origin
> lib instance, in patch 09/15 ("ArmVirtPkg/ArmVirtPlatformLib: remove
> support for uncached mappings"). I think this sentence should be dropped
> from the commit message.
>
> (2) There are other cleanups however:
>
> - factor out TableSize and TopOfMemory
>
> - replace EFI_D_* with DEBUG_*
>
> - fetch PcdFdBaseAddress with PcdGet64(), not FixedPcdGet64(). (This is
> matched by the [Pcd] / [FixedPcd] sections in the new INF file.)
>
> Can you elaborate on the last item? I wonder if that change qualifies as
> cleanup. (I'm fine if the change is justified by some other flexibility,
> but it should be documented please.)
>
> With the commit message updated:
>
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
>

Thanks. But is actually based on
ArmVirtPkg/Library/ArmQemuRelocatablePlatformLib, which explains the
latter point.

However, given that we will be sharing it between ArmVirtQemu and
ArmVirtQemuKernel later on (which is apparently justified, since git
can't even tell them apart), it makes sense to elaborate a bit on the
differences and changes wrt the originals.