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. Thanks! Laszlo
