diff mbox

[edk2,RFT] EmbeddedPkg/AndroidFastbootApp: remove dependency on deprecated BdsLib

Message ID 20170410165534.22292-1-ard.biesheuvel@linaro.org
State Accepted
Commit fcf41edf2f5383f3caca257fdcbb7d3992ff7a48
Headers show

Commit Message

Ard Biesheuvel April 10, 2017, 4:55 p.m. UTC 
One of the last remaining modules with a dependency on the deprecated
BdsLib implementation from ArmPkg is the Android fastboot application.

Its only dependency on BdsLib is BdsStartEfiApplication(), which is
used in the most peculiar way: the fastboot app loads the kernel image
into memory, and creates a MemoryMapped() device path for it. It then
proceeds and calls BdsStartEfiApplication(), which explicitly loads the
contents of the devicepath into memory, creating a second in-memory copy
of the kernel image, after which it invokes gBS->LoadImage() with a
buffer address and size (while it is perfectly capable of loading from
a devicepath directly)

Since we know the device path is fully qualified and connected, and does
not require any of the additional processing that BdsStartEfiApplication()
does when dereferencing a device path, we should be able to pass this
devicepath into LoadImage() directly.

So create a simplified local clone of BdsStartEfiApplication(), and drop
the dependency on BdsLib.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

---
 EmbeddedPkg/Application/AndroidFastboot/AndroidFastbootApp.inf   |  2 +-
 EmbeddedPkg/Application/AndroidFastboot/Arm/BootAndroidBootImg.c | 57 +++++++++++++++++++-
 2 files changed, 57 insertions(+), 2 deletions(-)

-- 
2.9.3

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

Comments

Ard Biesheuvel April 18, 2017, 6:08 p.m. UTC | #1 
On 10 April 2017 at 17:55, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> One of the last remaining modules with a dependency on the deprecated

> BdsLib implementation from ArmPkg is the Android fastboot application.

>

> Its only dependency on BdsLib is BdsStartEfiApplication(), which is

> used in the most peculiar way: the fastboot app loads the kernel image

> into memory, and creates a MemoryMapped() device path for it. It then

> proceeds and calls BdsStartEfiApplication(), which explicitly loads the

> contents of the devicepath into memory, creating a second in-memory copy

> of the kernel image, after which it invokes gBS->LoadImage() with a

> buffer address and size (while it is perfectly capable of loading from

> a devicepath directly)

>

> Since we know the device path is fully qualified and connected, and does

> not require any of the additional processing that BdsStartEfiApplication()

> does when dereferencing a device path, we should be able to pass this

> devicepath into LoadImage() directly.

>

> So create a simplified local clone of BdsStartEfiApplication(), and drop

> the dependency on BdsLib.

>

> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>


Should we just merge this and see what happens?


> ---

>  EmbeddedPkg/Application/AndroidFastboot/AndroidFastbootApp.inf   |  2 +-

>  EmbeddedPkg/Application/AndroidFastboot/Arm/BootAndroidBootImg.c | 57 +++++++++++++++++++-

>  2 files changed, 57 insertions(+), 2 deletions(-)

>

> diff --git a/EmbeddedPkg/Application/AndroidFastboot/AndroidFastbootApp.inf b/EmbeddedPkg/Application/AndroidFastboot/AndroidFastbootApp.inf

> index 3e115171ce01..8d2b23c6f8b8 100644

> --- a/EmbeddedPkg/Application/AndroidFastboot/AndroidFastbootApp.inf

> +++ b/EmbeddedPkg/Application/AndroidFastboot/AndroidFastbootApp.inf

> @@ -30,7 +30,6 @@ [Sources.ARM, Sources.AARCH64]

>  [LibraryClasses]

>    BaseLib

>    BaseMemoryLib

> -  BdsLib

>    DebugLib

>    DevicePathLib

>    DxeServicesTableLib

> @@ -45,6 +44,7 @@ [LibraryClasses]

>  [Protocols]

>    gAndroidFastbootTransportProtocolGuid

>    gAndroidFastbootPlatformProtocolGuid

> +  gEfiLoadedImageProtocolGuid

>    gEfiSimpleTextOutProtocolGuid

