public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
From: "Laszlo Ersek" <lersek@redhat.com>
To: devel@edk2.groups.io, ard.biesheuvel@linaro.org
Subject: Re: [edk2-devel] [PATCH 11/13] OvmfPkg/PlatformBootManagerLib: switch to QemuLoadImageLib
Date: Tue, 3 Mar 2020 10:53:00 +0100	[thread overview]
Message-ID: <b5637541-2291-50cc-961c-4639f01c9cc6@redhat.com> (raw)
In-Reply-To: <f832ad31-0724-ebb7-1a55-e96c797454c1@redhat.com>

On 03/03/20 10:52, Laszlo Ersek wrote:
> On 03/02/20 08:29, Ard Biesheuvel wrote:
>> Replace the open coded sequence to load Linux on x86 with a short and
>> generic sequence invoking QemuLoadImageLib, which can be provided by
>> a generic version that only supports the LoadImage and StartImage boot
>> services, and one that incorporates the entire legacy loading sequence
>> as well.
>>
>> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2566
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> ---
>>  OvmfPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf |   2 +-
>>  OvmfPkg/Library/PlatformBootManagerLib/QemuKernel.c               | 157 +++-----------------
>>  2 files changed, 24 insertions(+), 135 deletions(-)
>>
>> diff --git a/OvmfPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf b/OvmfPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf
>> index f89cce187942..40ac5dd7f9d5 100644
>> --- a/OvmfPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf
>> +++ b/OvmfPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf
>> @@ -48,7 +48,7 @@ [LibraryClasses]
>>    NvVarsFileLib
>>    QemuFwCfgLib
>>    QemuFwCfgS3Lib
>> -  LoadLinuxLib
>> +  QemuLoadImageLib
>>    QemuBootOrderLib
>>    ReportStatusCodeLib
>>    UefiLib
>> diff --git a/OvmfPkg/Library/PlatformBootManagerLib/QemuKernel.c b/OvmfPkg/Library/PlatformBootManagerLib/QemuKernel.c
>> index ddfef925edd3..a15b48d360d2 100644
>> --- a/OvmfPkg/Library/PlatformBootManagerLib/QemuKernel.c
>> +++ b/OvmfPkg/Library/PlatformBootManagerLib/QemuKernel.c
>> @@ -9,11 +9,8 @@
>>  
>>  #include <Library/BaseLib.h>
>>  #include <Library/DebugLib.h>
>> -#include <Library/LoadLinuxLib.h>
>> -#include <Library/MemoryAllocationLib.h>
>> -#include <Library/QemuFwCfgLib.h>
>> +#include <Library/QemuLoadImageLib.h>
>>  #include <Library/ReportStatusCodeLib.h>
>> -#include <Library/UefiBootServicesTableLib.h>
>>  #include <Library/UefiLib.h>
>>  
>>  
>> @@ -23,146 +20,38 @@ TryRunningQemuKernel (
>>    )
>>  {
>>    EFI_STATUS                Status;
>> -  UINTN                     KernelSize;
>> -  UINTN                     KernelInitialSize;
>> -  VOID                      *KernelBuf;
>> -  UINTN                     SetupSize;
>> -  VOID                      *SetupBuf;
>> -  UINTN                     CommandLineSize;
>> -  CHAR8                     *CommandLine;
>> -  UINTN                     InitrdSize;
>> -  VOID*                     InitrdData;
>> -
>> -  SetupBuf = NULL;
>> -  SetupSize = 0;
>> -  KernelBuf = NULL;
>> -  KernelInitialSize = 0;
>> -  CommandLine = NULL;
>> -  CommandLineSize = 0;
>> -  InitrdData = NULL;
>> -  InitrdSize = 0;
>> -
>> -  if (!QemuFwCfgIsAvailable ()) {
>> -    return EFI_NOT_FOUND;
>> -  }
>> -
>> -  QemuFwCfgSelectItem (QemuFwCfgItemKernelSize);
>> -  KernelSize = (UINTN) QemuFwCfgRead64 ();
>> -
>> -  QemuFwCfgSelectItem (QemuFwCfgItemKernelSetupSize);
>> -  SetupSize = (UINTN) QemuFwCfgRead64 ();
>> -
>> -  if (KernelSize == 0 || SetupSize == 0) {
>> -    DEBUG ((EFI_D_INFO, "qemu -kernel was not used.\n"));
>> -    return EFI_NOT_FOUND;
>> -  }
>> -
>> -  SetupBuf = LoadLinuxAllocateKernelSetupPages (EFI_SIZE_TO_PAGES (SetupSize));
>> -  if (SetupBuf == NULL) {
>> -    DEBUG ((EFI_D_ERROR, "Unable to allocate memory for kernel setup!\n"));
>> -    return EFI_OUT_OF_RESOURCES;
>> -  }
>> -
>> -  DEBUG ((EFI_D_INFO, "Setup size: 0x%x\n", (UINT32) SetupSize));
>> -  DEBUG ((EFI_D_INFO, "Reading kernel setup image ..."));
>> -  QemuFwCfgSelectItem (QemuFwCfgItemKernelSetupData);
>> -  QemuFwCfgReadBytes (SetupSize, SetupBuf);
>> -  DEBUG ((EFI_D_INFO, " [done]\n"));
>> -
>> -  Status = LoadLinuxCheckKernelSetup (SetupBuf, SetupSize);
>> -  if (EFI_ERROR (Status)) {
>> -    goto FreeAndReturn;
>> -  }
>> -
>> -  Status = LoadLinuxInitializeKernelSetup (SetupBuf);
>> -  if (EFI_ERROR (Status)) {
>> -    goto FreeAndReturn;
>> -  }
>> -
>> -  KernelInitialSize = LoadLinuxGetKernelSize (SetupBuf, KernelSize);
>> -  if (KernelInitialSize == 0) {
>> -    Status = EFI_UNSUPPORTED;
>> -    goto FreeAndReturn;
>> -  }
>> -
>> -  KernelBuf = LoadLinuxAllocateKernelPages (
>> -                SetupBuf,
>> -                EFI_SIZE_TO_PAGES (KernelInitialSize));
>> -  if (KernelBuf == NULL) {
>> -    DEBUG ((EFI_D_ERROR, "Unable to allocate memory for kernel!\n"));
>> -    Status = EFI_OUT_OF_RESOURCES;
>> -    goto FreeAndReturn;
>> -  }
>> -
>> -  DEBUG ((EFI_D_INFO, "Kernel size: 0x%x\n", (UINT32) KernelSize));
>> -  DEBUG ((EFI_D_INFO, "Reading kernel image ..."));
>> -  QemuFwCfgSelectItem (QemuFwCfgItemKernelData);
>> -  QemuFwCfgReadBytes (KernelSize, KernelBuf);
>> -  DEBUG ((EFI_D_INFO, " [done]\n"));
>> -
>> -  QemuFwCfgSelectItem (QemuFwCfgItemCommandLineSize);
>> -  CommandLineSize = (UINTN) QemuFwCfgRead64 ();
>> -
>> -  if (CommandLineSize > 0) {
>> -    CommandLine = LoadLinuxAllocateCommandLinePages (
>> -                    EFI_SIZE_TO_PAGES (CommandLineSize));
>> -    QemuFwCfgSelectItem (QemuFwCfgItemCommandLineData);
>> -    QemuFwCfgReadBytes (CommandLineSize, CommandLine);
>> -  } else {
>> -    CommandLine = NULL;
>> -  }
>> -
>> -  Status = LoadLinuxSetCommandLine (SetupBuf, CommandLine);
>> -  if (EFI_ERROR (Status)) {
>> -    goto FreeAndReturn;
>> -  }
>> -
>> -  QemuFwCfgSelectItem (QemuFwCfgItemInitrdSize);
>> -  InitrdSize = (UINTN) QemuFwCfgRead64 ();
>> -
>> -  if (InitrdSize > 0) {
>> -    InitrdData = LoadLinuxAllocateInitrdPages (
>> -                   SetupBuf,
>> -                   EFI_SIZE_TO_PAGES (InitrdSize)
>> -                   );
>> -    DEBUG ((EFI_D_INFO, "Initrd size: 0x%x\n", (UINT32) InitrdSize));
>> -    DEBUG ((EFI_D_INFO, "Reading initrd image ..."));
>> -    QemuFwCfgSelectItem (QemuFwCfgItemInitrdData);
>> -    QemuFwCfgReadBytes (InitrdSize, InitrdData);
>> -    DEBUG ((EFI_D_INFO, " [done]\n"));
>> -  } else {
>> -    InitrdData = NULL;
>> -  }
>> -
>> -  Status = LoadLinuxSetInitrd (SetupBuf, InitrdData, InitrdSize);
>> -  if (EFI_ERROR (Status)) {
>> -    goto FreeAndReturn;
>> +  EFI_HANDLE                KernelImageHandle;
>> +
>> +  Status = QemuLoadKernelImage (&KernelImageHandle);
>> +  if (EFI_ERROR (Status)) {
>> +    if (Status != EFI_SECURITY_VIOLATION) {
> 
> (1) This special treatment should disappear, once QemuLoadKernelImage()
> keeps EFI_SECURITY_VIOLATION inside.
> 
>> +      return Status;
>> +    }
>> +    //
>> +    // From the resource allocation perspective, EFI_SECURITY_VIOLATION means
>> +    // "success", so we must roll back the image loading.
>> +    //
>> +    goto UnloadKernelImage;
>>    }
>>  
>>    //
>> -  // Signal the EVT_SIGNAL_READY_TO_BOOT event
>> +  // Signal the EFI_EVENT_GROUP_READY_TO_BOOT event.
>>    //
>>    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);
>> +  //
>> +  // Start the image.
>> +  //
>> +  Status = QemuStartKernelImage (KernelImageHandle);
>> +  if (EFI_ERROR (Status)) {
>> +    DEBUG ((EFI_D_ERROR, "%a: StartImage(): %r\n", __FUNCTION__, Status));
>> +  }
> 
> (2) Please run "PatchCheck.py" on the series; it will reject "EFI_D_ERROR".

(3) also, the debug message should likely refer to
QemuStartKernelImage(), not StartImage().

Thanks
Laszlo


  reply	other threads:[~2020-03-03  9:53 UTC|newest]

Thread overview: 33+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-03-02  7:29 [PATCH 00/13] Ovmf: use LoadImage/StartImage for loading command line images Ard Biesheuvel
2020-03-02  7:29 ` [PATCH 01/13] OvmfPkg: add GUID for the QEMU kernel loader fs media device path Ard Biesheuvel
2020-03-02 13:22   ` [edk2-devel] " Laszlo Ersek
2020-03-02  7:29 ` [PATCH 02/13] OvmfPkg: export abstract QEMU blob filesystem in standalone driver Ard Biesheuvel
2020-03-02 13:45   ` [edk2-devel] " Laszlo Ersek
2020-03-02  7:29 ` [PATCH 03/13] OvmfPkg: introduce QemuLoadImageLib library class Ard Biesheuvel
2020-03-02 14:07   ` [edk2-devel] " Laszlo Ersek
2020-03-02  7:29 ` [PATCH 04/13] OvmfPkg: provide a generic implementation of QemuLoadImageLib Ard Biesheuvel
2020-03-02 17:12   ` [edk2-devel] " Laszlo Ersek
2020-03-03  7:36     ` Laszlo Ersek
2020-03-02  7:29 ` [PATCH 05/13] ArmVirtPkg: incorporate the new QEMU kernel loader driver and library Ard Biesheuvel
2020-03-02 17:15   ` [edk2-devel] " Laszlo Ersek
2020-03-02  7:29 ` [PATCH 06/13] ArmVirtPkg/PlatformBootManagerLib: switch to separate QEMU loader Ard Biesheuvel
2020-03-02 17:26   ` [edk2-devel] " Laszlo Ersek
2020-03-02  7:29 ` [PATCH 07/13] OvmfPkg/QemuKernelLoaderFsDxe: don't expose kernel command line Ard Biesheuvel
2020-03-02 17:31   ` [edk2-devel] " Laszlo Ersek
2020-03-02  7:29 ` [PATCH 08/13] OvmfPkg/QemuKernelLoaderFsDxe: add support for the kernel setup block Ard Biesheuvel
2020-03-02 17:58   ` [edk2-devel] " Laszlo Ersek
2020-03-02  7:29 ` [PATCH 09/13] OvmfPkg: implement QEMU loader library for X86 with legacy fallback Ard Biesheuvel
2020-03-03  9:45   ` [edk2-devel] " Laszlo Ersek
2020-03-03 10:08     ` Ard Biesheuvel
2020-03-03 11:20       ` Laszlo Ersek
2020-03-02  7:29 ` [PATCH 10/13] OvmfPkg: add new QEMU kernel image loader components Ard Biesheuvel
2020-03-03  9:47   ` [edk2-devel] " Laszlo Ersek
2020-03-02  7:29 ` [PATCH 11/13] OvmfPkg/PlatformBootManagerLib: switch to QemuLoadImageLib Ard Biesheuvel
2020-03-03  9:52   ` [edk2-devel] " Laszlo Ersek
2020-03-03  9:53     ` Laszlo Ersek [this message]
2020-03-02  7:29 ` [PATCH 12/13] OvmfPkg/QemuKernelLoaderFsDxe: add support for new Linux initrd device path Ard Biesheuvel
2020-03-03 10:10   ` [edk2-devel] " Laszlo Ersek
2020-03-03 10:18     ` Ard Biesheuvel
2020-03-03 11:27       ` Laszlo Ersek
2020-03-02  7:29 ` [PATCH 13/13] OvmfPkg: use generic QEMU image loader for secure boot enabled builds Ard Biesheuvel
2020-03-03 10:13   ` [edk2-devel] " Laszlo Ersek

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-list from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=b5637541-2291-50cc-961c-4639f01c9cc6@redhat.com \
    --to=devel@edk2.groups.io \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox