Re: [RFC PATCH 5/7] ArmPlatformPkg/MemoryInitPeim: take MAX_ALLOC_ADDRESS into account

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

On Fri, 7 Dec 2018 at 13:47, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>
> On Fri, 7 Dec 2018 at 13:46, Laszlo Ersek <lersek@redhat.com> wrote:
> >
> > On 12/07/18 12:23, Ard Biesheuvel wrote:
> > > Limit the PEI memory region so it will not extend beyond what we can
> > > address architecturally when running with 4 KB pages.
> > >
> > > Contributed-under: TianoCore Contribution Agreement 1.1
> > > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > > ---
> > >  ArmPlatformPkg/MemoryInitPei/MemoryInitPeim.c | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/ArmPlatformPkg/MemoryInitPei/MemoryInitPeim.c b/ArmPlatformPkg/MemoryInitPei/MemoryInitPeim.c
> > > index 389a2e6f1abd..25db87fb2374 100644
> > > --- a/ArmPlatformPkg/MemoryInitPei/MemoryInitPeim.c
> > > +++ b/ArmPlatformPkg/MemoryInitPei/MemoryInitPeim.c
> > > @@ -109,8 +109,8 @@ InitializeMemory (
> > >
> > >    SystemMemoryBase = (UINTN)PcdGet64 (PcdSystemMemoryBase);
> > >    SystemMemoryTop = SystemMemoryBase + PcdGet64 (PcdSystemMemorySize);
> > > -  if (SystemMemoryTop - 1 > MAX_ADDRESS) {
> > > -    SystemMemoryTop = (UINT64)MAX_ADDRESS + 1;
> > > +  if (SystemMemoryTop - 1 > MAX_ALLOC_ADDRESS) {
> > > +    SystemMemoryTop = (UINT64)MAX_ALLOC_ADDRESS + 1;
> > >    }
> > >    FdBase = (UINTN)PcdGet64 (PcdFdBaseAddress);
> > >    FdTop = FdBase + (UINTN)PcdGet32 (PcdFdSize);
> > >
> >
> > Shouldn't you also update
> >
> >   ASSERT (PcdGet64 (PcdSystemMemoryBase) < (UINT64)MAX_ADDRESS);
> >
> > just above the context visible here?
> >
>
> No, it's fine for PcdSystemMemoryBase/PcdSystemMemorySize to describe
> a region we cannot access in its entirety, as long as we don't use the
> top part (See the next patch as well)

Whoops, no you are right. That is Base not Base+Size, so indeed, Base
should be below MAX_ALLOC_ADDRESS