From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-1.mimecast.com (us-smtp-delivery-1.mimecast.com [205.139.110.61]) by mx.groups.io with SMTP id smtpd.web09.18559.1583171922611526098 for ; Mon, 02 Mar 2020 09:58:42 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=i03vVhCm; spf=pass (domain: redhat.com, ip: 205.139.110.61, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1583171921; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=kkzOFQajBNr8HEXGr5ml3wLujs3ChcLUm31zsmgfrYs=; b=i03vVhCmEDJYnRLr3jqbJpSDRkFnpyShqcvOSmqyefvLkoGzQ0kgsvWYBcWRyOl/cNEbb0 +tZMAS2k//V8M56uLdx4zAQdJYYGHQ1IQEP7bd1XT+lls0rkWaDjQJRhXcTUPgYTkSgs2z J6dYR6AMFTIYoLT+90kcC8jr/90oTaw= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-348-wuCnRY5ZNuGzTjkG0bK8LA-1; Mon, 02 Mar 2020 12:58:37 -0500 X-MC-Unique: wuCnRY5ZNuGzTjkG0bK8LA-1 Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 6BADA19251D1; Mon, 2 Mar 2020 17:58:36 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-117-197.ams2.redhat.com [10.36.117.197]) by smtp.corp.redhat.com (Postfix) with ESMTP id 9B2EA8C09A; Mon, 2 Mar 2020 17:58:35 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH 08/13] OvmfPkg/QemuKernelLoaderFsDxe: add support for the kernel setup block To: devel@edk2.groups.io, ard.biesheuvel@linaro.org References: <20200302072936.29221-1-ard.biesheuvel@linaro.org> <20200302072936.29221-9-ard.biesheuvel@linaro.org> From: "Laszlo Ersek" Message-ID: Date: Mon, 2 Mar 2020 18:58:34 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <20200302072936.29221-9-ard.biesheuvel@linaro.org> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.11 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit 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 > --- > 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