[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v3] hw/block: better reporting on pflash backing
From: |
Philippe Mathieu-Daudé |
Subject: |
Re: [Qemu-devel] [PATCH v3] hw/block: better reporting on pflash backing file mismatch |
Date: |
Fri, 22 Feb 2019 09:56:05 +0100 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.5.1 |
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