Re: [PATCH v2 4/5] MdeModulePkg/DxeCorePerformanceLib: use AllocatePeiAccessiblePages

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

On 24 May 2018 at 14:49, Laszlo Ersek <lersek@redhat.com> wrote:
> On 05/24/18 11:09, Ard Biesheuvel wrote:
>> Replace the call to and implementation of the function
>> FpdtAllocateReservedMemoryBelow4G() with a call to
>> AllocatePeiAccessiblePages, which boils down to the same on X64,
>> but does not crash non-X64 systems that lack memory below 4 GB.
>>
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> ---
>> Note that the ZeroMem() call is dropped, but it is redundant anyway, given
>> that the subsequent CopyMem() call supersedes it immediately.
>
> I'm not sure the ZeroMem() is redundant. The allocation size -- before rounding up to full pages -- is computed like this:
>
>   BootPerformanceDataSize = sizeof (BOOT_PERFORMANCE_TABLE) + mPerformanceLength + PcdGet32 (PcdExtFpdtBootRecordPadSize);
>   if (SmmCommData != NULL && SmmBootRecordData != NULL) {
>     BootPerformanceDataSize += SmmBootRecordDataSize;
>   }
>
> ZeroMem() covers all of the above, but the CopyMem() calls don't seem to cover the optional padding (from the PCD).
>
> I'm unsure if that matters, of course.
>

You're quite right. I'm not sure how I missed that.

In any case, I can add back the ZeroMem () quite easily after the
single invocation of AllocatePeiAccessiblePages() that I am adding in
this file.

> The patch looks good to me otherwise.
>
> Thanks
> Laszlo
>
>
>>
>>  MdeModulePkg/Library/DxeCorePerformanceLib/DxeCorePerformanceLib.c | 45 ++------------------
>>  1 file changed, 4 insertions(+), 41 deletions(-)
>>
>> diff --git a/MdeModulePkg/Library/DxeCorePerformanceLib/DxeCorePerformanceLib.c b/MdeModulePkg/Library/DxeCorePerformanceLib/DxeCorePerformanceLib.c
>> index 71d624fc9ce9..b1f09710b65c 100644
>> --- a/MdeModulePkg/Library/DxeCorePerformanceLib/DxeCorePerformanceLib.c
>> +++ b/MdeModulePkg/Library/DxeCorePerformanceLib/DxeCorePerformanceLib.c
>> @@ -165,46 +165,6 @@ IsKnownID (
>>    }
>>  }
>>
>> -/**
>> -  Allocate EfiReservedMemoryType below 4G memory address.
>> -
>> -  This function allocates EfiReservedMemoryType below 4G memory address.
>> -
>> -  @param[in]  Size   Size of memory to allocate.
>> -
>> -  @return Allocated address for output.
>> -
>> -**/
>> -VOID *
>> -FpdtAllocateReservedMemoryBelow4G (
>> -  IN UINTN       Size
>> -  )
>> -{
>> -  UINTN                 Pages;
>> -  EFI_PHYSICAL_ADDRESS  Address;
>> -  EFI_STATUS            Status;
>> -  VOID                  *Buffer;
>> -
>> -  Buffer  = NULL;
>> -  Pages   = EFI_SIZE_TO_PAGES (Size);
>> -  Address = 0xffffffff;
>> -
>> -  Status = gBS->AllocatePages (
>> -                  AllocateMaxAddress,
>> -                  EfiReservedMemoryType,
>> -                  Pages,
>> -                  &Address
>> -                  );
>> -  ASSERT_EFI_ERROR (Status);
>> -
>> -  if (!EFI_ERROR (Status)) {
>> -    Buffer = (VOID *) (UINTN) Address;
>> -    ZeroMem (Buffer, Size);
>> -  }
>> -
>> -  return Buffer;
>> -}
>> -
>>  /**
>>    Allocate buffer for Boot Performance table.
>>
>> @@ -348,7 +308,10 @@ AllocateBootPerformanceTable (
>>      //
>>      // Fail to allocate at specified address, continue to allocate at any address.
>>      //
>> -    mAcpiBootPerformanceTable = (BOOT_PERFORMANCE_TABLE *) FpdtAllocateReservedMemoryBelow4G (BootPerformanceDataSize);
>> +    mAcpiBootPerformanceTable = (BOOT_PERFORMANCE_TABLE *) AllocatePeiAccessiblePages (
>> +                                                             EfiReservedMemoryType,
>> +                                                             EFI_SIZE_TO_PAGES (BootPerformanceDataSize)
>> +                                                             );
>>    }
>>    DEBUG ((DEBUG_INFO, "DxeCorePerformanceLib: ACPI Boot Performance Table address = 0x%x\n", mAcpiBootPerformanceTable));
>>
>>
>