Re: [PATCH v2 23/28] NXP/LS1043aRdbPkg/ArmPlatformLib: Use Allocate pool

["Leif Lindholm" <leif@nuviainc.com>]

+Ard

On Mon, Apr 06, 2020 at 15:26:45 +0000, Pankaj Bansal (OSS) wrote:
> > -----Original Message-----
> > From: Leif Lindholm <leif@nuviainc.com>
> > Sent: Wednesday, April 1, 2020 11:34 PM
> > To: Pankaj Bansal (OSS) <pankaj.bansal@oss.nxp.com>
> > Cc: Meenakshi Aggarwal <meenakshi.aggarwal@nxp.com>; Michael D Kinney
> > <michael.d.kinney@intel.com>; devel@edk2.groups.io; Varun Sethi
> > <V.Sethi@nxp.com>; Samer El-Haj-Mahmoud <Samer.El-Haj-
> > Mahmoud@arm.com>; Jon Nettleton <jon@solid-run.com>
> > Subject: Re: [PATCH v2 23/28] NXP/LS1043aRdbPkg/ArmPlatformLib: Use
> > Allocate pool
> > 
> > On Fri, Mar 20, 2020 at 20:05:38 +0530, Pankaj Bansal wrote:
> > > From: Pankaj Bansal <pankaj.bansal@nxp.com>
> > >
> > > Allocate Pages may allocate more memory than required for
> > > VirtualMemoryTable.
> > > There is no special requirement that VirtualMemoryTable size should be
> > > page size aligned.
> > >
> > > Therefore, replace AllocatePages with AllocatePool.
> > >
> > > Signed-off-by: Pankaj Bansal <pankaj.bansal@nxp.com>
> > 
> > I don't object to this as such (although one comment), but what is the
> > purpose of this change?
> > 
> > My comment is that most other platforms use AllocatePages for this. So
> > this is diverging from the norm. 
> 
> I referred ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c

Sure. This one was converted to AllocatePool because the QEMU virt
machine is very simple (because it does not emulate much real
hardware) and the port rarely changes.

Ard's what's your opinion - do you think this worth it even for a
platform that has
#define MAX_VIRTUAL_MEMORY_MAP_DESCRIPTORS          25
?

Clearly there is still wasteage going on, but it's 16% less bad.

> > Secondly, while I don't necessarily
> > *like* the current design (copied across most ARM platforms), it's
> > somewhat mitigated by the AllocatePages giving you a minimum of 128
> > entries before you start overwriting things. I don't know what you'll
> > overwrite if you do go too far, but you will overwrite it *before* the
> > ASSERT ever gets evaluated.
> > 
> 
> We can improve this by evaluating ASSERT after each entry like this :
>   VirtualMemoryTable[Index].PhysicalBase = 0xXXXXXXXX;
>   VirtualMemoryTable[Index].VirtualBase  = 0xXXXXXXXX;
>   VirtualMemoryTable[Index].Length       = 0xXXXXXXXX;
>   VirtualMemoryTable[Index++].Attributes   = 0xXXXXXXXX;
> 
>   ASSERT (Index < MAX_VIRTUAL_MEMORY_MAP_DESCRIPTORS);

Whilst functionally preferable, I think that would make for very
tedious reading. I'll let Ard call this one.

/
    Leif

> > /
> >     Leif
> > 
> > > ---
> > >  .../LS1043aRdbPkg/Library/ArmPlatformLib/ArmPlatformLib.inf  | 1 +
> > >  .../LS1043aRdbPkg/Library/ArmPlatformLib/ArmPlatformLibMem.c | 5 +++--
> > >  2 files changed, 4 insertions(+), 2 deletions(-)
> > >
> > > diff --git
> > a/Platform/NXP/LS1043aRdbPkg/Library/ArmPlatformLib/ArmPlatformLib.inf
> > b/Platform/NXP/LS1043aRdbPkg/Library/ArmPlatformLib/ArmPlatformLib.inf
> > > index 1faf99b99c54..c64032f32772 100644
> > > ---
> > a/Platform/NXP/LS1043aRdbPkg/Library/ArmPlatformLib/ArmPlatformLib.inf
> > > +++
> > b/Platform/NXP/LS1043aRdbPkg/Library/ArmPlatformLib/ArmPlatformLib.inf
> > > @@ -25,6 +25,7 @@ [Packages]
> > >
> > >  [LibraryClasses]
> > >    ArmLib
> > > +  DebugLib
> > >    SocLib
> > >
> > >  [Sources.common]
> > > diff --git
> > a/Platform/NXP/LS1043aRdbPkg/Library/ArmPlatformLib/ArmPlatformLibMem.
> > c
> > b/Platform/NXP/LS1043aRdbPkg/Library/ArmPlatformLib/ArmPlatformLibMem.
> > c
> > > index f5fa308551aa..f8dd642e3cff 100644
> > > ---
> > a/Platform/NXP/LS1043aRdbPkg/Library/ArmPlatformLib/ArmPlatformLibMem.
> > c
> > > +++
> > b/Platform/NXP/LS1043aRdbPkg/Library/ArmPlatformLib/ArmPlatformLibMem.
> > c
> > > @@ -43,10 +43,11 @@ ArmPlatformGetVirtualMemoryMap (
> > >
> > >    ASSERT (VirtualMemoryMap != NULL);
> > >
> > > -  VirtualMemoryTable =
> > (ARM_MEMORY_REGION_DESCRIPTOR*)AllocatePages (
> > > -          EFI_SIZE_TO_PAGES (sizeof (ARM_MEMORY_REGION_DESCRIPTOR) *
> > MAX_VIRTUAL_MEMORY_MAP_DESCRIPTORS));
> > > +  VirtualMemoryTable = AllocatePool (sizeof
> > (ARM_MEMORY_REGION_DESCRIPTOR) *
> > > +                                     MAX_VIRTUAL_MEMORY_MAP_DESCRIPTORS);
> > >
> > >    if (VirtualMemoryTable == NULL) {
> > > +    DEBUG ((DEBUG_ERROR, "%a: Error: Failed AllocatePool()\n",
> > __FUNCTION__));
> > >      return;
> > >    }
> > >
> > > --
> > > 2.17.1
> > >