| Message ID | 20190221184857.22434-1-alex.bennee@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | [v3] hw/block: better reporting on pflash backing file mismatch | expand |
On 02/21/19 19:48, Alex Bennée wrote: > 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. > > Signed-off-by: Alex Bennée <alex.bennee@linaro.org> > > --- > 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?) > --- > hw/block/pflash_cfi01.c | 28 +++++++++++++++++++++------- > 1 file changed, 21 insertions(+), 7 deletions(-) > > diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c > index 00c2efd0d7..37d7513c45 100644 > --- a/hw/block/pflash_cfi01.c > +++ b/hw/block/pflash_cfi01.c > @@ -714,13 +714,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, > @@ -747,6 +740,27 @@ 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 null pad the region first. If the user > + * supplies a larger file we silently accept it. (1) I recommend adding "and ignore the tail". > + */ > + uint64_t backing_len = blk_getlength(pfl->blk); (2) Didn't we intend to check for blk_getlength() errors (or assert that there would be none)? > + > + if (backing_len < total_len) { > + if (pfl->ro) { > + memset(pfl->storage, 0, total_len); (3) Should we "optimize" (well, okay, de-pessimize) this to: memset((uint8_t*)pfl->storage + backing_len, 0, total_len - backing_len); ? > + total_len = backing_len; > + } else { > + error_setg(errp, "device(s) needs %" PRIu64 " bytes, " (4) not too important, I'm just curious: why the optional plural? > + "backing file provides only %" PRIu64 " bytes", > + total_len, backing_len); > + return; > + } > + } > + > /* read the initial flash content */ > ret = blk_pread(pfl->blk, 0, pfl->storage, total_len); > > I don't feel too strongly about these, so if you disagree, I won't push. Thanks! Laszlo
Laszlo Ersek <lersek@redhat.com> writes: > On 02/21/19 19:48, Alex Bennée wrote: >> 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. >> >> Signed-off-by: Alex Bennée <alex.bennee@linaro.org> >> >> --- >> 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?) >> --- >> hw/block/pflash_cfi01.c | 28 +++++++++++++++++++++------- >> 1 file changed, 21 insertions(+), 7 deletions(-) >> >> diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c >> index 00c2efd0d7..37d7513c45 100644 >> --- a/hw/block/pflash_cfi01.c >> +++ b/hw/block/pflash_cfi01.c >> @@ -714,13 +714,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, >> @@ -747,6 +740,27 @@ 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 null pad the region first. If the user >> + * supplies a larger file we silently accept it. > > (1) I recommend adding "and ignore the tail". > >> + */ >> + uint64_t backing_len = blk_getlength(pfl->blk); > > (2) Didn't we intend to check for blk_getlength() errors (or assert that > there would be none)? Oops, yes I'll fix that. > >> + >> + if (backing_len < total_len) { >> + if (pfl->ro) { >> + memset(pfl->storage, 0, total_len); > > (3) Should we "optimize" (well, okay, de-pessimize) this to: > > memset((uint8_t*)pfl->storage + backing_len, 0, > total_len - backing_len); > > ? I mean in the grand scheme of things it's unlikely to show up in any benchmarks so I went for simple and easy to get right. > >> + total_len = backing_len; >> + } else { >> + error_setg(errp, "device(s) needs %" PRIu64 " bytes, " > > (4) not too important, I'm just curious: why the optional plural? I discovered the difference between device_len and total_len and found (for some reason) the efivars came out as multiple devices. > >> + "backing file provides only %" PRIu64 " bytes", >> + total_len, backing_len); >> + return; >> + } >> + } >> + >> /* read the initial flash content */ >> ret = blk_pread(pfl->blk, 0, pfl->storage, total_len); >> >> > > I don't feel too strongly about these, so if you disagree, I won't push. > > Thanks! > Laszlo -- Alex Bennée
Hi Alex, On 2/21/19 7:48 PM, Alex Bennée wrote: > 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. > > Signed-off-by: Alex Bennée <alex.bennee@linaro.org> > > --- > 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?) > --- > hw/block/pflash_cfi01.c | 28 +++++++++++++++++++++------- > 1 file changed, 21 insertions(+), 7 deletions(-) > > diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c > index 00c2efd0d7..37d7513c45 100644 > --- a/hw/block/pflash_cfi01.c > +++ b/hw/block/pflash_cfi01.c > @@ -714,13 +714,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, > @@ -747,6 +740,27 @@ 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 null pad the region first. If the user > + * supplies a larger file we silently accept it. > + */ > + uint64_t backing_len = blk_getlength(pfl->blk); > + > + if (backing_len < total_len) { > + if (pfl->ro) { > + memset(pfl->storage, 0, total_len); When you erasing a NOR flash sector, all bits are set to 1. Then you can program a word by setting its bits to 0. It is not possible to set bits from 0 to 1 on a word boundary, it is only possible to set bits to 1 by erasing a whole sector (sector boundary). The pflash QEMU models are of NOR type (is different for other flash technologies). If we want to set the padded area to the fabric blank state, we should use the NOR flash erased value of 0xff. > + total_len = backing_len; > + } else { > + error_setg(errp, "device(s) needs %" PRIu64 " bytes, " > + "backing file provides only %" PRIu64 " bytes", > + total_len, backing_len); > + return; > + } > + } > + > /* read the initial flash content */ > ret = blk_pread(pfl->blk, 0, pfl->storage, total_len); > >
On 02/21/19 21:42, Philippe Mathieu-Daudé wrote: > Hi Alex, > > On 2/21/19 7:48 PM, Alex Bennée wrote: >> 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. >> >> Signed-off-by: Alex Bennée <alex.bennee@linaro.org> >> >> --- >> 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?) >> --- >> hw/block/pflash_cfi01.c | 28 +++++++++++++++++++++------- >> 1 file changed, 21 insertions(+), 7 deletions(-) >> >> diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c >> index 00c2efd0d7..37d7513c45 100644 >> --- a/hw/block/pflash_cfi01.c >> +++ b/hw/block/pflash_cfi01.c >> @@ -714,13 +714,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, >> @@ -747,6 +740,27 @@ 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 null pad the region first. If the user >> + * supplies a larger file we silently accept it. >> + */ >> + uint64_t backing_len = blk_getlength(pfl->blk); >> + >> + if (backing_len < total_len) { >> + if (pfl->ro) { >> + memset(pfl->storage, 0, total_len); > > When you erasing a NOR flash sector, all bits are set to 1. > Then you can program a word by setting its bits to 0. > It is not possible to set bits from 0 to 1 on a word boundary, > it is only possible to set bits to 1 by erasing a whole sector (sector > boundary). > > The pflash QEMU models are of NOR type (is different for other flash > technologies). > If we want to set the padded area to the fabric blank state, we should > use the NOR flash erased value of 0xff. Good point, thank you! This polarity is what the firmware assumes as well. Laszlo >> + total_len = backing_len; >> + } else { >> + error_setg(errp, "device(s) needs %" PRIu64 " bytes, " >> + "backing file provides only %" PRIu64 " bytes", >> + total_len, backing_len); >> + return; >> + } >> + } >> + >> /* read the initial flash content */ >> ret = blk_pread(pfl->blk, 0, pfl->storage, total_len); >> >>
On 02/21/19 21:07, Alex Bennée wrote: > > Laszlo Ersek <lersek@redhat.com> writes: > >> On 02/21/19 19:48, Alex Bennée wrote: >>> 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. >>> >>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org> >>> >>> --- >>> 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?) >>> --- >>> hw/block/pflash_cfi01.c | 28 +++++++++++++++++++++------- >>> 1 file changed, 21 insertions(+), 7 deletions(-) >>> >>> diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c >>> index 00c2efd0d7..37d7513c45 100644 >>> --- a/hw/block/pflash_cfi01.c >>> +++ b/hw/block/pflash_cfi01.c >>> @@ -714,13 +714,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, >>> @@ -747,6 +740,27 @@ 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 null pad the region first. If the user >>> + * supplies a larger file we silently accept it. >> >> (1) I recommend adding "and ignore the tail". >> >>> + */ >>> + uint64_t backing_len = blk_getlength(pfl->blk); >> >> (2) Didn't we intend to check for blk_getlength() errors (or assert that >> there would be none)? > > Oops, yes I'll fix that. > >> >>> + >>> + if (backing_len < total_len) { >>> + if (pfl->ro) { >>> + memset(pfl->storage, 0, total_len); >> >> (3) Should we "optimize" (well, okay, de-pessimize) this to: >> >> memset((uint8_t*)pfl->storage + backing_len, 0, >> total_len - backing_len); >> >> ? > > I mean in the grand scheme of things it's unlikely to show up in any > benchmarks so I went for simple and easy to get right. OK. > >> >>> + total_len = backing_len; >>> + } else { >>> + error_setg(errp, "device(s) needs %" PRIu64 " bytes, " >> >> (4) not too important, I'm just curious: why the optional plural? > > I discovered the difference between device_len and total_len and found > (for some reason) the efivars came out as multiple devices. It is true that the executable code is in one chip, and the UEFI varstore in another (in the most common & most recommended setup anyway); however, the varstore itself doesn't need multiple chips, and more importantly, I think the realize function of any given single chip should only report errors about that one chip. (Unless we have some higher level invariant binding the chips together, but I'm unaware of any such in this case.) If I understand correctly, when we set the error here, QEMU will produce an error report that is tied to the specific pflash chip / command line option that triggered the error. That looks like the right thing to me. Again I don't really insist on plural vs. singular here, I just wanted to understand your train of thought. Thanks! Laszlo > >> >>> + "backing file provides only %" PRIu64 " bytes", >>> + total_len, backing_len); >>> + return; >>> + } >>> + } >>> + >>> /* read the initial flash content */ >>> ret = blk_pread(pfl->blk, 0, pfl->storage, total_len); >>> >>> >> >> I don't feel too strongly about these, so if you disagree, I won't push. >> >> Thanks! >> Laszlo > > > -- > Alex Bennée >
On 2/22/19 9:09 AM, Laszlo Ersek wrote: > On 02/21/19 21:07, Alex Bennée wrote: >> Laszlo Ersek <lersek@redhat.com> writes: >> >>> On 02/21/19 19:48, Alex Bennée wrote: >>>> 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. >>>> >>>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org> >>>> >>>> --- >>>> 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?) >>>> --- >>>> hw/block/pflash_cfi01.c | 28 +++++++++++++++++++++------- [...]>>>> + total_len = backing_len; >>>> + } else { >>>> + error_setg(errp, "device(s) needs %" PRIu64 " bytes, " >>> >>> (4) not too important, I'm just curious: why the optional plural? >> >> I discovered the difference between device_len and total_len and found >> (for some reason) the efivars came out as multiple devices. > > It is true that the executable code is in one chip, and the UEFI > varstore in another (in the most common & most recommended setup > anyway); however, the varstore itself doesn't need multiple chips, and > more importantly, I think the realize function of any given single chip > should only report errors about that one chip. (Unless we have some I agree with László. > higher level invariant binding the chips together, but I'm unaware of > any such in this case.) > > If I understand correctly, when we set the error here, QEMU will produce > an error report that is tied to the specific pflash chip / command line > option that triggered the error. That looks like the right thing to me. > > Again I don't really insist on plural vs. singular here, I just wanted > to understand your train of thought. > > Thanks! > Laszlo
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. > > Signed-off-by: Alex Bennée <alex.bennee@linaro.org> > > --- > 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?) > --- > hw/block/pflash_cfi01.c | 28 +++++++++++++++++++++------- > 1 file changed, 21 insertions(+), 7 deletions(-) > > diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c > index 00c2efd0d7..37d7513c45 100644 > --- a/hw/block/pflash_cfi01.c > +++ b/hw/block/pflash_cfi01.c > @@ -714,13 +714,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, > @@ -747,6 +740,27 @@ 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 null pad the region first. If the user > + * supplies a larger file we silently accept it. > + */ > + uint64_t backing_len = blk_getlength(pfl->blk); > + > + if (backing_len < total_len) { > + if (pfl->ro) { > + memset(pfl->storage, 0, total_len); > + total_len = backing_len; > + } else { > + error_setg(errp, "device(s) needs %" PRIu64 " bytes, " > + "backing file provides only %" PRIu64 " bytes", > + total_len, backing_len); > + return; > + } > + } > + > /* read the initial flash content */ > ret = blk_pread(pfl->blk, 0, pfl->storage, total_len); Cases: * (MATCH) If the image size matches the device size: accept Good. * (SHORT-RO): If the image is smaller than the device, and the device is read-only: accept, silently pad to device size. New convenience feature to save you the trouble of padding the image. Personally, I'm wary of such conveniences; I'd rather force users to be explicit about their intent. Advice, not objection. * (SHORT-RW): If the image is smaller than the device, and the device is read/write: reject. Good. The alternative would be "padding, and writes to the padded area aren't actually persistent", but that would be awful. * (LONG) If the image is larger than the device: accept, silently ignore the image's extra bytes. I know this is what we've always done, but that doesn't make it a good idea. What's the use case for silently truncating firmware images? Other than giving users yet another way to create guests that perplexingly fail to boot.
Markus Armbruster <armbru@redhat.com> writes: > 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. >> >> Signed-off-by: Alex Bennée <alex.bennee@linaro.org> >> >> --- >> 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?) >> --- >> hw/block/pflash_cfi01.c | 28 +++++++++++++++++++++------- >> 1 file changed, 21 insertions(+), 7 deletions(-) >> >> diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c >> index 00c2efd0d7..37d7513c45 100644 >> --- a/hw/block/pflash_cfi01.c >> +++ b/hw/block/pflash_cfi01.c >> @@ -714,13 +714,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, >> @@ -747,6 +740,27 @@ 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 null pad the region first. If the user >> + * supplies a larger file we silently accept it. >> + */ >> + uint64_t backing_len = blk_getlength(pfl->blk); >> + >> + if (backing_len < total_len) { >> + if (pfl->ro) { >> + memset(pfl->storage, 0, total_len); >> + total_len = backing_len; >> + } else { >> + error_setg(errp, "device(s) needs %" PRIu64 " bytes, " >> + "backing file provides only %" PRIu64 " bytes", >> + total_len, backing_len); >> + return; >> + } >> + } >> + >> /* read the initial flash content */ >> ret = blk_pread(pfl->blk, 0, pfl->storage, total_len); > > Cases: > > * (MATCH) If the image size matches the device size: accept > > Good. > > * (SHORT-RO): If the image is smaller than the device, and the device is > read-only: accept, silently pad to device size. > > New convenience feature to save you the trouble of padding the image. > Personally, I'm wary of such conveniences; I'd rather force users to > be explicit about their intent. Advice, not objection. It's more a porting of existing behaviour with -bios to the "new" way of properly specifying flash. That said I'd happily report a warning to the user to say that's what we have done but I'm unsure what the best way to do that is. I messed around with warn_report but it seems to be designed for adding notifications to existing error failures so doesn't work unless *errp points at something. Falling back to an fprintf doesn't seem right. > > * (SHORT-RW): If the image is smaller than the device, and the device is > read/write: reject. > > Good. The alternative would be "padding, and writes to the padded > area aren't actually persistent", but that would be awful. > > * (LONG) If the image is larger than the device: accept, silently ignore > the image's extra bytes. > > I know this is what we've always done, but that doesn't make it a good > idea. What's the use case for silently truncating firmware images? > Other than giving users yet another way to create guests that > perplexingly fail to boot. Again I'd happily be a bit noisier to the user here but not stop what already worked from continuing to do so. -- Alex Bennée
Alex Bennée <alex.bennee@linaro.org> writes: > Markus Armbruster <armbru@redhat.com> writes: > >> 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. >>> >>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org> >>> >>> --- >>> 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?) >>> --- >>> hw/block/pflash_cfi01.c | 28 +++++++++++++++++++++------- >>> 1 file changed, 21 insertions(+), 7 deletions(-) >>> >>> diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c >>> index 00c2efd0d7..37d7513c45 100644 >>> --- a/hw/block/pflash_cfi01.c >>> +++ b/hw/block/pflash_cfi01.c >>> @@ -714,13 +714,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, >>> @@ -747,6 +740,27 @@ 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 null pad the region first. If the user >>> + * supplies a larger file we silently accept it. >>> + */ >>> + uint64_t backing_len = blk_getlength(pfl->blk); >>> + >>> + if (backing_len < total_len) { >>> + if (pfl->ro) { >>> + memset(pfl->storage, 0, total_len); >>> + total_len = backing_len; >>> + } else { >>> + error_setg(errp, "device(s) needs %" PRIu64 " bytes, " >>> + "backing file provides only %" PRIu64 " bytes", >>> + total_len, backing_len); >>> + return; >>> + } >>> + } >>> + >>> /* read the initial flash content */ >>> ret = blk_pread(pfl->blk, 0, pfl->storage, total_len); >> >> Cases: >> >> * (MATCH) If the image size matches the device size: accept >> >> Good. >> >> * (SHORT-RO): If the image is smaller than the device, and the device is >> read-only: accept, silently pad to device size. >> >> New convenience feature to save you the trouble of padding the image. >> Personally, I'm wary of such conveniences; I'd rather force users to >> be explicit about their intent. Advice, not objection. > > It's more a porting of existing behaviour with -bios to the "new" way of > properly specifying flash. That said I'd happily report a warning to the > user to say that's what we have done but I'm unsure what the best way to > do that is. I messed around with warn_report but it seems to be designed > for adding notifications to existing error failures so doesn't work > unless *errp points at something. Falling back to an fprintf doesn't > seem right. We don't have good options for warnings in a context where errors get passed in Error objects. The symmetric way would be to pass warnings just like errors, and let the recipient decide what to do with them, just like for errors. But Error objects aren't capable of holding warnings. All we really have is warn_report(). It works well enough when whatever is handling errors would report the warning to stderr or HMP monitor. This is commonly the case when we're processing command line options or executing an HMP command. Not so when we're executing a QMP command: there, the warning appears out of nowhere on stderr, leaving the QMP client none the wiser. Oh well, QMP has no concept of warning anyway. Still, warn_report() would be better than nothing here. >> * (SHORT-RW): If the image is smaller than the device, and the device is >> read/write: reject. >> >> Good. The alternative would be "padding, and writes to the padded >> area aren't actually persistent", but that would be awful. >> >> * (LONG) If the image is larger than the device: accept, silently ignore >> the image's extra bytes. >> >> I know this is what we've always done, but that doesn't make it a good >> idea. What's the use case for silently truncating firmware images? >> Other than giving users yet another way to create guests that >> perplexingly fail to boot. > > Again I'd happily be a bit noisier to the user here but not stop what > already worked from continuing to do so. Keeping questionable usage working is a valid argument, just not a particularly strong one. The tacit reasoning behind it often goes like "I don't want to struggle with whether existing feature X has sane uses or is merely a trap for the unwary, so I withdraw to the hill of we've always done X, and leave the struggling to users." That said, if I can get a warning here for asking nicely, and maybe, just maybe an error for fighting mightily, I'll take the warning, thank you so much ;)
diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c index 00c2efd0d7..37d7513c45 100644 --- a/hw/block/pflash_cfi01.c +++ b/hw/block/pflash_cfi01.c @@ -714,13 +714,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, @@ -747,6 +740,27 @@ 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 null pad the region first. If the user + * supplies a larger file we silently accept it. + */ + uint64_t backing_len = blk_getlength(pfl->blk); + + if (backing_len < total_len) { + if (pfl->ro) { + memset(pfl->storage, 0, total_len); + total_len = backing_len; + } else { + error_setg(errp, "device(s) needs %" PRIu64 " bytes, " + "backing file provides only %" PRIu64 " bytes", + total_len, backing_len); + return; + } + } + /* read the initial flash content */ ret = blk_pread(pfl->blk, 0, pfl->storage, total_len);
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. Signed-off-by: Alex Bennée <alex.bennee@linaro.org> --- 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?) --- hw/block/pflash_cfi01.c | 28 +++++++++++++++++++++------- 1 file changed, 21 insertions(+), 7 deletions(-) -- 2.20.1