Re: [Qemu-devel] [PATCH] hw/block: report when pflash backing file isn't aligned
On 02/14/19 23:38, Alex Bennée wrote:
>
> Laszlo Ersek writes:
>
>> On 02/14/19 16:57, 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.
>>>
>>> Signed-off-by: Alex Bennée
>>> ---
>>> hw/block/pflash_cfi01.c | 19 +--
>>> 1 file changed, 13 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
>>> index bffb4c40e7..f3251b236c 100644
>>> --- a/hw/block/pflash_cfi01.c
>>> +++ b/hw/block/pflash_cfi01.c
>>> @@ -722,12 +722,19 @@ 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
>>> +/*
>>> + * Validate the backing store is the right size for pflash
>>> + * devices. It has to be padded to a multiple of the flash block
>>> + * size.
>>> + */
>>> +if (pfl->blk) {
>>> +uint64_t backing_len = blk_getlength(pfl->blk);
>>> +if (device_len != backing_len) {
>>> +error_setg(errp, "backing file wrong size "
>>> + "(%" PRId64 " != %" PRId64")", backing_len,
>>> device_len);
>>> +return;
>>> +}
>>> +}
>>>
>>> memory_region_init_rom_device(
>>> >mem, OBJECT(dev),
>>>
>>
>> I have two suggestions:
>> - backing_len and device_len are both uint64_t; we should print them
>> with PRIu64
>
> blk_getlength actually returns int64_t for some reason (do signed
> lengths even make sense? maybe it's for error handling?).
Ah, sorry, I didn't realize. I guess we should cover negative values
then explicitly? Not sure.
> But sure I can
> make it PRIu64
>
>> - from a user POV, I find it more useful if the error message also shows
>> which quantity is which, not just two inequal numbers.
>
> How about:
>
> "backing file size (%) not enough for whole device (%)"
"not enough" means "<", but the C expression uses "!=". How about
"backing file size (...) does not match device size (...)"?
Or "does not equal", whichever sounds more palatable.
Thanks!
Laszlo
Re: [Qemu-devel] [PATCH] hw/block: report when pflash backing file isn't aligned
On Thu, Feb 14, 2019 at 10:38:35PM +, Alex Bennée wrote:
> Laszlo Ersek writes:
> > On 02/14/19 16:57, Alex Bennée wrote:
> >> +/*
> >> + * Validate the backing store is the right size for pflash
> >> + * devices. It has to be padded to a multiple of the flash block
> >> + * size.
> >> + */
> >> +if (pfl->blk) {
> >> +uint64_t backing_len = blk_getlength(pfl->blk);
> >> +if (device_len != backing_len) {
> >> +error_setg(errp, "backing file wrong size "
> >> + "(%" PRId64 " != %" PRId64")", backing_len,
> >> device_len);
> >> +return;
> >> +}
> >> +}
> >>
> >> memory_region_init_rom_device(
> >> >mem, OBJECT(dev),
> >>
>
> > - from a user POV, I find it more useful if the error message also shows
> > which quantity is which, not just two inequal numbers.
>
> How about:
>
> "backing file size (%) not enough for whole device (%)"
>
> ?
>
from a user POV, I find it more useful if the error messsage
resembles the actual test in the source. Above is test !=
How about
"backing file size (%) not equal to whole device size (%)"
?
Groeten
Geert Stappers
--
Leven en laten leven
Re: [Qemu-devel] [PATCH] hw/block: report when pflash backing file isn't aligned
Alex Bennée writes:
> Laszlo Ersek writes:
>
>> On 02/14/19 16:57, 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.
>>>
>>> Signed-off-by: Alex Bennée
>>> ---
>>> hw/block/pflash_cfi01.c | 19 +--
>>> 1 file changed, 13 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
>>> index bffb4c40e7..f3251b236c 100644
>>> --- a/hw/block/pflash_cfi01.c
>>> +++ b/hw/block/pflash_cfi01.c
>>> @@ -722,12 +722,19 @@ 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
>>> +/*
>>> + * Validate the backing store is the right size for pflash
>>> + * devices. It has to be padded to a multiple of the flash block
>>> + * size.
>>> + */
>>> +if (pfl->blk) {
>>> +uint64_t backing_len = blk_getlength(pfl->blk);
>>> +if (device_len != backing_len) {
>>> +error_setg(errp, "backing file wrong size "
>>> + "(%" PRId64 " != %" PRId64")", backing_len,
>>> device_len);
>>> +return;
>>> +}
>>> +}
>>>
>>> memory_region_init_rom_device(
>>> >mem, OBJECT(dev),
>>>
>>
>> I have two suggestions:
>> - backing_len and device_len are both uint64_t; we should print them
>> with PRIu64
>
> blk_getlength actually returns int64_t for some reason (do signed
> lengths even make sense? maybe it's for error handling?). But sure I can
> make it PRIu64
Use of signed integers for file offsets is pervasive in the block layer.
It's convenient when we return either a non-negative offset or a
negative error code. It's admittedly sloppy anywhere else.
>> - from a user POV, I find it more useful if the error message also shows
>> which quantity is which, not just two inequal numbers.
>
> How about:
>
> "backing file size (%) not enough for whole device (%)"
>
> ?
Not bad. Another one, avoiding parentheses:
"device needs DDD bytes, backing file provides only BBB bytes"
>
>>
>> I don't feel too strongly about this, so up to you.
Re: [Qemu-devel] [PATCH] hw/block: report when pflash backing file isn't aligned
Laszlo Ersek writes:
> On 02/14/19 16:57, 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.
>>
>> Signed-off-by: Alex Bennée
>> ---
>> hw/block/pflash_cfi01.c | 19 +--
>> 1 file changed, 13 insertions(+), 6 deletions(-)
>>
>> diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
>> index bffb4c40e7..f3251b236c 100644
>> --- a/hw/block/pflash_cfi01.c
>> +++ b/hw/block/pflash_cfi01.c
>> @@ -722,12 +722,19 @@ 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
>> +/*
>> + * Validate the backing store is the right size for pflash
>> + * devices. It has to be padded to a multiple of the flash block
>> + * size.
>> + */
>> +if (pfl->blk) {
>> +uint64_t backing_len = blk_getlength(pfl->blk);
>> +if (device_len != backing_len) {
>> +error_setg(errp, "backing file wrong size "
>> + "(%" PRId64 " != %" PRId64")", backing_len,
>> device_len);
>> +return;
>> +}
>> +}
>>
>> memory_region_init_rom_device(
>> >mem, OBJECT(dev),
>>
>
> I have two suggestions:
> - backing_len and device_len are both uint64_t; we should print them
> with PRIu64
blk_getlength actually returns int64_t for some reason (do signed
lengths even make sense? maybe it's for error handling?). But sure I can
make it PRIu64
> - from a user POV, I find it more useful if the error message also shows
> which quantity is which, not just two inequal numbers.
How about:
"backing file size (%) not enough for whole device (%)"
?
>
> I don't feel too strongly about this, so up to you.
>
> Thanks,
> Laszlo
--
Alex Bennée
Re: [Qemu-devel] [PATCH] hw/block: report when pflash backing file isn't aligned
On 02/14/19 16:57, 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.
>
> Signed-off-by: Alex Bennée
> ---
> hw/block/pflash_cfi01.c | 19 +--
> 1 file changed, 13 insertions(+), 6 deletions(-)
>
> diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
> index bffb4c40e7..f3251b236c 100644
> --- a/hw/block/pflash_cfi01.c
> +++ b/hw/block/pflash_cfi01.c
> @@ -722,12 +722,19 @@ 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
> +/*
> + * Validate the backing store is the right size for pflash
> + * devices. It has to be padded to a multiple of the flash block
> + * size.
> + */
> +if (pfl->blk) {
> +uint64_t backing_len = blk_getlength(pfl->blk);
> +if (device_len != backing_len) {
> +error_setg(errp, "backing file wrong size "
> + "(%" PRId64 " != %" PRId64")", backing_len,
> device_len);
> +return;
> +}
> +}
>
> memory_region_init_rom_device(
> >mem, OBJECT(dev),
>
I have two suggestions:
- backing_len and device_len are both uint64_t; we should print them
with PRIu64
- from a user POV, I find it more useful if the error message also shows
which quantity is which, not just two inequal numbers.
I don't feel too strongly about this, so up to you.
Thanks,
Laszlo
[Qemu-devel] [PATCH] hw/block: report when pflash backing file isn't aligned
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.
Signed-off-by: Alex Bennée
---
hw/block/pflash_cfi01.c | 19 +--
1 file changed, 13 insertions(+), 6 deletions(-)
diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
index bffb4c40e7..f3251b236c 100644
--- a/hw/block/pflash_cfi01.c
+++ b/hw/block/pflash_cfi01.c
@@ -722,12 +722,19 @@ 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
+/*
+ * Validate the backing store is the right size for pflash
+ * devices. It has to be padded to a multiple of the flash block
+ * size.
+ */
+if (pfl->blk) {
+uint64_t backing_len = blk_getlength(pfl->blk);
+if (device_len != backing_len) {
+error_setg(errp, "backing file wrong size "
+ "(%" PRId64 " != %" PRId64")", backing_len, device_len);
+return;
+}
+}
memory_region_init_rom_device(
>mem, OBJECT(dev),
--
2.20.1
