Re: [edk2-devel] [PATCH 08/13] OvmfPkg/QemuKernelLoaderFsDxe: add support for the kernel setup block

["Laszlo Ersek" <lersek@redhat.com>]

On 03/02/20 08:29, Ard Biesheuvel wrote:
> On x86, the kernel image consists of a setup block and the actual kernel,
> and QEMU presents these as separate blobs, whereas on disk (and in terms
> of PE/COFF image signing), they consist of a single image.
> 
> So add support to our FS loader driver to expose files via the abstract
> file system that consist of up to two concatenated blobs, and redefine
> the kernel file so it consists of the setup and kernel blobs, on every
> architecture (on non-x86, the setup block is simply 0 bytes and is
> therefore ignored implicitly)
> 
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2566
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  OvmfPkg/QemuKernelLoaderFsDxe/QemuKernelLoaderFsDxe.c | 70 ++++++++++++++------
>  1 file changed, 49 insertions(+), 21 deletions(-)
> 
> diff --git a/OvmfPkg/QemuKernelLoaderFsDxe/QemuKernelLoaderFsDxe.c b/OvmfPkg/QemuKernelLoaderFsDxe/QemuKernelLoaderFsDxe.c
> index b8d64e2781fc..77d8fedb738a 100644
> --- a/OvmfPkg/QemuKernelLoaderFsDxe/QemuKernelLoaderFsDxe.c
> +++ b/OvmfPkg/QemuKernelLoaderFsDxe/QemuKernelLoaderFsDxe.c
> @@ -34,16 +34,29 @@ typedef enum {
>  } KERNEL_BLOB_TYPE;
>  
>  typedef struct {
> -  FIRMWARE_CONFIG_ITEM CONST SizeKey;
> -  FIRMWARE_CONFIG_ITEM CONST DataKey;
> -  CONST CHAR16 *       CONST Name;
> -  UINT32                     Size;
> -  UINT8                      *Data;
> +  CONST CHAR16                  Name[8];
> +  struct {
> +    FIRMWARE_CONFIG_ITEM CONST  SizeKey;
> +    FIRMWARE_CONFIG_ITEM CONST  DataKey;
> +    UINT32                      Size;
> +  }                             FwCfgItem[2];
> +  UINT32                        Size;
> +  UINT8                         *Data;
>  } KERNEL_BLOB;
>  
>  STATIC KERNEL_BLOB mKernelBlob[KernelBlobTypeMax] = {
> -  { QemuFwCfgItemKernelSize,      QemuFwCfgItemKernelData,      L"kernel"  },
> -  { QemuFwCfgItemInitrdSize,      QemuFwCfgItemInitrdData,      L"initrd"  },
> +  {
> +    L"kernel",
> +    {
> +      { QemuFwCfgItemKernelSetupSize, QemuFwCfgItemKernelSetupData, },
> +      { QemuFwCfgItemKernelSize,      QemuFwCfgItemKernelData,      },
> +    }
> +  }, {
> +    L"initrd",
> +    {
> +      { QemuFwCfgItemInitrdSize,      QemuFwCfgItemInitrdData,      },
> +    }
> +  }
>  };
>  
>  STATIC UINT64 mTotalBlobBytes;
> @@ -850,12 +863,20 @@ FetchBlob (
>    )
>  {
>    UINT32 Left;
> +  UINTN  Idx;
> +  UINT8  *ChunkData;
>  
>    //
>    // Read blob size.
>    //
> -  QemuFwCfgSelectItem (Blob->SizeKey);
> -  Blob->Size = QemuFwCfgRead32 ();
> +  Blob->Size = 0;
> +  for (Idx = 0; Idx < ARRAY_SIZE (Blob->FwCfgItem); Idx++) {
> +    if (Blob->FwCfgItem[Idx].SizeKey == 0) {
> +      break;
> +    }
> +    QemuFwCfgSelectItem (Blob->FwCfgItem[Idx].SizeKey);
> +    Blob->Size += Blob->FwCfgItem[Idx].Size = QemuFwCfgRead32 ();

(1) Please break up these assignments into two statements.

> +  }
>    if (Blob->Size == 0) {
>      return EFI_SUCCESS;
>    }
> @@ -872,18 +893,25 @@ FetchBlob (
>  
>    DEBUG ((DEBUG_INFO, "%a: loading %Ld bytes for \"%s\"\n", __FUNCTION__,
>      (INT64)Blob->Size, Blob->Name));
> -  QemuFwCfgSelectItem (Blob->DataKey);
> -
> -  Left = Blob->Size;
> -  do {
> -    UINT32 Chunk;
> -
> -    Chunk = (Left < SIZE_1MB) ? Left : SIZE_1MB;
> -    QemuFwCfgReadBytes (Chunk, Blob->Data + (Blob->Size - Left));
> -    Left -= Chunk;
> -    DEBUG ((DEBUG_VERBOSE, "%a: %Ld bytes remaining for \"%s\"\n",
> -      __FUNCTION__, (INT64)Left, Blob->Name));
> -  } while (Left > 0);
> +
> +  ChunkData = Blob->Data;
> +  for (Idx = 0; Idx < ARRAY_SIZE (Blob->FwCfgItem); Idx++) {
> +    QemuFwCfgSelectItem (Blob->FwCfgItem[Idx].DataKey);

(2) For the initrd, this will write a zero selector when (Idx==1), if I
understand correctly. We shouldn't do that; please break out of the loop
early, like in the previous loop. (Check either SizeKey or DataKey
against 0.)

> +
> +    Left = Blob->FwCfgItem[Idx].Size;
> +    do {

Previously, the "do" loop was appropriate, because "Left" was guaranteed
positive here. That's no longer true: according to your description, for
non-x86, the setup block has zero size. In that case, we shouldn't enter
the inner loop body at all.

(3) So please turn this into a "while" loop.

> +      UINT32 Chunk;
> +
> +      Chunk = (Left < SIZE_1MB) ? Left : SIZE_1MB;
> +      QemuFwCfgReadBytes (Chunk, ChunkData + Blob->FwCfgItem[Idx].Size - Left);
> +      Left -= Chunk;
> +      DEBUG ((DEBUG_VERBOSE, "%a: %Ld bytes remaining for \"%s\" (%d)\n",
> +        __FUNCTION__, (INT64)Left, Blob->Name, Idx));

(4) Idx is a UINTN, we shouldn't log it with "%d". The fully portable
approach is to use %Lu as the format specifier and cast Idx to UINT64.

If we are sure Idx fits into an INT32, then we can stick with %d, but we
should still cast Idx to INT32.

> +    } while (Left > 0);
> +
> +    ChunkData += Blob->FwCfgItem[Idx].Size;
> +  }
> +
>    return EFI_SUCCESS;
>  }
>  
> 

Thanks
Laszlo