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

["Pankaj Bansal" <pankaj.bansal@nxp.com>]

Hi Ard,

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

Can you please reply ? I have to send v3 of these patches.
Please let me know if I need to keep or remove these changes ?

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

This is also a minor improvement that I am proposing.
Please comment if this should be done or not ?