Factored out of pc_system_firmware_init() so the next commit can reuse
it in hw/arm/virt.c.
Signed-off-by: Markus Armbruster <armbru@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.
+ * Map unit#i (0 < i <= @ndev) to @dev[i]'s property "drive".
+ */
+void pflash_cfi01_legacy_drive(PFlashCFI01 *dev[], int ndev)
+{
+ int i;
+ 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) {
+ 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);
}
/* 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 */
--
2.17.2
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 <armbru@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]).)
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 */
>
>
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 <armbru@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.
Thanks,
Laszlo
>
> 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 */
>>
>>
>
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 <armbru@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 */
>>>
>>>
>>
>
Laszlo Ersek <lersek@redhat.com> 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 <armbru@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"
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 <armbru@redhat.com>
---
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]) {
--
2.17.2
On 4/12/19 9:52 AM, Markus Armbruster wrote:
> 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 <armbru@redhat.com>
> ---
> 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]) {
> -- 2.17.2
Works for me! Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Markus Armbruster <armbru@redhat.com> writes:
> Laszlo Ersek <lersek@redhat.com> writes:
[...]
>> 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.
Inserting here:
Note that the first loop updates pcms->flash[i]->blk for -drive
if=pflash with qdev_prop_set_drive(). The second loop gets the
(possibly updated) value from pflash_cfi01_get_blk().
>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
> 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);
Uh, this should be pflash_cfi01_get_blk(pcms->flash[i]) instead.
> + }
> +
> /* Reject gaps */
> for (i = 1; i < ARRAY_SIZE(pcms->flash); i++) {
> if (pflash_blk[i] && !pflash_blk[i - 1]) {
On 04/12/19 09:52, Markus Armbruster wrote:
> Laszlo Ersek <lersek@redhat.com> 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 <armbru@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"
>
> 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 <armbru@redhat.com>
> ---
> 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 <lersek@redhat.com>
... 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
© 2016 - 2026 Red Hat, Inc.