On 7/15/20 11:00 AM, Markus Armbruster wrote:
> Philippe Mathieu-Daudé <f4...@amsat.org> writes:
>
>> On 7/14/20 6:21 PM, Markus Armbruster wrote:
>>> Philippe Mathieu-Daudé <f4...@amsat.org> writes:
>>>
>>>> + qemu-block experts.
>>>>
>>>> On 7/14/20 11:16 AM, Markus Armbruster wrote:
>>>>> Havard Skinnemoen <hskinnem...@google.com> writes:
>>>>>
>>>>>> On Mon, Jul 13, 2020 at 7:57 AM Cédric Le Goater <c...@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 <kft...@nuvoton.com>
>>>>>>>> Signed-off-by: Havard Skinnemoen <hskinnem...@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?
CLI issue mostly.
We can solve it similarly to the recent "sdcard: Do not allow invalid SD
card sizes" patch:
if (!dinfo) {
error_setg(errp, "Missing SPI flash drive");
error_append_hint(errp, "You can use a dummy drive using:\n");
error_append_hint(errp, "-drive if=mtd,driver=null-co,"
"read-ones=on,size=64M\n);
return;
}
having npcm7xx_connect_flash() taking an Error* argument,
and MachineClass::init() call it with &error_fatal.
>
> 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.
