[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [RFC PATCH v6 2/4] hw/block: Pad undersized read-only i
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-devel] [RFC PATCH v6 2/4] hw/block: Pad undersized read-only images with 0xFF |
Date: |
Thu, 07 Mar 2019 18:57:36 +0100 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/26.1 (gnu/linux) |
Kevin Wolf <address@hidden> writes:
> Am 07.03.2019 um 11:05 hat Markus Armbruster geschrieben:
>> Markus Armbruster <address@hidden> writes:
>>
>> > From: Alex Bennée <address@hidden>
>> >
>> > 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 <address@hidden>
>> > Reviewed-by: Laszlo Ersek <address@hidden>
>> > Signed-off-by: Markus Armbruster <address@hidden>
>>
>> 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)
By the way, when you write to the padding, the block layer grows the
file to the next multiple of 512.
>>
>> 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.
Yes, it could well turn out to be overly restrictive.
> 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.
Makes sense to me.
Could perhaps be done in or near blkconf_blocksizes().
>> 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.
Okay. Let's take just the error reporting improvment then (PATCH 1 and
its fixup). Since we all seem to agree on upgrading the warning to an
error, do that. Don't take the convenience feature now (PATCH 2 and its
fixup). We can always revisit it later.
This should give us a good chance at getting it done in time for the
soft freeze.
I'll prepare v7.
Thanks!