> -----Original Message-----
> From: Leif Lindholm <l...@nuviainc.com>
> Sent: Wednesday, April 1, 2020 11:34 PM
> To: Pankaj Bansal (OSS) <pankaj.ban...@oss.nxp.com>
> Cc: Meenakshi Aggarwal <meenakshi.aggar...@nxp.com>; Michael D Kinney
> <michael.d.kin...@intel.com>; devel@edk2.groups.io; Varun Sethi
> <v.se...@nxp.com>; Samer El-Haj-Mahmoud <Samer.El-Haj-
> mahm...@arm.com>; Jon Nettleton <j...@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.ban...@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.ban...@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
> 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);
> /
> 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
> >
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#56990): https://edk2.groups.io/g/devel/message/56990
Mute This Topic: https://groups.io/mt/72077458/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
