[PATCH v1 0/3] OvmfPkg: Use QemuKernelLoaderFs to read cmdline/initrd

[PATCH v1 0/3] OvmfPkg: Use QemuKernelLoaderFs to read cmdline/initrd

[Dov Murik]

In order to support measured SEV boot with kernel/initrd/cmdline, we'd
like to have one place that reads those blobs; in the future we'll add
the measurement and verification in that place.

We already have a synthetic filesystem (QemuKernelLoaderFs) which holds
three files: "kernel", "initrd", and "cmdline".  The kernel is indeed
read from this filesystem in LoadImage; but the cmdline (and the length
of initrd) are read from QemuFwCfgLib items.

This patch series modifies GenericQemuLoadImageLib to read cmdline (and
the initrd size) from the QemuKernelLoaderFs synthetic filesystem, thus
removing the dependency on QemuFwCfgLib.

Note that X86QemuLoadImageLib is not modified, because it contains a
QemuLoadLegacyImage() which reads other items of the QemuFwCfg which are
not available in QemuKernelLoaderFs.  Since we don't want to support the
legacy boot path in the future measured SEV boot, we leave
X86QemuLoadImageLib as-is (except for a comment addition in patch 3) and
will force use for GenericQemuLoadImageLib in the measured SEV boot
implementation.

Relevant discussion threads start in:
https://edk2.groups.io/g/devel/message/76069

To test this on x86_64, I forced the use of GenericQemuLoadImageLib
using the following local patch:


diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
index 0a237a905866..46442b543bcf 100644
--- a/OvmfPkg/OvmfPkgX64.dsc
+++ b/OvmfPkg/OvmfPkgX64.dsc
@@ -404,7 +404,7 @@ [LibraryClasses.common.DXE_DRIVER]
   PciLib|OvmfPkg/Library/DxePciLibI440FxQ35/DxePciLibI440FxQ35.inf
   MpInitLib|UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
   QemuFwCfgS3Lib|OvmfPkg/Library/QemuFwCfgS3Lib/DxeQemuFwCfgS3LibFwCfg.inf
-  QemuLoadImageLib|OvmfPkg/Library/X86QemuLoadImageLib/X86QemuLoadImageLib.inf
+  QemuLoadImageLib|OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemuLoadImageLib.inf # XXX don't commit this or someone will be mad
 !if $(TPM_ENABLE) == TRUE
   Tpm12DeviceLib|SecurityPkg/Library/Tpm12DeviceLibTcg/Tpm12DeviceLibTcg.inf
   Tpm2DeviceLib|SecurityPkg/Library/Tpm2DeviceLibTcg2/Tpm2DeviceLibTcg2.inf


I tested boot with QEMU and OVMF with the following QEMU arguments:

  -kernel a
  -kernel a -initrd b
  -kernel a -cmdline c
  -kernel a -initrd b -cmdline c

(and also without -kernel)


Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: James Bottomley <jejb@linux.ibm.com>
Cc: Tobin Feldman-Fitzthum <tobin@linux.ibm.com>

Dov Murik (3):
  Revert "OvmfPkg/QemuKernelLoaderFsDxe: don't expose kernel command
    line"
  OvmfPkg/GenericQemuLoadImageLib: Read cmdline from QemuKernelLoaderFs
  OvmfPkg/X86QemuLoadImageLib: State dependency on fw_cfg in file header

 OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemuLoadImageLib.inf |   2 +-
 OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemuLoadImageLib.c   | 132 ++++++++++++++++++--
 OvmfPkg/Library/X86QemuLoadImageLib/X86QemuLoadImageLib.c           |   3 +
 OvmfPkg/QemuKernelLoaderFsDxe/QemuKernelLoaderFsDxe.c               |  11 +-
 4 files changed, 137 insertions(+), 11 deletions(-)

-- 
2.25.1

[PATCH v1 1/3] Revert "OvmfPkg/QemuKernelLoaderFsDxe: don't expose kernel command line"

[Dov Murik]

This reverts commit efc52d67e1573ce174d301b52fa1577d552c8441.

Manually fixed conflicts in:
  OvmfPkg/QemuKernelLoaderFsDxe/QemuKernelLoaderFsDxe.c

