| Message ID | 20190307093723.655-3-armbru@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | [RFC,v6,1/4] hw/block: better reporting on pflash backing file mismatch | expand |
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. 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? [*] Message-ID: <87r2bl5oah.fsf@dusky.pond.sub.org> https://lists.nongnu.org/archive/html/qemu-devel/2019-03/msg01115.html
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
On 3/7/19 8:55 AM, Kevin Wolf wrote: >> 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. nbdkit has a filter that allows padding any non-sector-aligned image out to the next sector alignment, where reads from the tail see zero, writes _of zero_ to the tail succeed, but writes of non-zero fail (I don't know if it prefers EIO or ENOSPC, but that's secondary). I have an open bug against the nbd block driver where we can trigger assertions on unaligned file sizes; and it would definitely be nice to fix it globally in the block layer rather than copy-and-paste piecemeal in every affected driver. I may still fix NBD for 4.0 during soft freeze to avoid the assert, but that fix will be a bare-bones bandaid (enough to avoid the assertion failures, and no more). But yes, I definitely agree that a 4.1 project of making bdrv_getlength() be byte-aware coupled with block-layer smarts about handling the tail when widening from a smaller alignment to a larger is going to make a lot of things nicer. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Kevin Wolf <kwolf@redhat.com> writes: > 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) 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!
On 03/07/19 11:05, Markus Armbruster wrote: > 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. I'm fine with that. Thanks, Laszlo > 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. > > 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? > > > [*] Message-ID: <87r2bl5oah.fsf@dusky.pond.sub.org> > https://lists.nongnu.org/archive/html/qemu-devel/2019-03/msg01115.html >
diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c index 75ce8ef489..00980316dc 100644 --- a/hw/block/pflash_cfi01.c +++ b/hw/block/pflash_cfi01.c @@ -759,8 +759,9 @@ static void pflash_cfi01_realize(DeviceState *dev, Error **errp) if (pfl->blk) { /* * Validate the backing store is the right size for pflash - * devices. If the user supplies a larger file we ignore the - * tail. + * devices. If the device is read-only we can elide the check + * and just pad the region first. If the user supplies a + * larger file we ignore the tail. */ int64_t backing_len = blk_getlength(pfl->blk); if (backing_len < 0) { @@ -769,10 +770,21 @@ static void pflash_cfi01_realize(DeviceState *dev, Error **errp) } if (backing_len < total_len) { - error_setg(errp, "device needs %" PRIu64 " bytes, " - "backing file provides only %" PRIu64 " bytes", - total_len, backing_len); - return; + if (pfl->ro) { + size_t pad_bytes = total_len - backing_len; + /* pad with NOR erase pattern */ + memset((uint8_t *)pfl->storage + backing_len, + 0xff, pad_bytes); + warn_report("device needs %" PRIu64 + " bytes, padded with %zu 0xff bytes", + total_len, pad_bytes); + total_len = backing_len; + } else { + error_setg(errp, "device needs %" PRIu64 " bytes, " + "backing file provides only %" PRIu64 " bytes", + total_len, backing_len); + return; + } } else if (backing_len > total_len) { warn_report("device needs %" PRIu64 " bytes, rest ignored", total_len);