Re: [Qemu-devel] bundling edk2 platform firmware images with QEMU

[Laszlo Ersek <lersek@redhat.com>]

On 03/08/19 16:06, Daniel P. Berrangé wrote:
> On Fri, Mar 08, 2019 at 03:48:07PM +0100, Laszlo Ersek wrote:
>> On 03/08/19 14:42, Daniel P. Berrangé wrote:
>>>>  pc-bios/edk2-aarch64-code.fd                   | Bin 0 -> 67108864 bytes
>>>>  pc-bios/edk2-arm-code.fd                       | Bin 0 -> 67108864 bytes
>>>>  pc-bios/edk2-arm-vars.fd                       | Bin 0 -> 67108864 bytes
>>>>  pc-bios/edk2-i386-code.fd                      | Bin 0 -> 3653632 bytes
>>>>  pc-bios/edk2-i386-secure-code.fd               | Bin 0 -> 3653632 bytes
>>>>  pc-bios/edk2-i386-vars.fd                      | Bin 0 -> 540672 bytes
>>>>  pc-bios/edk2-licenses.txt                      | 209 +++++++++++++++
>>>>  pc-bios/edk2-x86_64-code.fd                    | Bin 0 -> 3653632 bytes
>>>>  pc-bios/edk2-x86_64-secure-code.fd             | Bin 0 -> 3653632 bytes
>>>
>>>
>>> Yikes, am I really reading those sizes right ? The biggest ROMs there
>>> are 64 MB, so this is proposing to add ~206 MB of binaries to the
>>> pc-bios directory ?
>>
>> Sort of.
>>
>> This is because of how the "virt" machine type's pflash chips are sized.
>> The edk2 build produces a 2MB firmware executable and a 768KB varstore
>> template for each of aarch64 and arm, but (a) QEMU doesn't auto-pad
>> either [*], and (b) if we used qcow2 with compression, then libvirt
>> couldn't deal with the images [**]. Hence we have to pad the files
>> manually, after the edk2 build completes.
>>
>> [*] We've just been discussing auto-padding in a parallel patch from
>> Alex and Markus (the latest version is "[PATCH v7] pflash: Require
>> backend size to match device, improve errors", in which the padding has
>> already been dropped). But such padding would only work for read-only
>> pflash mappings anyway, not for variable store files.
>>
>> [**] The root cause for not using qcow2 with pflash images is that qcow2
>> would make them candidates for "savevm" to dump live memory contents
>> into them, which is Very Bad (TM).
> 
> If using  qcow2 compressed images for ROMs is the desired best answer,
> then surely we can figure out a way to fix this savevm problem ? Even
> if it is just an internal hack there must be a way we can mark a qcow2
> file as being used by the ROM so savevm ignores it.

This was one of the first ideas (if not "the" first one) that came up years ago (in January 2016), and it was rejected. I'm not saying it's wrong, but it was rejected.

* [Qemu-devel] [PATCH 1/1] blk: do not select PFLASH device for internal snapshot
  https://lists.gnu.org/archive/html/qemu-devel/2016-01/msg01824.html
  http://mid.mail-archive.com/1452578622-4492-1-git-send-email-den@openvz.org

* https://bugzilla.redhat.com/show_bug.cgi?id=1180955

* https://bugzilla.redhat.com/show_bug.cgi?id=1214187

> We've not been very motivated to solve this before

We gave up in frustration, not because we didn't care.

The kludge that we put in place (in libvirtd) simply prevents us from "going there" now, through libvirt anyway:

* https://bugzilla.redhat.com/show_bug.cgi?id=1431852
* commit 9e2465834f4b ("qemu: snapshot: Forbid internal snapshots with pflash firmware", 2017-03-01)

> since "savevm" command
> is supposedly deprecated to be replaced by a sane QMP variant, but we've
> been waiting 5 years for that to happen, so in the mean time a hack to
> resolve the problem with firmware images feels prudent.

I've attempted this route multiple times in the past, speaking in general. Folks generally keep rejecting patches they have rejected  before.

(And if they do change their minds, just because a number of years have passed, then I'm left wondering how that is reasonable.

That's not to say that I wouldn't be happy about it, of course -- I just don't have capacity for likely futile tries.)

>>> Consider that we'll need to refresh those ROMs multiple times a year to
>>> track updates or security fixes. It will result in our .git repo size
>>> growing massively over time.
>>
>> Actually, it's not *that* bad. Earlier I investigated how the formatted
>> variant of the patch (adding these blobs) would look like.
>>
>> First, I compressed all the "edk2-*fd" files (individually) with "gzip
>> --best", just to get a byte count. That yielded almost precisely 9MB, in
>> total.
>>
>> Second, the formatted patch, adding these blobs, is 12.6MB. Taking the
>> 4/3 blowup factor from base64 encoding, the "unexplained overhead" is
>> not that huge (0.6 MB).
> 
> Ok, that's less alarming.
> 
> Though we're still basically doubling the size of the pc-bios dir
> which is already significant.
> 
> I wonder how much of out .git/objects 230 MB size is due to the current
> ROMs.

I think it would be awesome if git provided some statistics like this! "What is the footprint of the full history of this set of files?"

> 
>> - "git checkout" is smart enough to punch holes into the files in the
>> working directory.
>>
>> $ du -csh edk2-{arm,aarch64}-code.fd edk2-arm-vars.fd
>> 2.1M    edk2-arm-code.fd
>> 2.1M    edk2-aarch64-code.fd
>> 772K    edk2-arm-vars.fd
>> 4.8M    total
> 
> That's nice.
> 
>> This happens despite the fact that the Makefile performs the padding
>> with "cat /dev/zero" and "head".
> 
> You can resize a file using holes with dd if you set count=0 and
> tell it to seek in output eg something like this probably works:
> 
>   dd if=/dev/zero of=firmware.img seek=64 bs=1M count=0

Well the actual command I use is

  cat FILE-TO-PAD /dev/zero | head -c 64M > PADDED-FILE

Maybe this can be reworked to

  cp FILE-TO-PAD PADDED-FILE
  [dd command with conv=notrunc?]

It would be even easier with the "truncate" utility:

  cp FILE-TO-PAD PADDED-FILE
  truncate --size=64M PADDED-FILE

"truncate" is part of GNU coreutils, so maybe I should use "truncate" after all.

I'll work this into the series.

>> I'm 100% fine with not bundling any firmware binaries for end-users;
>> however I've been doing this work because Igor needs full platform
>> firmware binaries for ACPI unit tests. In particular, in aarch64 guests,
>> there is no ACPI without UEFI, and in UEFI guests, finding the ACPI
>> entry point (RSD PTR) is convoluted enough that it needs dedicated guest
>> support.
> 
> Yes tricky. I don't have a particularly good answer for that, especially
> if it is a test that you'd expect all developers to run every time. If
> it is a rarely run test then it could be justified in having it download
> large blobs from a lookaside cache similar to how we download disk images
> for *BSD tests.

I think it would be part of "make check".

Thanks
Laszlo