On 04/11/19 21:50, Laszlo Ersek wrote: > On 04/11/19 21:35, Laszlo Ersek wrote: >> On 04/11/19 15:56, Markus Armbruster wrote: >>> Factored out of pc_system_firmware_init() so the next commit can reuse >>> it in hw/arm/virt.c. >>> >>> Signed-off-by: Markus Armbruster <arm...@redhat.com> >>> --- >>> hw/block/pflash_cfi01.c | 30 ++++++++++++++++++++++++++++++ >>> hw/i386/pc_sysfw.c | 19 ++----------------- >>> include/hw/block/flash.h | 1 + >>> 3 files changed, 33 insertions(+), 17 deletions(-) >>> >>> diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c >>> index 16dfae14b8..eba01b1447 100644 >>> --- a/hw/block/pflash_cfi01.c >>> +++ b/hw/block/pflash_cfi01.c >>> @@ -44,9 +44,12 @@ >>> #include "qapi/error.h" >>> #include "qemu/timer.h" >>> #include "qemu/bitops.h" >>> +#include "qemu/error-report.h" >>> #include "qemu/host-utils.h" >>> #include "qemu/log.h" >>> +#include "qemu/option.h" >>> #include "hw/sysbus.h" >>> +#include "sysemu/blockdev.h" >>> #include "sysemu/sysemu.h" >>> #include "trace.h" >>> >>> @@ -968,6 +971,33 @@ MemoryRegion *pflash_cfi01_get_memory(PFlashCFI01 *fl) >>> return &fl->mem; >>> } >>> >>> +/* >>> + * Handle -drive if=pflash for machines that use machine properties. >> >> (1) Can we improve readability with quotes? "-drive if=pflash" >> >>> + * Map unit#i (0 < i <= @ndev) to @dev[i]'s property "drive". >> >> (2) I think you meant (0 <= i < @ndev) >> >>> + */ >>> +void pflash_cfi01_legacy_drive(PFlashCFI01 *dev[], int ndev) >>> +{ >>> + int i; >> >> (3) For utter pedantry, "ndev" and "i" should have type "size_t" (in >> particular because we expect the caller to fill "ndev" with ARRAY_SIZE(). >> >> But, this would go beyond refactoring -- the original "int"s have served >> us just fine, and we're usually counting up (exclusively) to the huge >> number... two. :) >> >>> + DriveInfo *dinfo; >>> + Location loc; >>> + >>> + for (i = 0; i < ndev; i++) { >>> + dinfo = drive_get(IF_PFLASH, 0, i); >>> + if (!dinfo) { >>> + continue; >>> + } >>> + loc_push_none(&loc); >>> + qemu_opts_loc_restore(dinfo->opts); >>> + if (dev[i]->blk) { >> >> (4) Is this really identical to the code being removed? The above >> controlling expression amounts to: >> >> pcms->flash[i]->blk >> >> but the original boils down to >> >> pflash_cfi01_get_blk(pcms->flash[i]) >> >> Hmm... looking at pflash_cfi01_get_blk(), they are the same. Is there a >> particular reason for not using the wrapper function any longer? As in: >> >> pflash_cfi01_get_blk(dev[i]) >> >>> + error_report("clashes with -machine"); >>> + exit(1); >>> + } >>> + qdev_prop_set_drive(DEVICE(dev[i]), "drive", >>> + blk_by_legacy_dinfo(dinfo), &error_fatal); >>> + loc_pop(&loc); >>> + } >>> +} >>> + >>> static void postload_update_cb(void *opaque, int running, RunState state) >>> { >>> PFlashCFI01 *pfl = opaque; >>> diff --git a/hw/i386/pc_sysfw.c b/hw/i386/pc_sysfw.c >>> index c628540774..d58e47184c 100644 >>> --- a/hw/i386/pc_sysfw.c >>> +++ b/hw/i386/pc_sysfw.c >>> @@ -269,32 +269,17 @@ void pc_system_firmware_init(PCMachineState *pcms, >>> { >>> PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms); >>> int i; >>> - DriveInfo *pflash_drv; >>> BlockBackend *pflash_blk[ARRAY_SIZE(pcms->flash)]; >>> - Location loc; >>> >>> if (!pcmc->pci_enabled) { >>> old_pc_system_rom_init(rom_memory, true); >>> return; >>> } >>> >>> - /* Map legacy -drive if=pflash to machine properties */ >>> + pflash_cfi01_legacy_drive(pcms->flash, ARRAY_SIZE(pcms->flash)); >>> + >>> for (i = 0; i < ARRAY_SIZE(pcms->flash); i++) { >>> pflash_blk[i] = pflash_cfi01_get_blk(pcms->flash[i]); >>> - pflash_drv = drive_get(IF_PFLASH, 0, i); >>> - if (!pflash_drv) { >>> - continue; >>> - } >>> - loc_push_none(&loc); >>> - qemu_opts_loc_restore(pflash_drv->opts); >>> - if (pflash_blk[i]) { >>> - error_report("clashes with -machine"); >>> - exit(1); >>> - } >>> - pflash_blk[i] = blk_by_legacy_dinfo(pflash_drv); >>> - qdev_prop_set_drive(DEVICE(pcms->flash[i]), >>> - "drive", pflash_blk[i], &error_fatal); >>> - loc_pop(&loc); >>> } >> >> (5) I think you deleted too much here. After this loop, post-patch, for >> all "i", we'll have >> >> pflash_blk[i] = pflash_cfi01_get_blk(pcms->flash[i]); >> >> But pre-patch, for all "i" where the "continue" didn't fire, we'd end up >> with: >> >> pflash_blk[i] = blk_by_legacy_dinfo(pflash_drv); >> >> IOW the original loop both verifies and *collects*, for the gap check >> that comes just below. >> >> IIRC, this allows for mixing "-drive if=pflash,unit=N" with the machine >> properties, as long as you have neither conflicts, nor gaps. >> >> Post-patch however, this kind of mixing would break, because we'd report >> gaps for the legacy ("-drive if=pflash") options. >> >> >> In addition, it could break the "use ROM mode" branch below, which is >> based on pflash_blk[0]. >> >> I think we should extend pflash_cfi01_legacy_drive() to populate >> "pflash_blk[0..(ndev-1)]" on output (assuming pflash_blk is not NULL on >> input). >> >> (Because, I'm assuming that qdev_prop_set_drive() -- which now occurs in >> the factored-out loop *before* the loop that we preserve here -- has no >> effect on pflash_cfi01_get_blk(pcms->flash[i]).) > > Hmmm, that's likely precisely what I'm wrong about. I've now looked at > DEFINE_PROP_DRIVE / qdev_prop_drive / set_drive / parse_drive... Does > qdev_prop_set_drive() actually *turn* the legacy options into genuine > machine type properties?... The removed comment does indicate that: > > "Map legacy -drive if=pflash to machine properties" > > So I guess the remaining loop is correct after all, but the new comment > > "Handle -drive if=pflash for machines that use machine properties" > > is less clear to me.
OK, OK. I'm being dense. I guess the case is that qdev_prop_set_drive(DEVICE(dev[i]), "drive", blk_by_legacy_dinfo(dinfo), &error_fatal); in the new function basically *assigns* a non-NULL value to dev[i]->blk which was checked to be NULL just before. ... This is incredibly non-intuitive, especially given that the pre-patch code performed a *similar* assignment explicitly :( In other words, we could have written the pre-patch (original) code like this: diff --git a/hw/i386/pc_sysfw.c b/hw/i386/pc_sysfw.c index c6285407748e..ed6e713d0982 100644 --- a/hw/i386/pc_sysfw.c +++ b/hw/i386/pc_sysfw.c @@ -291,9 +291,10 @@ void pc_system_firmware_init(PCMachineState *pcms, error_report("clashes with -machine"); exit(1); } - pflash_blk[i] = blk_by_legacy_dinfo(pflash_drv); qdev_prop_set_drive(DEVICE(pcms->flash[i]), - "drive", pflash_blk[i], &error_fatal); + "drive", blk_by_legacy_dinfo(pflash_drv), + &error_fatal); + pflash_blk[i] = pflash_cfi01_get_blk(pcms->flash[i]); loc_pop(&loc); } and the behavior would have been identical. For the sake of QOM / blk dummies like me, can you please split this refactoring to a separate patch, before extracting the new function? Thanks, Laszlo >> >>> >>> /* Reject gaps */ >>> diff --git a/include/hw/block/flash.h b/include/hw/block/flash.h >>> index a0f488732a..f6a68c2a4c 100644 >>> --- a/include/hw/block/flash.h >>> +++ b/include/hw/block/flash.h >>> @@ -24,6 +24,7 @@ PFlashCFI01 *pflash_cfi01_register(hwaddr base, >>> int be); >>> BlockBackend *pflash_cfi01_get_blk(PFlashCFI01 *fl); >>> MemoryRegion *pflash_cfi01_get_memory(PFlashCFI01 *fl); >>> +void pflash_cfi01_legacy_drive(PFlashCFI01 *dev[], int ndev); >>> >>> /* pflash_cfi02.c */ >>> >>> >> >