| Message ID | 20190227111347.15063-1-alex.bennee@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | [v5] hw/block: better reporting on pflash backing file mismatch | expand |
Patchew URL: https://patchew.org/QEMU/20190227111347.15063-1-alex.bennee@linaro.org/
Hi,
This series seems to have some coding style problems. See output below for
more information:
Message-id: 20190227111347.15063-1-alex.bennee@linaro.org
Subject: [Qemu-devel] [PATCH v5] hw/block: better reporting on pflash backing file mismatch
Type: series
=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===
Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
Switched to a new branch 'test'
276ae9a9a2 hw/block: better reporting on pflash backing file mismatch
=== OUTPUT BEGIN ===
ERROR: "(foo*)" should be "(foo *)"
#70: FILE: hw/block/pflash_cfi01.c:769:
+ memset((uint8_t*)pfl->storage + backing_len, 0xff, pad_bytes);
WARNING: line over 80 characters
#82: FILE: hw/block/pflash_cfi01.c:781:
+ warn_report("device needs %" PRIu64 " bytes, rest ignored", total_len);
total: 1 errors, 1 warnings, 58 lines checked
Commit 276ae9a9a2ca (hw/block: better reporting on pflash backing file mismatch) has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
=== OUTPUT END ===
Test command exited with code: 1
The full log is available at
http://patchew.org/logs/20190227111347.15063-1-alex.bennee@linaro.org/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@redhat.com
You neglected to cc: the maintainers of hw/block, I fixed that for you. Alex Bennée <alex.bennee@linaro.org> writes: > It looks like there was going to be code to check we had some sort of > alignment so lets replace it with an actual check. This is a bit more > useful than the enigmatic "failed to read the initial flash content" > when we attempt to read the number of bytes the device should have. > > This is a potential 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> Philippe and I talked about various pflash issues last night. He explained to me how physical flash memory works and is used. This brought back my doubts on the wisdom of automatic padding. Errors in my recounting of his explanations are almost certainly entirely mine. Please correct them. We're talking about NOR flash. NAND flash works differently. You can: * Read a cell. * Write a cell: change it from 1 to 0. * Erase a whole sector (block): change all cells to 1. This is slow, burns power, and you can do it only so often before the flash wears out Say your physical machine has 1 MiB of NOR flash in 16 sectors of 64 KiB each (unrealistic, as Philippe has pointed out elsewhere, but it'll do here). You compile your firmware, and the build process spits out a flat image of 200000 bytes. Here are a few distinct ways to deploy it to your freshly erased flash memory: (1) You write your image to the flash. Everything after byte 200000 remains writable. This is nice for development. With a bit of ingenuity, you can come up with a patching scheme that lets you avoid rewriting the whole flash for every little fix, saving flash wear. (2) You zero-pad your image to the full flash size, and write that to the flash. Everything after byte 200000 becomes unwritable. You can't erase the first 4 blocks (they hold your firmware), but you can still erase the remaining 12. (3) You zero-pad your image to the next sector boundary, and write that to the flash. The remainder of block 4 becomes unwritable (and you can't erase the block without destroying your firmware). The remaining 12 blocks remain writable. This is commonly done for production, because it reduces the ways a sector holding code can be corrupted, making its checksum invalid. My point is: in the physical world, there is no single true way to pad. Back to your patch. I think it conflates three changes: * We reject an undersized image with a sub-optimal error message. Improve that message. * We silently ignore an oversized image's tail. Warn instead. * As a convenience feature, don't reject undersized read-only image, but pad it with 0xff instead, to simulate (1) above. Squashing the first two under a "better reporting on pflash backing file mismatch" heading seems fine to me. The last one is not about "better reporting", and should therefore be a separate patch. I'm willing to do the split in the respin of my pflash fixes series. For the record, I'd summarily reject oversized images, and I'd drop the convenience feature, but I'm not the maintainer here. It's up to Kevin and Max. > --- > v3 > - tweak commit title/commentary > - use total_len instead of device_len for checks > - if the device is read-only do the padding for them > - accept baking_len > total_len (how to warn_report with NULL *errp?) > v4 > - error check blk_getlength > - optimise memset and use NOR erase pattern > - restore singular device (overly confusing) > - add warn_report for when we do magic > v5 > - remove mention of null padding > - use %zu for size_t fmt string > - add Laszlo r-b > --- > hw/block/pflash_cfi01.c | 40 +++++++++++++++++++++++++++++++++------- > 1 file changed, 33 insertions(+), 7 deletions(-) > > diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c > index 9d1c356eb6..d8cfa4789a 100644 > --- a/hw/block/pflash_cfi01.c > +++ b/hw/block/pflash_cfi01.c > @@ -45,6 +45,7 @@ > #include "qemu/bitops.h" > #include "qemu/host-utils.h" > #include "qemu/log.h" > +#include "qemu/error-report.h" > #include "hw/sysbus.h" > #include "sysemu/sysemu.h" > #include "trace.h" > @@ -730,13 +731,6 @@ static void pflash_cfi01_realize(DeviceState *dev, Error **errp) > } > device_len = sector_len_per_device * blocks_per_device; > > - /* XXX: to be fixed */ > -#if 0 > - if (total_len != (8 * 1024 * 1024) && total_len != (16 * 1024 * 1024) && > - total_len != (32 * 1024 * 1024) && total_len != (64 * 1024 * 1024)) > - return NULL; > -#endif > - > memory_region_init_rom_device( > &pfl->mem, OBJECT(dev), > &pflash_cfi01_ops, > @@ -763,6 +757,38 @@ static void pflash_cfi01_realize(DeviceState *dev, Error **errp) > } > > if (pfl->blk) { > + /* > + * Validate the backing store is the right size for pflash > + * devices. It should be padded to a multiple of the flash > + * block size. 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) { > + error_setg(errp, "unable to check size of backing file"); > + return; > + } > + > + if (backing_len < total_len) { > + 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); If I add this patch to my series, I can fix up the white-space to make checkpatch happy. > + 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); Likewise, I can break this line. > + } > + > /* read the initial flash content */ > ret = blk_pread(pfl->blk, 0, pfl->storage, total_len);
On 03/05/19 16:33, Markus Armbruster wrote: > You neglected to cc: the maintainers of hw/block, I fixed that for you. > > Alex Bennée <alex.bennee@linaro.org> writes: > >> It looks like there was going to be code to check we had some sort of >> alignment so lets replace it with an actual check. This is a bit more >> useful than the enigmatic "failed to read the initial flash content" >> when we attempt to read the number of bytes the device should have. >> >> This is a potential 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> > > Philippe and I talked about various pflash issues last night. He > explained to me how physical flash memory works and is used. This > brought back my doubts on the wisdom of automatic padding. > > Errors in my recounting of his explanations are almost certainly > entirely mine. Please correct them. > > We're talking about NOR flash. NAND flash works differently. > > You can: > > * Read a cell. > > * Write a cell: change it from 1 to 0. > > * Erase a whole sector (block): change all cells to 1. This is slow, > burns power, and you can do it only so often before the flash wears > out > > Say your physical machine has 1 MiB of NOR flash in 16 sectors of 64 KiB > each (unrealistic, as Philippe has pointed out elsewhere, but it'll do > here). You compile your firmware, and the build process spits out a > flat image of 200000 bytes. Here are a few distinct ways to deploy it > to your freshly erased flash memory: > > (1) You write your image to the flash. Everything after byte 200000 > remains writable. This is nice for development. With a bit of > ingenuity, you can come up with a patching scheme that lets you avoid > rewriting the whole flash for every little fix, saving flash wear. > > (2) You zero-pad your image to the full flash size, and write that to > the flash. Everything after byte 200000 becomes unwritable. You can't > erase the first 4 blocks (they hold your firmware), but you can still > erase the remaining 12. > > (3) You zero-pad your image to the next sector boundary, and write that > to the flash. The remainder of block 4 becomes unwritable (and you > can't erase the block without destroying your firmware). The remaining > 12 blocks remain writable. This is commonly done for production, > because it reduces the ways a sector holding code can be corrupted, > making its checksum invalid. > > My point is: in the physical world, there is no single true way to pad. > > Back to your patch. I think it conflates three changes: > > * We reject an undersized image with a sub-optimal error message. > Improve that message. > > * We silently ignore an oversized image's tail. Warn instead. > > * As a convenience feature, don't reject undersized read-only image, but > pad it with 0xff instead, to simulate (1) above. > > Squashing the first two under a "better reporting on pflash backing file > mismatch" heading seems fine to me. The last one is not about "better > reporting", and should therefore be a separate patch. > > I'm willing to do the split in the respin of my pflash fixes series. > > For the record, I'd summarily reject oversized images, Rejection is not a bad idea IMO; I don't remember any use case where the user benefits from the acceptance of an oversized image (with or without warning). > and I'd drop the > convenience feature, but I'm not the maintainer here. It's up to Kevin > and Max. Auto-padding can save some space wherever a raw image is provided, even when QEMU is used through libvirt. It's not hugely important IMO but nice to have. (Especially if we decide *not* to describe pflash block count and size traits in the firmware descriptor files.) Thanks Laszlo > >> --- >> v3 >> - tweak commit title/commentary >> - use total_len instead of device_len for checks >> - if the device is read-only do the padding for them >> - accept baking_len > total_len (how to warn_report with NULL *errp?) >> v4 >> - error check blk_getlength >> - optimise memset and use NOR erase pattern >> - restore singular device (overly confusing) >> - add warn_report for when we do magic >> v5 >> - remove mention of null padding >> - use %zu for size_t fmt string >> - add Laszlo r-b >> --- >> hw/block/pflash_cfi01.c | 40 +++++++++++++++++++++++++++++++++------- >> 1 file changed, 33 insertions(+), 7 deletions(-) >> >> diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c >> index 9d1c356eb6..d8cfa4789a 100644 >> --- a/hw/block/pflash_cfi01.c >> +++ b/hw/block/pflash_cfi01.c >> @@ -45,6 +45,7 @@ >> #include "qemu/bitops.h" >> #include "qemu/host-utils.h" >> #include "qemu/log.h" >> +#include "qemu/error-report.h" >> #include "hw/sysbus.h" >> #include "sysemu/sysemu.h" >> #include "trace.h" >> @@ -730,13 +731,6 @@ static void pflash_cfi01_realize(DeviceState *dev, Error **errp) >> } >> device_len = sector_len_per_device * blocks_per_device; >> >> - /* XXX: to be fixed */ >> -#if 0 >> - if (total_len != (8 * 1024 * 1024) && total_len != (16 * 1024 * 1024) && >> - total_len != (32 * 1024 * 1024) && total_len != (64 * 1024 * 1024)) >> - return NULL; >> -#endif >> - >> memory_region_init_rom_device( >> &pfl->mem, OBJECT(dev), >> &pflash_cfi01_ops, >> @@ -763,6 +757,38 @@ static void pflash_cfi01_realize(DeviceState *dev, Error **errp) >> } >> >> if (pfl->blk) { >> + /* >> + * Validate the backing store is the right size for pflash >> + * devices. It should be padded to a multiple of the flash >> + * block size. 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) { >> + error_setg(errp, "unable to check size of backing file"); >> + return; >> + } >> + >> + if (backing_len < total_len) { >> + 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); > > If I add this patch to my series, I can fix up the white-space to make > checkpatch happy. > >> + 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); > > Likewise, I can break this line. > >> + } >> + >> /* read the initial flash content */ >> ret = blk_pread(pfl->blk, 0, pfl->storage, total_len);
One more thing... Alex Bennée <alex.bennee@linaro.org> writes: > It looks like there was going to be code to check we had some sort of > alignment so lets replace it with an actual check. This is a bit more > useful than the enigmatic "failed to read the initial flash content" > when we attempt to read the number of bytes the device should have. > > This is a potential 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> > > --- > v3 > - tweak commit title/commentary > - use total_len instead of device_len for checks > - if the device is read-only do the padding for them > - accept baking_len > total_len (how to warn_report with NULL *errp?) > v4 > - error check blk_getlength > - optimise memset and use NOR erase pattern > - restore singular device (overly confusing) > - add warn_report for when we do magic > v5 > - remove mention of null padding > - use %zu for size_t fmt string > - add Laszlo r-b > --- > hw/block/pflash_cfi01.c | 40 +++++++++++++++++++++++++++++++++------- > 1 file changed, 33 insertions(+), 7 deletions(-) > > diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c > index 9d1c356eb6..d8cfa4789a 100644 > --- a/hw/block/pflash_cfi01.c > +++ b/hw/block/pflash_cfi01.c > @@ -45,6 +45,7 @@ > #include "qemu/bitops.h" > #include "qemu/host-utils.h" > #include "qemu/log.h" > +#include "qemu/error-report.h" > #include "hw/sysbus.h" > #include "sysemu/sysemu.h" > #include "trace.h" > @@ -730,13 +731,6 @@ static void pflash_cfi01_realize(DeviceState *dev, Error **errp) > } > device_len = sector_len_per_device * blocks_per_device; > > - /* XXX: to be fixed */ > -#if 0 > - if (total_len != (8 * 1024 * 1024) && total_len != (16 * 1024 * 1024) && > - total_len != (32 * 1024 * 1024) && total_len != (64 * 1024 * 1024)) > - return NULL; > -#endif > - > memory_region_init_rom_device( > &pfl->mem, OBJECT(dev), > &pflash_cfi01_ops, > @@ -763,6 +757,38 @@ static void pflash_cfi01_realize(DeviceState *dev, Error **errp) > } > > if (pfl->blk) { > + /* > + * Validate the backing store is the right size for pflash > + * devices. It should be padded to a multiple of the flash > + * block size. 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. > + */ I can't see how "It should be padded to a multiple of the flash block size" is related to the code you add. > + int64_t backing_len = blk_getlength(pfl->blk); > + if (backing_len < 0) { > + error_setg(errp, "unable to check size of backing file"); > + return; > + } > + > + if (backing_len < total_len) { > + 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); > + } > + > /* read the initial flash content */ > ret = blk_pread(pfl->blk, 0, pfl->storage, total_len);
Laszlo Ersek <lersek@redhat.com> writes: > On 03/05/19 16:33, Markus Armbruster wrote: >> You neglected to cc: the maintainers of hw/block, I fixed that for you. >> >> Alex Bennée <alex.bennee@linaro.org> writes: >> >>> It looks like there was going to be code to check we had some sort of >>> alignment so lets replace it with an actual check. This is a bit more >>> useful than the enigmatic "failed to read the initial flash content" >>> when we attempt to read the number of bytes the device should have. >>> >>> This is a potential 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> >> >> Philippe and I talked about various pflash issues last night. He >> explained to me how physical flash memory works and is used. This >> brought back my doubts on the wisdom of automatic padding. >> >> Errors in my recounting of his explanations are almost certainly >> entirely mine. Please correct them. >> >> We're talking about NOR flash. NAND flash works differently. >> >> You can: >> >> * Read a cell. >> >> * Write a cell: change it from 1 to 0. >> >> * Erase a whole sector (block): change all cells to 1. This is slow, >> burns power, and you can do it only so often before the flash wears >> out >> >> Say your physical machine has 1 MiB of NOR flash in 16 sectors of 64 KiB >> each (unrealistic, as Philippe has pointed out elsewhere, but it'll do >> here). You compile your firmware, and the build process spits out a >> flat image of 200000 bytes. Here are a few distinct ways to deploy it >> to your freshly erased flash memory: >> >> (1) You write your image to the flash. Everything after byte 200000 >> remains writable. This is nice for development. With a bit of >> ingenuity, you can come up with a patching scheme that lets you avoid >> rewriting the whole flash for every little fix, saving flash wear. >> >> (2) You zero-pad your image to the full flash size, and write that to >> the flash. Everything after byte 200000 becomes unwritable. You can't >> erase the first 4 blocks (they hold your firmware), but you can still >> erase the remaining 12. >> >> (3) You zero-pad your image to the next sector boundary, and write that >> to the flash. The remainder of block 4 becomes unwritable (and you >> can't erase the block without destroying your firmware). The remaining >> 12 blocks remain writable. This is commonly done for production, >> because it reduces the ways a sector holding code can be corrupted, >> making its checksum invalid. >> >> My point is: in the physical world, there is no single true way to pad. >> >> Back to your patch. I think it conflates three changes: >> >> * We reject an undersized image with a sub-optimal error message. >> Improve that message. >> >> * We silently ignore an oversized image's tail. Warn instead. >> >> * As a convenience feature, don't reject undersized read-only image, but >> pad it with 0xff instead, to simulate (1) above. >> >> Squashing the first two under a "better reporting on pflash backing file >> mismatch" heading seems fine to me. The last one is not about "better >> reporting", and should therefore be a separate patch. >> >> I'm willing to do the split in the respin of my pflash fixes series. >> >> For the record, I'd summarily reject oversized images, > > Rejection is not a bad idea IMO; I don't remember any use case where the > user benefits from the acceptance of an oversized image (with or without > warning). Fair enough, I can just error out here. > >> and I'd drop the >> convenience feature, but I'm not the maintainer here. It's up to Kevin >> and Max. > > Auto-padding can save some space wherever a raw image is provided, even > when QEMU is used through libvirt. It's not hugely important IMO but > nice to have. (Especially if we decide *not* to describe pflash block > count and size traits in the firmware descriptor files.) It's a potential point of confusion but we can just error out with a more useful error message. However we provide the convenience for -bios so why not on a read-only bios image? > > Thanks > Laszlo > >> >>> --- >>> v3 >>> - tweak commit title/commentary >>> - use total_len instead of device_len for checks >>> - if the device is read-only do the padding for them >>> - accept baking_len > total_len (how to warn_report with NULL *errp?) >>> v4 >>> - error check blk_getlength >>> - optimise memset and use NOR erase pattern >>> - restore singular device (overly confusing) >>> - add warn_report for when we do magic >>> v5 >>> - remove mention of null padding >>> - use %zu for size_t fmt string >>> - add Laszlo r-b >>> --- >>> hw/block/pflash_cfi01.c | 40 +++++++++++++++++++++++++++++++++------- >>> 1 file changed, 33 insertions(+), 7 deletions(-) >>> >>> diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c >>> index 9d1c356eb6..d8cfa4789a 100644 >>> --- a/hw/block/pflash_cfi01.c >>> +++ b/hw/block/pflash_cfi01.c >>> @@ -45,6 +45,7 @@ >>> #include "qemu/bitops.h" >>> #include "qemu/host-utils.h" >>> #include "qemu/log.h" >>> +#include "qemu/error-report.h" >>> #include "hw/sysbus.h" >>> #include "sysemu/sysemu.h" >>> #include "trace.h" >>> @@ -730,13 +731,6 @@ static void pflash_cfi01_realize(DeviceState *dev, Error **errp) >>> } >>> device_len = sector_len_per_device * blocks_per_device; >>> >>> - /* XXX: to be fixed */ >>> -#if 0 >>> - if (total_len != (8 * 1024 * 1024) && total_len != (16 * 1024 * 1024) && >>> - total_len != (32 * 1024 * 1024) && total_len != (64 * 1024 * 1024)) >>> - return NULL; >>> -#endif >>> - >>> memory_region_init_rom_device( >>> &pfl->mem, OBJECT(dev), >>> &pflash_cfi01_ops, >>> @@ -763,6 +757,38 @@ static void pflash_cfi01_realize(DeviceState *dev, Error **errp) >>> } >>> >>> if (pfl->blk) { >>> + /* >>> + * Validate the backing store is the right size for pflash >>> + * devices. It should be padded to a multiple of the flash >>> + * block size. 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) { >>> + error_setg(errp, "unable to check size of backing file"); >>> + return; >>> + } >>> + >>> + if (backing_len < total_len) { >>> + 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); >> >> If I add this patch to my series, I can fix up the white-space to make >> checkpatch happy. >> >>> + 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); >> >> Likewise, I can break this line. >> >>> + } >>> + >>> /* read the initial flash content */ >>> ret = blk_pread(pfl->blk, 0, pfl->storage, total_len); -- Alex Bennée
Alex Bennée <alex.bennee@linaro.org> writes: > Laszlo Ersek <lersek@redhat.com> writes: > >> On 03/05/19 16:33, Markus Armbruster wrote: >>> You neglected to cc: the maintainers of hw/block, I fixed that for you. >>> >>> Alex Bennée <alex.bennee@linaro.org> writes: >>> >>>> It looks like there was going to be code to check we had some sort of >>>> alignment so lets replace it with an actual check. This is a bit more >>>> useful than the enigmatic "failed to read the initial flash content" >>>> when we attempt to read the number of bytes the device should have. >>>> >>>> This is a potential 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> >>> >>> Philippe and I talked about various pflash issues last night. He >>> explained to me how physical flash memory works and is used. This >>> brought back my doubts on the wisdom of automatic padding. >>> >>> Errors in my recounting of his explanations are almost certainly >>> entirely mine. Please correct them. >>> >>> We're talking about NOR flash. NAND flash works differently. >>> >>> You can: >>> >>> * Read a cell. >>> >>> * Write a cell: change it from 1 to 0. >>> >>> * Erase a whole sector (block): change all cells to 1. This is slow, >>> burns power, and you can do it only so often before the flash wears >>> out >>> >>> Say your physical machine has 1 MiB of NOR flash in 16 sectors of 64 KiB >>> each (unrealistic, as Philippe has pointed out elsewhere, but it'll do >>> here). You compile your firmware, and the build process spits out a >>> flat image of 200000 bytes. Here are a few distinct ways to deploy it >>> to your freshly erased flash memory: >>> >>> (1) You write your image to the flash. Everything after byte 200000 >>> remains writable. This is nice for development. With a bit of >>> ingenuity, you can come up with a patching scheme that lets you avoid >>> rewriting the whole flash for every little fix, saving flash wear. >>> >>> (2) You zero-pad your image to the full flash size, and write that to >>> the flash. Everything after byte 200000 becomes unwritable. You can't >>> erase the first 4 blocks (they hold your firmware), but you can still >>> erase the remaining 12. >>> >>> (3) You zero-pad your image to the next sector boundary, and write that >>> to the flash. The remainder of block 4 becomes unwritable (and you >>> can't erase the block without destroying your firmware). The remaining >>> 12 blocks remain writable. This is commonly done for production, >>> because it reduces the ways a sector holding code can be corrupted, >>> making its checksum invalid. >>> >>> My point is: in the physical world, there is no single true way to pad. >>> >>> Back to your patch. I think it conflates three changes: >>> >>> * We reject an undersized image with a sub-optimal error message. >>> Improve that message. >>> >>> * We silently ignore an oversized image's tail. Warn instead. >>> >>> * As a convenience feature, don't reject undersized read-only image, but >>> pad it with 0xff instead, to simulate (1) above. >>> >>> Squashing the first two under a "better reporting on pflash backing file >>> mismatch" heading seems fine to me. The last one is not about "better >>> reporting", and should therefore be a separate patch. >>> >>> I'm willing to do the split in the respin of my pflash fixes series. >>> >>> For the record, I'd summarily reject oversized images, >> >> Rejection is not a bad idea IMO; I don't remember any use case where the >> user benefits from the acceptance of an oversized image (with or without >> warning). > > Fair enough, I can just error out here. Happy to do that for you if I should end up respinning this patch. >>> and I'd drop the >>> convenience feature, but I'm not the maintainer here. It's up to Kevin >>> and Max. >> >> Auto-padding can save some space wherever a raw image is provided, even >> when QEMU is used through libvirt. It's not hugely important IMO but >> nice to have. (Especially if we decide *not* to describe pflash block >> count and size traits in the firmware descriptor files.) > > It's a potential point of confusion but we can just error out with a > more useful error message. However we provide the convenience for -bios > so why not on a read-only bios image? I consider it a bad idea for -bios, too. Perhaps more seriously, the block layer interferes with this patch's padding. -bios doesn't go through the block layer. For details, please see Subject: Re: [RFC PATCH v6 2/4] hw/block: Pad undersized read-only images with 0xFF Message-ID: <87h8cft2x6.fsf@dusky.pond.sub.org> [...]
diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c index 9d1c356eb6..d8cfa4789a 100644 --- a/hw/block/pflash_cfi01.c +++ b/hw/block/pflash_cfi01.c @@ -45,6 +45,7 @@ #include "qemu/bitops.h" #include "qemu/host-utils.h" #include "qemu/log.h" +#include "qemu/error-report.h" #include "hw/sysbus.h" #include "sysemu/sysemu.h" #include "trace.h" @@ -730,13 +731,6 @@ static void pflash_cfi01_realize(DeviceState *dev, Error **errp) } device_len = sector_len_per_device * blocks_per_device; - /* XXX: to be fixed */ -#if 0 - if (total_len != (8 * 1024 * 1024) && total_len != (16 * 1024 * 1024) && - total_len != (32 * 1024 * 1024) && total_len != (64 * 1024 * 1024)) - return NULL; -#endif - memory_region_init_rom_device( &pfl->mem, OBJECT(dev), &pflash_cfi01_ops, @@ -763,6 +757,38 @@ static void pflash_cfi01_realize(DeviceState *dev, Error **errp) } if (pfl->blk) { + /* + * Validate the backing store is the right size for pflash + * devices. It should be padded to a multiple of the flash + * block size. 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) { + error_setg(errp, "unable to check size of backing file"); + return; + } + + if (backing_len < total_len) { + 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); + } + /* read the initial flash content */ ret = blk_pread(pfl->blk, 0, pfl->storage, total_len);