Note that besides re-exposing the kernel command line as a file in the
synthetic filesystem, we also revert back to AllocatePool instead of
AllocatePages.

Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: James Bottomley <jejb@linux.ibm.com>
Cc: Tobin Feldman-Fitzthum <tobin@linux.ibm.com>
Signed-off-by: Dov Murik <dovmurik@linux.ibm.com>
---
 OvmfPkg/QemuKernelLoaderFsDxe/QemuKernelLoaderFsDxe.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/OvmfPkg/QemuKernelLoaderFsDxe/QemuKernelLoaderFsDxe.c b/OvmfPkg/QemuKernelLoaderFsDxe/QemuKernelLoaderFsDxe.c
index b09ff6a3590d..c7ddd86f5c75 100644
--- a/OvmfPkg/QemuKernelLoaderFsDxe/QemuKernelLoaderFsDxe.c
+++ b/OvmfPkg/QemuKernelLoaderFsDxe/QemuKernelLoaderFsDxe.c
@@ -33,6 +33,7 @@
 typedef enum {
   KernelBlobTypeKernel,
   KernelBlobTypeInitrd,
+  KernelBlobTypeCommandLine,
   KernelBlobTypeMax
 } KERNEL_BLOB_TYPE;
 
@@ -59,6 +60,11 @@ STATIC KERNEL_BLOB mKernelBlob[KernelBlobTypeMax] = {
     {
       { QemuFwCfgItemInitrdSize,      QemuFwCfgItemInitrdData,      },
     }
+  }, {
+    L"cmdline",
+    {
+      { QemuFwCfgItemCommandLineSize, QemuFwCfgItemCommandLineData, },
+    }
   }
 };
 
