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

2018-05-24 Thread Bi, Dandan 
Hi Star,

Yes. You are right. mAcpiBootPerformanceTable->Header.Length will be updated 
accordingly. We can get the real table length.
Currently there are some contents in the table are not updated, just use the 
value in the memory.
Previously the memory value is zero.  But without ZeroMem(), the value may be 
non-zero. That's the difference. It may impact the code to parse the table.

Thanks,
Dandan

-Original Message-
From: Zeng, Star 
Sent: Friday, May 25, 2018 10:45 AM
To: Bi, Dandan <dandan...@intel.com>; Ard Biesheuvel 
<ard.biesheu...@linaro.org>; Laszlo Ersek <ler...@redhat.com>
Cc: Dong, Eric <eric.d...@intel.com>; edk2-devel@lists.01.org; Gao, Liming 
<liming@intel.com>; Leif Lindholm <leif.lindh...@linaro.org>; Kinney, 
Michael D <michael.d.kin...@intel.com>; Zeng, Star <star.z...@intel.com>
Subject: RE: [edk2] [PATCH v2 4/5] MdeModulePkg/DxeCorePerformanceLib: use 
AllocatePeiAccessiblePages

mAcpiBootPerformanceTable->Header.Length could reflect the real table length, 
right?
When the padding (from the PCD) memory is used, 
mAcpiBootPerformanceTable->Header.Length will be updated accordingly.
If that is the case, it seems safe without ZeroMem.
Anyway it needs to be verified.


But, to be simple, I do not object to have ZeroMem to be equivalent with 
previous code.



Thanks,
Star
-Original Message-
From: Bi, Dandan
Sent: Friday, May 25, 2018 10:01 AM
To: Ard Biesheuvel <ard.biesheu...@linaro.org>; Laszlo Ersek <ler...@redhat.com>
Cc: Dong, Eric <eric.d...@intel.com>; edk2-devel@lists.01.org; Gao, Liming 
<liming@intel.com>; Leif Lindholm <leif.lindh...@linaro.org>; Kinney, 
Michael D <michael.d.kin...@intel.com>; Zeng, Star <star.z...@intel.com>
Subject: RE: [edk2] [PATCH v2 4/5] MdeModulePkg/DxeCorePerformanceLib: use 
AllocatePeiAccessiblePages

Hi Ard,

Thank you very much for helping fix this issue.

And for ZeroMem(), it should be added back.
Because after the memory is allocated, the optional padding (from the PCD) 
memory will be used directly. 
If ZeoMem() is dropped, then some contents in the padding memory will not be 
zero. Thus will impact the tool to parse the contents.

Thanks,
Dandan
-Original Message-
From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Ard 
Biesheuvel
Sent: Thursday, May 24, 2018 8:54 PM
To: Laszlo Ersek <ler...@redhat.com>
Cc: Dong, Eric <eric.d...@intel.com>; edk2-devel@lists.01.org; Gao, Liming 
<liming@intel.com>; Bi, Dandan <dandan...@intel.com>; Leif Lindholm 
<leif.lindh...@linaro.org>; Kinney, Michael D <michael.d.kin...@intel.com>; 
Zeng, Star <star.z...@intel.com>
Subject: Re: [edk2] [PATCH v2 4/5] MdeModulePkg/DxeCorePerformanceLib: use 
AllocatePeiAccessiblePages

On 24 May 2018 at 14:49, Laszlo Ersek <ler...@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.biesheu...@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/Li

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

2018-05-24 Thread Zeng, Star 
mAcpiBootPerformanceTable->Header.Length could reflect the real table length, 
right?
When the padding (from the PCD) memory is used, 
mAcpiBootPerformanceTable->Header.Length will be updated accordingly.
If that is the case, it seems safe without ZeroMem.
Anyway it needs to be verified.


But, to be simple, I do not object to have ZeroMem to be equivalent with 
previous code.



Thanks,
Star
-Original Message-
From: Bi, Dandan 
Sent: Friday, May 25, 2018 10:01 AM
To: Ard Biesheuvel <ard.biesheu...@linaro.org>; Laszlo Ersek <ler...@redhat.com>
Cc: Dong, Eric <eric.d...@intel.com>; edk2-devel@lists.01.org; Gao, Liming 
<liming@intel.com>; Leif Lindholm <leif.lindh...@linaro.org>; Kinney, 
Michael D <michael.d.kin...@intel.com>; Zeng, Star <star.z...@intel.com>
Subject: RE: [edk2] [PATCH v2 4/5] MdeModulePkg/DxeCorePerformanceLib: use 
AllocatePeiAccessiblePages

Hi Ard,

Thank you very much for helping fix this issue.

And for ZeroMem(), it should be added back.
Because after the memory is allocated, the optional padding (from the PCD) 
memory will be used directly. 
If ZeoMem() is dropped, then some contents in the padding memory will not be 
zero. Thus will impact the tool to parse the contents.

