* [PATCH v2 1/5] OvmfPkg/PlatformBootManagerLib: add missing report status code call
2018-05-24 9:09 [PATCH v2 0/5] Abstract allocation of PEI accessible memory Ard Biesheuvel
@ 2018-05-24 9:09 ` Ard Biesheuvel
2018-05-24 9:09 ` [PATCH v2 2/5] ArmVirtPkg/PlatformBootManagerLib: " Ard Biesheuvel
` (4 subsequent siblings)
5 siblings, 0 replies; 15+ messages in thread
From: Ard Biesheuvel @ 2018-05-24 9:09 UTC (permalink / raw)
To: edk2-devel
Cc: Ard Biesheuvel, Laszlo Ersek, Leif Lindholm, Michael D Kinney,
Liming Gao, Star Zeng, Eric Dong, Dandan Bi
Consumers of status code reports may rely on a status code to be
reported when the ReadyToBoot event is signalled. For instance,
FirmwarePerformanceDxe will fail to install the FPDT ACPI table
in this case. So add the missing call.
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
---
OvmfPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf | 1 +
OvmfPkg/Library/PlatformBootManagerLib/QemuKernel.c | 4 ++++
2 files changed, 5 insertions(+)
diff --git a/OvmfPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf b/OvmfPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf
index 27789b7377bc..f10b68424b91 100644
--- a/OvmfPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf
+++ b/OvmfPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf
@@ -55,6 +55,7 @@ [LibraryClasses]
QemuFwCfgS3Lib
LoadLinuxLib
QemuBootOrderLib
+ ReportStatusCodeLib
UefiLib
[Pcd]
diff --git a/OvmfPkg/Library/PlatformBootManagerLib/QemuKernel.c b/OvmfPkg/Library/PlatformBootManagerLib/QemuKernel.c
index ef728dfdeb60..f20df9533fda 100644
--- a/OvmfPkg/Library/PlatformBootManagerLib/QemuKernel.c
+++ b/OvmfPkg/Library/PlatformBootManagerLib/QemuKernel.c
@@ -18,6 +18,7 @@
#include <Library/LoadLinuxLib.h>
#include <Library/MemoryAllocationLib.h>
#include <Library/QemuFwCfgLib.h>
+#include <Library/ReportStatusCodeLib.h>
#include <Library/UefiBootServicesTableLib.h>
#include <Library/UefiLib.h>
@@ -149,6 +150,9 @@ TryRunningQemuKernel (
//
EfiSignalEventReadyToBoot();
+ REPORT_STATUS_CODE (EFI_PROGRESS_CODE,
+ (EFI_SOFTWARE_DXE_BS_DRIVER | EFI_SW_DXE_BS_PC_READY_TO_BOOT_EVENT));
+
Status = LoadLinux (KernelBuf, SetupBuf);
FreeAndReturn:
--
2.17.0
^ permalink raw reply related [flat|nested] 15+ messages in thread* [PATCH v2 2/5] ArmVirtPkg/PlatformBootManagerLib: add missing report status code call
2018-05-24 9:09 [PATCH v2 0/5] Abstract allocation of PEI accessible memory Ard Biesheuvel
2018-05-24 9:09 ` [PATCH v2 1/5] OvmfPkg/PlatformBootManagerLib: add missing report status code call Ard Biesheuvel
@ 2018-05-24 9:09 ` Ard Biesheuvel
2018-05-24 9:09 ` [PATCH v2 3/5] MdePkg/DxeServicesLib: introduce AllocatePeiAccessiblePages routine Ard Biesheuvel
` (3 subsequent siblings)
5 siblings, 0 replies; 15+ messages in thread
From: Ard Biesheuvel @ 2018-05-24 9:09 UTC (permalink / raw)
To: edk2-devel
Cc: Ard Biesheuvel, Laszlo Ersek, Leif Lindholm, Michael D Kinney,
Liming Gao, Star Zeng, Eric Dong, Dandan Bi
Consumers of status code reports may rely on a status code to be
reported when the ReadyToBoot event is signalled. For instance,
FirmwarePerformanceDxe will fail to install the FPDT ACPI table
in this case. So add the missing call.
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
---
ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf | 1 +
ArmVirtPkg/Library/PlatformBootManagerLib/QemuKernel.c | 4 ++++
2 files changed, 5 insertions(+)
diff --git a/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf b/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf
index d6c1ef95dc44..0cbc82f5d27d 100644
--- a/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf
+++ b/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf
@@ -53,6 +53,7 @@ [LibraryClasses]
PrintLib
QemuBootOrderLib
QemuFwCfgLib
+ ReportStatusCodeLib
UefiBootManagerLib
UefiBootServicesTableLib
UefiLib
diff --git a/ArmVirtPkg/Library/PlatformBootManagerLib/QemuKernel.c b/ArmVirtPkg/Library/PlatformBootManagerLib/QemuKernel.c
index ac47d21e71c8..7b59f57eb19f 100644
--- a/ArmVirtPkg/Library/PlatformBootManagerLib/QemuKernel.c
+++ b/ArmVirtPkg/Library/PlatformBootManagerLib/QemuKernel.c
@@ -20,6 +20,7 @@
#include <Guid/FileSystemVolumeLabelInfo.h>
#include <Library/PrintLib.h>
#include <Library/QemuFwCfgLib.h>
+#include <Library/ReportStatusCodeLib.h>
#include <Protocol/DevicePath.h>
#include <Protocol/LoadedImage.h>
#include <Protocol/SimpleFileSystem.h>
@@ -1072,6 +1073,9 @@ TryRunningQemuKernel (
//
EfiSignalEventReadyToBoot();
+ REPORT_STATUS_CODE (EFI_PROGRESS_CODE,
+ (EFI_SOFTWARE_DXE_BS_DRIVER | EFI_SW_DXE_BS_PC_READY_TO_BOOT_EVENT));
+
//
// Start the image.
//
--
2.17.0
^ permalink raw reply related [flat|nested] 15+ messages in thread* [PATCH v2 3/5] MdePkg/DxeServicesLib: introduce AllocatePeiAccessiblePages routine
2018-05-24 9:09 [PATCH v2 0/5] Abstract allocation of PEI accessible memory Ard Biesheuvel
2018-05-24 9:09 ` [PATCH v2 1/5] OvmfPkg/PlatformBootManagerLib: add missing report status code call Ard Biesheuvel
2018-05-24 9:09 ` [PATCH v2 2/5] ArmVirtPkg/PlatformBootManagerLib: " Ard Biesheuvel
@ 2018-05-24 9:09 ` Ard Biesheuvel
2018-05-24 12:36 ` Laszlo Ersek
2018-05-24 9:09 ` [PATCH v2 4/5] MdeModulePkg/DxeCorePerformanceLib: use AllocatePeiAccessiblePages Ard Biesheuvel
` (2 subsequent siblings)
5 siblings, 1 reply; 15+ messages in thread
From: Ard Biesheuvel @ 2018-05-24 9:09 UTC (permalink / raw)
To: edk2-devel
Cc: Ard Biesheuvel, Laszlo Ersek, Leif Lindholm, Michael D Kinney,
Liming Gao, Star Zeng, Eric Dong, Dandan Bi
Add a routine to DxeServicesLib that abstracts the allocation of memory
that should be accessible by PEI after a warm reboot. We will use it to
replace open coded implementations that limit the address to < 4 GB,
which may not be possible on non-Intel systems that have no 32-bit
addressable memory at all.
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
MdePkg/Include/Library/DxeServicesLib.h | 23 +++++++-
MdePkg/Library/DxeServicesLib/DxeServicesLib.c | 55 ++++++++++++++++++++
MdePkg/Library/DxeServicesLib/DxeServicesLib.inf | 3 ++
3 files changed, 80 insertions(+), 1 deletion(-)
diff --git a/MdePkg/Include/Library/DxeServicesLib.h b/MdePkg/Include/Library/DxeServicesLib.h
index 7c1c62236d96..20aee68af558 100644
--- a/MdePkg/Include/Library/DxeServicesLib.h
+++ b/MdePkg/Include/Library/DxeServicesLib.h
@@ -305,5 +305,26 @@ GetFileDevicePathFromAnyFv (
OUT EFI_DEVICE_PATH_PROTOCOL **FvFileDevicePath
);
-#endif
+/**
+ Allocates one or more 4KB pages of a given type from a memory region that is
+ accessible to PEI.
+
+ Allocates the number of 4KB pages of type 'MemoryType' and returns a
+ pointer to the allocated buffer. The buffer returned is aligned on a 4KB
+ boundary. If Pages is 0, then NULL is returned. If there is not enough
+ memory remaining to satisfy the request, then NULL is returned.
+ @param[in] MemoryType The memory type to allocate
+ @param[in] Pages The number of 4 KB pages to allocate.
+
+ @return A pointer to the allocated buffer or NULL if allocation fails.
+
+**/
+VOID *
+EFIAPI
+AllocatePeiAccessiblePages (
+ IN EFI_MEMORY_TYPE MemoryType,
+ IN UINTN Pages
+ );
+
+#endif
diff --git a/MdePkg/Library/DxeServicesLib/DxeServicesLib.c b/MdePkg/Library/DxeServicesLib/DxeServicesLib.c
index 1827c9216fbc..6719aabe5d04 100644
--- a/MdePkg/Library/DxeServicesLib/DxeServicesLib.c
+++ b/MdePkg/Library/DxeServicesLib/DxeServicesLib.c
@@ -16,6 +16,7 @@
#include <PiDxe.h>
#include <Library/DebugLib.h>
+#include <Library/HobLib.h>
#include <Library/MemoryAllocationLib.h>
#include <Library/UefiBootServicesTableLib.h>
#include <Library/DevicePathLib.h>
@@ -1093,3 +1094,57 @@ Done:
return Status;
}
+
+/**
+ Allocates one or more 4KB pages of a given type from a memory region that is
+ accessible to PEI.
+
+ Allocates the number of 4KB pages of type 'MemoryType' and returns a
+ pointer to the allocated buffer. The buffer returned is aligned on a 4KB
+ boundary. If Pages is 0, then NULL is returned. If there is not enough
+ memory remaining to satisfy the request, then NULL is returned.
+
+ @param[in] MemoryType The memory type to allocate
+ @param[in] Pages The number of 4 KB pages to allocate.
+
+ @return A pointer to the allocated buffer or NULL if allocation fails.
+
+**/
+VOID *
+EFIAPI
+AllocatePeiAccessiblePages (
+ IN EFI_MEMORY_TYPE MemoryType,
+ IN UINTN Pages
+ )
+{
+ EFI_STATUS Status;
+ EFI_ALLOCATE_TYPE AllocType;
+ EFI_PHYSICAL_ADDRESS Memory;
+#ifdef MDE_CPU_X64
+ EFI_HOB_HANDOFF_INFO_TABLE *PhitHob;
+#endif
+
+ if (Pages == 0) {
+ return NULL;
+ }
+
+ AllocType = AllocateAnyPages;
+#ifdef MDE_CPU_X64
+ //
+ // On X64 systems, a X64 build of DXE may be combined with a 32-bit build of
+ // PEI, and so we need to check the memory limit set by PEI, and allocate
+ // below 4 GB if the limit is set to 4 GB or lower.
+ //
+ PhitHob = (EFI_HOB_HANDOFF_INFO_TABLE *)GetHobList ();
+ if (PhitHob->EfiFreeMemoryTop <= MAX_UINT32) {
+ AllocType = AllocateMaxAddress;
+ Memory = MAX_UINT32;
+ }
+#endif
+
+ Status = gBS->AllocatePages (AllocType, MemoryType, Pages, &Memory);
+ if (EFI_ERROR (Status)) {
+ return NULL;
+ }
+ return (VOID *)(UINTN)Memory;
+}
diff --git a/MdePkg/Library/DxeServicesLib/DxeServicesLib.inf b/MdePkg/Library/DxeServicesLib/DxeServicesLib.inf
index bd2faf2f6f2d..b0306c09872f 100644
--- a/MdePkg/Library/DxeServicesLib/DxeServicesLib.inf
+++ b/MdePkg/Library/DxeServicesLib/DxeServicesLib.inf
@@ -44,6 +44,9 @@ [LibraryClasses]
UefiLib
UefiBootServicesTableLib
+[LibraryClasses.common.X64]
+ HobLib
+
[Guids]
gEfiFileInfoGuid ## SOMETIMES_CONSUMES ## UNDEFINED
--
2.17.0
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [PATCH v2 3/5] MdePkg/DxeServicesLib: introduce AllocatePeiAccessiblePages routine
2018-05-24 9:09 ` [PATCH v2 3/5] MdePkg/DxeServicesLib: introduce AllocatePeiAccessiblePages routine Ard Biesheuvel
@ 2018-05-24 12:36 ` Laszlo Ersek
0 siblings, 0 replies; 15+ messages in thread
From: Laszlo Ersek @ 2018-05-24 12:36 UTC (permalink / raw)
To: Ard Biesheuvel, edk2-devel
Cc: Leif Lindholm, Michael D Kinney, Liming Gao, Star Zeng, Eric Dong,
Dandan Bi
On 05/24/18 11:09, Ard Biesheuvel wrote:
> Add a routine to DxeServicesLib that abstracts the allocation of memory
> that should be accessible by PEI after a warm reboot. We will use it to
> replace open coded implementations that limit the address to < 4 GB,
> which may not be possible on non-Intel systems that have no 32-bit
> addressable memory at all.
>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
> MdePkg/Include/Library/DxeServicesLib.h | 23 +++++++-
> MdePkg/Library/DxeServicesLib/DxeServicesLib.c | 55 ++++++++++++++++++++
> MdePkg/Library/DxeServicesLib/DxeServicesLib.inf | 3 ++
> 3 files changed, 80 insertions(+), 1 deletion(-)
>
> diff --git a/MdePkg/Include/Library/DxeServicesLib.h b/MdePkg/Include/Library/DxeServicesLib.h
> index 7c1c62236d96..20aee68af558 100644
> --- a/MdePkg/Include/Library/DxeServicesLib.h
> +++ b/MdePkg/Include/Library/DxeServicesLib.h
> @@ -305,5 +305,26 @@ GetFileDevicePathFromAnyFv (
> OUT EFI_DEVICE_PATH_PROTOCOL **FvFileDevicePath
> );
>
> -#endif
> +/**
> + Allocates one or more 4KB pages of a given type from a memory region that is
> + accessible to PEI.
> +
> + Allocates the number of 4KB pages of type 'MemoryType' and returns a
> + pointer to the allocated buffer. The buffer returned is aligned on a 4KB
> + boundary. If Pages is 0, then NULL is returned. If there is not enough
> + memory remaining to satisfy the request, then NULL is returned.
>
> + @param[in] MemoryType The memory type to allocate
> + @param[in] Pages The number of 4 KB pages to allocate.
> +
> + @return A pointer to the allocated buffer or NULL if allocation fails.
> +
> +**/
> +VOID *
> +EFIAPI
> +AllocatePeiAccessiblePages (
> + IN EFI_MEMORY_TYPE MemoryType,
> + IN UINTN Pages
> + );
> +
> +#endif
> diff --git a/MdePkg/Library/DxeServicesLib/DxeServicesLib.c b/MdePkg/Library/DxeServicesLib/DxeServicesLib.c
> index 1827c9216fbc..6719aabe5d04 100644
> --- a/MdePkg/Library/DxeServicesLib/DxeServicesLib.c
> +++ b/MdePkg/Library/DxeServicesLib/DxeServicesLib.c
> @@ -16,6 +16,7 @@
>
> #include <PiDxe.h>
> #include <Library/DebugLib.h>
> +#include <Library/HobLib.h>
> #include <Library/MemoryAllocationLib.h>
> #include <Library/UefiBootServicesTableLib.h>
> #include <Library/DevicePathLib.h>
> @@ -1093,3 +1094,57 @@ Done:
>
> return Status;
> }
> +
> +/**
> + Allocates one or more 4KB pages of a given type from a memory region that is
> + accessible to PEI.
> +
> + Allocates the number of 4KB pages of type 'MemoryType' and returns a
> + pointer to the allocated buffer. The buffer returned is aligned on a 4KB
> + boundary. If Pages is 0, then NULL is returned. If there is not enough
> + memory remaining to satisfy the request, then NULL is returned.
> +
> + @param[in] MemoryType The memory type to allocate
> + @param[in] Pages The number of 4 KB pages to allocate.
> +
> + @return A pointer to the allocated buffer or NULL if allocation fails.
> +
> +**/
> +VOID *
> +EFIAPI
> +AllocatePeiAccessiblePages (
> + IN EFI_MEMORY_TYPE MemoryType,
> + IN UINTN Pages
> + )
> +{
> + EFI_STATUS Status;
> + EFI_ALLOCATE_TYPE AllocType;
> + EFI_PHYSICAL_ADDRESS Memory;
> +#ifdef MDE_CPU_X64
> + EFI_HOB_HANDOFF_INFO_TABLE *PhitHob;
> +#endif
> +
> + if (Pages == 0) {
> + return NULL;
> + }
> +
> + AllocType = AllocateAnyPages;
> +#ifdef MDE_CPU_X64
> + //
> + // On X64 systems, a X64 build of DXE may be combined with a 32-bit build of
> + // PEI, and so we need to check the memory limit set by PEI, and allocate
> + // below 4 GB if the limit is set to 4 GB or lower.
> + //
> + PhitHob = (EFI_HOB_HANDOFF_INFO_TABLE *)GetHobList ();
> + if (PhitHob->EfiFreeMemoryTop <= MAX_UINT32) {
> + AllocType = AllocateMaxAddress;
> + Memory = MAX_UINT32;
> + }
> +#endif
> +
> + Status = gBS->AllocatePages (AllocType, MemoryType, Pages, &Memory);
> + if (EFI_ERROR (Status)) {
> + return NULL;
> + }
> + return (VOID *)(UINTN)Memory;
> +}
> diff --git a/MdePkg/Library/DxeServicesLib/DxeServicesLib.inf b/MdePkg/Library/DxeServicesLib/DxeServicesLib.inf
> index bd2faf2f6f2d..b0306c09872f 100644
> --- a/MdePkg/Library/DxeServicesLib/DxeServicesLib.inf
> +++ b/MdePkg/Library/DxeServicesLib/DxeServicesLib.inf
> @@ -44,6 +44,9 @@ [LibraryClasses]
> UefiLib
> UefiBootServicesTableLib
>
> +[LibraryClasses.common.X64]
> + HobLib
> +
> [Guids]
> gEfiFileInfoGuid ## SOMETIMES_CONSUMES ## UNDEFINED
>
>
Perhaps the #include <Library/HobLib.h> directive could be guarded by
#ifdef MDE_CPU_X64 as well, just for symmetry with the HobLib lib class,
but it's not too important.
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v2 4/5] MdeModulePkg/DxeCorePerformanceLib: use AllocatePeiAccessiblePages
2018-05-24 9:09 [PATCH v2 0/5] Abstract allocation of PEI accessible memory Ard Biesheuvel
` (2 preceding siblings ...)
2018-05-24 9:09 ` [PATCH v2 3/5] MdePkg/DxeServicesLib: introduce AllocatePeiAccessiblePages routine Ard Biesheuvel
@ 2018-05-24 9:09 ` Ard Biesheuvel
2018-05-24 12:49 ` Laszlo Ersek
2018-05-24 9:09 ` [PATCH v2 5/5] MdeModulePkg/FirmwarePerformanceDataTableDxe: " Ard Biesheuvel
2018-05-24 16:57 ` [PATCH v2 0/5] Abstract allocation of PEI accessible memory Kinney, Michael D
5 siblings, 1 reply; 15+ messages in thread
From: Ard Biesheuvel @ 2018-05-24 9:09 UTC (permalink / raw)
To: edk2-devel
Cc: Ard Biesheuvel, Laszlo Ersek, Leif Lindholm, Michael D Kinney,
Liming Gao, Star Zeng, Eric Dong, Dandan Bi
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.
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));
--
2.17.0
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [PATCH v2 4/5] MdeModulePkg/DxeCorePerformanceLib: use AllocatePeiAccessiblePages
2018-05-24 9:09 ` [PATCH v2 4/5] MdeModulePkg/DxeCorePerformanceLib: use AllocatePeiAccessiblePages Ard Biesheuvel
@ 2018-05-24 12:49 ` Laszlo Ersek
2018-05-24 12:54 ` Ard Biesheuvel
0 siblings, 1 reply; 15+ messages in thread
From: Laszlo Ersek @ 2018-05-24 12:49 UTC (permalink / raw)
To: Ard Biesheuvel, edk2-devel
Cc: Leif Lindholm, Michael D Kinney, Liming Gao, Star Zeng, Eric Dong,
Dandan Bi
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.
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));
>
>
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH v2 4/5] MdeModulePkg/DxeCorePerformanceLib: use AllocatePeiAccessiblePages
2018-05-24 12:49 ` Laszlo Ersek
@ 2018-05-24 12:54 ` Ard Biesheuvel
2018-05-24 13:09 ` Laszlo Ersek
2018-05-25 2:00 ` Bi, Dandan
0 siblings, 2 replies; 15+ messages in thread
From: Ard Biesheuvel @ 2018-05-24 12:54 UTC (permalink / raw)
To: Laszlo Ersek
Cc: edk2-devel@lists.01.org, Leif Lindholm, Michael D Kinney,
Liming Gao, Star Zeng, Eric Dong, Dandan Bi
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));
>>
>>
>
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH v2 4/5] MdeModulePkg/DxeCorePerformanceLib: use AllocatePeiAccessiblePages
2018-05-24 12:54 ` Ard Biesheuvel
@ 2018-05-24 13:09 ` Laszlo Ersek
2018-05-25 2:00 ` Bi, Dandan
1 sibling, 0 replies; 15+ messages in thread
From: Laszlo Ersek @ 2018-05-24 13:09 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: edk2-devel@lists.01.org, Leif Lindholm, Michael D Kinney,
Liming Gao, Star Zeng, Eric Dong, Dandan Bi
On 05/24/18 14:54, Ard Biesheuvel wrote:
> 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.
Thanks! With that:
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
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_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));
>>>
>>>
>>
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH v2 4/5] MdeModulePkg/DxeCorePerformanceLib: use AllocatePeiAccessiblePages
2018-05-24 12:54 ` Ard Biesheuvel
2018-05-24 13:09 ` Laszlo Ersek
@ 2018-05-25 2:00 ` Bi, Dandan
2018-05-25 2:44 ` Zeng, Star
1 sibling, 1 reply; 15+ messages in thread
From: Bi, Dandan @ 2018-05-25 2:00 UTC (permalink / raw)
To: Ard Biesheuvel, Laszlo Ersek
Cc: Dong, Eric, edk2-devel@lists.01.org, Gao, Liming, Leif Lindholm,
Kinney, Michael D, Zeng, Star
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-bounces@lists.01.org] On Behalf Of Ard Biesheuvel
Sent: Thursday, May 24, 2018 8:54 PM
To: Laszlo Ersek <lersek@redhat.com>
Cc: Dong, Eric <eric.dong@intel.com>; edk2-devel@lists.01.org; Gao, Liming <liming.gao@intel.com>; Bi, Dandan <dandan.bi@intel.com>; Leif Lindholm <leif.lindholm@linaro.org>; Kinney, Michael D <michael.d.kinney@intel.com>; Zeng, Star <star.zeng@intel.com>
Subject: Re: [edk2] [PATCH v2 4/5] MdeModulePkg/DxeCorePerformanceLib: use AllocatePeiAccessiblePages
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/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_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));
>>
>>
>
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH v2 4/5] MdeModulePkg/DxeCorePerformanceLib: use AllocatePeiAccessiblePages
2018-05-25 2:00 ` Bi, Dandan
@ 2018-05-25 2:44 ` Zeng, Star
2018-05-25 3:07 ` Bi, Dandan
0 siblings, 1 reply; 15+ messages in thread
From: Zeng, Star @ 2018-05-25 2:44 UTC (permalink / raw)
To: Bi, Dandan, Ard Biesheuvel, Laszlo Ersek
Cc: Dong, Eric, edk2-devel@lists.01.org, Gao, Liming, Leif Lindholm,
Kinney, Michael D, 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.biesheuvel@linaro.org>; Laszlo Ersek <lersek@redhat.com>
Cc: Dong, Eric <eric.dong@intel.com>; edk2-devel@lists.01.org; Gao, Liming <liming.gao@intel.com>; Leif Lindholm <leif.lindholm@linaro.org>; Kinney, Michael D <michael.d.kinney@intel.com>; Zeng, Star <star.zeng@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-bounces@lists.01.org] On Behalf Of Ard Biesheuvel
Sent: Thursday, May 24, 2018 8:54 PM
To: Laszlo Ersek <lersek@redhat.com>
Cc: Dong, Eric <eric.dong@intel.com>; edk2-devel@lists.01.org; Gao, Liming <liming.gao@intel.com>; Bi, Dandan <dandan.bi@intel.com>; Leif Lindholm <leif.lindholm@linaro.org>; Kinney, Michael D <michael.d.kinney@intel.com>; Zeng, Star <star.zeng@intel.com>
Subject: Re: [edk2] [PATCH v2 4/5] MdeModulePkg/DxeCorePerformanceLib: use AllocatePeiAccessiblePages
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/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_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));
>>
>>
>
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH v2 4/5] MdeModulePkg/DxeCorePerformanceLib: use AllocatePeiAccessiblePages
2018-05-25 2:44 ` Zeng, Star
@ 2018-05-25 3:07 ` Bi, Dandan
0 siblings, 0 replies; 15+ messages in thread
From: Bi, Dandan @ 2018-05-25 3:07 UTC (permalink / raw)
To: Zeng, Star, Ard Biesheuvel, Laszlo Ersek
Cc: Dong, Eric, edk2-devel@lists.01.org, Gao, Liming, Leif Lindholm,
Kinney, Michael D
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.bi@intel.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org>; Laszlo Ersek <lersek@redhat.com>
Cc: Dong, Eric <eric.dong@intel.com>; edk2-devel@lists.01.org; Gao, Liming <liming.gao@intel.com>; Leif Lindholm <leif.lindholm@linaro.org>; Kinney, Michael D <michael.d.kinney@intel.com>; Zeng, Star <star.zeng@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.biesheuvel@linaro.org>; Laszlo Ersek <lersek@redhat.com>
Cc: Dong, Eric <eric.dong@intel.com>; edk2-devel@lists.01.org; Gao, Liming <liming.gao@intel.com>; Leif Lindholm <leif.lindholm@linaro.org>; Kinney, Michael D <michael.d.kinney@intel.com>; Zeng, Star <star.zeng@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-bounces@lists.01.org] On Behalf Of Ard Biesheuvel
Sent: Thursday, May 24, 2018 8:54 PM
To: Laszlo Ersek <lersek@redhat.com>
Cc: Dong, Eric <eric.dong@intel.com>; edk2-devel@lists.01.org; Gao, Liming <liming.gao@intel.com>; Bi, Dandan <dandan.bi@intel.com>; Leif Lindholm <leif.lindholm@linaro.org>; Kinney, Michael D <michael.d.kinney@intel.com>; Zeng, Star <star.zeng@intel.com>
Subject: Re: [edk2] [PATCH v2 4/5] MdeModulePkg/DxeCorePerformanceLib: use AllocatePeiAccessiblePages
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/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_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));
>>
>>
>
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v2 5/5] MdeModulePkg/FirmwarePerformanceDataTableDxe: use AllocatePeiAccessiblePages
2018-05-24 9:09 [PATCH v2 0/5] Abstract allocation of PEI accessible memory Ard Biesheuvel
` (3 preceding siblings ...)
2018-05-24 9:09 ` [PATCH v2 4/5] MdeModulePkg/DxeCorePerformanceLib: use AllocatePeiAccessiblePages Ard Biesheuvel
@ 2018-05-24 9:09 ` Ard Biesheuvel
2018-05-24 12:53 ` Laszlo Ersek
2018-05-24 16:57 ` [PATCH v2 0/5] Abstract allocation of PEI accessible memory Kinney, Michael D
5 siblings, 1 reply; 15+ messages in thread
From: Ard Biesheuvel @ 2018-05-24 9:09 UTC (permalink / raw)
To: edk2-devel
Cc: Ard Biesheuvel, Laszlo Ersek, Leif Lindholm, Michael D Kinney,
Liming Gao, Star Zeng, Eric Dong, Dandan Bi
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 in both cases, the subsequent CopyMem() call supersedes it immediately.
MdeModulePkg/Universal/Acpi/FirmwarePerformanceDataTableDxe/FirmwarePerformanceDxe.c | 51 ++++----------------
MdeModulePkg/Universal/Acpi/FirmwarePerformanceDataTableDxe/FirmwarePerformanceDxe.inf | 1 +
2 files changed, 10 insertions(+), 42 deletions(-)
diff --git a/MdeModulePkg/Universal/Acpi/FirmwarePerformanceDataTableDxe/FirmwarePerformanceDxe.c b/MdeModulePkg/Universal/Acpi/FirmwarePerformanceDataTableDxe/FirmwarePerformanceDxe.c
index e719e9e482cb..ded817f37301 100644
--- a/MdeModulePkg/Universal/Acpi/FirmwarePerformanceDataTableDxe/FirmwarePerformanceDxe.c
+++ b/MdeModulePkg/Universal/Acpi/FirmwarePerformanceDataTableDxe/FirmwarePerformanceDxe.c
@@ -32,6 +32,7 @@
#include <Library/UefiRuntimeServicesTableLib.h>
#include <Library/BaseLib.h>
#include <Library/DebugLib.h>
+#include <Library/DxeServicesLib.h>
#include <Library/TimerLib.h>
#include <Library/BaseMemoryLib.h>
#include <Library/MemoryAllocationLib.h>
@@ -179,46 +180,6 @@ FpdtAcpiTableChecksum (
Buffer[ChecksumOffset] = CalculateCheckSum8 (Buffer, Size);
}
-/**
- 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;
-}
-
/**
Callback function upon VariableArchProtocol and LockBoxProtocol
to allocate S3 performance table memory and save the pointer to LockBox.
@@ -287,7 +248,10 @@ FpdtAllocateS3PerformanceTableMemory (
//
// Fail to allocate at specified address, continue to allocate at any address.
//
- mAcpiS3PerformanceTable = (S3_PERFORMANCE_TABLE *) FpdtAllocateReservedMemoryBelow4G (sizeof (S3_PERFORMANCE_TABLE));
+ mAcpiS3PerformanceTable = (S3_PERFORMANCE_TABLE *) AllocatePeiAccessiblePages (
+ EfiReservedMemoryType,
+ EFI_SIZE_TO_PAGES (sizeof (S3_PERFORMANCE_TABLE))
+ );
}
DEBUG ((EFI_D_INFO, "FPDT: ACPI S3 Performance Table address = 0x%x\n", mAcpiS3PerformanceTable));
if (mAcpiS3PerformanceTable != NULL) {
@@ -368,7 +332,10 @@ InstallFirmwarePerformanceDataTable (
//
// 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, "FPDT: ACPI Boot Performance Table address = 0x%x\n", mAcpiBootPerformanceTable));
if (mAcpiBootPerformanceTable == NULL) {
diff --git a/MdeModulePkg/Universal/Acpi/FirmwarePerformanceDataTableDxe/FirmwarePerformanceDxe.inf b/MdeModulePkg/Universal/Acpi/FirmwarePerformanceDataTableDxe/FirmwarePerformanceDxe.inf
index 8757bbd0aaa9..3d2dd6eb732f 100644
--- a/MdeModulePkg/Universal/Acpi/FirmwarePerformanceDataTableDxe/FirmwarePerformanceDxe.inf
+++ b/MdeModulePkg/Universal/Acpi/FirmwarePerformanceDataTableDxe/FirmwarePerformanceDxe.inf
@@ -44,6 +44,7 @@ [LibraryClasses]
UefiRuntimeServicesTableLib
BaseLib
DebugLib
+ DxeServicesLib
TimerLib
BaseMemoryLib
MemoryAllocationLib
--
2.17.0
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [PATCH v2 5/5] MdeModulePkg/FirmwarePerformanceDataTableDxe: use AllocatePeiAccessiblePages
2018-05-24 9:09 ` [PATCH v2 5/5] MdeModulePkg/FirmwarePerformanceDataTableDxe: " Ard Biesheuvel
@ 2018-05-24 12:53 ` Laszlo Ersek
0 siblings, 0 replies; 15+ messages in thread
From: Laszlo Ersek @ 2018-05-24 12:53 UTC (permalink / raw)
To: Ard Biesheuvel, edk2-devel
Cc: Leif Lindholm, Michael D Kinney, Liming Gao, Star Zeng, Eric Dong,
Dandan Bi
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 in both cases, the subsequent CopyMem() call supersedes it immediately.
>
> MdeModulePkg/Universal/Acpi/FirmwarePerformanceDataTableDxe/FirmwarePerformanceDxe.c | 51 ++++----------------
> MdeModulePkg/Universal/Acpi/FirmwarePerformanceDataTableDxe/FirmwarePerformanceDxe.inf | 1 +
> 2 files changed, 10 insertions(+), 42 deletions(-)
>
> diff --git a/MdeModulePkg/Universal/Acpi/FirmwarePerformanceDataTableDxe/FirmwarePerformanceDxe.c b/MdeModulePkg/Universal/Acpi/FirmwarePerformanceDataTableDxe/FirmwarePerformanceDxe.c
> index e719e9e482cb..ded817f37301 100644
> --- a/MdeModulePkg/Universal/Acpi/FirmwarePerformanceDataTableDxe/FirmwarePerformanceDxe.c
> +++ b/MdeModulePkg/Universal/Acpi/FirmwarePerformanceDataTableDxe/FirmwarePerformanceDxe.c
> @@ -32,6 +32,7 @@
> #include <Library/UefiRuntimeServicesTableLib.h>
> #include <Library/BaseLib.h>
> #include <Library/DebugLib.h>
> +#include <Library/DxeServicesLib.h>
> #include <Library/TimerLib.h>
> #include <Library/BaseMemoryLib.h>
> #include <Library/MemoryAllocationLib.h>
> @@ -179,46 +180,6 @@ FpdtAcpiTableChecksum (
> Buffer[ChecksumOffset] = CalculateCheckSum8 (Buffer, Size);
> }
>
> -/**
> - 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;
> -}
> -
> /**
> Callback function upon VariableArchProtocol and LockBoxProtocol
> to allocate S3 performance table memory and save the pointer to LockBox.
> @@ -287,7 +248,10 @@ FpdtAllocateS3PerformanceTableMemory (
> //
> // Fail to allocate at specified address, continue to allocate at any address.
> //
> - mAcpiS3PerformanceTable = (S3_PERFORMANCE_TABLE *) FpdtAllocateReservedMemoryBelow4G (sizeof (S3_PERFORMANCE_TABLE));
> + mAcpiS3PerformanceTable = (S3_PERFORMANCE_TABLE *) AllocatePeiAccessiblePages (
> + EfiReservedMemoryType,
> + EFI_SIZE_TO_PAGES (sizeof (S3_PERFORMANCE_TABLE))
> + );
> }
> DEBUG ((EFI_D_INFO, "FPDT: ACPI S3 Performance Table address = 0x%x\n", mAcpiS3PerformanceTable));
> if (mAcpiS3PerformanceTable != NULL) {
> @@ -368,7 +332,10 @@ InstallFirmwarePerformanceDataTable (
> //
> // 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, "FPDT: ACPI Boot Performance Table address = 0x%x\n", mAcpiBootPerformanceTable));
> if (mAcpiBootPerformanceTable == NULL) {
> diff --git a/MdeModulePkg/Universal/Acpi/FirmwarePerformanceDataTableDxe/FirmwarePerformanceDxe.inf b/MdeModulePkg/Universal/Acpi/FirmwarePerformanceDataTableDxe/FirmwarePerformanceDxe.inf
> index 8757bbd0aaa9..3d2dd6eb732f 100644
> --- a/MdeModulePkg/Universal/Acpi/FirmwarePerformanceDataTableDxe/FirmwarePerformanceDxe.inf
> +++ b/MdeModulePkg/Universal/Acpi/FirmwarePerformanceDataTableDxe/FirmwarePerformanceDxe.inf
> @@ -44,6 +44,7 @@ [LibraryClasses]
> UefiRuntimeServicesTableLib
> BaseLib
> DebugLib
> + DxeServicesLib
> TimerLib
> BaseMemoryLib
> MemoryAllocationLib
>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 0/5] Abstract allocation of PEI accessible memory
2018-05-24 9:09 [PATCH v2 0/5] Abstract allocation of PEI accessible memory Ard Biesheuvel
` (4 preceding siblings ...)
2018-05-24 9:09 ` [PATCH v2 5/5] MdeModulePkg/FirmwarePerformanceDataTableDxe: " Ard Biesheuvel
@ 2018-05-24 16:57 ` Kinney, Michael D
5 siblings, 0 replies; 15+ messages in thread
From: Kinney, Michael D @ 2018-05-24 16:57 UTC (permalink / raw)
To: Ard Biesheuvel, edk2-devel@lists.01.org, Kinney, Michael D
Cc: Laszlo Ersek, Leif Lindholm, Gao, Liming, Zeng, Star, Dong, Eric,
Bi, Dandan
Ard,
Thanks for the update. This looks really close.
One comment is on the use of #ifdef MDE_CPU_X64
in a C file.
In general, the #ifdef for a specific type of CPU
should be limited to .h files and we should split
out CPU specific sources into their own source
files and use the INF file [Sources.<arch>] to
list the CPU specific source files.
Only other minor comment is that the summary still
mentions adding the new API to the UefiLib.
Mike
> -----Original Message-----
> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
> Sent: Thursday, May 24, 2018 2:10 AM
> To: edk2-devel@lists.01.org
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>; Laszlo
> Ersek <lersek@redhat.com>; Leif Lindholm
> <leif.lindholm@linaro.org>; Kinney, Michael D
> <michael.d.kinney@intel.com>; Gao, Liming
> <liming.gao@intel.com>; Zeng, Star
> <star.zeng@intel.com>; Dong, Eric <eric.dong@intel.com>;
> Bi, Dandan <dandan.bi@intel.com>
> Subject: [PATCH v2 0/5] Abstract allocation of PEI
> accessible memory
>
> At the moment, FirmwarePerformanceTableDataDxe or
> DxeCorePerformanceLib
> are unusable on systems such as AMD Seattle, because
> they unconditionally
> attempt to allocate memory below 4 GB, and ASSERT() if
> this fails. On AMD
> Seattle, no 32-bit addressable DRAM exists, and so the
> driver will always
> assert, and crash a running DEBUG build.
>
> The reason for this is that some platforms (i.e., X64
> builds consisting of
> a 32-bit PEI stage and 64-bit remaining stages) require
> these allocations
> to be below 4 GB, and for some reason, open coding this
> practice throughout
> the code without regard for other architectures has been
> regarded as an
> acceptable approach.
>
> Instead, I would like to propose the approach
> implemented in this series:
> - create an abstracted EfiAllocatePeiAccessiblePages()
> routine in UefiLib
> that simply allocates pages from any region on all
> architectures except
> X64, and allocate below 4 GB for X64
> - update the various call sites with calls to this new
> function.
>
> The above is implemented in patches #3 - #6. Patches #1
> and #2 are fixes
> for issues that I observed in ArmVirtPkg and OvmfPkg
> while working on
> these patches.
>
> Code can be found here:
> https://github.com/ardbiesheuvel/edk2/tree/allocate-pei-
> v2
>
> Changes since v1:
> - add Laszlo's ack to #1 - #2
> - move EfiAllocatePeiPages() to DxeServicesLib and drop
> Efi prefix
> - update implementation to take EfiFreeMemoryTop into
> account when built
> for X64
>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Leif Lindholm <leif.lindholm@linaro.org>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Cc: Liming Gao <liming.gao@intel.com>
> Cc: Star Zeng <star.zeng@intel.com>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Dandan Bi <dandan.bi@intel.com>
>
> Ard Biesheuvel (5):
> OvmfPkg/PlatformBootManagerLib: add missing report
> status code call
> ArmVirtPkg/PlatformBootManagerLib: add missing report
> status code call
> MdePkg/DxeServicesLib: introduce
> AllocatePeiAccessiblePages routine
> MdeModulePkg/DxeCorePerformanceLib: use
> AllocatePeiAccessiblePages
> MdeModulePkg/FirmwarePerformanceDataTableDxe: use
> AllocatePeiAccessiblePages
>
> .../PlatformBootManagerLib.inf | 1 +
> .../PlatformBootManagerLib/QemuKernel.c | 4 ++
> .../DxeCorePerformanceLib.c | 45 ++--
> -----------
> .../FirmwarePerformanceDxe.c | 51 +++-
> -------------
> .../FirmwarePerformanceDxe.inf | 1 +
> MdePkg/Include/Library/DxeServicesLib.h | 23
> +++++++-
> .../Library/DxeServicesLib/DxeServicesLib.c | 55
> +++++++++++++++++++
> .../Library/DxeServicesLib/DxeServicesLib.inf | 3 +
> .../PlatformBootManagerLib.inf | 1 +
> .../PlatformBootManagerLib/QemuKernel.c | 4 ++
> 10 files changed, 104 insertions(+), 84 deletions(-)
>
> --
> 2.17.0
^ permalink raw reply [flat|nested] 15+ messages in thread