@@ -948,7 +954,7 @@ FetchBlob (
   //
   // Read blob.
   //
-  Blob->Data = AllocatePages (EFI_SIZE_TO_PAGES ((UINTN)Blob->Size));
+  Blob->Data = AllocatePool (Blob->Size);
   if (Blob->Data == NULL) {
     DEBUG ((DEBUG_ERROR, "%a: failed to allocate %Ld bytes for \"%s\"\n",
       __FUNCTION__, (INT64)Blob->Size, Blob->Name));
@@ -1083,8 +1089,7 @@ FreeBlobs:
   while (BlobType > 0) {
     CurrentBlob = &mKernelBlob[--BlobType];
     if (CurrentBlob->Data != NULL) {
-      FreePages (CurrentBlob->Data,
-        EFI_SIZE_TO_PAGES ((UINTN)CurrentBlob->Size));
+      FreePool (CurrentBlob->Data);
       CurrentBlob->Size = 0;
       CurrentBlob->Data = NULL;
     }
-- 
2.25.1

[PATCH v1 2/3] OvmfPkg/GenericQemuLoadImageLib: Read cmdline from QemuKernelLoaderFs

[Dov Murik]

Remove the QemuFwCfgLib interface used to read the QEMU cmdline
(-append argument) and the initrd size.  Instead, use the synthetic
filesystem QemuKernelLoaderFs which has three files: "kernel", "initrd",
and "cmdline".

Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: James Bottomley <jejb@linux.ibm.com>
Cc: Tobin Feldman-Fitzthum <tobin@linux.ibm.com>
Signed-off-by: Dov Murik <dovmurik@linux.ibm.com>
---
 OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemuLoadImageLib.inf |   2 +-
 OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemuLoadImageLib.c   | 132 ++++++++++++++++++--
 2 files changed, 126 insertions(+), 8 deletions(-)

diff --git a/OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemuLoadImageLib.inf b/OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemuLoadImageLib.inf
index b262cb926a4d..f462fd6922cf 100644
--- a/OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemuLoadImageLib.inf
+++ b/OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemuLoadImageLib.inf
@@ -27,12 +27,12 @@ [LibraryClasses]
   DebugLib
   MemoryAllocationLib
   PrintLib
-  QemuFwCfgLib
   UefiBootServicesTableLib
 
 [Protocols]
   gEfiDevicePathProtocolGuid
   gEfiLoadedImageProtocolGuid
+  gEfiSimpleFileSystemProtocolGuid
 
 [Guids]
   gQemuKernelLoaderFsMediaGuid
diff --git a/OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemuLoadImageLib.c b/OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemuLoadImageLib.c
index 114db7e8441f..7d26c912e14f 100644
--- a/OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemuLoadImageLib.c
+++ b/OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemuLoadImageLib.c
@@ -11,9 +11,9 @@
 #include <Base.h>
 #include <Guid/QemuKernelLoaderFsMedia.h>
 #include <Library/DebugLib.h>
+#include <Library/FileHandleLib.h>
 #include <Library/MemoryAllocationLib.h>
 #include <Library/PrintLib.h>
-#include <Library/QemuFwCfgLib.h>
 #include <Library/QemuLoadImageLib.h>
 #include <Library/UefiBootServicesTableLib.h>
 #include <Protocol/DevicePath.h>
@@ -30,6 +30,11 @@ typedef struct {
   KERNEL_FILE_DEVPATH       FileNode;
   EFI_DEVICE_PATH_PROTOCOL  EndNode;
 } KERNEL_VENMEDIA_FILE_DEVPATH;
+
+typedef struct {
+  VENDOR_DEVICE_PATH       VenMediaNode;
+  EFI_DEVICE_PATH_PROTOCOL EndNode;
+} SINGLE_VENMEDIA_NODE_DEVPATH;
 #pragma pack ()
 
 STATIC CONST KERNEL_VENMEDIA_FILE_DEVPATH mKernelDevicePath = {
@@ -51,6 +56,78 @@ STATIC CONST KERNEL_VENMEDIA_FILE_DEVPATH mKernelDevicePath = {
   }
 };
 
+STATIC CONST SINGLE_VENMEDIA_NODE_DEVPATH mQemuKernelLoaderFileSystemDevicePath = {
+  {
+    {
+      MEDIA_DEVICE_PATH, MEDIA_VENDOR_DP,
+      { sizeof (VENDOR_DEVICE_PATH) }
+    },
+    QEMU_KERNEL_LOADER_FS_MEDIA_GUID
+  }, {
+    END_DEVICE_PATH_TYPE, END_ENTIRE_DEVICE_PATH_SUBTYPE,
+    { sizeof (EFI_DEVICE_PATH_PROTOCOL) }
+  }
+};
+
+STATIC
+EFI_STATUS
+GetQemuKernelBlobSize (
+  IN  EFI_FILE_HANDLE     Root,
+  IN  CHAR16              *FileName,
+  OUT UINTN               *Size
+  )
+{
+  EFI_STATUS      Status;
+  EFI_FILE_HANDLE FileHandle;
+  UINT64          FileSize;
+
+  Status = Root->Open (Root, &FileHandle, FileName, EFI_FILE_MODE_READ, 0);
+  if (EFI_ERROR (Status)) {
+    return Status;
+  }
+  Status = FileHandleGetSize (FileHandle, &FileSize);
+  if (EFI_ERROR (Status)) {
+    goto CloseFile;
+  }
+  *Size = FileSize;
+  Status = EFI_SUCCESS;
+CloseFile:
+  FileHandle->Close (FileHandle);
+  return Status;
+}
+
+STATIC
+EFI_STATUS
+ReadWholeQemuKernelBlob (
+  IN  EFI_FILE_HANDLE     Root,
+  IN  CHAR16              *FileName,
+  IN  UINTN               Size,
+  OUT VOID                *Buffer
+  )
+{
+  EFI_STATUS      Status;
+  EFI_FILE_HANDLE FileHandle;
+  UINTN           ReadSize;
+
+  Status = Root->Open (Root, &FileHandle, FileName, EFI_FILE_MODE_READ, 0);
+  if (EFI_ERROR (Status)) {
+    return Status;
+  }
+  ReadSize = Size;
+  Status = FileHandle->Read (FileHandle, &ReadSize, Buffer);
+  if (EFI_ERROR (Status)) {
+    goto CloseFile;
+  }
+  if (ReadSize != Size) {
+    Status = EFI_PROTOCOL_ERROR;
+    goto CloseFile;
+  }
+  Status = EFI_SUCCESS;
+CloseFile:
+  FileHandle->Close (FileHandle);
+  return Status;
+}
+
 /**
   Download the kernel, the initial ramdisk, and the kernel command line from
   QEMU's fw_cfg. The kernel will be instructed via its command line to load
@@ -79,6 +156,10 @@ QemuLoadKernelImage (
   EFI_STATUS                Status;
   EFI_HANDLE                KernelImageHandle;
   EFI_LOADED_IMAGE_PROTOCOL *KernelLoadedImage;
+  EFI_DEVICE_PATH_PROTOCOL  *DevicePathNode;
+  EFI_HANDLE                FsVolumeHandle;
+  EFI_SIMPLE_FILE_SYSTEM_PROTOCOL *FsProtocol;
+  EFI_FILE_HANDLE           Root;
   UINTN                     CommandLineSize;
   CHAR8                     *CommandLine;
   UINTN                     InitrdSize;
@@ -124,8 +205,38 @@ QemuLoadKernelImage (
                   );
   ASSERT_EFI_ERROR (Status);
 
-  QemuFwCfgSelectItem (QemuFwCfgItemCommandLineSize);
-  CommandLineSize = (UINTN)QemuFwCfgRead32 ();
+  //
+  // Open the Qemu Kernel Loader abstract filesystem (volume) which will be
+  // used to read the "initrd" and "cmdline" synthetic files.
+  //
+  DevicePathNode = (EFI_DEVICE_PATH_PROTOCOL *) &mQemuKernelLoaderFileSystemDevicePath;
+  Status = gBS->LocateDevicePath (
+                  &gEfiSimpleFileSystemProtocolGuid,
+                  &DevicePathNode,
+                  &FsVolumeHandle
+                  );
+  if (EFI_ERROR (Status)) {
+    return Status;
+  }
+
+  Status = gBS->HandleProtocol (
+                  FsVolumeHandle,
+                  &gEfiSimpleFileSystemProtocolGuid,
+                  (VOID **)&FsProtocol
+                  );
+  if (EFI_ERROR (Status)) {
+    return Status;
+  }
+
+  Status = FsProtocol->OpenVolume (FsVolumeHandle, &Root);
+  if (EFI_ERROR (Status)) {
+    return Status;
+  }
+
+  Status = GetQemuKernelBlobSize (Root, L"cmdline", &CommandLineSize);
+  if (EFI_ERROR (Status)) {
+    goto CloseRoot;
+  }
 
   if (CommandLineSize == 0) {
     KernelLoadedImage->LoadOptionsSize = 0;
@@ -136,8 +247,10 @@ QemuLoadKernelImage (
       goto UnloadImage;
     }
 
-    QemuFwCfgSelectItem (QemuFwCfgItemCommandLineData);
-    QemuFwCfgReadBytes (CommandLineSize, CommandLine);
+    Status = ReadWholeQemuKernelBlob (Root, L"cmdline", CommandLineSize, CommandLine);
+    if (EFI_ERROR (Status)) {
+      goto FreeCommandLine;
+    }
 
     //
     // Verify NUL-termination of the command line.
@@ -155,8 +268,10 @@ QemuLoadKernelImage (
     KernelLoadedImage->LoadOptionsSize = (UINT32)((CommandLineSize - 1) * 2);
   }
 
-  QemuFwCfgSelectItem (QemuFwCfgItemInitrdSize);
-  InitrdSize = (UINTN)QemuFwCfgRead32 ();
+  Status = GetQemuKernelBlobSize (Root, L"initrd", &InitrdSize);
+  if (EFI_ERROR (Status)) {
+    goto FreeCommandLine;
+  }
 
   if (InitrdSize > 0) {
     //
@@ -193,6 +308,7 @@ QemuLoadKernelImage (
   }
 
   *ImageHandle = KernelImageHandle;
+  Root->Close (Root);
   return EFI_SUCCESS;
 
 FreeCommandLine:
@@ -201,6 +317,8 @@ FreeCommandLine:
   }
 UnloadImage:
   gBS->UnloadImage (KernelImageHandle);
+CloseRoot:
+  Root->Close (Root);
 
   return Status;
 }
-- 
2.25.1

Re: [edk2-devel] [PATCH v1 2/3] OvmfPkg/GenericQemuLoadImageLib: Read cmdline from QemuKernelLoaderFs

[Ard Biesheuvel]

Hello Dov,

On Wed, 9 Jun 2021 at 14:18, Dov Murik <dovmurik@linux.ibm.com> wrote:
>
> Remove the QemuFwCfgLib interface used to read the QEMU cmdline
> (-append argument) and the initrd size.  Instead, use the synthetic
> filesystem QemuKernelLoaderFs which has three files: "kernel", "initrd",
> and "cmdline".
>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: James Bottomley <jejb@linux.ibm.com>
> Cc: Tobin Feldman-Fitzthum <tobin@linux.ibm.com>
> Signed-off-by: Dov Murik <dovmurik@linux.ibm.com>
> ---
>  OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemuLoadImageLib.inf |   2 +-
>  OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemuLoadImageLib.c   | 132 ++++++++++++++++++--
>  2 files changed, 126 insertions(+), 8 deletions(-)
>
> diff --git a/OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemuLoadImageLib.inf b/OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemuLoadImageLib.inf
> index b262cb926a4d..f462fd6922cf 100644
> --- a/OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemuLoadImageLib.inf
> +++ b/OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemuLoadImageLib.inf
> @@ -27,12 +27,12 @@ [LibraryClasses]
>    DebugLib
>    MemoryAllocationLib
>    PrintLib
> -  QemuFwCfgLib
>    UefiBootServicesTableLib
>
>  [Protocols]
>    gEfiDevicePathProtocolGuid
>    gEfiLoadedImageProtocolGuid
> +  gEfiSimpleFileSystemProtocolGuid
>
>  [Guids]
>    gQemuKernelLoaderFsMediaGuid
> diff --git a/OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemuLoadImageLib.c b/OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemuLoadImageLib.c
> index 114db7e8441f..7d26c912e14f 100644
> --- a/OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemuLoadImageLib.c
> +++ b/OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemuLoadImageLib.c
> @@ -11,9 +11,9 @@
>  #include <Base.h>
>  #include <Guid/QemuKernelLoaderFsMedia.h>
>  #include <Library/DebugLib.h>
> +#include <Library/FileHandleLib.h>
>  #include <Library/MemoryAllocationLib.h>
>  #include <Library/PrintLib.h>
> -#include <Library/QemuFwCfgLib.h>
>  #include <Library/QemuLoadImageLib.h>
>  #include <Library/UefiBootServicesTableLib.h>
>  #include <Protocol/DevicePath.h>
> @@ -30,6 +30,11 @@ typedef struct {
>    KERNEL_FILE_DEVPATH       FileNode;
>    EFI_DEVICE_PATH_PROTOCOL  EndNode;
>  } KERNEL_VENMEDIA_FILE_DEVPATH;
> +
> +typedef struct {
> +  VENDOR_DEVICE_PATH       VenMediaNode;
> +  EFI_DEVICE_PATH_PROTOCOL EndNode;
> +} SINGLE_VENMEDIA_NODE_DEVPATH;
>  #pragma pack ()
>
>  STATIC CONST KERNEL_VENMEDIA_FILE_DEVPATH mKernelDevicePath = {
> @@ -51,6 +56,78 @@ STATIC CONST KERNEL_VENMEDIA_FILE_DEVPATH mKernelDevicePath = {
>    }
>  };
>
> +STATIC CONST SINGLE_VENMEDIA_NODE_DEVPATH mQemuKernelLoaderFileSystemDevicePath = {
> +  {
> +    {
> +      MEDIA_DEVICE_PATH, MEDIA_VENDOR_DP,
> +      { sizeof (VENDOR_DEVICE_PATH) }
> +    },
> +    QEMU_KERNEL_LOADER_FS_MEDIA_GUID
> +  }, {
> +    END_DEVICE_PATH_TYPE, END_ENTIRE_DEVICE_PATH_SUBTYPE,
> +    { sizeof (EFI_DEVICE_PATH_PROTOCOL) }
> +  }
> +};
> +
> +STATIC
> +EFI_STATUS
> +GetQemuKernelBlobSize (

Could we please use a better name here? 'QemuKernel' is confusing as
it is used for initrd and cmdline only, in this case, right?

'QemuKernelLoader' is better in this regard, or other suggestions
welcome as well.


> +  IN  EFI_FILE_HANDLE     Root,
> +  IN  CHAR16              *FileName,
> +  OUT UINTN               *Size
> +  )
> +{
> +  EFI_STATUS      Status;
> +  EFI_FILE_HANDLE FileHandle;
> +  UINT64          FileSize;
> +
> +  Status = Root->Open (Root, &FileHandle, FileName, EFI_FILE_MODE_READ, 0);
> +  if (EFI_ERROR (Status)) {
> +    return Status;
> +  }
> +  Status = FileHandleGetSize (FileHandle, &FileSize);
> +  if (EFI_ERROR (Status)) {
> +    goto CloseFile;
> +  }
> +  *Size = FileSize;
> +  Status = EFI_SUCCESS;
> +CloseFile:
> +  FileHandle->Close (FileHandle);
> +  return Status;
> +}
> +
> +STATIC
> +EFI_STATUS
> +ReadWholeQemuKernelBlob (
> +  IN  EFI_FILE_HANDLE     Root,
> +  IN  CHAR16              *FileName,
> +  IN  UINTN               Size,
> +  OUT VOID                *Buffer
> +  )
> +{
> +  EFI_STATUS      Status;
> +  EFI_FILE_HANDLE FileHandle;
> +  UINTN           ReadSize;
> +
> +  Status = Root->Open (Root, &FileHandle, FileName, EFI_FILE_MODE_READ, 0);
> +  if (EFI_ERROR (Status)) {
> +    return Status;
> +  }
> +  ReadSize = Size;
> +  Status = FileHandle->Read (FileHandle, &ReadSize, Buffer);
> +  if (EFI_ERROR (Status)) {
> +    goto CloseFile;
> +  }
> +  if (ReadSize != Size) {
> +    Status = EFI_PROTOCOL_ERROR;
> +    goto CloseFile;
> +  }
> +  Status = EFI_SUCCESS;
> +CloseFile:
> +  FileHandle->Close (FileHandle);
> +  return Status;
> +}
> +
>  /**
>    Download the kernel, the initial ramdisk, and the kernel command line from
>    QEMU's fw_cfg. The kernel will be instructed via its command line to load
> @@ -79,6 +156,10 @@ QemuLoadKernelImage (
>    EFI_STATUS                Status;
>    EFI_HANDLE                KernelImageHandle;
>    EFI_LOADED_IMAGE_PROTOCOL *KernelLoadedImage;
> +  EFI_DEVICE_PATH_PROTOCOL  *DevicePathNode;
> +  EFI_HANDLE                FsVolumeHandle;
> +  EFI_SIMPLE_FILE_SYSTEM_PROTOCOL *FsProtocol;

Slightly tedious, but better to reindent the whole block.

> +  EFI_FILE_HANDLE           Root;
>    UINTN                     CommandLineSize;
>    CHAR8                     *CommandLine;
>    UINTN                     InitrdSize;
> @@ -124,8 +205,38 @@ QemuLoadKernelImage (
>                    );
>    ASSERT_EFI_ERROR (Status);
>
> -  QemuFwCfgSelectItem (QemuFwCfgItemCommandLineSize);
> -  CommandLineSize = (UINTN)QemuFwCfgRead32 ();
> +  //
> +  // Open the Qemu Kernel Loader abstract filesystem (volume) which will be
> +  // used to read the "initrd" and "cmdline" synthetic files.
> +  //
> +  DevicePathNode = (EFI_DEVICE_PATH_PROTOCOL *) &mQemuKernelLoaderFileSystemDevicePath;

No space between cast type and castee please.

> +  Status = gBS->LocateDevicePath (
> +                  &gEfiSimpleFileSystemProtocolGuid,
> +                  &DevicePathNode,
> +                  &FsVolumeHandle
> +                  );
> +  if (EFI_ERROR (Status)) {
> +    return Status;
> +  }
> +
> +  Status = gBS->HandleProtocol (
> +                  FsVolumeHandle,
> +                  &gEfiSimpleFileSystemProtocolGuid,
> +                  (VOID **)&FsProtocol
> +                  );
> +  if (EFI_ERROR (Status)) {
> +    return Status;
> +  }
> +
> +  Status = FsProtocol->OpenVolume (FsVolumeHandle, &Root);
> +  if (EFI_ERROR (Status)) {
> +    return Status;
> +  }
> +
> +  Status = GetQemuKernelBlobSize (Root, L"cmdline", &CommandLineSize);
> +  if (EFI_ERROR (Status)) {
> +    goto CloseRoot;
> +  }
>
>    if (CommandLineSize == 0) {
>      KernelLoadedImage->LoadOptionsSize = 0;
> @@ -136,8 +247,10 @@ QemuLoadKernelImage (
>        goto UnloadImage;
>      }
>
> -    QemuFwCfgSelectItem (QemuFwCfgItemCommandLineData);
> -    QemuFwCfgReadBytes (CommandLineSize, CommandLine);
> +    Status = ReadWholeQemuKernelBlob (Root, L"cmdline", CommandLineSize, CommandLine);
> +    if (EFI_ERROR (Status)) {
> +      goto FreeCommandLine;
> +    }
>
>      //
>      // Verify NUL-termination of the command line.
> @@ -155,8 +268,10 @@ QemuLoadKernelImage (
>      KernelLoadedImage->LoadOptionsSize = (UINT32)((CommandLineSize - 1) * 2);
>    }
>
> -  QemuFwCfgSelectItem (QemuFwCfgItemInitrdSize);
> -  InitrdSize = (UINTN)QemuFwCfgRead32 ();
> +  Status = GetQemuKernelBlobSize (Root, L"initrd", &InitrdSize);
> +  if (EFI_ERROR (Status)) {
> +    goto FreeCommandLine;
> +  }
>
>    if (InitrdSize > 0) {
>      //
> @@ -193,6 +308,7 @@ QemuLoadKernelImage (
>    }
>
>    *ImageHandle = KernelImageHandle;
> +  Root->Close (Root);
>    return EFI_SUCCESS;
>
>  FreeCommandLine:
> @@ -201,6 +317,8 @@ FreeCommandLine:
>    }
>  UnloadImage:
>    gBS->UnloadImage (KernelImageHandle);
> +CloseRoot:
> +  Root->Close (Root);
>
>    return Status;
>  }
> --
> 2.25.1
>
>
>
> ------------
> Groups.io Links: You receive all messages sent to this group.
> View/Reply Online (#76267): https://edk2.groups.io/g/devel/message/76267
> Mute This Topic: https://groups.io/mt/83418869/5717338
> Group Owner: devel+owner@edk2.groups.io
> Unsubscribe: https://edk2.groups.io/g/devel/unsub [ardb+tianocore@kernel.org]
> ------------
>
>

[PATCH v1 3/3] OvmfPkg/X86QemuLoadImageLib: State fw_cfg dependency in file header

[Dov Murik]

Make it clear that X86QemuLoadImageLib relies on fw_cfg; prepare the
ground to add a warning about the incompatibility with boot verification
process.

Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: James Bottomley <jejb@linux.ibm.com>
Cc: Tobin Feldman-Fitzthum <tobin@linux.ibm.com>
Signed-off-by: Dov Murik <dovmurik@linux.ibm.com>
---
 OvmfPkg/Library/X86QemuLoadImageLib/X86QemuLoadImageLib.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/OvmfPkg/Library/X86QemuLoadImageLib/X86QemuLoadImageLib.c b/OvmfPkg/Library/X86QemuLoadImageLib/X86QemuLoadImageLib.c
index 1177582ab051..dc9018f4333b 100644
--- a/OvmfPkg/Library/X86QemuLoadImageLib/X86QemuLoadImageLib.c
+++ b/OvmfPkg/Library/X86QemuLoadImageLib/X86QemuLoadImageLib.c
@@ -2,6 +2,9 @@
   X86 specific implementation of QemuLoadImageLib library class interface
   with support for loading mixed mode images and non-EFI stub images
 
+  Note that this implementation reads the cmdline (and possibly kernel, setup
+  data, and initrd in the legacy boot mode) from fw_cfg directly.
+
   Copyright (c) 2006 - 2015, Intel Corporation. All rights reserved.<BR>
   Copyright (c) 2020, ARM Ltd. All rights reserved.<BR>
 
-- 
2.25.1

Re: [edk2-devel] [PATCH v1 3/3] OvmfPkg/X86QemuLoadImageLib: State fw_cfg dependency in file header

[Laszlo Ersek]

On 06/09/21 14:18, Dov Murik wrote:
> Make it clear that X86QemuLoadImageLib relies on fw_cfg; prepare the
> ground to add a warning about the incompatibility with boot verification
> process.
> 
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: James Bottomley <jejb@linux.ibm.com>
> Cc: Tobin Feldman-Fitzthum <tobin@linux.ibm.com>
> Signed-off-by: Dov Murik <dovmurik@linux.ibm.com>
> ---
>  OvmfPkg/Library/X86QemuLoadImageLib/X86QemuLoadImageLib.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/OvmfPkg/Library/X86QemuLoadImageLib/X86QemuLoadImageLib.c b/OvmfPkg/Library/X86QemuLoadImageLib/X86QemuLoadImageLib.c
> index 1177582ab051..dc9018f4333b 100644
> --- a/OvmfPkg/Library/X86QemuLoadImageLib/X86QemuLoadImageLib.c
> +++ b/OvmfPkg/Library/X86QemuLoadImageLib/X86QemuLoadImageLib.c
> @@ -2,6 +2,9 @@
>    X86 specific implementation of QemuLoadImageLib library class interface
>    with support for loading mixed mode images and non-EFI stub images
>  
> +  Note that this implementation reads the cmdline (and possibly kernel, setup
> +  data, and initrd in the legacy boot mode) from fw_cfg directly.
> +
>    Copyright (c) 2006 - 2015, Intel Corporation. All rights reserved.<BR>
>    Copyright (c) 2020, ARM Ltd. All rights reserved.<BR>
>  
> 

(drive-by comment, no capacity for more atm):

please update the INF file's comment at the top similarly

Thanks
Laszlo

Re: [edk2-devel] [PATCH v1 3/3] OvmfPkg/X86QemuLoadImageLib: State fw_cfg dependency in file header

[Ard Biesheuvel]

On Wed, 9 Jun 2021 at 16:39, Laszlo Ersek <lersek@redhat.com> wrote:
>
> On 06/09/21 14:18, Dov Murik wrote:
> > Make it clear that X86QemuLoadImageLib relies on fw_cfg; prepare the
> > ground to add a warning about the incompatibility with boot verification
> > process.
> >
> > Cc: Laszlo Ersek <lersek@redhat.com>
> > Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
> > Cc: Jordan Justen <jordan.l.justen@intel.com>
> > Cc: James Bottomley <jejb@linux.ibm.com>
> > Cc: Tobin Feldman-Fitzthum <tobin@linux.ibm.com>
> > Signed-off-by: Dov Murik <dovmurik@linux.ibm.com>
> > ---
> >  OvmfPkg/Library/X86QemuLoadImageLib/X86QemuLoadImageLib.c | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/OvmfPkg/Library/X86QemuLoadImageLib/X86QemuLoadImageLib.c b/OvmfPkg/Library/X86QemuLoadImageLib/X86QemuLoadImageLib.c
> > index 1177582ab051..dc9018f4333b 100644
> > --- a/OvmfPkg/Library/X86QemuLoadImageLib/X86QemuLoadImageLib.c
> > +++ b/OvmfPkg/Library/X86QemuLoadImageLib/X86QemuLoadImageLib.c
> > @@ -2,6 +2,9 @@
> >    X86 specific implementation of QemuLoadImageLib library class interface
> >    with support for loading mixed mode images and non-EFI stub images
> >
> > +  Note that this implementation reads the cmdline (and possibly kernel, setup
> > +  data, and initrd in the legacy boot mode) from fw_cfg directly.
> > +
> >    Copyright (c) 2006 - 2015, Intel Corporation. All rights reserved.<BR>
> >    Copyright (c) 2020, ARM Ltd. All rights reserved.<BR>
> >
> >
>
> (drive-by comment, no capacity for more atm):
>
> please update the INF file's comment at the top similarly
>

+1