On 03/08/19 16:51, Igor Mammedov wrote:
> On Fri, 8 Mar 2019 15:06:12 +0000
> Daniel P. Berrangé <berra...@redhat.com> 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.
>>
>> We've not been very motivated to solve this before 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.
>>
>>>> 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.
>>
>>> - "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
>>
>>
>>
>>> 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'd say it's intended to run on every 'make check' for bios-tables-test
> (arm/x86 variants) like we do now with Seabios. The goal is to catch
> regressions in ACPI table (as we have zero coverage there).
> Secondary motivation is to help with merging NEMU fork. Intel generalizes
> a bunch ACPI code across x86 and ARM boards and I'm not ready to manually
> test every iteration posted on list.
>
> The last time we talked where to put firmware, we've got to consensus
> to bundle UEFI within qemu like we do with other firmware.
> Question of moving firmwares somewhere else is still open but it's
> a separate topic and shouldn't block us from properly testing arm board
> now (and other related projects that depend on it).
>
> As for soft-freeze, maybe it's acceptable to merge this series during it
> as its primary(the only) user would be 'make check'. I'll respin tests
> after rebasing previous iteration on top of this series.
OK, let me try this tonight (hopefully):
(1) I'll employ "truncate" for padding the arm images,
(2) I'll follow Daniel's advice about moving forward to the current edk2
master HEAD (which is pretty close to the upcoming edk2-stable201903
release)
(3) I'll push the series to github or wherever for easy fetching, and
generate the patches (for posting) with --no-binary.
We can discuss better while seeing specifics.
Thanks!
Laszlo