>    gEfiSimpleTextInProtocolGuid

>

> diff --git a/EmbeddedPkg/Application/AndroidFastboot/Arm/BootAndroidBootImg.c b/EmbeddedPkg/Application/AndroidFastboot/Arm/BootAndroidBootImg.c

> index 46a7ceb3a41c..f446cce2855a 100644

> --- a/EmbeddedPkg/Application/AndroidFastboot/Arm/BootAndroidBootImg.c

> +++ b/EmbeddedPkg/Application/AndroidFastboot/Arm/BootAndroidBootImg.c

> @@ -15,6 +15,7 @@

>  #include "AndroidFastbootApp.h"

>

>  #include <Protocol/DevicePath.h>

> +#include <Protocol/LoadedImage.h>

>

>  #include <Library/BdsLib.h>

>  #include <Library/DevicePathLib.h>

> @@ -50,6 +51,60 @@ STATIC CONST MEMORY_DEVICE_PATH MemoryDevicePathTemplate =

>    } // End

>  };

>

> +

> +/**

> +  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.

> +

> +**/

> +STATIC

> +EFI_STATUS

> +StartEfiApplication (

> +  IN EFI_HANDLE                  ParentImageHandle,

> +  IN EFI_DEVICE_PATH_PROTOCOL    *DevicePath,

> +  IN UINTN                       LoadOptionsSize,

> +  IN VOID*                       LoadOptions

> +  )