Thanks,
Dandan
-Original Message-
From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Ard 
Biesheuvel
Sent: Thursday, May 24, 2018 8:54 PM
To: Laszlo Ersek <ler...@redhat.com>
Cc: Dong, Eric <eric.d...@intel.com>; edk2-devel@lists.01.org; Gao, Liming 
<liming@intel.com>; Bi, Dandan <dandan...@intel.com>; Leif Lindholm 
<leif.lindh...@linaro.org>; Kinney, Michael D <michael.d.kin...@intel.com>; 
Zeng, Star <star.z...@intel.com>
Subject: Re: [edk2] [PATCH v2 4/5] MdeModulePkg/DxeCorePerformanceLib: use 
AllocatePeiAccessiblePages

On 24 May 2018 at 14:49, Laszlo Ersek <ler...@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.biesheu...@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/DxeCorePerformanceLi
>> +++ b.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_STATUSStatus;
>> -  VOID  *Buffer;
>> -
>> -  Buffer  = NULL;
>> -  Pages   = EFI_SIZE_TO_PAGES (Size);
>> -  Address = 0x;
>> -
>> -  Status = gBS->AllocatePages (
>> -  AllocateMax

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

2018-05-24 Thread Bi, Dandan 
Hi Ard,

Thank you very much for helping fix this issue.

And for ZeroMem(), it should be added back.
Because after the memory is allocated, the optional padding (from the PCD) 
memory will be used directly. 
If ZeoMem() is dropped, then some contents in the padding memory will not be 
zero. Thus will impact the tool to parse the contents.

Thanks,
Dandan
-Original Message-
From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Ard 
Biesheuvel
Sent: Thursday, May 24, 2018 8:54 PM
To: Laszlo Ersek <ler...@redhat.com>
Cc: Dong, Eric <eric.d...@intel.com>; edk2-devel@lists.01.org; Gao, Liming 
<liming@intel.com>; Bi, Dandan <dandan...@intel.com>; Leif Lindholm 
<leif.lindh...@linaro.org>; Kinney, Michael D <michael.d.kin...@intel.com>; 
Zeng, Star <star.z...@intel.com>
Subject: Re: [edk2] [PATCH v2 4/5] MdeModulePkg/DxeCorePerformanceLib: use 
AllocatePeiAccessiblePages

On 24 May 2018 at 14:49, Laszlo Ersek <ler...@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.biesheu...@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/DxeCorePerformanceLi
>> +++ b.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_STATUSStatus;
>> -  VOID  *Buffer;
>> -
>> -  Buffer  = NULL;
>> -  Pages   = EFI_SIZE_TO_PAGES (Size);
>> -  Address = 0x;
>> -
>> -  Status = gBS->AllocatePages (
>> -  AllocateMaxAddress,
>> -  EfiReservedMemoryType,
>> -  Pages,
>> -  
>> -  );
>> -  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

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

2018-05-24 Thread Laszlo Ersek 
On 05/24/18 14:54, Ard Biesheuvel wrote:
> On 24 May 2018 at 14:49, Laszlo Ersek  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 
>>> ---
>>> 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.

Thanks! With that:

Reviewed-by: Laszlo Ersek 

Laszlo

> 
>> 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_STATUSStatus;
>>> -  VOID  *Buffer;
>>> -
>>> -  Buffer  = NULL;
>>> -  Pages   = EFI_SIZE_TO_PAGES (Size);
>>> -  Address = 0x;
>>> -
>>> -  Status = gBS->AllocatePages (
>>> -  AllocateMaxAddress,
>>> -  EfiReservedMemoryType,
>>> -  Pages,
>>> -  
>>> -  );
>>> -  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));
>>>
>>>
>>

___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

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

2018-05-24 Thread Ard Biesheuvel 
On 24 May 2018 at 14:49, Laszlo Ersek  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 
>> ---
>> 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_STATUSStatus;
>> -  VOID  *Buffer;
>> -
>> -  Buffer  = NULL;
>> -  Pages   = EFI_SIZE_TO_PAGES (Size);
>> -  Address = 0x;
>> -
>> -  Status = gBS->AllocatePages (
>> -  AllocateMaxAddress,
>> -  EfiReservedMemoryType,
>> -  Pages,
>> -  
>> -  );
>> -  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));
>>
>>
>
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

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

2018-05-24 Thread Laszlo Ersek 
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 
> ---
> 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.

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_STATUSStatus;
> -  VOID  *Buffer;
> -
> -  Buffer  = NULL;
> -  Pages   = EFI_SIZE_TO_PAGES (Size);
> -  Address = 0x;
> -
> -  Status = gBS->AllocatePages (
> -  AllocateMaxAddress,
> -  EfiReservedMemoryType,
> -  Pages,
> -  
> -  );
> -  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));
>  
> 

___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

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

2018-05-24 Thread Ard Biesheuvel 
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 
---
Note that the ZeroMem() call is dropped, but it is redundant anyway, given
that the subsequent CopyMem() call supersedes it immediately.

 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_STATUSStatus;
-  VOID  *Buffer;
-
-  Buffer  = NULL;
-  Pages   = EFI_SIZE_TO_PAGES (Size);
-  Address = 0x;
-
-  Status = gBS->AllocatePages (
-  AllocateMaxAddress,
-  EfiReservedMemoryType,
-  Pages,
-  
-  );
-  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));
 
-- 
2.17.0

___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel