Re: [Qemu-devel] [PATCH v3] hw/block: better reporting on pflash backing file mismatch

[Philippe Mathieu-Daudé]

On 2/22/19 9:09 AM, Laszlo Ersek wrote:
> On 02/21/19 21:07, Alex Bennée wrote:
>> Laszlo Ersek <address@hidden> 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 <address@hidden>
>>>>
>>>> ---
>>>> 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