Re: [Qemu-devel] [PATCH 2/2] hw/arm/virt: Support firmware configuration with -blockdev

[Laszlo Ersek <lersek@redhat.com>]

On 04/12/19 19:49, Markus Armbruster wrote:
> Laszlo Ersek <lersek@redhat.com> writes:
>
>> On 04/11/19 15:56, Markus Armbruster wrote:
>>> The ARM virt machines put firmware in flash memory.  To configure it,
>>> you use -drive if=pflash,unit=0,... and optionally -drive
>>> if=pflash,unit=1,...
>>>
>>> Why two -drive?  This permits setting up one part of the flash memory
>>> read-only, and the other part read/write.  It also makes upgrading
>>> firmware on the host easier.  Below the hood, we get two separate
>>> flash devices, because we were too lazy to improve our flash device
>>> models to support sector protection.
>>>
>>> The problem at hand is to do the same with -blockdev somehow, as one
>>> more step towards deprecating -drive.
>>>
>>> We recently solved this problem for x86 PC machines, in commit
>>> ebc29e1beab.  See the commit message for design rationale.
>>>
>>> This commit solves it for ARM virt basically the same way: new machine
>>> properties pflash0, pflash1 forward to the onboard flash devices'
>>> properties.  Requires creating the onboard devices in the
>>> .instance_init() method virt_instance_init().  The existing code to
>>> pick up drives defined with -drive if=pflash is replaced by code to
>>> desugar into the machine properties.
>>>
>>> There are a few behavioral differences, though:
>>>
>>> * The flash devices are always present (x86: only present if
>>>   configured)
>>>
>>> * Flash base addresses and sizes are fixed (x86: sizes depend on
>>>   images, mapped back to back below a fixed address)
>>>
>>> * -bios configures contents of first pflash (x86: -bios configures ROM
>>>    contents)
>>>
>>> * -bios is rejected when first pflash is also configured with -machine
>>>    pflash0=... (x86: bios is silently ignored then)
>>>
>>> * -machine pflash1=... does not require -machine pflash0=... (x86: it
>>>    does).
>>>
>>> The actual code is a bit simpler than for x86 mostly due to the first
>>> two differences.
>>>
>>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>>> ---
>>>  hw/arm/virt.c         | 192 +++++++++++++++++++++++++++---------------
>>>  include/hw/arm/virt.h |   2 +
>>>  2 files changed, 124 insertions(+), 70 deletions(-)
>>
>> I tried to review this patch, but I can't follow it.
>>
>> I'm not saying it's wrong; in fact it *looks* right to me, but I can't
>> follow it. It does so many things at once that I can't keep them all in
>> mind.
>
> Happens.  I'll try to explain, and then we can talk about improving the
> patch.
>
> Did you follow "See the commit message [of ebc29e1beab] for design
> rationale" reference?

That commit carries my

"Acked-by: Laszlo Ersek <lersek@redhat.com>"

and it's not an R-b because:

- http://mid.mail-archive.com/9d2b6670-5d1e-9c7d-df34-226cb1469f99@redhat.com

  "I don't know enough to understand a large part of the commit message.
  I can make some (superficial) comments on the code."

- http://mid.mail-archive.com/12af2068-ecd6-4b77-495e-2f52b0b0b13e@redhat.com

  "Thanks for addressing my comments!"

So, the depth at which I understood commit ebc29e1beab isn't supporting
me here :)

