Re: [Qemu-devel] [PATCH v2] hw/block: report when pflash backing file isn't aligned

["Alex Bennée" <alex.bennee@linaro.org>]

Laszlo Ersek <lersek@redhat.com> writes:

> On 02/15/19 17:01, Markus Armbruster wrote:
>
>> The size of the flash chip is a property of the machine.  It is *fixed*.
>
> I'll have to disagree on this one; in OVMF's case, you can build OVMF in
> 1MB, 2MB, and 4MB *cumulative* size (that is, the numbers I give here
> each refer to the sum of both pflash chips, namely varstore and executable).
>
> The cumulative size that is picked at OVMF build time influences
> (obviously) the amount of crap ^W firmware features that one can squeeze
> into the executable image, but it also determines other things. For
> example, the 4MB build has a different (incompatible!) internal
> structure than the 2MB does. See the small table/comparison in the
> following commit message:
>
>   https://github.com/tianocore/edk2/commit/b24fca05751f
>
> In order to provide some numbers that one can observe simply on their
> host filesystem, the 2MB cumulative size consists of (128K varstore
> chip, 1920KB executable chip); while the 4MB one comprises (528K
> varstore chip, 3568KB executable chip).
>
>> Using whatever size the image has is sloppy modelling.
>
> Maybe so, but it's also very convenient, and also quite important, right
> now (given the multiple firmware image sizes used in the wild).
>
>> A machine may come in minor variations that aren't worth their own
>> machine type.  One such variation could be a different flash chip size.
>> Using the image size to select one from the set of fixed sizes is
>> tolerable.
>
> The problem is that this requires coordination between QEMU and firmware
> development.
>
> (Well, I have to agree that the present patch is *already* that kind of
> coordination; my point is that when I introduced the 4MB build for OVMF,
> I didn't have to touch QEMU. In retrospect, I'm extremely thankful for
> that, as the introduction of the 4MB build was difficult in itself.)
>
> "Using the image size to flexibly dictate the pflash size, in board
> code" would be preferable, to "select from a pre-determined set". (A
> similarly flexible approach would be if we required the user or mgmt app
> to specify base & size as pflash device properties -- see your option #1
> elsewhere --, but I digress.)
>
>> Aside: the image size can change between the place where we get it to
>> pick a flash chip size and realize().
>
> Haha :)
>
>> I guess that's a "don't do that then".
>
> I think so! :)
>
>> If the image size matches the flash chip's size exactly, all is well.
>>
>> Should we require the size to match the flash chip's size?
>
> So, this is hard to answer generally for all targets / boards.
>
> pc_system_flash_init() (on i440fx/q35, i.e. when OVMF is used) will
> guarantee this equality -- ignoring the TOCTOU that you point out above,
> anyway.
>
> For "virt", the answer carries a lot more weight, because *that* board
> code does not size the pflash from the fw image found in the filesystem.
> (See create_flash(), and a15memmap[VIRT_FLASH].)
>
> IIUC, this patch suggests, "yes, we should require equality". A no-op
> for OVMF (= pc_system_flash_init()), and helpful against user mistakes
> with the ArmVirtQemu platform firmware (= create_flash()).
>
>> If we accept a smaller image, we want to pad with zeros.  What about
>> writes beyond the size of the image?  Discard them, or let them extend
>> the image file?
>>
>> If we accept a larger image, we want to ignore its tail.
>>
>> Sorry for turning the tiny issue your patch tries to address into a much
>> larger one...
>
> I think the following would be useful:
>
> - reject images that are larger than the pflash chip size
> - pad smaller images with zero (not on the disk)
> - ignore guest writes to padding
>
> Because, in case of the ArmVirtQemu firmware at least, the build process
> produces the following two files:
>
>   2.0M QEMU_EFI.fd
>   768K QEMU_VARS.fd
>
> And we've always had to manually pad each of these to 64MB, with zeroes,
> on-disk, for use with the "virt" board. If QEMU could do that
> automatically (in memory), that would be a win, in my book.
>
> If Alex thinks such padding is out of scope for now, I will certainly
> not insist; I think the present patch (enforcing equality) is already a
> significant usability improvement. I'd just like the error message to be
> precise, and the error checking to be (more) complete.

I think padding should be in scope for read-only blobs. It does seem
pointless forcing distros to pad blobs that are ultimately never going
to be written to. The only reason I didn't look into it is because I was
unfamiliar with the blk_* api but I guess we can do it all in the pflash
code.

I'll have a look at Markus's clean-ups first and see if I can re-spin
on top of that.

--
Alex Bennée