Philippe Mathieu-Daudé <[email protected]> writes: > On 7/11/22 14:06, Peter Maydell wrote: >> On Mon, 7 Nov 2022 at 13:03, Sunil V L <[email protected]> wrote: >>> >>> The pflash implementation currently assumes fixed size of the >>> backend storage.
Intentional. commit 06f1521795207359a395996c253c306f4ab7586e Author: Markus Armbruster <[email protected]> Date: Tue Mar 19 17:35:50 2019 +0100 pflash: Require backend size to match device, improve errors We reject undersized backends with a rather enigmatic "failed to read the initial flash content" error. For instance: $ qemu-system-ppc64 -S -display none -M sam460ex -drive if=pflash,format=raw,file=eins.img qemu-system-ppc64: Initialization of device cfi.pflash02 failed: failed to read the initial flash content We happily accept oversized images, ignoring their tail. Throwing away parts of firmware that way is pretty much certain to end in an even more enigmatic failure to boot. Require the backend's size to match the device's size exactly. Report mismatch like this: qemu-system-ppc64: Initialization of device cfi.pflash01 failed: device requires 1048576 bytes, block backend provides 512 bytes Improve the error for actual read failures to "can't read block backend". To avoid duplicating even more code between the two pflash device models, do all that in new helper blk_check_size_and_read_all(). The error reporting can still be confusing. For instance: qemu-system-ppc64 -S -display none -M taihu -drive if=pflash,format=raw,file=eins.img -drive if=pflash,unit=1,format=raw,file=zwei.img qemu-system-ppc64: Initialization of device cfi.pflash02 failed: device requires 2097152 bytes, block backend provides 512 bytes Leaves the user guessing which of the two -drive is wrong. Mention the issue in a TODO comment. Suggested-by: Alex Bennée <[email protected]> Signed-off-by: Markus Armbruster <[email protected]> Message-Id: <[email protected]> Reviewed-by: Laszlo Ersek <[email protected]> Reviewed-by: Alex Bennée <[email protected]> Reviewed-by: Philippe Mathieu-Daudé <[email protected]> >>> Due to this, the backend storage file needs to be >>> exactly of size 32M. Otherwise, there will be an error like below. >>> >>> "device requires 33554432 bytes, block backend provides 4194304 bytes" Why is that a problem? Genuine question! >>> Fix this issue by using the actual size of the backing store. >>> >>> Signed-off-by: Sunil V L <[email protected]> >>> --- >> Do you really want the flash device size presented to the guest >> to be variable depending on what the user passed as a block backend? >> I don't think this is how we handle flash devices on other boards... Flash device is generally a property of the machine type. Similar to physical machines. Not an accident. > Ideally handling smaller/bigger backend size should be transparent for > machine frontend, but we never agreed on what are user expectations and > how to deal with such cases. > > Long term I'd go for: > > - if flash is read-only > > a/ bigger backend: display a warning and ignore extra backend data. Truncating images seems unlikely to be useful. > b/ smaller backend: assume flash block is in erased state and fill > missing gap with -1 (the default erase value), displaying a warning > on startup. Padding has a better chance to work. But is it worth the trouble? > > - if flash is read-write > > a/ bigger backend: display a warning and ignore extra backend data. > > b/ smaller backend: add a property to pflash device to handle missing > gap as erased data. If this flag is not set, display a hint and > exit with an error. What happens when the guest writes to the part that isn't backed by the backend? Is this worth the trouble? > In Sunil particular case, I suppose the issue comes from commit > 334c388f25 ("hw/block/pflash_cfi0{1, 2}: Error out if device length > isn't a power of two") which I'm going to revert because the code > base is not ready for such check: > > https://lore.kernel.org/qemu-devel/[email protected]/ > > Regards, > > Phil.