> +{

> +  EFI_STATUS                   Status;

> +  EFI_HANDLE                   ImageHandle;

> +  EFI_LOADED_IMAGE_PROTOCOL*   LoadedImage;

> +

> +  // Load the image from the device path with Boot Services function

> +  Status = gBS->LoadImage (TRUE, ParentImageHandle, DevicePath, NULL, 0,

> +                  &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;

> +}

> +

>  EFI_STATUS

>  BootAndroidBootImg (

>    IN UINTN    BufferSize,

> @@ -100,7 +155,7 @@ BootAndroidBootImg (

>      LoadOptions = NewLoadOptions;

>    }

>

> -  Status = BdsStartEfiApplication (gImageHandle,

> +  Status = StartEfiApplication (gImageHandle,

>               (EFI_DEVICE_PATH_PROTOCOL *) &KernelDevicePath,

>               StrSize (LoadOptions),

>               LoadOptions);

> --

> 2.9.3

>

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Leif Lindholm April 18, 2017, 7:21 p.m. UTC | #2 
On Tue, Apr 18, 2017 at 07:08:57PM +0100, Ard Biesheuvel wrote:
> On 10 April 2017 at 17:55, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:

> > One of the last remaining modules with a dependency on the deprecated

> > BdsLib implementation from ArmPkg is the Android fastboot application.

> >

> > Its only dependency on BdsLib is BdsStartEfiApplication(), which is

> > used in the most peculiar way: the fastboot app loads the kernel image

> > into memory, and creates a MemoryMapped() device path for it. It then

> > proceeds and calls BdsStartEfiApplication(), which explicitly loads the

> > contents of the devicepath into memory, creating a second in-memory copy

> > of the kernel image, after which it invokes gBS->LoadImage() with a

> > buffer address and size (while it is perfectly capable of loading from

> > a devicepath directly)

> >

> > Since we know the device path is fully qualified and connected, and does

> > not require any of the additional processing that BdsStartEfiApplication()

> > does when dereferencing a device path, we should be able to pass this

> > devicepath into LoadImage() directly.

> >

> > So create a simplified local clone of BdsStartEfiApplication(), and drop

> > the dependency on BdsLib.

> >

> > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

> 

> Should we just merge this and see what happens?


With no other comments so far - sure:
Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>


> > ---

> >  EmbeddedPkg/Application/AndroidFastboot/AndroidFastbootApp.inf   |  2 +-

> >  EmbeddedPkg/Application/AndroidFastboot/Arm/BootAndroidBootImg.c | 57 +++++++++++++++++++-

> >  2 files changed, 57 insertions(+), 2 deletions(-)

> >

> > diff --git a/EmbeddedPkg/Application/AndroidFastboot/AndroidFastbootApp.inf b/EmbeddedPkg/Application/AndroidFastboot/AndroidFastbootApp.inf

> > index 3e115171ce01..8d2b23c6f8b8 100644

> > --- a/EmbeddedPkg/Application/AndroidFastboot/AndroidFastbootApp.inf

> > +++ b/EmbeddedPkg/Application/AndroidFastboot/AndroidFastbootApp.inf

> > @@ -30,7 +30,6 @@ [Sources.ARM, Sources.AARCH64]

> >  [LibraryClasses]

> >    BaseLib

> >    BaseMemoryLib

> > -  BdsLib

> >    DebugLib

> >    DevicePathLib

> >    DxeServicesTableLib

> > @@ -45,6 +44,7 @@ [LibraryClasses]

> >  [Protocols]

> >    gAndroidFastbootTransportProtocolGuid

> >    gAndroidFastbootPlatformProtocolGuid

> > +  gEfiLoadedImageProtocolGuid

> >    gEfiSimpleTextOutProtocolGuid

> >    gEfiSimpleTextInProtocolGuid

> >

> > diff --git a/EmbeddedPkg/Application/AndroidFastboot/Arm/BootAndroidBootImg.c b/EmbeddedPkg/Application/AndroidFastboot/Arm/BootAndroidBootImg.c

> > index 46a7ceb3a41c..f446cce2855a 100644

> > --- a/EmbeddedPkg/Application/AndroidFastboot/Arm/BootAndroidBootImg.c

> > +++ b/EmbeddedPkg/Application/AndroidFastboot/Arm/BootAndroidBootImg.c

> > @@ -15,6 +15,7 @@

> >  #include "AndroidFastbootApp.h"

> >

> >  #include <Protocol/DevicePath.h>

> > +#include <Protocol/LoadedImage.h>

> >

> >  #include <Library/BdsLib.h>

> >  #include <Library/DevicePathLib.h>

> > @@ -50,6 +51,60 @@ STATIC CONST MEMORY_DEVICE_PATH MemoryDevicePathTemplate =

> >    } // End

> >  };

> >

> > +

> > +/**

> > +  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.

> > +

> > +**/

> > +STATIC

> > +EFI_STATUS

> > +StartEfiApplication (

> > +  IN EFI_HANDLE                  ParentImageHandle,

> > +  IN EFI_DEVICE_PATH_PROTOCOL    *DevicePath,

> > +  IN UINTN                       LoadOptionsSize,

> > +  IN VOID*                       LoadOptions

> > +  )

> > +{

> > +  EFI_STATUS                   Status;

> > +  EFI_HANDLE                   ImageHandle;

> > +  EFI_LOADED_IMAGE_PROTOCOL*   LoadedImage;

> > +

> > +  // Load the image from the device path with Boot Services function

> > +  Status = gBS->LoadImage (TRUE, ParentImageHandle, DevicePath, NULL, 0,

> > +                  &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;

> > +}

> > +

> >  EFI_STATUS

> >  BootAndroidBootImg (

> >    IN UINTN    BufferSize,

> > @@ -100,7 +155,7 @@ BootAndroidBootImg (

> >      LoadOptions = NewLoadOptions;

> >    }

> >

> > -  Status = BdsStartEfiApplication (gImageHandle,

> > +  Status = StartEfiApplication (gImageHandle,

> >               (EFI_DEVICE_PATH_PROTOCOL *) &KernelDevicePath,

> >               StrSize (LoadOptions),

> >               LoadOptions);

> > --

> > 2.9.3

> >

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Ard Biesheuvel April 19, 2017, 7:18 a.m. UTC | #3 
On 18 April 2017 at 20:21, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> On Tue, Apr 18, 2017 at 07:08:57PM +0100, Ard Biesheuvel wrote:

>> On 10 April 2017 at 17:55, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:

>> > One of the last remaining modules with a dependency on the deprecated

>> > BdsLib implementation from ArmPkg is the Android fastboot application.

>> >

>> > Its only dependency on BdsLib is BdsStartEfiApplication(), which is

>> > used in the most peculiar way: the fastboot app loads the kernel image

>> > into memory, and creates a MemoryMapped() device path for it. It then

>> > proceeds and calls BdsStartEfiApplication(), which explicitly loads the

>> > contents of the devicepath into memory, creating a second in-memory copy

>> > of the kernel image, after which it invokes gBS->LoadImage() with a

>> > buffer address and size (while it is perfectly capable of loading from

>> > a devicepath directly)

>> >

>> > Since we know the device path is fully qualified and connected, and does

>> > not require any of the additional processing that BdsStartEfiApplication()

>> > does when dereferencing a device path, we should be able to pass this

>> > devicepath into LoadImage() directly.

>> >

>> > So create a simplified local clone of BdsStartEfiApplication(), and drop

>> > the dependency on BdsLib.

>> >

>> > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

>>

>> Should we just merge this and see what happens?

>

> With no other comments so far - sure:

> Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>

>


OK, pushed.

Thanks,
Ard.
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
diff mbox

Patch

diff --git a/EmbeddedPkg/Application/AndroidFastboot/AndroidFastbootApp.inf b/EmbeddedPkg/Application/AndroidFastboot/AndroidFastbootApp.inf
index 3e115171ce01..8d2b23c6f8b8 100644
--- a/EmbeddedPkg/Application/AndroidFastboot/AndroidFastbootApp.inf
+++ b/EmbeddedPkg/Application/AndroidFastboot/AndroidFastbootApp.inf
@@ -30,7 +30,6 @@  [Sources.ARM, Sources.AARCH64]
 [LibraryClasses]
   BaseLib
   BaseMemoryLib
-  BdsLib
   DebugLib
   DevicePathLib
   DxeServicesTableLib
@@ -45,6 +44,7 @@  [LibraryClasses]
 [Protocols]
   gAndroidFastbootTransportProtocolGuid
   gAndroidFastbootPlatformProtocolGuid
+  gEfiLoadedImageProtocolGuid
   gEfiSimpleTextOutProtocolGuid
   gEfiSimpleTextInProtocolGuid
 
diff --git a/EmbeddedPkg/Application/AndroidFastboot/Arm/BootAndroidBootImg.c b/EmbeddedPkg/Application/AndroidFastboot/Arm/BootAndroidBootImg.c
index 46a7ceb3a41c..f446cce2855a 100644
--- a/EmbeddedPkg/Application/AndroidFastboot/Arm/BootAndroidBootImg.c
+++ b/EmbeddedPkg/Application/AndroidFastboot/Arm/BootAndroidBootImg.c
@@ -15,6 +15,7 @@ 
 #include "AndroidFastbootApp.h"
 
 #include <Protocol/DevicePath.h>
+#include <Protocol/LoadedImage.h>
 
 #include <Library/BdsLib.h>
 #include <Library/DevicePathLib.h>
@@ -50,6 +51,60 @@  STATIC CONST MEMORY_DEVICE_PATH MemoryDevicePathTemplate =
   } // End
 };
 
+
+/**
+  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.
+
+**/
+STATIC
+EFI_STATUS
+StartEfiApplication (
+  IN EFI_HANDLE                  ParentImageHandle,
+  IN EFI_DEVICE_PATH_PROTOCOL    *DevicePath,
+  IN UINTN                       LoadOptionsSize,
+  IN VOID*                       LoadOptions
+  )
+{
+  EFI_STATUS                   Status;
+  EFI_HANDLE                   ImageHandle;
+  EFI_LOADED_IMAGE_PROTOCOL*   LoadedImage;
+
+  // Load the image from the device path with Boot Services function
+  Status = gBS->LoadImage (TRUE, ParentImageHandle, DevicePath, NULL, 0,
+                  &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;
+}
+
 EFI_STATUS
 BootAndroidBootImg (
   IN UINTN    BufferSize,
@@ -100,7 +155,7 @@  BootAndroidBootImg (
     LoadOptions = NewLoadOptions;
   }
 
-  Status = BdsStartEfiApplication (gImageHandle,
+  Status = StartEfiApplication (gImageHandle,
              (EFI_DEVICE_PATH_PROTOCOL *) &KernelDevicePath,
              StrSize (LoadOptions),
              LoadOptions);