On Fri, Jul 17, 2020 at 1:52 PM Philippe Mathieu-Daudé <f4...@amsat.org> wrote: > > On 7/17/20 9:18 PM, Havard Skinnemoen wrote: > > On Fri, Jul 17, 2020 at 2:00 AM Philippe Mathieu-Daudé <f4...@amsat.org> > > wrote: > >> > >> 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 > >>>>>> <hskinnem...@google.com> wrote: > >>>>>>> > >>>>>>> On Wed, Jul 15, 2020 at 3:57 AM Philippe Mathieu-Daudé > >>>>>>> <f4...@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 :) > >>> > >>> 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); > >> } > >> } > > > > Could someone please remind me what problem we're trying to solve here? > > Sorry, out of the scope of your series, which is fine with the current > code base :) > > > Currently, if the user (or test) doesn't provide a drive, we pass NULL > > as the block backend to m25p80. This means we'll take the code path in > > m25p_realize() that does > > > > trace_m25p80_binding_no_bdrv(s); > > s->storage = blk_blockalign(NULL, s->size); > > memset(s->storage, 0xFF, s->size); > > > > which will look like a freshly chip-erased flash chip. > > > > Are we looking for a more elegant way to replace those three lines of > > code (+ a couple of conditionals in the writeback paths)? > > Yes, I am. Anyway, unrelated to your work, sorry if it confused you.
OK, great, I'll be happy to contribute to that. I was just a little worried that my series was getting blocked behind it. > > > > But we don't even have a dummy device that looks like an erased flash > > chip... > > No, this is still the design stage, but your series has a quality that > let us foreseen a bit where we are heading... > > > > > Havard > >