>
>> I started with:
>>
>>   virt_instance_init()
>>     virt_flash_create()
>>       virt_pflash_create1()
>>
>> and then I got confused by the object_property_add_*() calls, which
>> don't exist in the original code:
>>
>>> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
>>> index ce2664a30b..8ce7ecca80 100644
>>> --- a/hw/arm/virt.c
>>> +++ b/hw/arm/virt.c
>>> @@ -30,6 +30,7 @@
>>>
>>>  #include "qemu/osdep.h"
>>>  #include "qemu/units.h"
>>> +#include "qemu/option.h"
>>>  #include "qapi/error.h"
>>>  #include "hw/sysbus.h"
>>>  #include "hw/arm/arm.h"
>>> @@ -871,25 +872,19 @@ static void create_virtio_devices(const VirtMachineState *vms, qemu_irq *pic)
>>>      }
>>>  }
>>>
>>> -static void create_one_flash(const char *name, hwaddr flashbase,
>>> -                             hwaddr flashsize, const char *file,
>>> -                             MemoryRegion *sysmem)
>>> +#define VIRT_FLASH_SECTOR_SIZE (256 * KiB)
>>> +
>>> +static PFlashCFI01 *virt_pflash_create1(VirtMachineState *vms,
>>> +                                        const char *name,
>>> +                                        const char *alias_prop_name)
>>>  {
>>> -    /* Create and map a single flash device. We use the same
>>> -     * parameters as the flash devices on the Versatile Express board.
>>> +    /*
>>> +     * Create a single flash device.  We use the same parameters as
>>> +     * the flash devices on the Versatile Express board.
>>>       */
>>> -    DriveInfo *dinfo = drive_get_next(IF_PFLASH);
>>>      DeviceState *dev = qdev_create(NULL, TYPE_PFLASH_CFI01);
>>> -    SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
>>> -    const uint64_t sectorlength = 256 * 1024;
>>>
>>> -    if (dinfo) {
>>> -        qdev_prop_set_drive(dev, "drive", blk_by_legacy_dinfo(dinfo),
>>> -                            &error_abort);
>>> -    }
>>> -
>>> -    qdev_prop_set_uint32(dev, "num-blocks", flashsize / sectorlength);
>>> -    qdev_prop_set_uint64(dev, "sector-length", sectorlength);
>>> +    qdev_prop_set_uint64(dev, "sector-length", VIRT_FLASH_SECTOR_SIZE);
>>>      qdev_prop_set_uint8(dev, "width", 4);
>>>      qdev_prop_set_uint8(dev, "device-width", 2);
>>>      qdev_prop_set_bit(dev, "big-endian", false);
>>> @@ -898,41 +893,41 @@ static void create_one_flash(const char *name, hwaddr flashbase,
>>>      qdev_prop_set_uint16(dev, "id2", 0x00);
>>>      qdev_prop_set_uint16(dev, "id3", 0x00);
>>>      qdev_prop_set_string(dev, "name", name);
>>> +    object_property_add_child(OBJECT(vms), name, OBJECT(dev),
>>> +                              &error_abort);
>>
>> I guess this links the pflash device as a child under the
>> VirtMachineState object
>
> Yes.
>
>>                         -- but why wasn't that necessary before?
>
> For better or worse, a qdev device without a parent gets adopted on
> realize by container object "/machine/unattached".  See
> device_set_realized() under if (!obj->parent).
>
> Example to illustrate, feel free to skip ahead to "End of example".
>
> Here's the contents of "/machine/unattached" for a pretty minimal ARM
> virt machine (I use scripts/qmp/qom-fuse to mount the QOM tree to
> qom-fuse/ for easy examination):

(the fact that we have a genuine filesystem driver for navigating the
QOM tree, for debugging purposes, is brilliant and frightening at the
same time)

>
>     $ ls qom-fuse/machine/unattached/
>     'device[0]'   'device[19]'  'device[28]'  'device[37]'  'device[7]'
>     'device[10]'  'device[1]'   'device[29]'  'device[38]'  'device[8]'
>     'device[11]'  'device[20]'  'device[2]'   'device[39]'  'device[9]'
>     'device[12]'  'device[21]'  'device[30]'  'device[3]'   'io[0]'
>     'device[13]'  'device[22]'  'device[31]'  'device[40]'  'mach-virt.ram[0]'
>     'device[14]'  'device[23]'  'device[32]'  'device[41]'  'platform bus[0]'
>     'device[15]'  'device[24]'  'device[33]'  'device[42]'   sysbus
>     'device[16]'  'device[25]'  'device[34]'  'device[4]'   'system[0]'
>     'device[17]'  'device[26]'  'device[35]'  'device[5]'    type
>     'device[18]'  'device[27]'  'device[36]'  'device[6]'
>
> The device* are the adopted qdevs.  Let's look for pflash:
>
>     $ grep -a cfi qom-fuse/machine/unattached/device*/type
>     qom-fuse/machine/unattached/device[1]/type:cfi.pflash01
>     qom-fuse/machine/unattached/device[2]/type:cfi.pflash01
>
> Here are the children of /machine:
>
>     $ ls qom-fuse/machine/
>     accel                   fw_cfg        kernel             secure
>     append                  gic-version   kernel-irqchip     suppress-vmdesc
>     dt-compatible           graphics      kvm-shadow-mem     type
>     dtb                     highmem       mem-merge          unattached
>     dump-guest-core         igd-passthru  memory-encryption  usb
>     dumpdtb                 initrd        peripheral         virtualization
>     enforce-config-section  iommu         peripheral-anon
>     firmware                its           phandle-start
>
> This was before this patch.  Afterwards:
>
>     $ ls qom-fuse/machine/unattached/
>     'device[0]'   'device[19]'  'device[28]'  'device[37]'  'device[9]'
>     'device[10]'  'device[1]'   'device[29]'  'device[38]'  'io[0]'
>     'device[11]'  'device[20]'  'device[2]'   'device[39]'  'mach-virt.ram[0]'
>     'device[12]'  'device[21]'  'device[30]'  'device[3]'   'platform bus[0]'
>     'device[13]'  'device[22]'  'device[31]'  'device[40]'   sysbus
>     'device[14]'  'device[23]'  'device[32]'  'device[4]'   'system[0]'
>     'device[15]'  'device[24]'  'device[33]'  'device[5]'    type
>     'device[16]'  'device[25]'  'device[34]'  'device[6]'
>     'device[17]'  'device[26]'  'device[35]'  'device[7]'
>     'device[18]'  'device[27]'  'device[36]'  'device[8]'
>
> Two fewer device*.
>
>     $ ls qom-fuse/machine/
>     accel                   gic-version     kvm-shadow-mem     suppress-vmdesc
>     append                  graphics        mem-merge          type
>     dt-compatible           highmem         memory-encryption  unattached
>     dtb                     igd-passthru    peripheral         usb
>     dump-guest-core         initrd          peripheral-anon    virt.flash0
>     dumpdtb                 iommu           pflash0            virt.flash1
>     enforce-config-section  its             pflash1            virtualization
>     firmware                kernel          phandle-start
>     fw_cfg                  kernel-irqchip  secure
>
> Note new virt.flash0 and virt.flash1.

Four more children for /machine though: "virt.flashX" and "pflashX".

(... returning here from the end: apparently machine properties are also
children of /machine)

>
>     $ cat qom-fuse/machine/virt.flash*/type
>     cfi.pflash01
>     cfi.pflash01
>
> That's where the two went.
>
> End of example.
>
> Now I'm actually read to answer your question "why wasn't that necessary
> before?", or rather "why is it necessary now?"
>
> It wasn't necessary before because orphans get adopted.
>
> It might not even be necessary now, but I feel (strongly!) that an alias
> property should redirect to a child's property, not some random
> unrelated object's property (see right below).
>
> Regardless, I feel onboard devices should be proper children of the
> machine anyway, not stuffed into its unattached/ grabbag.

... Okay.

>>> +    object_property_add_alias(OBJECT(vms), alias_prop_name,
>>> +                              OBJECT(dev), "drive", &error_abort);
>>
>> What is this good for? Does this make the pflash0 / pflash1 properties
>> of the machine refer to the "drive" property of the pflash device?
>
> Correct!
>
> With object.h and a crystal ball, you should be able to work that out
> for yourself:
>
> /**
>  * object_property_add_alias:
>  * @obj: the object to add a property to
>  * @name: the name of the property
>  * @target_obj: the object to forward property access to
>  * @target_name: the name of the property on the forwarded object
>  * @errp: if an error occurs, a pointer to an area to store the error
>  *
>  * Add an alias for a property on an object.  This function will add a property
>  * of the same type as the forwarded property.
>  *
>  * The caller must ensure that <code>@target_obj</code> stays alive as long as
>  * this property exists.  In the case of a child object or an alias on the same
>  * object this will be the case.  For aliases to other objects the caller is
>  * responsible for taking a reference.
>  */
>
> Oh, no crystal ball?  Then it's spelunking through object.c, I guess.
>
>> Can we have a comment about that, or is that obvious for people that are
>> used to QOM code?
>
> If there are any, please speak up, so we can appoint you QOM maintainer.
>
> In my (weak) defense, I mentioned the property forwarding in my commit
> message ("new machine properties pflash0, pflash1 forward to the onboard
> flash devices' properties"), and I did point to commit ebc29e1beab for
> design rationale.  It has this paragraph:
>
>     More seriously, the properties do not belong to the machine, they
>     belong to the onboard flash devices.  Adding them to the machine would
>     then require bad magic to somehow transfer them to the flash devices.
>     Fortunately, QOM provides the means to handle exactly this case: add
>     alias properties to the machine that forward to the onboard devices'
>     properties.

Yup, I certainly neither remembered nor re-read this. Sorry about that.

>
>> ... So anyway, this is where I come to an almost complete halt. I
>> understand one more bit (in isolation only):
>>
>>> +    return PFLASH_CFI01(dev);
>>> +}
>>> +
>>> +static void virt_flash_create(VirtMachineState *vms)
>>> +{
>>> +    vms->flash[0] = virt_pflash_create1(vms, "virt.flash0", "pflash0");
>>> +    vms->flash[1] = virt_pflash_create1(vms, "virt.flash1", "pflash1");
>>> +}
>>> +
>>> +static void virt_flash_map1(PFlashCFI01 *flash,
>>> +                            hwaddr base, hwaddr size,
>>> +                            MemoryRegion *sysmem)
>>> +{
>>> +    DeviceState *dev = DEVICE(flash);
>>> +
>>> +    assert(size % VIRT_FLASH_SECTOR_SIZE == 0);
>>> +    assert(size / VIRT_FLASH_SECTOR_SIZE <= UINT32_MAX);
>>> +    qdev_prop_set_uint32(dev, "num-blocks", size / VIRT_FLASH_SECTOR_SIZE);
>>>      qdev_init_nofail(dev);
>>>
>>> -    memory_region_add_subregion(sysmem, flashbase,
>>> -                                sysbus_mmio_get_region(SYS_BUS_DEVICE(dev), 0));
>>> -
>>> -    if (file) {
>>> -        char *fn;
>>> -        int image_size;
>>> -
>>> -        if (drive_get(IF_PFLASH, 0, 0)) {
>>> -            error_report("The contents of the first flash device may be "
>>> -                         "specified with -bios or with -drive if=pflash... "
>>> -                         "but you cannot use both options at once");
>>> -            exit(1);
>>> -        }
>>> -        fn = qemu_find_file(QEMU_FILE_TYPE_BIOS, file);
>>> -        if (!fn) {
>>> -            error_report("Could not find ROM image '%s'", file);
>>> -            exit(1);
>>> -        }
>>> -        image_size = load_image_mr(fn, sysbus_mmio_get_region(sbd, 0));
>>> -        g_free(fn);
>>> -        if (image_size < 0) {
>>> -            error_report("Could not load ROM image '%s'", file);
>>> -            exit(1);
>>> -        }
>>> -    }
>>> +    memory_region_add_subregion(sysmem, base,
>>> +                                sysbus_mmio_get_region(SYS_BUS_DEVICE(dev),
>>> +                                                       0));
>>>  }
>>>
>>> -static void create_flash(const VirtMachineState *vms,
>>> -                         MemoryRegion *sysmem,
>>> -                         MemoryRegion *secure_sysmem)
>>> +static void virt_flash_map(VirtMachineState *vms,
>>> +                           MemoryRegion *sysmem,
>>> +                           MemoryRegion *secure_sysmem)
>>>  {
>>> -    /* Create two flash devices to fill the VIRT_FLASH space in the memmap.
>>> -     * Any file passed via -bios goes in the first of these.
>>> +    /*
>>> +     * Map two flash devices to fill the VIRT_FLASH space in the memmap.
>>>       * sysmem is the system memory space. secure_sysmem is the secure view
>>>       * of the system, and the first flash device should be made visible only
>>>       * there. The second flash device is visible to both secure and nonsecure.
>>> @@ -941,13 +936,21 @@ static void create_flash(const VirtMachineState *vms,
>>>       */
>>>      hwaddr flashsize = vms->memmap[VIRT_FLASH].size / 2;
>>>      hwaddr flashbase = vms->memmap[VIRT_FLASH].base;
>>> +
>>> +    virt_flash_map1(vms->flash[0], flashbase, flashsize,
>>> +                    secure_sysmem);
>>> +    virt_flash_map1(vms->flash[1], flashbase + flashsize, flashsize,
>>> +                    sysmem);
>>> +}
>>> +
>>> +static void virt_flash_fdt(VirtMachineState *vms,
>>> +                           MemoryRegion *sysmem,
>>> +                           MemoryRegion *secure_sysmem)
>>> +{
>>> +    hwaddr flashsize = vms->memmap[VIRT_FLASH].size / 2;
>>> +    hwaddr flashbase = vms->memmap[VIRT_FLASH].base;
>>>      char *nodename;
>>>
>>> -    create_one_flash("virt.flash0", flashbase, flashsize,
>>> -                     bios_name, secure_sysmem);
>>> -    create_one_flash("virt.flash1", flashbase + flashsize, flashsize,
>>> -                     NULL, sysmem);
>>> -
>>>      if (sysmem == secure_sysmem) {
>>>          /* Report both flash devices as a single node in the DT */
>>>          nodename = g_strdup_printf("/flash@%" PRIx64, flashbase);
>>> @@ -959,7 +962,8 @@ static void create_flash(const VirtMachineState *vms,
>>>          qemu_fdt_setprop_cell(vms->fdt, nodename, "bank-width", 4);
>>>          g_free(nodename);
>>>      } else {
>>> -        /* Report the devices as separate nodes so we can mark one as
>>> +        /*
>>> +         * Report the devices as separate nodes so we can mark one as
>>>           * only visible to the secure world.
>>>           */
>>>          nodename = g_strdup_printf("/secflash@%" PRIx64, flashbase);
>>> @@ -982,6 +986,48 @@ static void create_flash(const VirtMachineState *vms,
>>>      }
>>>  }
>>>
>>> +static bool virt_firmware_init(VirtMachineState *vms,
>>> +                               MemoryRegion *sysmem,
>>> +                               MemoryRegion *secure_sysmem)
>>> +{
>>> +    BlockBackend *pflash_blk0;
>>> +
>>> +    pflash_cfi01_legacy_drive(vms->flash, ARRAY_SIZE(vms->flash));
>>> +    virt_flash_map(vms, sysmem, secure_sysmem);
>>> +
>>> +    pflash_blk0 = pflash_cfi01_get_blk(vms->flash[0]);
>>> +
>>> +    if (bios_name) {
>>> +        char *fname;
>>> +        MemoryRegion *mr;
>>> +        int image_size;
>>> +
>>> +        if (pflash_blk0) {
>>> +            error_report("The contents of the first flash device may be "
>>> +                         "specified with -bios or with -drive if=pflash... "
>>> +                         "but you cannot use both options at once");
>>> +            exit(1);
>>> +        }
>>> +
>>> +        /* Fall back to -bios */
>>> +
>>> +        fname = qemu_find_file(QEMU_FILE_TYPE_BIOS, bios_name);
>>> +        if (!fname) {
>>> +            error_report("Could not find ROM image '%s'", bios_name);
>>> +            exit(1);
>>> +        }
>>> +        mr = sysbus_mmio_get_region(SYS_BUS_DEVICE(vms->flash[0]), 0);
>>> +        image_size = load_image_mr(fname, mr);
>>> +        g_free(fname);
>>> +        if (image_size < 0) {
>>> +            error_report("Could not load ROM image '%s'", bios_name);
>>> +            exit(1);
>>> +        }
>>> +    }
>>> +
>>> +    return pflash_blk0 || bios_name;
>>> +}
>>> +
>>>  static FWCfgState *create_fw_cfg(const VirtMachineState *vms, AddressSpace *as)
>>>  {
>>>      hwaddr base = vms->memmap[VIRT_FW_CFG].base;
>>> @@ -1421,7 +1467,7 @@ static void machvirt_init(MachineState *machine)
>>>      MemoryRegion *secure_sysmem = NULL;
>>>      int n, virt_max_cpus;
>>>      MemoryRegion *ram = g_new(MemoryRegion, 1);
>>> -    bool firmware_loaded = bios_name || drive_get(IF_PFLASH, 0, 0);
>>> +    bool firmware_loaded;
>>>      bool aarch64 = true;
>>>
>>>      /*
>>> @@ -1460,6 +1506,27 @@ static void machvirt_init(MachineState *machine)
>>>          exit(1);
>>>      }
>>>
>>> +    if (vms->secure) {
>>> +        if (kvm_enabled()) {
>>> +            error_report("mach-virt: KVM does not support Security extensions");
>>> +            exit(1);
>>> +        }
>>> +
>>> +        /*
>>> +         * The Secure view of the world is the same as the NonSecure,
>>> +         * but with a few extra devices. Create it as a container region
>>> +         * containing the system memory at low priority; any secure-only
>>> +         * devices go in at higher priority and take precedence.
>>> +         */
>>> +        secure_sysmem = g_new(MemoryRegion, 1);
>>> +        memory_region_init(secure_sysmem, OBJECT(machine), "secure-memory",
>>> +                           UINT64_MAX);
>>> +        memory_region_add_subregion_overlap(secure_sysmem, 0, sysmem, -1);
>>> +    }
>>> +
>>> +    firmware_loaded = virt_firmware_init(vms, sysmem,
>>> +                                         secure_sysmem ?: sysmem);
>>> +
>>>      /* If we have an EL3 boot ROM then the assumption is that it will
>>>       * implement PSCI itself, so disable QEMU's internal implementation
>>>       * so it doesn't get in the way. Instead of starting secondary
>>
>> Namely, at this point, I seem to understand that we have to hoist this
>> "vms->secure" block here, because:
>>
>> - below (not seen in the context), we have a condition on "firmware_loaded",
>
> Correct.
>
>> - "firmware_loaded" *now* depends on "secure_sysmem" (it used not to,
>> before)
>
> Correct again.
>
> Before the patch, we have
>
>     bool firmware_loaded = bios_name || drive_get(IF_PFLASH, 0, 0);
>
> which really means "-bios or -drive if=pflash,unit=0 given".  If true,
> create_flash() below will surely put firmware into the first flash chip.

OK, so here I almost challenged you, "that's not what create_flash()
does" -- but then I looked more closely, and create_flash() does call
create_one_flash() *once* with (file = bios_name), and then
create_one_flash() does mess with IF_PFLASH, partly dependent on "file"
being non-NULL...

I guess what's stunning me the most is how baroque this code is,
relative to the x86 version, intermixed with FDT management and secure
world and stuff. Obviously I have *contributed* to its complexity,
because "firmware_loaded" comes from me, IIRC. :/ No wonder I don't
understand your patch: I don't understand the pre-patch code.

>
> Afterwards, predicting "will put firmware" is more complex.  Instead of
> coding up a prediction of what create_flash()'s replacement
> virt_firmware_init() is going to do, I have virt_firmware_init() return
> what it did:
>
>         firmware_loaded = virt_firmware_init(vms, sysmem,
>                                              secure_sysmem ?: sysmem);
>
> Naturally, I have to do this before firmware_loaded is used.  So
> create_flash()'s replacement virt_firmware_init() moves up right before
> the first use of firmware_loaded.  On its way, ...
>
>> - and "secure_sysmem" depends on the "vms->secure" block that we're
>> moving up here.
>
> ... it bumps into the computation of secure_sysmem, and pushes it up,
> too.

OK.

>> And that's where I grind to a halt, because I don't understand what the
>> old functions are responsible for exactly, and how the responsibilities
>> are now re-distributed, between the new functions.
>>
>> I tried to look at this patch with full function context, and I also
>> tried to look at the full file (pre vs. post) through a colorized
>> side-by-side diff. To no use.
>>
>> Now, I'm not saying this is a fault with the patch. It's entirely
>> possible that the goal can only be achieved with such a "rewrite". I
>> think it plausible that the patch cannot be done in multiple smaller
>> patches, especially if we consider bisectability a requirement. (Maybe
>> splitting the patch to smaller logical steps would be possible if we
>> accepted halfway constructed and/or orphan objects, mid-series, that
>> build and cause no issues at runtime. I don't know.)
>
> Maybe it's possible, period.
>
>> So it's *my* fault -- it's like the code is sliced into small bits and
>> then reshuffled to a new story, and I can't follow the bits' dance.
>>
>> Can you add two high-level call trees, side-by-side, to the commit
>> message, not just with function names but also with responsibilities?
>>
>> I guess I won't ask for arrows from the left to the right :) I'll try to
>> draw them myself.
>
> Before the patch:

(ahh, fantastic. This is awesome. Thank you for it.

Whining a bit in parentheses: why oh why do we call heavy-weight
functions like qdev_create() as part of local variable initialization?
My eyes are by now used to edk2 code mostly, and there, local variable
initialization is *forbidden*, you can't even assign constants)

>     main()
>         machine_run_board_init()
>             machvirt_init() [method .init()]
>                 create_flash()
>                     create_one_flash() for flash[0]
>                         create
>                         configure
>                             includes obeying -drive if=pflash,unit=0

I guess that's the qdev_prop_set_drive() part

>                         realize
>                         map

... not even an empty line between the last qdev_prop_set_string() and
qdev_init_nofail()...

>                         fall back to -bios
>                     create_one_flash() for flash[1]
>                         create
>                         configure
>                             includes obeying -drive if=pflash,unit=1

nice, the "1" in flash[1] comes from create_flash(), i.e., the caller,
but "unit=1" for qdev_prop_set_drive() comes from... drive_get_next() in
the variable initializer? :)

... and then the bios-handling code at the bottom nonetheless uses
drive_get(), with an explicit unit number? ;)

(I'm not mocking the code, I'm laughing at myself: for brazenly
attempting to "skim" this code before)

>                         realize
>                         map
>                     update FDT
>
> All the action is in create_flash().

Awesome. Thanks!

>
> Creating onboard devices in the machine's .init() method is common, but
> that doesn't make it right.  By the time .init() runs, we're long done
> with setting properties.  So anything we create that late cannot expose
> properties.

Ahh, this is a recurring topic. I remember this from a recent downstream
review.

>  The proper place to create onboard devices is
> .instance_init().  This is why I need to split up create_flash().
>
> After the patch:
>
>     main()
>         current_machine = object_new()
>             ...
>                 virt_instance_init() [method .instance_init()]
>                     virt_flash_create()
>                         virt_flash_create1() for flash[0]

(typo: virt_pflash_create1)

>                             create

(easier to read now! at least qdev_create() stands reasonably isolated)

>                             configure: set defaults
>                             become child of machine
>                             add machine prop pflash0 as alias for drive

beautiful (I've applied your patch and am reading the source in parallel
to your road signs)

>                         virt_flash_create1() for flash[1]
>                             create
>                             configure: set defaults
>                             become child of machine [NEW]
>                             add machine prop pflash1 as alias for drive [NEW]

the [NEW] markers apply to the same steps in the invocation with
flash[0] too, don't they?

>         for all machine props: machine_set_property()

uhoh, I got lost a bit here, but according to the source, I think you
meant "for all machine properties from the *command line*"

>             ...
>                 property_set_alias() for machine props pflash0, pflash1
>                     ...
>                         set_drive() for cfi.pflash01 prop drive
>                             this is how -machine pflash0=... etc set

and the magic happens

we've split off and moved to the front:
- creation (defaults only)
- configuration

we added, as new:
- parenting both chips to the machine, not the unattached dump
- aliases to both chips' drive properties, by corresponding machine
  properties

and normal machine property parsing from the cmldine ended up setting
the drives. Yay!

we still need:
- configuration (non-defaults)
- realize (per flash)
- map (per flash)
- update FDT (single node for both chips, as long as there's no "secure"
  flash)

>         machine_run_board_init(current_machine);
>             virt_firmware_init()
>                 pflash_cfi01_legacy_drive()
>                     legacy -drive if=pflash,unit=0 and =1 [NEW]

OK, so this is where we turn the old style options into those machine
type properties that were *not* set on the cmdline, and hence not
recognized by the "for all machine props: ..." logic, in main()

>                 virt_flash_map()
>                     virt_flash_map1() for flash[0]
>                         configure: num-blocks
>                         realize
>                         map

definitely easier to read -- the function body is smaller (and I'm
starting to get the hang of it too)

>                     virt_flash_map1() for flash[1]
>                         configure: num-blocks
>                         realize
>                         map
>                 fall back to -bios

This was really thick, but I chewed my way through it!

>             virt_flash_fdt()
>                 update FDT
>
> Hope this helps!

I've spent more two hours on reading your email, the source code, and
writing my stupid little comments, and it's been a blast.

I'm sold on the patch. I still don't feel like I'm an authority on it,
so I can't offer an R-b, but I'm happy to give

Acked-by: Laszlo Ersek <lersek@redhat.com>

on one condition: I beg you to include the before-after call trees in
the commit message. :)

... Today's not been in vain! :)

You rock,
Laszlo

>
>> To re-state: I'm not complaining about the patch, just saying that I've
>> failed to understand it. I don't insist on understanding it (I'm fine if
>> others understand it and approve it), but if I'm expected to ACK, I'll
>> need more explanation. A lot more. :) Preferably captured within the series.
>>
>> Thanks (and I apologize)
>
> No problem.
>