Re: [Qemu-devel] [RFC PATCH v6 2/4] hw/block: Pad undersized read-only images with 0xFF

[Kevin Wolf <kwolf@redhat.com>]

Am 07.03.2019 um 11:05 hat Markus Armbruster geschrieben:
> Markus Armbruster <armbru@redhat.com> writes:
> 
> > From: Alex Bennée <alex.bennee@linaro.org>
> >
> > We reject undersized images.  As of the previous commit, even with a
> > decent error message.  Still, this is a potentially confusing
> > stumbling block when you move from using -bios to using -drive
> > if=pflash,file=blob,format=raw,readonly for loading your firmware
> > code. To mitigate that we automatically pad in the read-only case and
> > warn the user when we have performed magic to enable things to Just
> > Work (tm).
> >
> > Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> > Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> > Signed-off-by: Markus Armbruster <armbru@redhat.com>
> 
> I think this convenience feature is a bad idea, and this patch should
> not be applied.  Here are my reasons.
> 
> 1. As I explained in my disccussion of v5[*], there is no single true
>    way to pad.  This patch pads with 0xFF.  When working with physical
>    devices, you'd sometimes pad that way, but at other times, you'd pad
>    differently.
> 
> 2. Except this patch doesn't *actually* pad with 0xFF.  The block layer
>    silently pads with zeros up to the next multiple of 512.  Evidence:
> 
>     $ yes | dd of=4090b.img bs=1 count=4090
>     4090+0 records in
>     4090+0 records out
>     4090 bytes (4.1 kB, 4.0 KiB) copied, 0.0186459 s, 219 kB/s
>     $ qemu-io -f raw -c 'read -v 4000 96' 4090b.img
>     00000fa0:  79 0a 79 0a 79 0a 79 0a 79 0a 79 0a 79 0a 79 0a  y.y.y.y.y.y.y.y.
>     00000fb0:  79 0a 79 0a 79 0a 79 0a 79 0a 79 0a 79 0a 79 0a  y.y.y.y.y.y.y.y.
>     00000fc0:  79 0a 79 0a 79 0a 79 0a 79 0a 79 0a 79 0a 79 0a  y.y.y.y.y.y.y.y.
>     00000fd0:  79 0a 79 0a 79 0a 79 0a 79 0a 79 0a 79 0a 79 0a  y.y.y.y.y.y.y.y.
>     00000fe0:  79 0a 79 0a 79 0a 79 0a 79 0a 79 0a 79 0a 79 0a  y.y.y.y.y.y.y.y.
>     00000ff0:  79 0a 79 0a 79 0a 79 0a 79 0a 00 00 00 00 00 00  y.y.y.y.y.......
>     read 96/96 bytes at offset 4000
>     96 bytes, 1 ops; 0.0001 sec (694.444 KiB/sec and 7407.4074 ops/sec)
> 
>    This patch then pads some more with 0xFF to the flash memory size.
> 
>    Because of that the way the magic padding works makes no sense, to be
>    frank.  Going back to v3's zero padding would at least be explainable
>    without blushing.
> 
>    I consider the block layer's padding a misfeature here, but right now
>    we got to play the hand we've been dealt.

That will be solved as soon as the block layer is consistently converted
to byte granularity. We've already converted a lot, but bdrv_getlength()
is still sector (512 bytes) granularity.

You could argue that file-posix should just reject files that are not
sector aligned, but that's probably not right either because image
formats don't necessarily have that alignment for their files.

Maybe disk device should reject being attached to nodes that aren't a
multiple of their physical and logical sector size. A 512-byte aligned
image is probably suitable for most disks, but might not be for a virtual
native 4k disk.

> 3. Convenience magic has bitten us in the posterior so many times.  Just
>    say no unless there's a really compelling use case.  Where's the use
>    case for this one?  We've rejected undersized images for ages, and
>    nobody complained.  Why add convenience magic now?

I agree.

Kevin