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

[Markus Armbruster]

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

> On 7/17/20 10:27 AM, Philippe Mathieu-Daudé wrote:
>> On 7/17/20 10:03 AM, Thomas Huth wrote:
>>> On 17/07/2020 09.48, Philippe Mathieu-Daudé wrote:
>>>> +Thomas
>>>
>>>> On 7/16/20 10:56 PM, Havard Skinnemoen wrote:
>>>>> On Wed, Jul 15, 2020 at 1:54 PM Havard Skinnemoen
>>>>> <hskinnemoen@google.com> wrote:
>>>>>>
>>>>>> On Wed, Jul 15, 2020 at 3:57 AM Philippe Mathieu-Daudé <f4bug@amsat.org> 
>>>>>> wrote:
>>>>>>>
>>>>>>> On 7/15/20 11:00 AM, Markus Armbruster wrote:
>>>>>>>> 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.
>>>>>>
>>>>>> Erroring out if the user specifies a configuration that can't possibly
>>>>>> boot sounds good to me. Better than trying to come up with defaults
>>>>>> that are still not going to result in a bootable system.
>>>>>>
>>>>>> For testing recovery paths, I think it makes sense to explicitly
>>>>>> specify a null device as you suggest.
>>>>>
>>>>> Hmm, one problem. qom-test fails with
>>>>>
>>>>> qemu-system-aarch64: Missing SPI flash drive
>>>>> You can add a dummy drive using:
>>>>> -drive if=mtd,driver=null-co,read-zeroes=on,size=32M
>>>>> Broken pipe
>>>>> /usr/local/google/home/hskinnemoen/qemu/for-upstream/tests/qtest/libqtest.c:166:
>>>>> kill_qemu() tried to terminate QEMU process but encountered exit
>>>>> status 1 (expected 0)
>>>>> ERROR qom-test - too few tests run (expected 68, got 7)
>>>>>
>>>>> So it looks like we might need a different solution to this, unless we
>>>>> want to make generic tests more machine-aware...
>>>
>>> I didn't follow the other mails in this thread, but what we usually do
>>> in such a case: Add a "if (qtest_enabled())" check to the device or the
>>> machine to ignore the error if it is running in qtest mode.
>> 
>> Hmm I'm not sure it works in this case. We could do:
>> 
>>   if (!dinfo) {
>>      if (qtest) {
>>         /* create null drive for qtest */
>>         opts = ...;
>>         dinfo = drive_new(opts, IF_MTD, &error_abort);
>>      } else {
>>         /* teach user to use proper CLI */
>>         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);
>>      }
>>   }
>> 
>> But I'm not sure Markus will enjoy it :)

Using drive_new() for creating an internal dummy backend is wrong.

Doing it only when qtest_enabled() doesn't make it less wrong.

>> Markus, any better idea about how to handle that with automatic qtests?
>
> FWIW IDE device has a concept of "Anonymous BlockBackend for an empty
> drive":
>
> static void ide_dev_initfn(IDEDevice *dev, IDEDriveKind kind, Error **errp)
> {
>     IDEBus *bus = DO_UPCAST(IDEBus, qbus, dev->qdev.parent_bus);
>     IDEState *s = bus->ifs + dev->unit;
>     int ret;
>
>     if (!dev->conf.blk) {
>         if (kind != IDE_CD) {
>             error_setg(errp, "No drive specified");
>             return;
>         } else {
>             /* Anonymous BlockBackend for an empty drive */
>             dev->conf.blk = blk_new(qemu_get_aio_context(), 0, BLK_PERM_ALL);
>             ret = blk_attach_dev(dev->conf.blk, &dev->qdev);
>             assert(ret == 0);
>         }
>     }

I figure this creates an internal dummy backend the right way, just not
the kind you need.  For a non-empty one, you get to make up a
BlockDriverState, then use blk_new_with_bs().

Is the simplification of device code really worth making up a dummy
backend?