Re: [PATCH v2] EmbeddedPkg/PrePiMemoryAllocationLib: Check for space on offset allocation

["Jeff Brasen" <jbrasen@nvidia.com>]

> -----Original Message-----
> From: Ard Biesheuvel <ardb@kernel.org>
> Sent: Wednesday, September 7, 2022 10:16 AM
> To: Jeff Brasen <jbrasen@nvidia.com>
> Cc: devel@edk2.groups.io; ardb+tianocore@kernel.org;
> abner.chang@amd.com; git@danielschaefer.me; quic_llindhol@quicinc.com
> Subject: Re: [PATCH v2] EmbeddedPkg/PrePiMemoryAllocationLib: Check for
> space on offset allocation
> 
> External email: Use caution opening links or attachments
> 
> 
> On Wed, 7 Sept 2022 at 17:46, Jeff Brasen <jbrasen@nvidia.com> wrote:
> >
> > Update check for enough space to occur prior to alignment offset
> > modification. This prevents a case where EfiFreeMemoryTop could be
> > less than EfiFreeMemoryBottom
> >
> > Signed-off-by: Jeff Brasen <jbrasen@nvidia.com>
> 
> Thanks for the respin. I have caught up in the mean time.
> 
> 
> > ---
> >  .../MemoryAllocationLib.c                     | 53 +++++++++++--------
> >  1 file changed, 30 insertions(+), 23 deletions(-)
> >
> > diff --git
> >
> a/EmbeddedPkg/Library/PrePiMemoryAllocationLib/MemoryAllocationLib.c
> >
> b/EmbeddedPkg/Library/PrePiMemoryAllocationLib/MemoryAllocationLib.c
> > index 2cc2a71121..9208826565 100644
> > ---
> >
> a/EmbeddedPkg/Library/PrePiMemoryAllocationLib/MemoryAllocationLib.c
> > +++
> b/EmbeddedPkg/Library/PrePiMemoryAllocationLib/MemoryAllocationLib
> > +++ .c
> > @@ -27,37 +27,44 @@ InternalAllocatePages (
> >
> >    Hob.Raw = GetHobList ();
> >
> > -  // Check to see if on 4k boundary
> >    Offset = Hob.HandoffInformationTable->EfiFreeMemoryTop & 0xFFF;
> > +  //
> > +  // Verify that there is sufficient memory to satisfy the allocation
> > + and padding prior to updating anything  //  if
> > + ((Hob.HandoffInformationTable->EfiFreeMemoryTop - ((Pages *
> EFI_PAGE_SIZE) + sizeof (EFI_HOB_MEMORY_ALLOCATION)) - Offset) <
> Hob.HandoffInformationTable->EfiFreeMemoryBottom) {
> > +    if (Offset != 0) {
> > +      DEBUG ((DEBUG_ERROR, "Offset applied without enough
> space\r\n"));
> > +    } else {
> > +      DEBUG ((DEBUG_ERROR, "Out of memory\r\n"));
> > +    }
> > +
> > +    ASSERT (FALSE);
> > +    return 0;
> > +  }
> > +
> > +  // Check to see if on 4k boundary
> >    if (Offset != 0) {
> >      // If not aligned, make the allocation aligned.
> >      Hob.HandoffInformationTable->EfiFreeMemoryTop -= Offset;
> >    }
> >
> 
> I understand how you are trying to stick with the original code as much as
> possible, but this is all extremely clunky, and I'd prefer we just clean it up, if
> you don't mind.
> 
> Would something like the below work for you as well?

Yup this looks good to me and cleaner as well.

> 
> 
> 
> diff --git
> a/EmbeddedPkg/Library/PrePiMemoryAllocationLib/MemoryAllocationLib.c
> b/EmbeddedPkg/Library/PrePiMemoryAllocationLib/MemoryAllocationLib.c
> index 2cc2a7112197..547117dc13d6 100644
> ---
> a/EmbeddedPkg/Library/PrePiMemoryAllocationLib/MemoryAllocationLib.c
> +++
> b/EmbeddedPkg/Library/PrePiMemoryAllocationLib/MemoryAllocationLib.c
> @@ -23,41 +23,36 @@ InternalAllocatePages (
>    )
>  {
>    EFI_PEI_HOB_POINTERS  Hob;
> -  EFI_PHYSICAL_ADDRESS  Offset;
> +  EFI_PHYSICAL_ADDRESS  NewTop;
> 
>    Hob.Raw = GetHobList ();
> 
> -  // Check to see if on 4k boundary
> -  Offset = Hob.HandoffInformationTable->EfiFreeMemoryTop & 0xFFF;
> -  if (Offset != 0) {
> -    // If not aligned, make the allocation aligned.
> -    Hob.HandoffInformationTable->EfiFreeMemoryTop -= Offset;
> -  }
> +  NewTop = Hob.HandoffInformationTable->EfiFreeMemoryTop &
> ~(EFI_PHYSICAL_ADDRESS)EFI_PAGE_MASK;
> +  NewTop -= Pages * EFI_PAGE_SIZE;
> 
>    //
>    // Verify that there is sufficient memory to satisfy the allocation
>    //
> -  if (Hob.HandoffInformationTable->EfiFreeMemoryTop - ((Pages *
> EFI_PAGE_SIZE) + sizeof (EFI_HOB_MEMORY_ALLOCATION)) <
> Hob.HandoffInformationTable->EfiFreeMemoryBottom) {
> -    return 0;
> -  } else {
> -    //
> -    // Update the PHIT to reflect the memory usage
> -    //
> -    Hob.HandoffInformationTable->EfiFreeMemoryTop -= Pages *
> EFI_PAGE_SIZE;
> -
> -    // This routine used to create a memory allocation HOB a la PEI,
> but that's not
> -    // necessary for us.
> -
> -    //
> -    // Create a memory allocation HOB.
> -    //
> -    BuildMemoryAllocationHob (
> -      Hob.HandoffInformationTable->EfiFreeMemoryTop,
> -      Pages * EFI_PAGE_SIZE,
> -      MemoryType
> -      );
> -    return (VOID *)(UINTN)Hob.HandoffInformationTable-
> >EfiFreeMemoryTop;
> +  if (NewTop < (Hob.HandoffInformationTable->EfiFreeMemoryBottom +
> sizeof (EFI_HOB_MEMORY_ALLOCATION)))
> +  {
> +    return NULL;
>    }
> +
> +  //
> +  // Update the PHIT to reflect the memory usage  //
> + Hob.HandoffInformationTable->EfiFreeMemoryTop = NewTop;
> +
> +  //
> +  // Create a memory allocation HOB.
> +  //
> +  BuildMemoryAllocationHob (
> +    Hob.HandoffInformationTable->EfiFreeMemoryTop,
> +    Pages * EFI_PAGE_SIZE,
> +    MemoryType
> +    );
> +
> +  return (VOID *)(UINTN)Hob.HandoffInformationTable-
> >EfiFreeMemoryTop;
>  }
> 
>  /**