Re: [Qemu-block] [Qemu-devel] [PATCH 1/2] pflash_cfi01: New pflash_cfi01_legacy_drive()

[Laszlo Ersek]

On 04/12/19 09:52, Markus Armbruster wrote:
> Laszlo Ersek <address@hidden> writes:
> 
>> 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 <address@hidden>
>>>>> ---
>>>>>  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"
> 
> Sure.
> 
>>>>> + * Map unit#i (0 < i <= @ndev) to @dev[i]'s property "drive".
>>>>
>>>> (2) I think you meant (0 <= i < @ndev)
> 
> You're right, of course.
> 
>>>>> + */
>>>>> +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. :)
> 
> Exactly!
> 
>>>>> +    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])
> 
> I don't see the benefit in abstracting from the member access.
> 
> If I could have ->blk outside pflash_cfi01.c without having to expose
> all of PFlashCFI01, and have it read-only, I would.  But this is C, so I
> get to write yet another accessor function.
> 
>>>>> +            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.
> 
> In my defense, the complete new comment is
> 
>     /*
>      * Handle -drive if=pflash for machines that use machine properties.
>      * Map unit#i (0 < i <= @ndev) to @dev[i]'s property "drive".
>      */
> 
>> 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.
> 
> Correct!
> 
> Aside: the workings of qdev_prop_set_drive() used to be reasonably
> obvious until we rebased qdev onto QOM.  Since then it involves a
> conversion to string, a detour through QOM infrastructure, which (among
> other things) converts the string right back.  Progress!
> 
>> ... This is incredibly non-intuitive, especially given that the
>> pre-patch code performed a *similar* assignment explicitly :(
> 
> So, here's my thinking process that led to this patch.
> 
> To make ARM virt work, I gave myself permission to copy code from x86
> without thinking about code duplication.  Once it worked, I checked what
> I actually copied.  Just this loop:
> 
>     /* Map legacy -drive if=pflash to machine properties */
>     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);
>     }
> 
> The easy de-dupe is to put it in a fuction like this (just for
> illustration, not even compile-tested):
> 
>     void pflash_cfi01_legacy_drive(PFlashCFI01 *dev[], int ndev,
>                                    BlockBackend blk[])
>     {
>         int i;
>         DriveInfo *dinfo;
>         Location loc;
> 
>         // the original loop with pcms->flash replaced by dev,
>         // pflash_blk by blk, pflash_cfi01_get_blk(dev[i]) by
>         // dev[i]->blk, and (for good measure) pflash_drv by
>         // dinfo:
>         for (i = 0; i < ndev; i++) {
>             blk[i] = dev[i]->blk;
>             dinfo = drive_get(IF_PFLASH, 0, i);
>             if (!dinfo) {
>                 continue;
>             }
>             loc_push_none(&loc);
>             qemu_opts_loc_restore(dinfo->opts);
>             if (blk[i]) {
>                 error_report("clashes with -machine");
>                 exit(1);
>             }
>             blk[i] = blk_by_legacy_dinfo(dinfo);
>             qdev_prop_set_drive(DEVICE(dev[i]),
>                                 "drive", blk[i], &error_fatal);
>             loc_pop(&loc);
>         }
>     }
> 
> Called like
> 
>         pflash_cfi01_legacy_drive(pcms->flash, ARRAY_SIZE(pcms->flash),
>                                   pflash_blk);
> 
> The last parameter is awkward.  If you doubt me, try writing a contract.
> Further evidence: the ARM virt caller only ever uses blk[0].
> 
> The awkwardness is cause by the original loop doing two things: map
> legacy -drive to properties, and collect all the BlockBackends for use
> after the loop.
> 
> Better factor just the former out of the loop:
> 
>         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]);
>         }
> 
> Now transform pflash_cfi01_legacy_drive().  First step: replace the last
> parameter by a local variable:
> 
>         BlockBackend *blk[ndev];
> 
> Variable length array, no good.  But since its only use is blk[i] in the
> loop, we can collapse it to just
> 
>         BlockBackend *blk;
> 
> used like this:
> 
>         for (i = 0; i < ndev; i++) {
>             blk = dev[i]->blk;
>             dinfo = drive_get(IF_PFLASH, 0, i);
>             if (!dinfo) {
>                 continue;
>             }
>             loc_push_none(&loc);
>             qemu_opts_loc_restore(dinfo->opts);
>             if (blk) {
>                 error_report("clashes with -machine");
>                 exit(1);
>             }
>             blk = blk_by_legacy_dinfo(dinfo);
>             qdev_prop_set_drive(DEVICE(dev[i]),
>                                 "drive", blk, &error_fatal);
>             loc_pop(&loc);
>         }
> 
> Note that each of the two assignments to blk is used just once.  Meh.
> Eliminate the variable:
> 
>         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) {
>                 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);
>         }
> 
> And this is exactly what's in my patch.
> 
>> 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?
> 
> If you think a preparatory patch is called for, I'll create one.
> 
> I'd have difficulties coming up with a commit message for the one you
> proposed.
> 
> I append a patch I can describe.  Would it work for you?
> 
> 
> 
> pc: Split pc_system_firmware_init()'s legacy -drive loop
> 
> The loop does two things: map legacy -drive to properties, and collect
> all the backends for use after the loop.  The next patch will factor
> out the former for reuse in hw/arm/virt.c.  To make that easier, do
> each thing in its own loop.
> 
> Signed-off-by: Markus Armbruster <address@hidden>
> ---
>  hw/i386/pc_sysfw.c | 14 +++++++++-----
>  1 file changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/i386/pc_sysfw.c b/hw/i386/pc_sysfw.c
> index c628540774..90db9b57a4 100644
> --- a/hw/i386/pc_sysfw.c
> +++ b/hw/i386/pc_sysfw.c
> @@ -269,6 +269,7 @@ void pc_system_firmware_init(PCMachineState *pcms,
>  {
>      PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms);
>      int i;
> +    BlockBackend *blk;
>      DriveInfo *pflash_drv;
>      BlockBackend *pflash_blk[ARRAY_SIZE(pcms->flash)];
>      Location loc;
> @@ -280,23 +281,26 @@ void pc_system_firmware_init(PCMachineState *pcms,
>  
>      /* Map legacy -drive if=pflash to machine properties */
>      for (i = 0; i < ARRAY_SIZE(pcms->flash); i++) {
> -        pflash_blk[i] = pflash_cfi01_get_blk(pcms->flash[i]);
> +        blk = 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]) {
> +        if (blk) {
>              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);
> +        qdev_prop_set_drive(DEVICE(pcms->flash[i]), "drive",
> +                            blk_by_legacy_dinfo(pflash_drv), &error_fatal);
>          loc_pop(&loc);
>      }
>  
> +    for (i = 0; i < ARRAY_SIZE(pcms->flash); i++) {
> +        pflash_blk[i] = blk_by_legacy_dinfo(pflash_drv);

[*]

> +    }
> +
>      /* Reject gaps */
>      for (i = 1; i < ARRAY_SIZE(pcms->flash); i++) {
>          if (pflash_blk[i] && !pflash_blk[i - 1]) {
> 

Yes, this is a huge boost to readability, as far as I'm concerned --
primarily because it kills the *second* "pflash_blk[i]" assignment in
the original loop body. Please do include this patch, and append, at once:

Reviewed-by: Laszlo Ersek <address@hidden>

... BTW: you are assigning "blk" once, and reading it once. You might as
well drop "blk" and substitute "pflash_cfi01_get_blk(pcms->flash[i])".

[*] Yes, I'm seeing your update down-thread. :)

Thanks,
Laszlo