* [PATCH edk2-platforms 1/4] Platform/ARM: import ARM platform specific BdsLib header
2018-11-22 17:26 [PATCH edk2-platforms 0/4] Platform/ARM: fix DevicePath mishandling in BdsLib Ard Biesheuvel
@ 2018-11-22 17:26 ` Ard Biesheuvel
2018-11-22 17:36 ` Laszlo Ersek
2018-11-22 17:26 ` [PATCH edk2-platforms 2/4] Platform/ARM/BdsLid: drop unused BdsStartEfiApplication () Ard Biesheuvel
` (3 subsequent siblings)
4 siblings, 1 reply; 15+ messages in thread
From: Ard Biesheuvel @ 2018-11-22 17:26 UTC (permalink / raw)
To: edk2-devel
Cc: leif.lindholm, thomas.abraham, nariman.poushin, lersek, philmd,
Ard Biesheuvel
The BdsLib library has been moved into Platform/ARM a while ago,
but the BdsLib.h header was left behind, and so all users in
Platform/ARM are still relying on it to be available in ArmPkg.
So let's add a copy to Platform/ARM and wire it up, so we can
drop it from ArmPkg going forward. Note that the BdsLib
implementation included ArmLib.h from ArmPkg without using
anything it provides, so drop that false dependency as well.
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
Platform/ARM/ARM.dec | 3 +
Platform/ARM/Drivers/FdtPlatformDxe/FdtPlatformDxe.inf | 2 +-
Platform/ARM/Include/Library/BdsLib.h | 212 ++++++++++++++++++++
Platform/ARM/Library/BdsLib/BdsInternal.h | 1 -
Platform/ARM/Library/BdsLib/BdsLib.inf | 2 +-
5 files changed, 217 insertions(+), 3 deletions(-)
diff --git a/Platform/ARM/ARM.dec b/Platform/ARM/ARM.dec
index f9bf3294a0ca..6a6eeb6559fd 100644
--- a/Platform/ARM/ARM.dec
+++ b/Platform/ARM/ARM.dec
@@ -21,5 +21,8 @@
[Includes]
Include # Root include for the package
+[LibraryClasses]
+ BdsLib|Include/Library/BdsLib.h
+
[Guids]
gArmBootMonFsFileInfoGuid = { 0x41e26b9c, 0xada6, 0x45b3, { 0x80, 0x8e, 0x23, 0x57, 0xa3, 0x5b, 0x60, 0xd6 } }
diff --git a/Platform/ARM/Drivers/FdtPlatformDxe/FdtPlatformDxe.inf b/Platform/ARM/Drivers/FdtPlatformDxe/FdtPlatformDxe.inf
index f9a5aee3596e..d4aef4bfce92 100644
--- a/Platform/ARM/Drivers/FdtPlatformDxe/FdtPlatformDxe.inf
+++ b/Platform/ARM/Drivers/FdtPlatformDxe/FdtPlatformDxe.inf
@@ -28,10 +28,10 @@
ShellSetFdt.c
[Packages]
- ArmPkg/ArmPkg.dec
EmbeddedPkg/EmbeddedPkg.dec
MdeModulePkg/MdeModulePkg.dec
MdePkg/MdePkg.dec
+ Platform/ARM/ARM.dec
Platform/ARM/Drivers/FdtPlatformDxe/FdtPlatformDxe.dec
ShellPkg/ShellPkg.dec
diff --git a/Platform/ARM/Include/Library/BdsLib.h b/Platform/ARM/Include/Library/BdsLib.h
new file mode 100644
index 000000000000..4528c2e8739b
--- /dev/null
+++ b/Platform/ARM/Include/Library/BdsLib.h
@@ -0,0 +1,212 @@
+/** @file
+*
+* Copyright (c) 2013-2015, ARM Limited. All rights reserved.
+*
+* This program and the accompanying materials
+* are licensed and made available under the terms and conditions of the BSD License
+* which accompanies this distribution. The full text of the license may be found at
+* http://opensource.org/licenses/bsd-license.php
+*
+* THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
+* WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
+*
+**/
+
+#ifndef __BDS_ENTRY_H__
+#define __BDS_ENTRY_H__
+
+#define IS_DEVICE_PATH_NODE(node,type,subtype) \
+ (((node)->Type == (type)) && ((node)->SubType == (subtype)))
+
+/**
+ This is defined by the UEFI specs, don't change it
+**/
+typedef struct {
+ UINT16 LoadOptionIndex;
+ EFI_LOAD_OPTION *LoadOption;
+ UINTN LoadOptionSize;
+
+ UINT32 Attributes;
+ UINT16 FilePathListLength;
+ CHAR16 *Description;
+ EFI_DEVICE_PATH_PROTOCOL *FilePathList;
+
+ VOID* OptionalData;
+ UINTN OptionalDataSize;
+} BDS_LOAD_OPTION;
+
+/**
+ Connect a Device Path and return the handle of the driver that support this DevicePath
+
+ @param DevicePath Device Path of the File to connect
+ @param Handle Handle of the driver that support this DevicePath
+ @param RemainingDevicePath Remaining DevicePath nodes that do not match the driver DevicePath
+
+ @retval EFI_SUCCESS A driver that matches the Device Path has been found
+ @retval EFI_NOT_FOUND No handles match the search.
+ @retval EFI_INVALID_PARAMETER DevicePath or Handle is NULL
+
+**/
+EFI_STATUS
+BdsConnectDevicePath (
+ IN EFI_DEVICE_PATH_PROTOCOL* DevicePath,
+ OUT EFI_HANDLE *Handle,
+ OUT EFI_DEVICE_PATH_PROTOCOL **RemainingDevicePath
+ );
+
+/**
+ Connect all DXE drivers
+
+ @retval EFI_SUCCESS All drivers have been connected
+ @retval EFI_NOT_FOUND No handles match the search.
+ @retval EFI_OUT_OF_RESOURCES There is not resource pool memory to store the matching results.
+
+**/
+EFI_STATUS
+BdsConnectAllDrivers (
+ VOID
+ );
+
+/**
+ Return the value of a global variable defined by its VariableName.
+ The variable must be defined with the VendorGuid gEfiGlobalVariableGuid.
+
+ @param VariableName A Null-terminated string that is the name of the vendor's
+ variable.
+ @param DefaultValue Value returned by the function if the variable does not exist
+ @param DataSize On input, the size in bytes of the return Data buffer.
+ On output the size of data returned in Data.
+ @param Value Value read from the UEFI Variable or copy of the default value
+ if the UEFI Variable does not exist
+
+ @retval EFI_SUCCESS All drivers have been connected
+ @retval EFI_NOT_FOUND No handles match the search.
+ @retval EFI_OUT_OF_RESOURCES There is not resource pool memory to store the matching results.
+
+**/
+EFI_STATUS
+GetGlobalEnvironmentVariable (
+ IN CONST CHAR16* VariableName,
+ IN VOID* DefaultValue,
+ IN OUT UINTN* Size,
+ OUT VOID** Value
+ );
+
+/**
+ Return the value of the variable defined by its VariableName and VendorGuid
+
+ @param VariableName A Null-terminated string that is the name of the vendor's
+ variable.
+ @param VendorGuid A unique identifier for the vendor.
+ @param DefaultValue Value returned by the function if the variable does not exist
+ @param DataSize On input, the size in bytes of the return Data buffer.
+ On output the size of data returned in Data.
+ @param Value Value read from the UEFI Variable or copy of the default value
+ if the UEFI Variable does not exist
+
+ @retval EFI_SUCCESS All drivers have been connected
+ @retval EFI_NOT_FOUND No handles match the search.
+ @retval EFI_OUT_OF_RESOURCES There is not resource pool memory to store the matching results.
+
+**/
+EFI_STATUS
+GetEnvironmentVariable (
+ IN CONST CHAR16* VariableName,
+ IN EFI_GUID* VendorGuid,
+ IN VOID* DefaultValue,
+ IN OUT UINTN* Size,
+ OUT VOID** Value
+ );
+
+EFI_STATUS
+BootOptionFromLoadOptionIndex (
+ IN UINT16 LoadOptionIndex,
+ OUT BDS_LOAD_OPTION** BdsLoadOption
+ );
+
+EFI_STATUS
+BootOptionFromLoadOptionVariable (
+ IN CHAR16* BootVariableName,
+ OUT BDS_LOAD_OPTION** BdsLoadOption
+ );
+
+EFI_STATUS
+BootOptionToLoadOptionVariable (
+ IN BDS_LOAD_OPTION* BdsLoadOption
+ );
+
+UINT16
+BootOptionAllocateBootIndex (
+ VOID
+ );
+
+/**
+ Start an EFI Application from a Device Path
+
+ @param ParentImageHandle Handle of the calling image
+ @param DevicePath Location of the EFI Application
+
+ @retval EFI_SUCCESS All drivers have been connected
+ @retval EFI_NOT_FOUND The Linux kernel Device Path has not been found
+ @retval EFI_OUT_OF_RESOURCES There is not enough resource memory to store the matching results.
+
+**/
+EFI_STATUS
+BdsStartEfiApplication (
+ IN EFI_HANDLE ParentImageHandle,
+ IN EFI_DEVICE_PATH_PROTOCOL *DevicePath,
+ IN UINTN LoadOptionsSize,
+ IN VOID* LoadOptions
+ );
+
+EFI_STATUS
+BdsLoadImage (
+ IN EFI_DEVICE_PATH *DevicePath,
+ IN EFI_ALLOCATE_TYPE Type,
+ IN OUT EFI_PHYSICAL_ADDRESS* Image,
+ OUT UINTN *FileSize
+ );
+
+/**
+ * Call BS.ExitBootServices with the appropriate Memory Map information
+ */
+EFI_STATUS
+ShutdownUefiBootServices (
+ VOID
+ );
+
+/**
+ Locate an EFI application in a the Firmware Volumes by its name
+
+ @param EfiAppGuid Guid of the EFI Application into the Firmware Volume
+ @param DevicePath EFI Device Path of the EFI application
+
+ @return EFI_SUCCESS The function completed successfully.
+ @return EFI_NOT_FOUND The protocol could not be located.
+ @return EFI_OUT_OF_RESOURCES There are not enough resources to find the protocol.
+
+**/
+EFI_STATUS
+LocateEfiApplicationInFvByName (
+ IN CONST CHAR16* EfiAppName,
+ OUT EFI_DEVICE_PATH **DevicePath
+ );
+
+/**
+ Locate an EFI application in a the Firmware Volumes by its GUID
+
+ @param EfiAppGuid Guid of the EFI Application into the Firmware Volume
+ @param DevicePath EFI Device Path of the EFI application
+
+ @return EFI_SUCCESS The function completed successfully.
+ @return EFI_NOT_FOUND The protocol could not be located.
+ @return EFI_OUT_OF_RESOURCES There are not enough resources to find the protocol.
+
+**/
+EFI_STATUS
+LocateEfiApplicationInFvByGuid (
+ IN CONST EFI_GUID *EfiAppGuid,
+ OUT EFI_DEVICE_PATH **DevicePath
+ );
+
+#endif
diff --git a/Platform/ARM/Library/BdsLib/BdsInternal.h b/Platform/ARM/Library/BdsLib/BdsInternal.h
index f70aae603d69..bb4566e3a6c4 100644
--- a/Platform/ARM/Library/BdsLib/BdsInternal.h
+++ b/Platform/ARM/Library/BdsLib/BdsInternal.h
@@ -16,7 +16,6 @@
#define __BDS_INTERNAL_H__
#include <PiDxe.h>
-#include <Library/ArmLib.h>
#include <Library/BaseLib.h>
#include <Library/BaseMemoryLib.h>
#include <Library/DxeServicesTableLib.h>
diff --git a/Platform/ARM/Library/BdsLib/BdsLib.inf b/Platform/ARM/Library/BdsLib/BdsLib.inf
index 96c1d6e7e200..637ef5a08128 100644
--- a/Platform/ARM/Library/BdsLib/BdsLib.inf
+++ b/Platform/ARM/Library/BdsLib/BdsLib.inf
@@ -27,10 +27,10 @@
BdsLoadOption.c
[Packages]
- ArmPkg/ArmPkg.dec
EmbeddedPkg/EmbeddedPkg.dec
MdeModulePkg/MdeModulePkg.dec
MdePkg/MdePkg.dec
+ Platform/ARM/ARM.dec
[LibraryClasses]
ArmLib
--
2.17.1
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH edk2-platforms 1/4] Platform/ARM: import ARM platform specific BdsLib header
2018-11-22 17:26 ` [PATCH edk2-platforms 1/4] Platform/ARM: import ARM platform specific BdsLib header Ard Biesheuvel
@ 2018-11-22 17:36 ` Laszlo Ersek
0 siblings, 0 replies; 15+ messages in thread
From: Laszlo Ersek @ 2018-11-22 17:36 UTC (permalink / raw)
To: Ard Biesheuvel, edk2-devel
Cc: leif.lindholm, thomas.abraham, nariman.poushin, philmd
On 11/22/18 18:26, Ard Biesheuvel wrote:
> The BdsLib library has been moved into Platform/ARM a while ago,
> but the BdsLib.h header was left behind, and so all users in
> Platform/ARM are still relying on it to be available in ArmPkg.
>
> So let's add a copy to Platform/ARM and wire it up, so we can
> drop it from ArmPkg going forward. Note that the BdsLib
> implementation included ArmLib.h from ArmPkg without using
> anything it provides, so drop that false dependency as well.
>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
> Platform/ARM/ARM.dec | 3 +
> Platform/ARM/Drivers/FdtPlatformDxe/FdtPlatformDxe.inf | 2 +-
> Platform/ARM/Include/Library/BdsLib.h | 212 ++++++++++++++++++++
> Platform/ARM/Library/BdsLib/BdsInternal.h | 1 -
> Platform/ARM/Library/BdsLib/BdsLib.inf | 2 +-
> 5 files changed, 217 insertions(+), 3 deletions(-)
>
> diff --git a/Platform/ARM/ARM.dec b/Platform/ARM/ARM.dec
> index f9bf3294a0ca..6a6eeb6559fd 100644
> --- a/Platform/ARM/ARM.dec
> +++ b/Platform/ARM/ARM.dec
> @@ -21,5 +21,8 @@
> [Includes]
> Include # Root include for the package
>
> +[LibraryClasses]
> + BdsLib|Include/Library/BdsLib.h
> +
> [Guids]
> gArmBootMonFsFileInfoGuid = { 0x41e26b9c, 0xada6, 0x45b3, { 0x80, 0x8e, 0x23, 0x57, 0xa3, 0x5b, 0x60, 0xd6 } }
> diff --git a/Platform/ARM/Drivers/FdtPlatformDxe/FdtPlatformDxe.inf b/Platform/ARM/Drivers/FdtPlatformDxe/FdtPlatformDxe.inf
> index f9a5aee3596e..d4aef4bfce92 100644
> --- a/Platform/ARM/Drivers/FdtPlatformDxe/FdtPlatformDxe.inf
> +++ b/Platform/ARM/Drivers/FdtPlatformDxe/FdtPlatformDxe.inf
> @@ -28,10 +28,10 @@
> ShellSetFdt.c
>
> [Packages]
> - ArmPkg/ArmPkg.dec
> EmbeddedPkg/EmbeddedPkg.dec
> MdeModulePkg/MdeModulePkg.dec
> MdePkg/MdePkg.dec
> + Platform/ARM/ARM.dec
> Platform/ARM/Drivers/FdtPlatformDxe/FdtPlatformDxe.dec
> ShellPkg/ShellPkg.dec
>
> diff --git a/Platform/ARM/Include/Library/BdsLib.h b/Platform/ARM/Include/Library/BdsLib.h
> new file mode 100644
> index 000000000000..4528c2e8739b
> --- /dev/null
> +++ b/Platform/ARM/Include/Library/BdsLib.h
> @@ -0,0 +1,212 @@
> +/** @file
> +*
> +* Copyright (c) 2013-2015, ARM Limited. All rights reserved.
> +*
> +* This program and the accompanying materials
> +* are licensed and made available under the terms and conditions of the BSD License
> +* which accompanies this distribution. The full text of the license may be found at
> +* http://opensource.org/licenses/bsd-license.php
> +*
> +* THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
> +* WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
> +*
> +**/
> +
> +#ifndef __BDS_ENTRY_H__
> +#define __BDS_ENTRY_H__
> +
> +#define IS_DEVICE_PATH_NODE(node,type,subtype) \
> + (((node)->Type == (type)) && ((node)->SubType == (subtype)))
> +
> +/**
> + This is defined by the UEFI specs, don't change it
> +**/
> +typedef struct {
> + UINT16 LoadOptionIndex;
> + EFI_LOAD_OPTION *LoadOption;
> + UINTN LoadOptionSize;
> +
> + UINT32 Attributes;
> + UINT16 FilePathListLength;
> + CHAR16 *Description;
> + EFI_DEVICE_PATH_PROTOCOL *FilePathList;
> +
> + VOID* OptionalData;
> + UINTN OptionalDataSize;
> +} BDS_LOAD_OPTION;
> +
> +/**
> + Connect a Device Path and return the handle of the driver that support this DevicePath
> +
> + @param DevicePath Device Path of the File to connect
> + @param Handle Handle of the driver that support this DevicePath
> + @param RemainingDevicePath Remaining DevicePath nodes that do not match the driver DevicePath
> +
> + @retval EFI_SUCCESS A driver that matches the Device Path has been found
> + @retval EFI_NOT_FOUND No handles match the search.
> + @retval EFI_INVALID_PARAMETER DevicePath or Handle is NULL
> +
> +**/
> +EFI_STATUS
> +BdsConnectDevicePath (
> + IN EFI_DEVICE_PATH_PROTOCOL* DevicePath,
> + OUT EFI_HANDLE *Handle,
> + OUT EFI_DEVICE_PATH_PROTOCOL **RemainingDevicePath
> + );
> +
> +/**
> + Connect all DXE drivers
> +
> + @retval EFI_SUCCESS All drivers have been connected
> + @retval EFI_NOT_FOUND No handles match the search.
> + @retval EFI_OUT_OF_RESOURCES There is not resource pool memory to store the matching results.
> +
> +**/
> +EFI_STATUS
> +BdsConnectAllDrivers (
> + VOID
> + );
> +
> +/**
> + Return the value of a global variable defined by its VariableName.
> + The variable must be defined with the VendorGuid gEfiGlobalVariableGuid.
> +
> + @param VariableName A Null-terminated string that is the name of the vendor's
> + variable.
> + @param DefaultValue Value returned by the function if the variable does not exist
> + @param DataSize On input, the size in bytes of the return Data buffer.
> + On output the size of data returned in Data.
> + @param Value Value read from the UEFI Variable or copy of the default value
> + if the UEFI Variable does not exist
> +
> + @retval EFI_SUCCESS All drivers have been connected
> + @retval EFI_NOT_FOUND No handles match the search.
> + @retval EFI_OUT_OF_RESOURCES There is not resource pool memory to store the matching results.
> +
> +**/
> +EFI_STATUS
> +GetGlobalEnvironmentVariable (
> + IN CONST CHAR16* VariableName,
> + IN VOID* DefaultValue,
> + IN OUT UINTN* Size,
> + OUT VOID** Value
> + );
> +
> +/**
> + Return the value of the variable defined by its VariableName and VendorGuid
> +
> + @param VariableName A Null-terminated string that is the name of the vendor's
> + variable.
> + @param VendorGuid A unique identifier for the vendor.
> + @param DefaultValue Value returned by the function if the variable does not exist
> + @param DataSize On input, the size in bytes of the return Data buffer.
> + On output the size of data returned in Data.
> + @param Value Value read from the UEFI Variable or copy of the default value
> + if the UEFI Variable does not exist
> +
> + @retval EFI_SUCCESS All drivers have been connected
> + @retval EFI_NOT_FOUND No handles match the search.
> + @retval EFI_OUT_OF_RESOURCES There is not resource pool memory to store the matching results.
> +
> +**/
> +EFI_STATUS
> +GetEnvironmentVariable (
> + IN CONST CHAR16* VariableName,
> + IN EFI_GUID* VendorGuid,
> + IN VOID* DefaultValue,
> + IN OUT UINTN* Size,
> + OUT VOID** Value
> + );
> +
> +EFI_STATUS
> +BootOptionFromLoadOptionIndex (
> + IN UINT16 LoadOptionIndex,
> + OUT BDS_LOAD_OPTION** BdsLoadOption
> + );
> +
> +EFI_STATUS
> +BootOptionFromLoadOptionVariable (
> + IN CHAR16* BootVariableName,
> + OUT BDS_LOAD_OPTION** BdsLoadOption
> + );
> +
> +EFI_STATUS
> +BootOptionToLoadOptionVariable (
> + IN BDS_LOAD_OPTION* BdsLoadOption
> + );
> +
> +UINT16
> +BootOptionAllocateBootIndex (
> + VOID
> + );
> +
> +/**
> + Start an EFI Application from a Device Path
> +
> + @param ParentImageHandle Handle of the calling image
> + @param DevicePath Location of the EFI Application
> +
> + @retval EFI_SUCCESS All drivers have been connected
> + @retval EFI_NOT_FOUND The Linux kernel Device Path has not been found
> + @retval EFI_OUT_OF_RESOURCES There is not enough resource memory to store the matching results.
> +
> +**/
> +EFI_STATUS
> +BdsStartEfiApplication (
> + IN EFI_HANDLE ParentImageHandle,
> + IN EFI_DEVICE_PATH_PROTOCOL *DevicePath,
> + IN UINTN LoadOptionsSize,
> + IN VOID* LoadOptions
> + );
> +
> +EFI_STATUS
> +BdsLoadImage (
> + IN EFI_DEVICE_PATH *DevicePath,
> + IN EFI_ALLOCATE_TYPE Type,
> + IN OUT EFI_PHYSICAL_ADDRESS* Image,
> + OUT UINTN *FileSize
> + );
> +
> +/**
> + * Call BS.ExitBootServices with the appropriate Memory Map information
> + */
> +EFI_STATUS
> +ShutdownUefiBootServices (
> + VOID
> + );
> +
> +/**
> + Locate an EFI application in a the Firmware Volumes by its name
> +
> + @param EfiAppGuid Guid of the EFI Application into the Firmware Volume
> + @param DevicePath EFI Device Path of the EFI application
> +
> + @return EFI_SUCCESS The function completed successfully.
> + @return EFI_NOT_FOUND The protocol could not be located.
> + @return EFI_OUT_OF_RESOURCES There are not enough resources to find the protocol.
> +
> +**/
> +EFI_STATUS
> +LocateEfiApplicationInFvByName (
> + IN CONST CHAR16* EfiAppName,
> + OUT EFI_DEVICE_PATH **DevicePath
> + );
> +
> +/**
> + Locate an EFI application in a the Firmware Volumes by its GUID
> +
> + @param EfiAppGuid Guid of the EFI Application into the Firmware Volume
> + @param DevicePath EFI Device Path of the EFI application
> +
> + @return EFI_SUCCESS The function completed successfully.
> + @return EFI_NOT_FOUND The protocol could not be located.
> + @return EFI_OUT_OF_RESOURCES There are not enough resources to find the protocol.
> +
> +**/
> +EFI_STATUS
> +LocateEfiApplicationInFvByGuid (
> + IN CONST EFI_GUID *EfiAppGuid,
> + OUT EFI_DEVICE_PATH **DevicePath
> + );
> +
> +#endif
> diff --git a/Platform/ARM/Library/BdsLib/BdsInternal.h b/Platform/ARM/Library/BdsLib/BdsInternal.h
> index f70aae603d69..bb4566e3a6c4 100644
> --- a/Platform/ARM/Library/BdsLib/BdsInternal.h
> +++ b/Platform/ARM/Library/BdsLib/BdsInternal.h
> @@ -16,7 +16,6 @@
> #define __BDS_INTERNAL_H__
>
> #include <PiDxe.h>
> -#include <Library/ArmLib.h>
> #include <Library/BaseLib.h>
> #include <Library/BaseMemoryLib.h>
> #include <Library/DxeServicesTableLib.h>
> diff --git a/Platform/ARM/Library/BdsLib/BdsLib.inf b/Platform/ARM/Library/BdsLib/BdsLib.inf
> index 96c1d6e7e200..637ef5a08128 100644
> --- a/Platform/ARM/Library/BdsLib/BdsLib.inf
> +++ b/Platform/ARM/Library/BdsLib/BdsLib.inf
> @@ -27,10 +27,10 @@
> BdsLoadOption.c
>
> [Packages]
> - ArmPkg/ArmPkg.dec
> EmbeddedPkg/EmbeddedPkg.dec
> MdeModulePkg/MdeModulePkg.dec
> MdePkg/MdePkg.dec
> + Platform/ARM/ARM.dec
>
> [LibraryClasses]
> ArmLib
>
Seems OK to me.
Acked-by: Laszlo Ersek <lersek@redhat.com>
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH edk2-platforms 2/4] Platform/ARM/BdsLid: drop unused BdsStartEfiApplication ()
2018-11-22 17:26 [PATCH edk2-platforms 0/4] Platform/ARM: fix DevicePath mishandling in BdsLib Ard Biesheuvel
2018-11-22 17:26 ` [PATCH edk2-platforms 1/4] Platform/ARM: import ARM platform specific BdsLib header Ard Biesheuvel
@ 2018-11-22 17:26 ` Ard Biesheuvel
2018-11-22 17:55 ` Laszlo Ersek
2018-11-22 17:26 ` [PATCH edk2-platforms 3/4] Platform/ARM/BdsLib: don't clobber BdsLoadImage() DevicePath IN param Ard Biesheuvel
` (2 subsequent siblings)
4 siblings, 1 reply; 15+ messages in thread
From: Ard Biesheuvel @ 2018-11-22 17:26 UTC (permalink / raw)
To: edk2-devel
Cc: leif.lindholm, thomas.abraham, nariman.poushin, lersek, philmd,
Ard Biesheuvel
To prevent having to fix it, let's drop BdsStartEfiApplication
entirely since it is never used.
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
Platform/ARM/Include/Library/BdsLib.h | 19 -------
Platform/ARM/Library/BdsLib/BdsFilePath.c | 58 --------------------
2 files changed, 77 deletions(-)
diff --git a/Platform/ARM/Include/Library/BdsLib.h b/Platform/ARM/Include/Library/BdsLib.h
index 4528c2e8739b..23f777d4097d 100644
--- a/Platform/ARM/Include/Library/BdsLib.h
+++ b/Platform/ARM/Include/Library/BdsLib.h
@@ -140,25 +140,6 @@ BootOptionAllocateBootIndex (
VOID
);
-/**
- Start an EFI Application from a Device Path
-
- @param ParentImageHandle Handle of the calling image
- @param DevicePath Location of the EFI Application
-
- @retval EFI_SUCCESS All drivers have been connected
- @retval EFI_NOT_FOUND The Linux kernel Device Path has not been found
- @retval EFI_OUT_OF_RESOURCES There is not enough resource memory to store the matching results.
-
-**/
-EFI_STATUS
-BdsStartEfiApplication (
- IN EFI_HANDLE ParentImageHandle,
- IN EFI_DEVICE_PATH_PROTOCOL *DevicePath,
- IN UINTN LoadOptionsSize,
- IN VOID* LoadOptions
- );
-
EFI_STATUS
BdsLoadImage (
IN EFI_DEVICE_PATH *DevicePath,
diff --git a/Platform/ARM/Library/BdsLib/BdsFilePath.c b/Platform/ARM/Library/BdsLib/BdsFilePath.c
index 7a4a5052a786..67dafa4f3651 100644
--- a/Platform/ARM/Library/BdsLib/BdsFilePath.c
+++ b/Platform/ARM/Library/BdsLib/BdsFilePath.c
@@ -1353,61 +1353,3 @@ BdsLoadImage (
{
return BdsLoadImageAndUpdateDevicePath (&DevicePath, Type, Image, FileSize);
}
-
-/**
- Start an EFI Application from a Device Path
-
- @param ParentImageHandle Handle of the calling image
- @param DevicePath Location of the EFI Application
-
- @retval EFI_SUCCESS All drivers have been connected
- @retval EFI_NOT_FOUND The Linux kernel Device Path has not been found
- @retval EFI_OUT_OF_RESOURCES There is not enough resource memory to store the matching results.
-
-**/
-EFI_STATUS
-BdsStartEfiApplication (
- IN EFI_HANDLE ParentImageHandle,
- IN EFI_DEVICE_PATH_PROTOCOL *DevicePath,
- IN UINTN LoadOptionsSize,
- IN VOID* LoadOptions
- )
-{
- EFI_STATUS Status;
- EFI_HANDLE ImageHandle;
- EFI_PHYSICAL_ADDRESS BinaryBuffer;
- UINTN BinarySize;
- EFI_LOADED_IMAGE_PROTOCOL* LoadedImage;
-
- // Find the nearest supported file loader
- Status = BdsLoadImageAndUpdateDevicePath (&DevicePath, AllocateAnyPages, &BinaryBuffer, &BinarySize);
- if (EFI_ERROR (Status)) {
- return Status;
- }
-
- // Load the image from the Buffer with Boot Services function
- Status = gBS->LoadImage (TRUE, ParentImageHandle, DevicePath, (VOID*)(UINTN)BinaryBuffer, BinarySize, &ImageHandle);
- if (EFI_ERROR (Status)) {
- return Status;
- }
-
- // Passed LoadOptions to the EFI Application
- if (LoadOptionsSize != 0) {
- Status = gBS->HandleProtocol (ImageHandle, &gEfiLoadedImageProtocolGuid, (VOID **) &LoadedImage);
- if (EFI_ERROR (Status)) {
- return Status;
- }
-
- LoadedImage->LoadOptionsSize = LoadOptionsSize;
- LoadedImage->LoadOptions = LoadOptions;
- }
-
- // Before calling the image, enable the Watchdog Timer for the 5 Minute period
- gBS->SetWatchdogTimer (5 * 60, 0x0000, 0x00, NULL);
- // Start the image
- Status = gBS->StartImage (ImageHandle, NULL, NULL);
- // Clear the Watchdog Timer after the image returns
- gBS->SetWatchdogTimer (0x0000, 0x0000, 0x0000, NULL);
-
- return Status;
-}
--
2.17.1
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH edk2-platforms 2/4] Platform/ARM/BdsLid: drop unused BdsStartEfiApplication ()
2018-11-22 17:26 ` [PATCH edk2-platforms 2/4] Platform/ARM/BdsLid: drop unused BdsStartEfiApplication () Ard Biesheuvel
@ 2018-11-22 17:55 ` Laszlo Ersek
0 siblings, 0 replies; 15+ messages in thread
From: Laszlo Ersek @ 2018-11-22 17:55 UTC (permalink / raw)
To: Ard Biesheuvel, edk2-devel
Cc: leif.lindholm, thomas.abraham, nariman.poushin, philmd
On 11/22/18 18:26, Ard Biesheuvel wrote:
> To prevent having to fix it, let's drop BdsStartEfiApplication
> entirely since it is never used.
>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
> Platform/ARM/Include/Library/BdsLib.h | 19 -------
> Platform/ARM/Library/BdsLib/BdsFilePath.c | 58 --------------------
> 2 files changed, 77 deletions(-)
>
> diff --git a/Platform/ARM/Include/Library/BdsLib.h b/Platform/ARM/Include/Library/BdsLib.h
> index 4528c2e8739b..23f777d4097d 100644
> --- a/Platform/ARM/Include/Library/BdsLib.h
> +++ b/Platform/ARM/Include/Library/BdsLib.h
> @@ -140,25 +140,6 @@ BootOptionAllocateBootIndex (
> VOID
> );
>
> -/**
> - Start an EFI Application from a Device Path
> -
> - @param ParentImageHandle Handle of the calling image
> - @param DevicePath Location of the EFI Application
> -
> - @retval EFI_SUCCESS All drivers have been connected
> - @retval EFI_NOT_FOUND The Linux kernel Device Path has not been found
> - @retval EFI_OUT_OF_RESOURCES There is not enough resource memory to store the matching results.
> -
> -**/
> -EFI_STATUS
> -BdsStartEfiApplication (
> - IN EFI_HANDLE ParentImageHandle,
> - IN EFI_DEVICE_PATH_PROTOCOL *DevicePath,
> - IN UINTN LoadOptionsSize,
> - IN VOID* LoadOptions
> - );
> -
> EFI_STATUS
> BdsLoadImage (
> IN EFI_DEVICE_PATH *DevicePath,
> diff --git a/Platform/ARM/Library/BdsLib/BdsFilePath.c b/Platform/ARM/Library/BdsLib/BdsFilePath.c
> index 7a4a5052a786..67dafa4f3651 100644
> --- a/Platform/ARM/Library/BdsLib/BdsFilePath.c
> +++ b/Platform/ARM/Library/BdsLib/BdsFilePath.c
> @@ -1353,61 +1353,3 @@ BdsLoadImage (
> {
> return BdsLoadImageAndUpdateDevicePath (&DevicePath, Type, Image, FileSize);
> }
> -
> -/**
> - Start an EFI Application from a Device Path
> -
> - @param ParentImageHandle Handle of the calling image
> - @param DevicePath Location of the EFI Application
> -
> - @retval EFI_SUCCESS All drivers have been connected
> - @retval EFI_NOT_FOUND The Linux kernel Device Path has not been found
> - @retval EFI_OUT_OF_RESOURCES There is not enough resource memory to store the matching results.
> -
> -**/
> -EFI_STATUS
> -BdsStartEfiApplication (
> - IN EFI_HANDLE ParentImageHandle,
> - IN EFI_DEVICE_PATH_PROTOCOL *DevicePath,
> - IN UINTN LoadOptionsSize,
> - IN VOID* LoadOptions
> - )
> -{
> - EFI_STATUS Status;
> - EFI_HANDLE ImageHandle;
> - EFI_PHYSICAL_ADDRESS BinaryBuffer;
> - UINTN BinarySize;
> - EFI_LOADED_IMAGE_PROTOCOL* LoadedImage;
> -
> - // Find the nearest supported file loader
> - Status = BdsLoadImageAndUpdateDevicePath (&DevicePath, AllocateAnyPages, &BinaryBuffer, &BinarySize);
> - if (EFI_ERROR (Status)) {
> - return Status;
> - }
> -
> - // Load the image from the Buffer with Boot Services function
> - Status = gBS->LoadImage (TRUE, ParentImageHandle, DevicePath, (VOID*)(UINTN)BinaryBuffer, BinarySize, &ImageHandle);
> - if (EFI_ERROR (Status)) {
> - return Status;
> - }
> -
> - // Passed LoadOptions to the EFI Application
> - if (LoadOptionsSize != 0) {
> - Status = gBS->HandleProtocol (ImageHandle, &gEfiLoadedImageProtocolGuid, (VOID **) &LoadedImage);
> - if (EFI_ERROR (Status)) {
> - return Status;
> - }
> -
> - LoadedImage->LoadOptionsSize = LoadOptionsSize;
> - LoadedImage->LoadOptions = LoadOptions;
> - }
> -
> - // Before calling the image, enable the Watchdog Timer for the 5 Minute period
> - gBS->SetWatchdogTimer (5 * 60, 0x0000, 0x00, NULL);
> - // Start the image
> - Status = gBS->StartImage (ImageHandle, NULL, NULL);
> - // Clear the Watchdog Timer after the image returns
> - gBS->SetWatchdogTimer (0x0000, 0x0000, 0x0000, NULL);
> -
> - return Status;
> -}
>
Makes sense; first we import BdsLib.h, then fix it up together with the
(sole) lib instance.
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH edk2-platforms 3/4] Platform/ARM/BdsLib: don't clobber BdsLoadImage() DevicePath IN param
2018-11-22 17:26 [PATCH edk2-platforms 0/4] Platform/ARM: fix DevicePath mishandling in BdsLib Ard Biesheuvel
2018-11-22 17:26 ` [PATCH edk2-platforms 1/4] Platform/ARM: import ARM platform specific BdsLib header Ard Biesheuvel
2018-11-22 17:26 ` [PATCH edk2-platforms 2/4] Platform/ARM/BdsLid: drop unused BdsStartEfiApplication () Ard Biesheuvel
@ 2018-11-22 17:26 ` Ard Biesheuvel
2018-11-22 18:09 ` Laszlo Ersek
2018-11-22 17:26 ` [PATCH edk2-platforms 4/4] Platform/ARM/BdsLib: maintain alignment for DevicePaths Ard Biesheuvel
2018-11-23 4:20 ` [PATCH edk2-platforms 0/4] Platform/ARM: fix DevicePath mishandling in BdsLib Thomas Abraham
4 siblings, 1 reply; 15+ messages in thread
From: Ard Biesheuvel @ 2018-11-22 17:26 UTC (permalink / raw)
To: edk2-devel
Cc: leif.lindholm, thomas.abraham, nariman.poushin, lersek, philmd,
Ard Biesheuvel
BdsLoadImage () is part of the BdsLib library API and is not documented
as modifying its DevicePath argument, but does so nonetheless. So take
a copy instead, and free it after use.
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
Platform/ARM/Library/BdsLib/BdsFilePath.c | 13 ++++++++++++-
1 file changed, 12 insertions(+), 1 deletion(-)
diff --git a/Platform/ARM/Library/BdsLib/BdsFilePath.c b/Platform/ARM/Library/BdsLib/BdsFilePath.c
index 67dafa4f3651..74fdbbee773d 100644
--- a/Platform/ARM/Library/BdsLib/BdsFilePath.c
+++ b/Platform/ARM/Library/BdsLib/BdsFilePath.c
@@ -1351,5 +1351,16 @@ BdsLoadImage (
OUT UINTN *FileSize
)
{
- return BdsLoadImageAndUpdateDevicePath (&DevicePath, Type, Image, FileSize);
+ EFI_DEVICE_PATH *Path;
+ EFI_STATUS Status;
+
+ Path = DuplicateDevicePath (DevicePath);
+ if (Path == NULL) {
+ return EFI_OUT_OF_RESOURCES;
+ }
+
+ Status = BdsLoadImageAndUpdateDevicePath (&Path, Type, Image, FileSize);
+ FreePool (Path);
+
+ return Status;
}
--
2.17.1
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH edk2-platforms 3/4] Platform/ARM/BdsLib: don't clobber BdsLoadImage() DevicePath IN param
2018-11-22 17:26 ` [PATCH edk2-platforms 3/4] Platform/ARM/BdsLib: don't clobber BdsLoadImage() DevicePath IN param Ard Biesheuvel
@ 2018-11-22 18:09 ` Laszlo Ersek
2018-11-22 18:14 ` Ard Biesheuvel
0 siblings, 1 reply; 15+ messages in thread
From: Laszlo Ersek @ 2018-11-22 18:09 UTC (permalink / raw)
To: Ard Biesheuvel, edk2-devel
Cc: leif.lindholm, thomas.abraham, nariman.poushin, philmd
On 11/22/18 18:26, Ard Biesheuvel wrote:
> BdsLoadImage () is part of the BdsLib library API and is not documented
> as modifying its DevicePath argument, but does so nonetheless. So take
> a copy instead, and free it after use.
>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
> Platform/ARM/Library/BdsLib/BdsFilePath.c | 13 ++++++++++++-
> 1 file changed, 12 insertions(+), 1 deletion(-)
>
> diff --git a/Platform/ARM/Library/BdsLib/BdsFilePath.c b/Platform/ARM/Library/BdsLib/BdsFilePath.c
> index 67dafa4f3651..74fdbbee773d 100644
> --- a/Platform/ARM/Library/BdsLib/BdsFilePath.c
> +++ b/Platform/ARM/Library/BdsLib/BdsFilePath.c
> @@ -1351,5 +1351,16 @@ BdsLoadImage (
> OUT UINTN *FileSize
> )
> {
> - return BdsLoadImageAndUpdateDevicePath (&DevicePath, Type, Image, FileSize);
> + EFI_DEVICE_PATH *Path;
> + EFI_STATUS Status;
> +
> + Path = DuplicateDevicePath (DevicePath);
> + if (Path == NULL) {
> + return EFI_OUT_OF_RESOURCES;
> + }
This introduces a minor change in behavior.
Previously, if BdsLoadImage() got DevicePath==NULL, then
BdsLoadImageAndUpdateDevicePath() -> BdsConnectAndUpdateDevicePath()
would hit (*DevicePath == NULL), and return EFI_INVALID_PARAMETER.
Now, (DevicePath==NULL) causes DuplicateDevicePath() to return NULL, and
we translate that to EFI_OUT_OF_RESOURCES.
Can you check for (DevicePath==NULL) first, and preserve
EFI_INVALID_PARAMETER?
> +
> + Status = BdsLoadImageAndUpdateDevicePath (&Path, Type, Image, FileSize);
> + FreePool (Path);
This is not safe; BdsLoadImageAndUpdateDevicePath() may change Path.
Namely, in BdsConnectAndUpdateDevicePath(), we have at one location,
*DevicePath = NewDevicePath;
... Which, in fact, makes me wonder whether we need this patch at all. I
believe BdsLoadImageAndUpdateDevicePath() -- and
BdsConnectAndUpdateDevicePath() -- are supposed to update the caller's
*pointer* to the device path, and not the pointed-to device path itself.
Do you agree?
Thanks,
Laszlo
> +
> + return Status;
> }
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH edk2-platforms 3/4] Platform/ARM/BdsLib: don't clobber BdsLoadImage() DevicePath IN param
2018-11-22 18:09 ` Laszlo Ersek
@ 2018-11-22 18:14 ` Ard Biesheuvel
2018-11-22 18:23 ` Laszlo Ersek
0 siblings, 1 reply; 15+ messages in thread
From: Ard Biesheuvel @ 2018-11-22 18:14 UTC (permalink / raw)
To: Laszlo Ersek
Cc: edk2-devel@lists.01.org, Leif Lindholm,
Thomas Panakamattam Abraham, Nariman Poushin,
Philippe Mathieu-Daudé
On Thu, 22 Nov 2018 at 19:09, Laszlo Ersek <lersek@redhat.com> wrote:
>
> On 11/22/18 18:26, Ard Biesheuvel wrote:
> > BdsLoadImage () is part of the BdsLib library API and is not documented
> > as modifying its DevicePath argument, but does so nonetheless. So take
> > a copy instead, and free it after use.
> >
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > ---
> > Platform/ARM/Library/BdsLib/BdsFilePath.c | 13 ++++++++++++-
> > 1 file changed, 12 insertions(+), 1 deletion(-)
> >
> > diff --git a/Platform/ARM/Library/BdsLib/BdsFilePath.c b/Platform/ARM/Library/BdsLib/BdsFilePath.c
> > index 67dafa4f3651..74fdbbee773d 100644
> > --- a/Platform/ARM/Library/BdsLib/BdsFilePath.c
> > +++ b/Platform/ARM/Library/BdsLib/BdsFilePath.c
> > @@ -1351,5 +1351,16 @@ BdsLoadImage (
> > OUT UINTN *FileSize
> > )
> > {
> > - return BdsLoadImageAndUpdateDevicePath (&DevicePath, Type, Image, FileSize);
> > + EFI_DEVICE_PATH *Path;
> > + EFI_STATUS Status;
> > +
> > + Path = DuplicateDevicePath (DevicePath);
> > + if (Path == NULL) {
> > + return EFI_OUT_OF_RESOURCES;
> > + }
>
> This introduces a minor change in behavior.
>
> Previously, if BdsLoadImage() got DevicePath==NULL, then
> BdsLoadImageAndUpdateDevicePath() -> BdsConnectAndUpdateDevicePath()
> would hit (*DevicePath == NULL), and return EFI_INVALID_PARAMETER.
>
> Now, (DevicePath==NULL) causes DuplicateDevicePath() to return NULL, and
> we translate that to EFI_OUT_OF_RESOURCES.
>
> Can you check for (DevicePath==NULL) first, and preserve
> EFI_INVALID_PARAMETER?
>
> > +
> > + Status = BdsLoadImageAndUpdateDevicePath (&Path, Type, Image, FileSize);
> > + FreePool (Path);
>
> This is not safe; BdsLoadImageAndUpdateDevicePath() may change Path.
> Namely, in BdsConnectAndUpdateDevicePath(), we have at one location,
>
> *DevicePath = NewDevicePath;
>
> ... Which, in fact, makes me wonder whether we need this patch at all. I
> believe BdsLoadImageAndUpdateDevicePath() -- and
> BdsConnectAndUpdateDevicePath() -- are supposed to update the caller's
> *pointer* to the device path, and not the pointed-to device path itself.
>
> Do you agree?
>
Indeed.
EFI_STATUS
BdsLoadImage (
IN EFI_DEVICE_PATH *DevicePath,
vs
EFI_STATUS
BdsLoadImageAndUpdateDevicePath (
IN OUT EFI_DEVICE_PATH **DevicePath,
and I didn't spot the diference in * vs **
So you are right: BdsConnectAndUpdateDevicePath() assigns to
*DevicePath, which means it updates BdsLoadImage()'s local copy of the
pointer, but not the memory it points to.
The IN/OUT notation makes this a bit ambiguous, though. Having
something like EFI_DEVICE_PATH CONST ** vs EFI_DEVICE_PATH * CONST *
is not necessarily easier to read, but less ambiguous.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH edk2-platforms 3/4] Platform/ARM/BdsLib: don't clobber BdsLoadImage() DevicePath IN param
2018-11-22 18:14 ` Ard Biesheuvel
@ 2018-11-22 18:23 ` Laszlo Ersek
0 siblings, 0 replies; 15+ messages in thread
From: Laszlo Ersek @ 2018-11-22 18:23 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: edk2-devel@lists.01.org, Leif Lindholm,
Thomas Panakamattam Abraham, Nariman Poushin,
Philippe Mathieu-Daudé
On 11/22/18 19:14, Ard Biesheuvel wrote:
> On Thu, 22 Nov 2018 at 19:09, Laszlo Ersek <lersek@redhat.com> wrote:
>>
>> On 11/22/18 18:26, Ard Biesheuvel wrote:
>>> BdsLoadImage () is part of the BdsLib library API and is not documented
>>> as modifying its DevicePath argument, but does so nonetheless. So take
>>> a copy instead, and free it after use.
>>>
>>> Contributed-under: TianoCore Contribution Agreement 1.1
>>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>> ---
>>> Platform/ARM/Library/BdsLib/BdsFilePath.c | 13 ++++++++++++-
>>> 1 file changed, 12 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/Platform/ARM/Library/BdsLib/BdsFilePath.c b/Platform/ARM/Library/BdsLib/BdsFilePath.c
>>> index 67dafa4f3651..74fdbbee773d 100644
>>> --- a/Platform/ARM/Library/BdsLib/BdsFilePath.c
>>> +++ b/Platform/ARM/Library/BdsLib/BdsFilePath.c
>>> @@ -1351,5 +1351,16 @@ BdsLoadImage (
>>> OUT UINTN *FileSize
>>> )
>>> {
>>> - return BdsLoadImageAndUpdateDevicePath (&DevicePath, Type, Image, FileSize);
>>> + EFI_DEVICE_PATH *Path;
>>> + EFI_STATUS Status;
>>> +
>>> + Path = DuplicateDevicePath (DevicePath);
>>> + if (Path == NULL) {
>>> + return EFI_OUT_OF_RESOURCES;
>>> + }
>>
>> This introduces a minor change in behavior.
>>
>> Previously, if BdsLoadImage() got DevicePath==NULL, then
>> BdsLoadImageAndUpdateDevicePath() -> BdsConnectAndUpdateDevicePath()
>> would hit (*DevicePath == NULL), and return EFI_INVALID_PARAMETER.
>>
>> Now, (DevicePath==NULL) causes DuplicateDevicePath() to return NULL, and
>> we translate that to EFI_OUT_OF_RESOURCES.
>>
>> Can you check for (DevicePath==NULL) first, and preserve
>> EFI_INVALID_PARAMETER?
>>
>>> +
>>> + Status = BdsLoadImageAndUpdateDevicePath (&Path, Type, Image, FileSize);
>>> + FreePool (Path);
>>
>> This is not safe; BdsLoadImageAndUpdateDevicePath() may change Path.
>> Namely, in BdsConnectAndUpdateDevicePath(), we have at one location,
>>
>> *DevicePath = NewDevicePath;
>>
>> ... Which, in fact, makes me wonder whether we need this patch at all. I
>> believe BdsLoadImageAndUpdateDevicePath() -- and
>> BdsConnectAndUpdateDevicePath() -- are supposed to update the caller's
>> *pointer* to the device path, and not the pointed-to device path itself.
>>
>> Do you agree?
>>
>
> Indeed.
>
> EFI_STATUS
> BdsLoadImage (
> IN EFI_DEVICE_PATH *DevicePath,
>
> vs
>
> EFI_STATUS
> BdsLoadImageAndUpdateDevicePath (
> IN OUT EFI_DEVICE_PATH **DevicePath,
>
> and I didn't spot the diference in * vs **
>
> So you are right: BdsConnectAndUpdateDevicePath() assigns to
> *DevicePath, which means it updates BdsLoadImage()'s local copy of the
> pointer, but not the memory it points to.
>
> The IN/OUT notation makes this a bit ambiguous, though. Having
> something like EFI_DEVICE_PATH CONST ** vs EFI_DEVICE_PATH * CONST *
> is not necessarily easier to read, but less ambiguous.
>
Exactly!
Laszlo
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH edk2-platforms 4/4] Platform/ARM/BdsLib: maintain alignment for DevicePaths
2018-11-22 17:26 [PATCH edk2-platforms 0/4] Platform/ARM: fix DevicePath mishandling in BdsLib Ard Biesheuvel
` (2 preceding siblings ...)
2018-11-22 17:26 ` [PATCH edk2-platforms 3/4] Platform/ARM/BdsLib: don't clobber BdsLoadImage() DevicePath IN param Ard Biesheuvel
@ 2018-11-22 17:26 ` Ard Biesheuvel
2018-11-22 18:35 ` Laszlo Ersek
2018-11-23 4:20 ` [PATCH edk2-platforms 0/4] Platform/ARM: fix DevicePath mishandling in BdsLib Thomas Abraham
4 siblings, 1 reply; 15+ messages in thread
From: Ard Biesheuvel @ 2018-11-22 17:26 UTC (permalink / raw)
To: edk2-devel
Cc: leif.lindholm, thomas.abraham, nariman.poushin, lersek, philmd,
Ard Biesheuvel
DevicePath node types may have any size, and so it is up to the
code that manipulates them to ensure that dereferencing them only
occurs when the pointer is aligned explicitly.
Since BdsConnectAndUpdateDevicePath() has only two callers, one of
which itself, we can simply duplicate the device path (similar to
how DxeCore's CoreConnectController () does it), and free the pool
allocation again on the way out. (Note that the allocation only
occurs when the non-recursive path is taken)
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
Platform/ARM/Library/BdsLib/BdsFilePath.c | 10 +++++++---
1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/Platform/ARM/Library/BdsLib/BdsFilePath.c b/Platform/ARM/Library/BdsLib/BdsFilePath.c
index 74fdbbee773d..543ac8f83086 100644
--- a/Platform/ARM/Library/BdsLib/BdsFilePath.c
+++ b/Platform/ARM/Library/BdsLib/BdsFilePath.c
@@ -421,7 +421,7 @@ BdsConnectAndUpdateDevicePath (
}
if (RemainingDevicePath) {
- *RemainingDevicePath = Remaining;
+ *RemainingDevicePath = DuplicateDevicePath (Remaining);
}
return Status;
@@ -1333,14 +1333,18 @@ BdsLoadImageAndUpdateDevicePath (
}
FileLoader = FileLoaders;
+ Status = EFI_UNSUPPORTED;
while (FileLoader->Support != NULL) {
if (FileLoader->Support (*DevicePath, Handle, RemainingDevicePath)) {
- return FileLoader->LoadImage (DevicePath, Handle, RemainingDevicePath, Type, Image, FileSize);
+ Status = FileLoader->LoadImage (DevicePath, Handle, RemainingDevicePath,
+ Type, Image, FileSize);
+ break;
}
FileLoader++;
}
- return EFI_UNSUPPORTED;
+ FreePool (RemainingDevicePath);
+ return Status;
}
EFI_STATUS
--
2.17.1
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH edk2-platforms 4/4] Platform/ARM/BdsLib: maintain alignment for DevicePaths
2018-11-22 17:26 ` [PATCH edk2-platforms 4/4] Platform/ARM/BdsLib: maintain alignment for DevicePaths Ard Biesheuvel
@ 2018-11-22 18:35 ` Laszlo Ersek
2018-11-23 8:35 ` Ard Biesheuvel
0 siblings, 1 reply; 15+ messages in thread
From: Laszlo Ersek @ 2018-11-22 18:35 UTC (permalink / raw)
To: Ard Biesheuvel, edk2-devel
Cc: leif.lindholm, thomas.abraham, nariman.poushin, philmd
On 11/22/18 18:26, Ard Biesheuvel wrote:
> DevicePath node types may have any size, and so it is up to the
> code that manipulates them to ensure that dereferencing them only
> occurs when the pointer is aligned explicitly.
>
> Since BdsConnectAndUpdateDevicePath() has only two callers,
at d9e68a756cfb ("Platform/ARM/SgiPkg: increase max variable size to
8KB", 2018-11-20), it seems to have three callers:
- itself
- BdsConnectDevicePath()
- BdsLoadImageAndUpdateDevicePath()
> one of
> which itself, we can simply duplicate the device path (similar to
> how DxeCore's CoreConnectController () does it), and free the pool
> allocation again on the way out. (Note that the allocation only
> occurs when the non-recursive path is taken)
I think this rather works around than fixes the problem -- just because
every remaining device path "slice" is realigned as we advance, it's not
guaranteed that any and all CHAR16 fields in the now-first node will be
naturally aligned.
... However, it certainly applies to FILEPATH_DEVICE_PATH.PathName,
which is likely the only such field that we care about. :)
>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
> Platform/ARM/Library/BdsLib/BdsFilePath.c | 10 +++++++---
> 1 file changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/Platform/ARM/Library/BdsLib/BdsFilePath.c b/Platform/ARM/Library/BdsLib/BdsFilePath.c
> index 74fdbbee773d..543ac8f83086 100644
> --- a/Platform/ARM/Library/BdsLib/BdsFilePath.c
> +++ b/Platform/ARM/Library/BdsLib/BdsFilePath.c
> @@ -421,7 +421,7 @@ BdsConnectAndUpdateDevicePath (
> }
>
> if (RemainingDevicePath) {
> - *RemainingDevicePath = Remaining;
> + *RemainingDevicePath = DuplicateDevicePath (Remaining);
> }
>
> return Status;
OK, so this makes BdsConnectAndUpdateDevicePath()'s RemainingDevicePath
output param dynamically allocated. And this change works fine with the
recursive logic too, as you say in the commit message.
> @@ -1333,14 +1333,18 @@ BdsLoadImageAndUpdateDevicePath (
> }
We already need some error handling here. The control flow in
BdsConnectAndUpdateDevicePath() boggles my mind a bit, but I think it
can output a dynamically allocated RemainingDevicePath *and* return an
error.
Namely, assume that TryRemovableDevice() is reached, and it fails.
So, I think we should add an error handling label
("FreeRemainingDevicePath"), and jump to it, from both first "return"
statements in this function.
Also, we should likely set RemainingDevicePath to NULL at the top of the
function, and check it at the end, because... ugh...
BdsConnectAndUpdateDevicePath() might also fail without assigning
*RemainingDevicePath?
>
> FileLoader = FileLoaders;
> + Status = EFI_UNSUPPORTED;
> while (FileLoader->Support != NULL) {
> if (FileLoader->Support (*DevicePath, Handle, RemainingDevicePath)) {
> - return FileLoader->LoadImage (DevicePath, Handle, RemainingDevicePath, Type, Image, FileSize);
> + Status = FileLoader->LoadImage (DevicePath, Handle, RemainingDevicePath,
> + Type, Image, FileSize);
> + break;
> }
> FileLoader++;
> }
>
> - return EFI_UNSUPPORTED;
> + FreePool (RemainingDevicePath);
> + return Status;
> }
>
> EFI_STATUS
>
As I mention near the commit message, BdsConnectDevicePath() is not
updated. Is that OK? ... Oh wait, BdsConnectDevicePath() is not called
by anything. Append another patch to drop it, like
BdsStartEfiApplication()? Then this patch will be fine, assuming you add
the "goto"s.
Thanks!
Laszlo
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH edk2-platforms 4/4] Platform/ARM/BdsLib: maintain alignment for DevicePaths
2018-11-22 18:35 ` Laszlo Ersek
@ 2018-11-23 8:35 ` Ard Biesheuvel
2018-11-23 9:39 ` Laszlo Ersek
0 siblings, 1 reply; 15+ messages in thread
From: Ard Biesheuvel @ 2018-11-23 8:35 UTC (permalink / raw)
To: Laszlo Ersek
Cc: edk2-devel@lists.01.org, Leif Lindholm,
Thomas Panakamattam Abraham, Nariman Poushin,
Philippe Mathieu-Daudé
On Thu, 22 Nov 2018 at 19:35, Laszlo Ersek <lersek@redhat.com> wrote:
>
> On 11/22/18 18:26, Ard Biesheuvel wrote:
> > DevicePath node types may have any size, and so it is up to the
> > code that manipulates them to ensure that dereferencing them only
> > occurs when the pointer is aligned explicitly.
> >
> > Since BdsConnectAndUpdateDevicePath() has only two callers,
>
> at d9e68a756cfb ("Platform/ARM/SgiPkg: increase max variable size to
> 8KB", 2018-11-20), it seems to have three callers:
>
> - itself
> - BdsConnectDevicePath()
> - BdsLoadImageAndUpdateDevicePath()
>
Indeed. I am updating the second patch to get rid of everything in
BdsLib we are not currently using.
> > one of
> > which itself, we can simply duplicate the device path (similar to
> > how DxeCore's CoreConnectController () does it), and free the pool
> > allocation again on the way out. (Note that the allocation only
> > occurs when the non-recursive path is taken)
>
> I think this rather works around than fixes the problem -- just because
> every remaining device path "slice" is realigned as we advance, it's not
> guaranteed that any and all CHAR16 fields in the now-first node will be
> naturally aligned.
>
> ... However, it certainly applies to FILEPATH_DEVICE_PATH.PathName,
> which is likely the only such field that we care about. :)
>
Looking at 56bed2f41022afcbadecc9f2d537bd31c3d44cbc ("^W never mind ...
the intent appears to be that device path struct members do appear
naturally aligned, even if the size of the data structure is not a
multiple of the max alignment we expect to encounter.
Presumably, this is why CoreConnectController () does the same in this regard.
> >
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > ---
> > Platform/ARM/Library/BdsLib/BdsFilePath.c | 10 +++++++---
> > 1 file changed, 7 insertions(+), 3 deletions(-)
> >
> > diff --git a/Platform/ARM/Library/BdsLib/BdsFilePath.c b/Platform/ARM/Library/BdsLib/BdsFilePath.c
> > index 74fdbbee773d..543ac8f83086 100644
> > --- a/Platform/ARM/Library/BdsLib/BdsFilePath.c
> > +++ b/Platform/ARM/Library/BdsLib/BdsFilePath.c
> > @@ -421,7 +421,7 @@ BdsConnectAndUpdateDevicePath (
> > }
> >
> > if (RemainingDevicePath) {
> > - *RemainingDevicePath = Remaining;
> > + *RemainingDevicePath = DuplicateDevicePath (Remaining);
> > }
> >
> > return Status;
>
> OK, so this makes BdsConnectAndUpdateDevicePath()'s RemainingDevicePath
> output param dynamically allocated. And this change works fine with the
> recursive logic too, as you say in the commit message.
>
Yep.
> > @@ -1333,14 +1333,18 @@ BdsLoadImageAndUpdateDevicePath (
> > }
>
> We already need some error handling here. The control flow in
> BdsConnectAndUpdateDevicePath() boggles my mind a bit, but I think it
> can output a dynamically allocated RemainingDevicePath *and* return an
> error.
>
> Namely, assume that TryRemovableDevice() is reached, and it fails.
>
That doesn't make sense. I'll update that routine to only do the clone
if it returns EFI_SUCCESS.
> So, I think we should add an error handling label
> ("FreeRemainingDevicePath"), and jump to it, from both first "return"
> statements in this function.
>
> Also, we should likely set RemainingDevicePath to NULL at the top of the
> function, and check it at the end, because... ugh...
> BdsConnectAndUpdateDevicePath() might also fail without assigning
> *RemainingDevicePath?
>
The above change should fix that as well afaict.
> >
> > FileLoader = FileLoaders;
> > + Status = EFI_UNSUPPORTED;
> > while (FileLoader->Support != NULL) {
> > if (FileLoader->Support (*DevicePath, Handle, RemainingDevicePath)) {
> > - return FileLoader->LoadImage (DevicePath, Handle, RemainingDevicePath, Type, Image, FileSize);
> > + Status = FileLoader->LoadImage (DevicePath, Handle, RemainingDevicePath,
> > + Type, Image, FileSize);
> > + break;
> > }
> > FileLoader++;
> > }
> >
> > - return EFI_UNSUPPORTED;
> > + FreePool (RemainingDevicePath);
> > + return Status;
> > }
> >
> > EFI_STATUS
> >
>
> As I mention near the commit message, BdsConnectDevicePath() is not
> updated. Is that OK? ... Oh wait, BdsConnectDevicePath() is not called
> by anything. Append another patch to drop it, like
> BdsStartEfiApplication()? Then this patch will be fine, assuming you add
> the "goto"s.
>
> Thanks!
> Laszlo
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH edk2-platforms 4/4] Platform/ARM/BdsLib: maintain alignment for DevicePaths
2018-11-23 8:35 ` Ard Biesheuvel
@ 2018-11-23 9:39 ` Laszlo Ersek
0 siblings, 0 replies; 15+ messages in thread
From: Laszlo Ersek @ 2018-11-23 9:39 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: edk2-devel@lists.01.org, Leif Lindholm,
Thomas Panakamattam Abraham, Nariman Poushin,
Philippe Mathieu-Daudé
On 11/23/18 09:35, Ard Biesheuvel wrote:
> On Thu, 22 Nov 2018 at 19:35, Laszlo Ersek <lersek@redhat.com> wrote:
>> [...]
> Indeed. I am updating the second patch to get rid of everything in
> BdsLib we are not currently using.
Thanks!
> Looking at 56bed2f41022afcbadecc9f2d537bd31c3d44cbc ("^W never mind
> ...
heh :)
> the intent appears to be that device path struct members do appear
> naturally aligned, even if the size of the data structure is not a
> multiple of the max alignment we expect to encounter.
>
> Presumably, this is why CoreConnectController () does the same in this
> regard.
Yeah, that's certainly for working around bugs elsewhere in the
firmware. The UEFI spec 2.7 says in "10.3.1 Generic Device Path
Structures":
A Device Path is a series of generic Device Path nodes. The first
Device Path node starts at byte offset zero of the Device Path. The
next Device Path node starts at the end of the previous Device Path
node. Therefore all nodes are byte-packed data structures that may
appear on any byte boundary. *All code references to device path
notes must assume all fields are unaligned.*
(Emphasis mine. And yes, the exact sentence that I'm quoting this
section for contains a typo, s/notes/nodes/.)
[...]
Thanks!
Laszlo
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH edk2-platforms 0/4] Platform/ARM: fix DevicePath mishandling in BdsLib
2018-11-22 17:26 [PATCH edk2-platforms 0/4] Platform/ARM: fix DevicePath mishandling in BdsLib Ard Biesheuvel
` (3 preceding siblings ...)
2018-11-22 17:26 ` [PATCH edk2-platforms 4/4] Platform/ARM/BdsLib: maintain alignment for DevicePaths Ard Biesheuvel
@ 2018-11-23 4:20 ` Thomas Abraham
2018-11-23 8:44 ` Ard Biesheuvel
4 siblings, 1 reply; 15+ messages in thread
From: Thomas Abraham @ 2018-11-23 4:20 UTC (permalink / raw)
To: Ard Biesheuvel; +Cc: edk2-devel, lersek, Nariman Poushin
On Thu, Nov 22, 2018 at 10:56 PM Ard Biesheuvel
<ard.biesheuvel@linaro.org> wrote:
>
> The deprecated BdsLib library class in ArmPkg is still depended upon, but
> only a single implementation exists, which now resides in edk2-platforms.
>
> This implementation has some issues in how it deals with Device Paths,
> so let's fix those, but first move over the library interface declaration.
> This will permit dropping it from ArmPkg in EDK2.
>
> Ard Biesheuvel (4):
> Platform/ARM: import ARM platform specific BdsLib header
> Platform/ARM/BdsLid: drop unused BdsStartEfiApplication ()
> Platform/ARM/BdsLib: don't clobber BdsLoadImage() DevicePath IN param
> Platform/ARM/BdsLib: maintain alignment for DevicePaths
This patch series when applied with the below two patch series
- [PATCH v2 0/5] ArmPlatformPkg, ArmVirtPkg: discover NOR flash banks from DTB
- [PATCH edk2-platforms 0/3] drop GUIDs from NOR flash bank descriptors
does solve the boot issue on the Juno board.
>
> Platform/ARM/ARM.dec | 3 +
> .../Drivers/FdtPlatformDxe/FdtPlatformDxe.inf | 2 +-
> Platform/ARM/Include/Library/BdsLib.h | 193 ++++++++++++++++++
> Platform/ARM/Library/BdsLib/BdsFilePath.c | 71 ++-----
> Platform/ARM/Library/BdsLib/BdsInternal.h | 1 -
> Platform/ARM/Library/BdsLib/BdsLib.inf | 2 +-
> 6 files changed, 212 insertions(+), 60 deletions(-)
> create mode 100644 Platform/ARM/Include/Library/BdsLib.h
>
> --
> 2.17.1
>
> _______________________________________________
> 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 edk2-platforms 0/4] Platform/ARM: fix DevicePath mishandling in BdsLib
2018-11-23 4:20 ` [PATCH edk2-platforms 0/4] Platform/ARM: fix DevicePath mishandling in BdsLib Thomas Abraham
@ 2018-11-23 8:44 ` Ard Biesheuvel
0 siblings, 0 replies; 15+ messages in thread
From: Ard Biesheuvel @ 2018-11-23 8:44 UTC (permalink / raw)
To: Thomas Panakamattam Abraham
Cc: edk2-devel@lists.01.org, Laszlo Ersek, Nariman Poushin
On Fri, 23 Nov 2018 at 05:20, Thomas Abraham <thomas.abraham@arm.com> wrote:
>
> On Thu, Nov 22, 2018 at 10:56 PM Ard Biesheuvel
> <ard.biesheuvel@linaro.org> wrote:
> >
> > The deprecated BdsLib library class in ArmPkg is still depended upon, but
> > only a single implementation exists, which now resides in edk2-platforms.
> >
> > This implementation has some issues in how it deals with Device Paths,
> > so let's fix those, but first move over the library interface declaration.
> > This will permit dropping it from ArmPkg in EDK2.
> >
> > Ard Biesheuvel (4):
> > Platform/ARM: import ARM platform specific BdsLib header
> > Platform/ARM/BdsLid: drop unused BdsStartEfiApplication ()
> > Platform/ARM/BdsLib: don't clobber BdsLoadImage() DevicePath IN param
> > Platform/ARM/BdsLib: maintain alignment for DevicePaths
>
> This patch series when applied with the below two patch series
> - [PATCH v2 0/5] ArmPlatformPkg, ArmVirtPkg: discover NOR flash banks from DTB
> - [PATCH edk2-platforms 0/3] drop GUIDs from NOR flash bank descriptors
>
> does solve the boot issue on the Juno board.
Wonderful! Thanks for confirming.
> >
> > Platform/ARM/ARM.dec | 3 +
> > .../Drivers/FdtPlatformDxe/FdtPlatformDxe.inf | 2 +-
> > Platform/ARM/Include/Library/BdsLib.h | 193 ++++++++++++++++++
> > Platform/ARM/Library/BdsLib/BdsFilePath.c | 71 ++-----
> > Platform/ARM/Library/BdsLib/BdsInternal.h | 1 -
> > Platform/ARM/Library/BdsLib/BdsLib.inf | 2 +-
> > 6 files changed, 212 insertions(+), 60 deletions(-)
> > create mode 100644 Platform/ARM/Include/Library/BdsLib.h
> >
> > --
> > 2.17.1
> >
> > _______________________________________________
> > 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