Re: [PATCH v5 10/11] hw/arm: Wire up BMC boot flash for npcm750-evb and quanta-gsj

[Markus Armbruster <armbru@redhat.com>]

Philippe Mathieu-Daudé <f4bug@amsat.org> writes:

> On 7/14/20 6:21 PM, Markus Armbruster wrote:
>> Philippe Mathieu-Daudé <f4bug@amsat.org> writes:
>> 
>>> + qemu-block experts.
>>>
>>> On 7/14/20 11:16 AM, Markus Armbruster wrote:
>>>> Havard Skinnemoen <hskinnemoen@google.com> writes:
>>>>
>>>>> On Mon, Jul 13, 2020 at 7:57 AM Cédric Le Goater <clg@kaod.org> wrote:
>>>>>>
>>>>>> On 7/9/20 2:36 AM, Havard Skinnemoen wrote:
>>>>>>> This allows these NPCM7xx-based boards to boot from a flash image, e.g.
>>>>>>> one built with OpenBMC. For example like this:
>>>>>>>
>>>>>>> IMAGE=${OPENBMC}/build/tmp/deploy/images/gsj/image-bmc
>>>>>>> qemu-system-arm -machine quanta-gsj -nographic \
>>>>>>>       -bios ~/qemu/bootrom/npcm7xx_bootrom.bin \
>>>>>>>       -drive file=${IMAGE},if=mtd,bus=0,unit=0,format=raw,snapshot=on
>>>>>>>
>>>>>>> Reviewed-by: Tyrone Ting <kfting@nuvoton.com>
>>>>>>> Signed-off-by: Havard Skinnemoen <hskinnemoen@google.com>
>>>>>>
>>>>>> May be we don't need to create the flash object if dinfo is NULL.
>>>>>
>>>>> It's soldered on the board, so you can't really boot the board without
>>>>> it. But if you think it's better to remove it altogether if we don't
>>>>> have an image to load into it, I can do that.
>>>>
>>>> If a device is a fixed part of the physical board, it should be a fixed
>>>> part of the virtual board, too.
>>>
>>> We agree so far but ... how to do it?
>>>
>>> I never used this API, does that makes sense?
>>>
>>>     if (!dinfo) {
>>>         QemuOpts *opts;
>>>
>>>         opts = qemu_opts_create(NULL, "spi-flash", 1, &error_abort);
>>>         qdict_put_str(opts, "format", "null-co");
>>>         qdict_put_int(opts, BLOCK_OPT_SIZE, 64 * MiB);
>>>         qdict_put_bool(opts, NULL_OPT_ZEROES, false); // XXX
>>>
>>>         dinfo = drive_new(opts, IF_MTD, &error_abort);
>>>         qemu_opts_del(opts);
>>>     }
>> 
>> I believe existing code special-cases "no backend" instead of making one
>> up.
>> 
>> Example: pflash_cfi0?.c
>> 
>> If ->blk is non-null, we read its contents into the memory buffer and
>> write updates back, else we leave it blank and don't write updates back.
>> 
>> Making one up could be more elegant.  To find out, you have to try.
>
> I'd rather avoid ad-hoc code in each device. I2C EEPROM do that too,
> it is a source of head aches.
>
>>From the emulation PoV I'd prefer to always use a block backend,
> regardless the user provide a drive.
>
>> 
>> We make up a few default drives (i.e. drives the user doesn't specify):
>> floppy, CD-ROM and SD card.  Ancient part of the user interface, uses
>> DriveInfo.  I doubt we should create more of them.
>> 
>> I believe block backends we make up for internal use should stay away
>> from DriveInfo.  Kevin, what do you think?  How would you make up a
>> null-co block backend for a device's internal use?
>
> I read 'DriveInfo' is the legacy interface, but all the code base use it
> so it is confusing, I don't understand what is the correct interface to
> use.

I admit the "legacy" bit is still aspirational.  We still haven't
managed to replace it for configuring certain onboard devices.

The thing being configured is a device's BlockBackend.

To understand the point I'm trying to make, please ignore "legacy", and
focus on the actual purpose of DriveInfo: it's (one kind of) user
configuration for a BlockBackend.

Now let me try to state the problem you're trying to solve.  Instead of
special-casing "no backend" in device code like pflash_cfi0?.c do, you
want to make up a "dummy" backend instead.  You need the dummy to read
some blank value and ignore writes.  One of the null block drivers
should fit the bill.

Now my point.  Why first make up user configuration, then use that to
create a BlockBackend, when you could just go ahead and create the
BlockBackend?

Sadly, I'm not sufficiently familiar with the block API anymore to tell
you exactly how.  blk_new_with_bs() looks promising.  Perhaps Kevin can
advise.

>>> We should probably add a public helper for that.
>> 
>> If we decide we want to make up backends, then yes, we should do that in
>> a helper, not in each device.
>> 
>>> 'XXX' because NOR flashes erase content is when hardware bit
>>> is set, so it would be more useful to return -1/0xff... rather
>>> than zeroes.