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.biesheu...@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
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#55227): https://edk2.groups.io/g/devel/message/55227
Mute This Topic: https://groups.io/mt/71669021/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
