[Qemu-devel] [PATCH v2 00/12] pc: Support firmware configuration with -blockdev

[Qemu-devel] [PATCH v2 00/12] pc: Support firmware configuration with -blockdev

[Markus Armbruster]

Based-on: <20190307130323.9353-1-armbru@redhat.com>

v2:
* PATCH 05,06: New [Philippe]
* PATCH 07: Old PATCH 5 rebased onto new patches, with new comment
  [Philippe]
* PATCH 08+10: New, factored out of old PATCH 6 [by Philippe]
* PATCH 09: New [by Philippe]
* PATCH 11: Remainder of old PATCH 6, with FIXME resolved, literal
  4096 de-duplicated [László], extraneous error check deleted
  [László], and comment typos fixed
* PATCH 12: New

Markus Armbruster (9):
  qdev: Fix latent bug with compat_props and onboard devices
  qom: Move compat_props machinery from qdev to QOM
  vl: Fix latent bug with -global and onboard devices
  sysbus: Fix latent bug with onboard devices
  vl: Improve legibility of BlockdevOptions queue
  vl: Factor configure_blockdev() out of main()
  vl: Create block backends before setting machine properties
  pc: Support firmware configuration with -blockdev
  docs/interop/firmware.json: Prefer -machine to if=pflash

Philippe Mathieu-Daudé (3):
  pflash_cfi01: Add pflash_cfi01_get_blk() helper
  pc_sysfw: Remove unused PcSysFwDevice
  pc_sysfw: Pass PCMachineState to pc_system_firmware_init()

 accel/accel.c              |   1 +
 docs/interop/firmware.json |  21 +++-
 hw/block/pflash_cfi01.c    |   5 +
 hw/core/qdev.c             |  21 +---
 hw/core/sysbus.c           |   3 -
 hw/i386/pc.c               |   4 +-
 hw/i386/pc_sysfw.c         | 249 ++++++++++++++++++++++++-------------
 include/hw/block/flash.h   |   1 +
 include/hw/i386/pc.h       |   6 +-
 include/hw/qdev-core.h     |   2 -
 include/qom/object.h       |   3 +
 qom/object.c               |  39 ++++++
 vl.c                       | 122 +++++++++---------
 13 files changed, 301 insertions(+), 176 deletions(-)

-- 
2.17.2

[Qemu-devel] [PATCH v2 01/12] qdev: Fix latent bug with compat_props and onboard devices

[Markus Armbruster]

Compatibility properties started life as a qdev property thing: we
supported them only for qdev properties, and implemented them with the
machinery backing command line option -global.

Recent commit fa0cb34d221 put them to use (tacitly) with memory
backend objects (subtypes of TYPE_MEMORY_BACKEND).  To make that
possible, we first moved the work of applying them from the -global
machinery into TYPE_DEVICE's .instance_post_init() method
device_post_init(), in commits ea9ce8934c5 and b66bbee39f6, then made
it available to TYPE_MEMORY_BACKEND's .instance_post_init() method
host_memory_backend_post_init() as object_apply_compat_props(), in
commit 1c3994f6d2a.

Note the code smell: we now have function name starting with object_
in hw/core/qdev.c.  It has to be there rather than in qom/, because it
calls qdev_get_machine() to find the current accelerator's and
machine's compat_props.

Turns out calling qdev_get_machine() there is problematic.  If we
qdev_create() from a machine's .instance_init() method, we call
device_post_init() and thus qdev_get_machine() before main() can
create "/machine" in QOM.  qdev_get_machine() tries to get it with
container_get(), which "helpfully" creates it as "container" object,
and returns that.  object_apply_compat_props() tries to paper over the
problem by doing nothing when the value of qdev_get_machine() isn't a
TYPE_MACHINE.  But the damage is done already: when main() later
attempts to create the real "/machine", it fails with "attempt to add
duplicate property 'machine' to object (type 'container')", and
aborts.

Since no machine .instance_init() calls qdev_create() so far, the bug
is latent.  But since I want to do that, I get to fix the bug first.

Observe that object_apply_compat_props() doesn't actually need the
MachineState, only its the compat_props member of its MachineClass and
AccelClass.  This permits a simple fix: register MachineClass and
AccelClass compat_props with the object_apply_compat_props() machinery
right after these classes get selected.

This is actually similar to how things worked before commits
ea9ce8934c5 and b66bbee39f6, except we now register much earlier.  The
old code registered them only after the machine's .instance_init()
ran, which would've broken compatibility properties for any devices
created there.

Cc: Marc-André Lureau <marcandre.lureau@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 accel/accel.c          |  1 +
 hw/core/qdev.c         | 48 ++++++++++++++++++++++++++++++++----------
 include/hw/qdev-core.h |  2 ++
 vl.c                   |  1 +
 4 files changed, 41 insertions(+), 11 deletions(-)

diff --git a/accel/accel.c b/accel/accel.c
index 68b6d56323..4a1670e404 100644
--- a/accel/accel.c
+++ b/accel/accel.c
@@ -66,6 +66,7 @@ static int accel_init_machine(AccelClass *acc, MachineState *ms)
         *(acc->allowed) = false;
         object_unref(OBJECT(accel));
     }
+    object_set_accelerator_compat_props(acc->compat_props);
     return ret;
 }
 
diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index d59071b8ed..1a86c7990a 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -970,25 +970,51 @@ static void device_initfn(Object *obj)
     QLIST_INIT(&dev->gpios);
 }
 
+/*
+ * Global property defaults
+ * Slot 0: accelerator's global property defaults
+ * Slot 1: machine's global property defaults
+ * Each is a GPtrArray of of GlobalProperty.
+ * Applied in order, later entries override earlier ones.
+ */
+static GPtrArray *object_compat_props[2];
+
+/*
+ * Set machine's global property defaults to @compat_props.
+ * May be called at most once.
+ */
+void object_set_machine_compat_props(GPtrArray *compat_props)
+{
+    assert(!object_compat_props[1]);
+    object_compat_props[1] = compat_props;
+}
+
+/*
+ * Set accelerator's global property defaults to @compat_props.
+ * May be called at most once.
+ */
+void object_set_accelerator_compat_props(GPtrArray *compat_props)
+{
+    assert(!object_compat_props[0]);
+    object_compat_props[0] = compat_props;
+}
+
 void object_apply_compat_props(Object *obj)
 {
-    if (object_dynamic_cast(qdev_get_machine(), TYPE_MACHINE)) {
-        MachineState *m = MACHINE(qdev_get_machine());
-        MachineClass *mc = MACHINE_GET_CLASS(m);
+    int i;
 
-        if (m->accelerator) {
-            AccelClass *ac = ACCEL_GET_CLASS(m->accelerator);
-
-            if (ac->compat_props) {
-                object_apply_global_props(obj, ac->compat_props, &error_abort);
-            }
-        }
-        object_apply_global_props(obj, mc->compat_props, &error_abort);
+    for (i = 0; i < ARRAY_SIZE(object_compat_props); i++) {
+        object_apply_global_props(obj, object_compat_props[i],
+                                  &error_abort);
     }
 }
 
 static void device_post_init(Object *obj)
 {
+    /*
+     * Note: ordered so that the user's global properties take
+     * precedence.
+     */
     object_apply_compat_props(obj);
     qdev_prop_set_globals(DEVICE(obj));
 }
diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index e70a4bfa49..c1dc06e9bb 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -418,6 +418,8 @@ const char *qdev_fw_name(DeviceState *dev);
 
 Object *qdev_get_machine(void);
 
+void object_set_machine_compat_props(GPtrArray *compat_props);
+void object_set_accelerator_compat_props(GPtrArray *compat_props);
 void object_apply_compat_props(Object *obj);
 
 /* FIXME: make this a link<> */
diff --git a/vl.c b/vl.c
index fd0d51320d..ea58a16a4a 100644
--- a/vl.c
+++ b/vl.c
@@ -3954,6 +3954,7 @@ int main(int argc, char **argv, char **envp)
     configure_rtc(qemu_find_opts_singleton("rtc"));
 
     machine_class = select_machine();
+    object_set_machine_compat_props(machine_class->compat_props);
 
     set_memory_options(&ram_slots, &maxram_size, machine_class);
 
-- 
2.17.2

[Qemu-devel] [PATCH v2 02/12] qom: Move compat_props machinery from qdev to QOM

[Markus Armbruster]

See the previous commit for rationale.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/core/qdev.c         | 39 ---------------------------------------
 include/hw/qdev-core.h |  4 ----
 include/qom/object.h   |  3 +++
 qom/object.c           | 39 +++++++++++++++++++++++++++++++++++++++
 4 files changed, 42 insertions(+), 43 deletions(-)

diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index 1a86c7990a..25dffad3ed 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -970,45 +970,6 @@ static void device_initfn(Object *obj)
     QLIST_INIT(&dev->gpios);
 }
 
-/*
- * Global property defaults
- * Slot 0: accelerator's global property defaults
- * Slot 1: machine's global property defaults
- * Each is a GPtrArray of of GlobalProperty.
- * Applied in order, later entries override earlier ones.
- */
-static GPtrArray *object_compat_props[2];
-
-/*
- * Set machine's global property defaults to @compat_props.
- * May be called at most once.
- */
-void object_set_machine_compat_props(GPtrArray *compat_props)
-{
-    assert(!object_compat_props[1]);
-    object_compat_props[1] = compat_props;
-}
-
-/*
- * Set accelerator's global property defaults to @compat_props.
- * May be called at most once.
- */
-void object_set_accelerator_compat_props(GPtrArray *compat_props)
-{
-    assert(!object_compat_props[0]);
-    object_compat_props[0] = compat_props;
-}
-
-void object_apply_compat_props(Object *obj)
-{
-    int i;
-
-    for (i = 0; i < ARRAY_SIZE(object_compat_props); i++) {
-        object_apply_global_props(obj, object_compat_props[i],
-                                  &error_abort);
-    }
-}
-
 static void device_post_init(Object *obj)
 {
     /*
diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index c1dc06e9bb..db9bb2054f 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -418,10 +418,6 @@ const char *qdev_fw_name(DeviceState *dev);
 
 Object *qdev_get_machine(void);
 
-void object_set_machine_compat_props(GPtrArray *compat_props);
-void object_set_accelerator_compat_props(GPtrArray *compat_props);
-void object_apply_compat_props(Object *obj);
-
 /* FIXME: make this a link<> */
 void qdev_set_parent_bus(DeviceState *dev, BusState *bus);
 
diff --git a/include/qom/object.h b/include/qom/object.h
index e0262962b5..288cdddf44 100644
--- a/include/qom/object.h
+++ b/include/qom/object.h
@@ -677,6 +677,9 @@ Object *object_new_with_propv(const char *typename,
 
 void object_apply_global_props(Object *obj, const GPtrArray *props,
                                Error **errp);
+void object_set_machine_compat_props(GPtrArray *compat_props);
+void object_set_accelerator_compat_props(GPtrArray *compat_props);
+void object_apply_compat_props(Object *obj);
 
 /**
  * object_set_props:
diff --git a/qom/object.c b/qom/object.c
index 05a8567041..e3206d6799 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -408,6 +408,45 @@ void object_apply_global_props(Object *obj, const GPtrArray *props, Error **errp
     }
 }
 
+/*
+ * Global property defaults
+ * Slot 0: accelerator's global property defaults
+ * Slot 1: machine's global property defaults
+ * Each is a GPtrArray of of GlobalProperty.
+ * Applied in order, later entries override earlier ones.
+ */
+static GPtrArray *object_compat_props[2];
+
+/*
+ * Set machine's global property defaults to @compat_props.
+ * May be called at most once.
+ */
+void object_set_machine_compat_props(GPtrArray *compat_props)
+{
+    assert(!object_compat_props[1]);
+    object_compat_props[1] = compat_props;
+}
+
+/*
+ * Set accelerator's global property defaults to @compat_props.
+ * May be called at most once.
+ */
+void object_set_accelerator_compat_props(GPtrArray *compat_props)
+{
+    assert(!object_compat_props[0]);
+    object_compat_props[0] = compat_props;
+}
+
+void object_apply_compat_props(Object *obj)
+{
+    int i;
+
+    for (i = 0; i < ARRAY_SIZE(object_compat_props); i++) {
+        object_apply_global_props(obj, object_compat_props[i],
+                                  &error_abort);
+    }
+}
+
 static void object_initialize_with_type(void *data, size_t size, TypeImpl *type)
 {
     Object *obj = data;
-- 
2.17.2

[Qemu-devel] [PATCH v2 03/12] vl: Fix latent bug with -global and onboard devices

[Markus Armbruster]

main() registers the user's -global only after we create the machine
object, i.e. too late for devices created in the machine's
.instance_init().

Fortunately, we know the bug is only latent: the commit before
previous fixed a bug that would've crashed any attempt to create a
device in an .instance_init().

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 vl.c | 19 ++-----------------
 1 file changed, 2 insertions(+), 17 deletions(-)

diff --git a/vl.c b/vl.c
index ea58a16a4a..3f41e72892 100644
--- a/vl.c
+++ b/vl.c
@@ -2939,17 +2939,6 @@ static void user_register_global_props(void)
                       global_init_func, NULL, NULL);
 }
 
-/*
- * Note: we should see that these properties are actually having a
- * priority: accel < machine < user. This means e.g. when user
- * specifies something in "-global", it'll always be used with highest
- * priority than either machine/accelerator compat properties.
- */
-static void register_global_properties(MachineState *ms)
-{
-    user_register_global_props();
-}
-
 int main(int argc, char **argv, char **envp)
 {
     int i;
@@ -3943,6 +3932,8 @@ int main(int argc, char **argv, char **envp)
      */
     loc_set_none();
 
+    user_register_global_props();
+
     replay_configure(icount_opts);
 
     if (incoming && !preconfig_exit_requested) {
@@ -4251,12 +4242,6 @@ int main(int argc, char **argv, char **envp)
                      machine_class->name, machine_class->deprecation_reason);
     }
 
-    /*
-     * Register all the global properties, including accel properties,
-     * machine properties, and user-specified ones.
-     */
-    register_global_properties(current_machine);
-
     /*
      * Migration object can only be created after global properties
      * are applied correctly.
-- 
2.17.2

[Qemu-devel] [PATCH v2 04/12] sysbus: Fix latent bug with onboard devices

[Markus Armbruster]

The first call of sysbus_get_default() creates the main system bus and
stores it in QOM as "/machine/unattached/sysbus".  This must not
happen before main() creates "/machine", or else container_get() would
"helpfully" create it as "container" object, and the real creation of
"/machine" would later abort with "attempt to add duplicate property
'machine' to object (type 'container')".  Has been that way ever since
we wired up busses in QOM (commit f968fc6892d, v1.2.0).

I believe the bug is latent.  I got it to bite by trying to
qdev_create() a sysbus device from a machine's .instance_init()
method.

The fix is obvious: store the main system bus in QOM right after
creating "/machine".

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Reviewed-by: Thomas Huth <thuth@redhat.com>
---
 hw/core/sysbus.c | 3 ---
 vl.c             | 4 ++++
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/hw/core/sysbus.c b/hw/core/sysbus.c
index 9f9edbcab9..307cf90a51 100644
--- a/hw/core/sysbus.c
+++ b/hw/core/sysbus.c
@@ -357,9 +357,6 @@ static void main_system_bus_create(void)
     qbus_create_inplace(main_system_bus, system_bus_info.instance_size,
                         TYPE_SYSTEM_BUS, NULL, "main-system-bus");
     OBJECT(main_system_bus)->free = g_free;
-    object_property_add_child(container_get(qdev_get_machine(),
-                                            "/unattached"),
-                              "sysbus", OBJECT(main_system_bus), NULL);
 }
 
 BusState *sysbus_get_default(void)
diff --git a/vl.c b/vl.c
index 3f41e72892..32144d25df 100644
--- a/vl.c
+++ b/vl.c
@@ -3990,6 +3990,10 @@ int main(int argc, char **argv, char **envp)
     }
     object_property_add_child(object_get_root(), "machine",
                               OBJECT(current_machine), &error_abort);
+    object_property_add_child(container_get(OBJECT(current_machine),
+                                            "/unattached"),
+                              "sysbus", OBJECT(sysbus_get_default()),
+                              NULL);
 
     if (machine_class->minimum_page_bits) {
         if (!set_preferred_target_page_bits(machine_class->minimum_page_bits)) {
-- 
2.17.2

[Qemu-devel] [PATCH v2 05/12] vl: Improve legibility of BlockdevOptions queue

[Markus Armbruster]

Give the queue head type a name: BlockdevOptionsQueue.

Rename the queue entry type from BlockdevOptions_queue to
BlockdevOptionsQueueEntry.

Signed-off-by: Markus Armbruster <armbru@redhat.com>

# Conflicts:
#	vl.c
---
 vl.c | 22 ++++++++++++----------
 1 file changed, 12 insertions(+), 10 deletions(-)

diff --git a/vl.c b/vl.c
index 32144d25df..00cb8ea7ca 100644
--- a/vl.c
+++ b/vl.c
@@ -1193,6 +1193,14 @@ static void default_drive(int enable, int snapshot, BlockInterfaceType type,
 
 }
 
+typedef struct BlockdevOptionsQueueEntry {
+    BlockdevOptions *bdo;
+    Location loc;
+    QSIMPLEQ_ENTRY(BlockdevOptionsQueueEntry) entry;
+} BlockdevOptionsQueueEntry;
+
+typedef QSIMPLEQ_HEAD(, BlockdevOptionsQueueEntry) BlockdevOptionsQueue;
+
 static QemuOptsList qemu_smp_opts = {
     .name = "smp-opts",
     .implied_opt_name = "cpus",
@@ -2973,13 +2981,7 @@ int main(int argc, char **argv, char **envp)
     Error *err = NULL;
     bool list_data_dirs = false;
     char *dir, **dirs;
-    typedef struct BlockdevOptions_queue {
-        BlockdevOptions *bdo;
-        Location loc;
-        QSIMPLEQ_ENTRY(BlockdevOptions_queue) entry;
-    } BlockdevOptions_queue;
-    QSIMPLEQ_HEAD(, BlockdevOptions_queue) bdo_queue
-        = QSIMPLEQ_HEAD_INITIALIZER(bdo_queue);
+    BlockdevOptionsQueue bdo_queue = QSIMPLEQ_HEAD_INITIALIZER(bdo_queue);
 
     module_call_init(MODULE_INIT_TRACE);
 
@@ -3102,12 +3104,12 @@ int main(int argc, char **argv, char **envp)
             case QEMU_OPTION_blockdev:
                 {
                     Visitor *v;
-                    BlockdevOptions_queue *bdo;
+                    BlockdevOptionsQueueEntry *bdo;
 
                     v = qobject_input_visitor_new_str(optarg, "driver",
                                                       &error_fatal);
 
-                    bdo = g_new(BlockdevOptions_queue, 1);
+                    bdo = g_new(BlockdevOptionsQueueEntry, 1);
                     visit_type_BlockdevOptions(v, NULL, &bdo->bdo,
                                                &error_fatal);
                     visit_free(v);
@@ -4367,7 +4369,7 @@ int main(int argc, char **argv, char **envp)
 
     /* open the virtual block devices */
     while (!QSIMPLEQ_EMPTY(&bdo_queue)) {
-        BlockdevOptions_queue *bdo = QSIMPLEQ_FIRST(&bdo_queue);
+        BlockdevOptionsQueueEntry *bdo = QSIMPLEQ_FIRST(&bdo_queue);
 
         QSIMPLEQ_REMOVE_HEAD(&bdo_queue, entry);
         loc_push_restore(&bdo->loc);
-- 
2.17.2

[Qemu-devel] [PATCH v2 06/12] vl: Factor configure_blockdev() out of main()

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@redhat.com>

# Conflicts:
#	vl.c
---
 vl.c | 71 +++++++++++++++++++++++++++++++++---------------------------
 1 file changed, 39 insertions(+), 32 deletions(-)

diff --git a/vl.c b/vl.c
index 00cb8ea7ca..5a19d2a8ec 100644
--- a/vl.c
+++ b/vl.c
@@ -1201,6 +1201,44 @@ typedef struct BlockdevOptionsQueueEntry {
 
 typedef QSIMPLEQ_HEAD(, BlockdevOptionsQueueEntry) BlockdevOptionsQueue;
 
+static void configure_blockdev(BlockdevOptionsQueue *bdo_queue,
+                               MachineClass *machine_class, int snapshot)
+{
+    /* If the currently selected machine wishes to override the units-per-bus
+     * property of its default HBA interface type, do so now. */
+    if (machine_class->units_per_default_bus) {
+        override_max_devs(machine_class->block_default_type,
+                          machine_class->units_per_default_bus);
+    }
+
+    /* open the virtual block devices */
+    while (!QSIMPLEQ_EMPTY(bdo_queue)) {
+        BlockdevOptionsQueueEntry *bdo = QSIMPLEQ_FIRST(bdo_queue);
+
+        QSIMPLEQ_REMOVE_HEAD(bdo_queue, entry);
+        loc_push_restore(&bdo->loc);
+        qmp_blockdev_add(bdo->bdo, &error_fatal);
+        loc_pop(&bdo->loc);
+        qapi_free_BlockdevOptions(bdo->bdo);
+        g_free(bdo);
+    }
+    if (snapshot || replay_mode != REPLAY_MODE_NONE) {
+        qemu_opts_foreach(qemu_find_opts("drive"), drive_enable_snapshot,
+                          NULL, NULL);
+    }
+    if (qemu_opts_foreach(qemu_find_opts("drive"), drive_init_func,
+                          &machine_class->block_default_type, &error_fatal)) {
+        /* We printed help */
+        exit(0);
+    }
+
+    default_drive(default_cdrom, snapshot, machine_class->block_default_type, 2,
+                  CDROM_OPTS);
+    default_drive(default_floppy, snapshot, IF_FLOPPY, 0, FD_OPTS);
+    default_drive(default_sdcard, snapshot, IF_SD, 0, SD_OPTS);
+
+}
+
 static QemuOptsList qemu_smp_opts = {
     .name = "smp-opts",
     .implied_opt_name = "cpus",
@@ -4360,38 +4398,7 @@ int main(int argc, char **argv, char **envp)
     ram_mig_init();
     dirty_bitmap_mig_init();
 
-    /* If the currently selected machine wishes to override the units-per-bus
-     * property of its default HBA interface type, do so now. */
-    if (machine_class->units_per_default_bus) {
-        override_max_devs(machine_class->block_default_type,
-                          machine_class->units_per_default_bus);
-    }
-
-    /* open the virtual block devices */
-    while (!QSIMPLEQ_EMPTY(&bdo_queue)) {
-        BlockdevOptionsQueueEntry *bdo = QSIMPLEQ_FIRST(&bdo_queue);
-
-        QSIMPLEQ_REMOVE_HEAD(&bdo_queue, entry);
-        loc_push_restore(&bdo->loc);
-        qmp_blockdev_add(bdo->bdo, &error_fatal);
-        loc_pop(&bdo->loc);
-        qapi_free_BlockdevOptions(bdo->bdo);
-        g_free(bdo);
-    }
-    if (snapshot || replay_mode != REPLAY_MODE_NONE) {
-        qemu_opts_foreach(qemu_find_opts("drive"), drive_enable_snapshot,
-                          NULL, NULL);
-    }
-    if (qemu_opts_foreach(qemu_find_opts("drive"), drive_init_func,
-                          &machine_class->block_default_type, &error_fatal)) {
-        /* We printed help */
-        exit(0);
-    }
-
-    default_drive(default_cdrom, snapshot, machine_class->block_default_type, 2,
-                  CDROM_OPTS);
-    default_drive(default_floppy, snapshot, IF_FLOPPY, 0, FD_OPTS);
-    default_drive(default_sdcard, snapshot, IF_SD, 0, SD_OPTS);
+    configure_blockdev(&bdo_queue, machine_class, snapshot);
 
     qemu_opts_foreach(qemu_find_opts("mon"),
                       mon_init_func, NULL, &error_fatal);
-- 
2.17.2

Re: [Qemu-devel] [PATCH v2 06/12] vl: Factor configure_blockdev() out of main()

[Philippe Mathieu-Daudé]

On 3/7/19 6:23 PM, Markus Armbruster wrote:
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> 
> # Conflicts:
> #	vl.c

^ leftover?

> ---
>  vl.c | 71 +++++++++++++++++++++++++++++++++---------------------------
>  1 file changed, 39 insertions(+), 32 deletions(-)
> 
> diff --git a/vl.c b/vl.c
> index 00cb8ea7ca..5a19d2a8ec 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -1201,6 +1201,44 @@ typedef struct BlockdevOptionsQueueEntry {
>  
>  typedef QSIMPLEQ_HEAD(, BlockdevOptionsQueueEntry) BlockdevOptionsQueue;
>  
> +static void configure_blockdev(BlockdevOptionsQueue *bdo_queue,
> +                               MachineClass *machine_class, int snapshot)
> +{
> +    /* If the currently selected machine wishes to override the units-per-bus
> +     * property of its default HBA interface type, do so now. */
> +    if (machine_class->units_per_default_bus) {
> +        override_max_devs(machine_class->block_default_type,
> +                          machine_class->units_per_default_bus);
> +    }
> +
> +    /* open the virtual block devices */
> +    while (!QSIMPLEQ_EMPTY(bdo_queue)) {
> +        BlockdevOptionsQueueEntry *bdo = QSIMPLEQ_FIRST(bdo_queue);
> +
> +        QSIMPLEQ_REMOVE_HEAD(bdo_queue, entry);
> +        loc_push_restore(&bdo->loc);
> +        qmp_blockdev_add(bdo->bdo, &error_fatal);
> +        loc_pop(&bdo->loc);
> +        qapi_free_BlockdevOptions(bdo->bdo);
> +        g_free(bdo);
> +    }
> +    if (snapshot || replay_mode != REPLAY_MODE_NONE) {
> +        qemu_opts_foreach(qemu_find_opts("drive"), drive_enable_snapshot,
> +                          NULL, NULL);
> +    }
> +    if (qemu_opts_foreach(qemu_find_opts("drive"), drive_init_func,
> +                          &machine_class->block_default_type, &error_fatal)) {
> +        /* We printed help */
> +        exit(0);
> +    }
> +
> +    default_drive(default_cdrom, snapshot, machine_class->block_default_type, 2,
> +                  CDROM_OPTS);
> +    default_drive(default_floppy, snapshot, IF_FLOPPY, 0, FD_OPTS);
> +    default_drive(default_sdcard, snapshot, IF_SD, 0, SD_OPTS);
> +
> +}
> +
>  static QemuOptsList qemu_smp_opts = {
>      .name = "smp-opts",
>      .implied_opt_name = "cpus",
> @@ -4360,38 +4398,7 @@ int main(int argc, char **argv, char **envp)
>      ram_mig_init();
>      dirty_bitmap_mig_init();
>  
> -    /* If the currently selected machine wishes to override the units-per-bus
> -     * property of its default HBA interface type, do so now. */
> -    if (machine_class->units_per_default_bus) {
> -        override_max_devs(machine_class->block_default_type,
> -                          machine_class->units_per_default_bus);
> -    }
> -
> -    /* open the virtual block devices */
> -    while (!QSIMPLEQ_EMPTY(&bdo_queue)) {
> -        BlockdevOptionsQueueEntry *bdo = QSIMPLEQ_FIRST(&bdo_queue);
> -
> -        QSIMPLEQ_REMOVE_HEAD(&bdo_queue, entry);
> -        loc_push_restore(&bdo->loc);
> -        qmp_blockdev_add(bdo->bdo, &error_fatal);
> -        loc_pop(&bdo->loc);
> -        qapi_free_BlockdevOptions(bdo->bdo);
> -        g_free(bdo);
> -    }
> -    if (snapshot || replay_mode != REPLAY_MODE_NONE) {
> -        qemu_opts_foreach(qemu_find_opts("drive"), drive_enable_snapshot,
> -                          NULL, NULL);
> -    }
> -    if (qemu_opts_foreach(qemu_find_opts("drive"), drive_init_func,
> -                          &machine_class->block_default_type, &error_fatal)) {
> -        /* We printed help */
> -        exit(0);
> -    }
> -
> -    default_drive(default_cdrom, snapshot, machine_class->block_default_type, 2,
> -                  CDROM_OPTS);
> -    default_drive(default_floppy, snapshot, IF_FLOPPY, 0, FD_OPTS);
> -    default_drive(default_sdcard, snapshot, IF_SD, 0, SD_OPTS);
> +    configure_blockdev(&bdo_queue, machine_class, snapshot);

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

>  
>      qemu_opts_foreach(qemu_find_opts("mon"),
>                        mon_init_func, NULL, &error_fatal);
> 

Re: [Qemu-devel] [PATCH v2 06/12] vl: Factor configure_blockdev() out of main()

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 603 bytes --]

On 3/7/19 11:23 AM, Markus Armbruster wrote:
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> 
> # Conflicts:
> #	vl.c

How'd you get git to preserve the leading #? Generally, I find conflicts
details useful for cherry-picked backports, but pointless for rebased
patches intended as original upstream material. And git defaults to
stripping lines with leading # when composing a commit message. May be
worth cleaning up before the actual pull request.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [PATCH v2 06/12] vl: Factor configure_blockdev() out of main()

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 3/7/19 11:23 AM, Markus Armbruster wrote:
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> 
>> # Conflicts:
>> #	vl.c
>
> How'd you get git to preserve the leading #? Generally, I find conflicts
> details useful for cherry-picked backports, but pointless for rebased
> patches intended as original upstream material. And git defaults to
> stripping lines with leading # when composing a commit message.

I've messed up too many commit message by having fill-paragraph flow a #
to the beginning of a line, so I added

    [commit]
            cleanup = scissors

to my .gitconfig.  I've been messing up commit messages with leftover
crap ever since, but leftover crap has proven less confusing to my
reviewers than missing lines.

>                                                                 May be
> worth cleaning up before the actual pull request.

Certainly.


git-commit(1):

       --cleanup=<mode>
           This option determines how the supplied commit message should be
           cleaned up before committing. The <mode> can be strip, whitespace,
           verbatim, scissors or default.

           strip
               Strip leading and trailing empty lines, trailing whitespace,
               commentary and collapse consecutive empty lines.

           whitespace
               Same as strip except #commentary is not removed.

           verbatim
               Do not change the message at all.

           scissors
               Same as whitespace except that everything from (and including)
               the line found below is truncated, if the message is to be
               edited. "#" can be customized with core.commentChar.

                   # ------------------------ >8 ------------------------

           default
               Same as strip if the message is to be edited. Otherwise
               whitespace.

           The default can be changed by the commit.cleanup configuration
           variable (see git-config(1)).

Re: [Qemu-devel] [PATCH v2 06/12] vl: Factor configure_blockdev() out of main()

[Laszlo Ersek]

On 03/07/19 20:08, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> writes:
> 
>> On 3/7/19 11:23 AM, Markus Armbruster wrote:
>>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>>>
>>> # Conflicts:
>>> #	vl.c
>>
>> How'd you get git to preserve the leading #? Generally, I find conflicts
>> details useful for cherry-picked backports, but pointless for rebased
>> patches intended as original upstream material. And git defaults to
>> stripping lines with leading # when composing a commit message.
> 
> I've messed up too many commit message by having fill-paragraph flow a #
> to the beginning of a line, so I added
> 
>     [commit]
>             cleanup = scissors

I'm going to steal this now. :)

> 
> to my .gitconfig.  I've been messing up commit messages with leftover
> crap ever since, but leftover crap has proven less confusing to my
> reviewers than missing lines.
> 
>>                                                                 May be
>> worth cleaning up before the actual pull request.
> 
> Certainly.
> 
> 
> git-commit(1):
> 
>        --cleanup=<mode>
>            This option determines how the supplied commit message should be
>            cleaned up before committing. The <mode> can be strip, whitespace,
>            verbatim, scissors or default.
> 
>            strip
>                Strip leading and trailing empty lines, trailing whitespace,
>                commentary and collapse consecutive empty lines.
> 
>            whitespace
>                Same as strip except #commentary is not removed.
> 
>            verbatim
>                Do not change the message at all.
> 
>            scissors
>                Same as whitespace except that everything from (and including)
>                the line found below is truncated, if the message is to be
>                edited. "#" can be customized with core.commentChar.
> 
>                    # ------------------------ >8 ------------------------
> 
>            default
>                Same as strip if the message is to be edited. Otherwise
>                whitespace.
> 
>            The default can be changed by the commit.cleanup configuration
>            variable (see git-config(1)).
> 

Yeah I can tell this documentation was written by a programmer. The
documentation most likely follows the implementation closely (nested
"if"s?), with the "double except". Just try to expand the definition of
"scissors" without a mental stack overflow:

    Same as strip except #commentary is not removed except that
    everything from (and including) the line found below is truncated

Geez.

Anyway, thank you again for the tip!
Laszlo

[Qemu-devel] [PATCH v2 07/12] vl: Create block backends before setting machine properties

[Markus Armbruster]

qemu-system-FOO's main() acts on command line arguments in its own
idiosyncratic order.  There's not much method to its madness.
Whenever we find a case where one kind of command line argument needs
to refer to something created for another kind later, we rejigger the
order.

Block devices get created long after machine properties get processed.
Therefore, block device machine properties can be created, but not
set.  No such properties exist.  But the next commit will create some.
Time to rejigger again: create block devices earlier.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 vl.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/vl.c b/vl.c
index 5a19d2a8ec..c250483fc1 100644
--- a/vl.c
+++ b/vl.c
@@ -4272,6 +4272,13 @@ int main(int argc, char **argv, char **envp)
         exit(0);
     }
 
+    /*
+     * Note: we need to create block backends before
+     * machine_set_property(), so machine properties can refer to
+     * them.
+     */
+    configure_blockdev(&bdo_queue, machine_class, snapshot);
+
     machine_opts = qemu_get_machine_opts();
     qemu_opt_foreach(machine_opts, machine_set_property, current_machine,
                      &error_fatal);
@@ -4398,8 +4405,6 @@ int main(int argc, char **argv, char **envp)
     ram_mig_init();
     dirty_bitmap_mig_init();
 
-    configure_blockdev(&bdo_queue, machine_class, snapshot);
-
     qemu_opts_foreach(qemu_find_opts("mon"),
                       mon_init_func, NULL, &error_fatal);
 
-- 
2.17.2

Re: [Qemu-devel] [PATCH v2 07/12] vl: Create block backends before setting machine properties

[Philippe Mathieu-Daudé]

On 3/7/19 6:23 PM, Markus Armbruster wrote:
> qemu-system-FOO's main() acts on command line arguments in its own
> idiosyncratic order.  There's not much method to its madness.
> Whenever we find a case where one kind of command line argument needs
> to refer to something created for another kind later, we rejigger the
> order.
> 
> Block devices get created long after machine properties get processed.
> Therefore, block device machine properties can be created, but not
> set.  No such properties exist.  But the next commit will create some.
> Time to rejigger again: create block devices earlier.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  vl.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/vl.c b/vl.c
> index 5a19d2a8ec..c250483fc1 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -4272,6 +4272,13 @@ int main(int argc, char **argv, char **envp)
>          exit(0);
>      }
>  
> +    /*
> +     * Note: we need to create block backends before
> +     * machine_set_property(), so machine properties can refer to
> +     * them.
> +     */

Thanks!

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

> +    configure_blockdev(&bdo_queue, machine_class, snapshot);
> +
>      machine_opts = qemu_get_machine_opts();
>      qemu_opt_foreach(machine_opts, machine_set_property, current_machine,
>                       &error_fatal);
> @@ -4398,8 +4405,6 @@ int main(int argc, char **argv, char **envp)
>      ram_mig_init();
>      dirty_bitmap_mig_init();
>  
> -    configure_blockdev(&bdo_queue, machine_class, snapshot);
> -
>      qemu_opts_foreach(qemu_find_opts("mon"),
>                        mon_init_func, NULL, &error_fatal);
>  
> 

[Qemu-devel] [PATCH v2 08/12] pflash_cfi01: Add pflash_cfi01_get_blk() helper

[Markus Armbruster]

From: Philippe Mathieu-Daudé <philmd@redhat.com>

Add an helper to access the opaque struct PFlashCFI01.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
---
 hw/block/pflash_cfi01.c  | 5 +++++
 include/hw/block/flash.h | 1 +
 2 files changed, 6 insertions(+)

diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
index 00980316dc..60de11717b 100644
--- a/hw/block/pflash_cfi01.c
+++ b/hw/block/pflash_cfi01.c
@@ -994,6 +994,11 @@ PFlashCFI01 *pflash_cfi01_register(hwaddr base,
     return PFLASH_CFI01(dev);
 }
 
+BlockBackend *pflash_cfi01_get_blk(PFlashCFI01 *fl)
+{
+    return fl->blk;
+}
+
 MemoryRegion *pflash_cfi01_get_memory(PFlashCFI01 *fl)
 {
     return &fl->mem;
diff --git a/include/hw/block/flash.h b/include/hw/block/flash.h
index 914932eaec..a0f488732a 100644
--- a/include/hw/block/flash.h
+++ b/include/hw/block/flash.h
@@ -22,6 +22,7 @@ PFlashCFI01 *pflash_cfi01_register(hwaddr base,
                                    uint16_t id0, uint16_t id1,
                                    uint16_t id2, uint16_t id3,
                                    int be);
+BlockBackend *pflash_cfi01_get_blk(PFlashCFI01 *fl);
 MemoryRegion *pflash_cfi01_get_memory(PFlashCFI01 *fl);
 
 /* pflash_cfi02.c */
-- 
2.17.2

[Qemu-devel] [PATCH v2 09/12] pc_sysfw: Remove unused PcSysFwDevice

[Markus Armbruster]

From: Philippe Mathieu-Daudé <philmd@redhat.com>

This structure is not used since commit 6dd2a5c98a.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/i386/pc_sysfw.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/hw/i386/pc_sysfw.c b/hw/i386/pc_sysfw.c
index 34727c5b1f..46b87afe23 100644
--- a/hw/i386/pc_sysfw.c
+++ b/hw/i386/pc_sysfw.c
@@ -40,11 +40,6 @@
 
 #define BIOS_FILENAME "bios.bin"
 
-typedef struct PcSysFwDevice {
-    SysBusDevice busdev;
-    uint8_t isapc_ram_fw;
-} PcSysFwDevice;
-
 static void pc_isa_bios_init(MemoryRegion *rom_memory,
                              MemoryRegion *flash_mem,
                              int ram_size)
-- 
2.17.2

[Qemu-devel] [PATCH v2 10/12] pc_sysfw: Pass PCMachineState to pc_system_firmware_init()

[Markus Armbruster]

From: Philippe Mathieu-Daudé <philmd@redhat.com>

pc_system_firmware_init() parameter @isapc_ram_fw is PCMachineState
member pci_enabled negated.  The next commit will need more of
PCMachineState.  To prepare for that, pass a PCMachineState *, and
drop the now redundant parameter @isapc_ram_fw.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/i386/pc.c         | 2 +-
 hw/i386/pc_sysfw.c   | 5 ++++-
 include/hw/i386/pc.h | 3 +--
 3 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 3889eccdc3..3cd8ed1794 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1830,7 +1830,7 @@ void pc_memory_init(PCMachineState *pcms,
     }
 
     /* Initialize PC system firmware */
-    pc_system_firmware_init(rom_memory, !pcmc->pci_enabled);
+    pc_system_firmware_init(pcms, rom_memory);
 
     option_rom_mr = g_malloc(sizeof(*option_rom_mr));
     memory_region_init_ram(option_rom_mr, NULL, "pc.rom", PC_ROM_SIZE,
diff --git a/hw/i386/pc_sysfw.c b/hw/i386/pc_sysfw.c
index 46b87afe23..785123252c 100644
--- a/hw/i386/pc_sysfw.c
+++ b/hw/i386/pc_sysfw.c
@@ -231,8 +231,11 @@ static void old_pc_system_rom_init(MemoryRegion *rom_memory, bool isapc_ram_fw)
                                 bios);
 }
 
-void pc_system_firmware_init(MemoryRegion *rom_memory, bool isapc_ram_fw)
+void pc_system_firmware_init(PCMachineState *pcms,
+                             MemoryRegion *rom_memory)
 {
+    PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms);
+    bool isapc_ram_fw = !pcmc->pci_enabled;
     DriveInfo *pflash_drv;
 
     pflash_drv = drive_get(IF_PFLASH, 0, 0);
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index 3ff127ebd0..eb7360feac 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -278,8 +278,7 @@ extern PCIDevice *piix4_dev;
 int piix4_init(PCIBus *bus, ISABus **isa_bus, int devfn);
 
 /* pc_sysfw.c */
-void pc_system_firmware_init(MemoryRegion *rom_memory,
-                             bool isapc_ram_fw);
+void pc_system_firmware_init(PCMachineState *pcms, MemoryRegion *rom_memory);
 
 /* acpi-build.c */
 void pc_madt_cpu_entry(AcpiDeviceIf *adev, int uid,
-- 
2.17.2

Re: [Qemu-devel] [PATCH v2 10/12] pc_sysfw: Pass PCMachineState to pc_system_firmware_init()

[Laszlo Ersek]

On 03/07/19 18:23, Markus Armbruster wrote:
> From: Philippe Mathieu-Daudé <philmd@redhat.com>
> 
> pc_system_firmware_init() parameter @isapc_ram_fw is PCMachineState
> member pci_enabled negated.  The next commit will need more of
> PCMachineState.  To prepare for that, pass a PCMachineState *, and
> drop the now redundant parameter @isapc_ram_fw.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  hw/i386/pc.c         | 2 +-
>  hw/i386/pc_sysfw.c   | 5 ++++-
>  include/hw/i386/pc.h | 3 +--
>  3 files changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 3889eccdc3..3cd8ed1794 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -1830,7 +1830,7 @@ void pc_memory_init(PCMachineState *pcms,
>      }
>  
>      /* Initialize PC system firmware */
> -    pc_system_firmware_init(rom_memory, !pcmc->pci_enabled);
> +    pc_system_firmware_init(pcms, rom_memory);
>  
>      option_rom_mr = g_malloc(sizeof(*option_rom_mr));
>      memory_region_init_ram(option_rom_mr, NULL, "pc.rom", PC_ROM_SIZE,
> diff --git a/hw/i386/pc_sysfw.c b/hw/i386/pc_sysfw.c
> index 46b87afe23..785123252c 100644
> --- a/hw/i386/pc_sysfw.c
> +++ b/hw/i386/pc_sysfw.c
> @@ -231,8 +231,11 @@ static void old_pc_system_rom_init(MemoryRegion *rom_memory, bool isapc_ram_fw)
>                                  bios);
>  }
>  
> -void pc_system_firmware_init(MemoryRegion *rom_memory, bool isapc_ram_fw)
> +void pc_system_firmware_init(PCMachineState *pcms,
> +                             MemoryRegion *rom_memory)
>  {
> +    PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms);
> +    bool isapc_ram_fw = !pcmc->pci_enabled;
>      DriveInfo *pflash_drv;
>  
>      pflash_drv = drive_get(IF_PFLASH, 0, 0);
> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> index 3ff127ebd0..eb7360feac 100644
> --- a/include/hw/i386/pc.h
> +++ b/include/hw/i386/pc.h
> @@ -278,8 +278,7 @@ extern PCIDevice *piix4_dev;
>  int piix4_init(PCIBus *bus, ISABus **isa_bus, int devfn);
>  
>  /* pc_sysfw.c */
> -void pc_system_firmware_init(MemoryRegion *rom_memory,
> -                             bool isapc_ram_fw);
> +void pc_system_firmware_init(PCMachineState *pcms, MemoryRegion *rom_memory);
>  
>  /* acpi-build.c */
>  void pc_madt_cpu_entry(AcpiDeviceIf *adev, int uid,
> 

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

Thanks!
Laszlo

[Qemu-devel] [PATCH v2 11/12] pc: Support firmware configuration with -blockdev

[Markus Armbruster]

The PC machines put firmware in ROM by default.  To get it put into
flash memory (required by OVMF), you have to 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.  Below the hood, it creates
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.

Mapping -drive if=none,... to -blockdev is a solved problem.  With
if=T other than if=none, -drive additionally configures a block device
frontend.  For non-onboard devices, that part maps to -device.  Also a
solved problem.  For onboard devices such as PC flash memory, we have
an unsolved problem.

This is actually an instance of a wider problem: our general device
configuration interface doesn't cover onboard devices.  Instead, we
have a zoo of ad hoc interfaces that are much more limited.  Some of
them we'd rather deprecate (-drive, -net), but can't until we have
suitable replacements.

Sadly, I can't attack the wider problem today.  So back to the narrow
problem.

My first idea was to reduce it to its solved buddy by using pluggable
instead of onboard devices for the flash memory.  Workable, but it
requires some extra smarts in firmware descriptors and libvirt.  Paolo
had an idea that is simpler for libvirt: keep the devices onboard, and
add machine properties for their block backends.

The implementation is less than straightforward, I'm afraid.

First, block backend properties are *qdev* properties.  Machines can't
have those, as they're not devices.  I could duplicate these qdev
properties as QOM properties, but I hate that.

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.

Properties need to be created in .instance_init() methods.  For PC
machines, that's pc_machine_initfn().  To make alias properties work,
we need to create the onboard flash devices there, too.  Requires
several bug fixes, in the previous commits.  We also have to realize
the devices.  More on that below.

If the user sets pflash0, firmware resides in flash memory.
pc_system_firmware_init() maps and realizes the flash devices.

Else, firmware resides in ROM.  The onboard flash devices aren't used
then.  pc_system_firmware_init() destroys them unrealized, along with
the alias properties.

The existing code to pick up drives defined with -drive if=pflash is
replaced by code to desugar into the machine properties.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/i386/pc.c         |   2 +
 hw/i386/pc_sysfw.c   | 243 ++++++++++++++++++++++++++++---------------
 include/hw/i386/pc.h |   3 +
 3 files changed, 166 insertions(+), 82 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 3cd8ed1794..420a0c5c9e 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -2671,6 +2671,8 @@ static void pc_machine_initfn(Object *obj)
     pcms->smbus_enabled = true;
     pcms->sata_enabled = true;
     pcms->pit_enabled = true;
+
+    pc_system_flash_create(pcms);
 }
 
 static void pc_machine_reset(void)
diff --git a/hw/i386/pc_sysfw.c b/hw/i386/pc_sysfw.c
index 785123252c..ccf2221acb 100644
--- a/hw/i386/pc_sysfw.c
+++ b/hw/i386/pc_sysfw.c
@@ -40,6 +40,17 @@
 
 #define BIOS_FILENAME "bios.bin"
 
+/*
+ * We don't have a theoretically justifiable exact lower bound on the base
+ * address of any flash mapping. In practice, the IO-APIC MMIO range is
+ * [0xFEE00000..0xFEE01000] -- see IO_APIC_DEFAULT_ADDRESS --, leaving free
+ * only 18MB-4KB below 4G. For now, restrict the cumulative mapping to 8MB in
+ * size.
+ */
+#define FLASH_SIZE_LIMIT (8 * MiB)
+
+#define FLASH_SECTOR_SIZE 4096
+
 static void pc_isa_bios_init(MemoryRegion *rom_memory,
                              MemoryRegion *flash_mem,
                              int ram_size)
@@ -71,96 +82,128 @@ static void pc_isa_bios_init(MemoryRegion *rom_memory,
     memory_region_set_readonly(isa_bios, true);
 }
 
-#define FLASH_MAP_UNIT_MAX 2
+static PFlashCFI01 *pc_pflash_create(const char *name)
+{
+    DeviceState *dev = qdev_create(NULL, TYPE_PFLASH_CFI01);
 
-/* We don't have a theoretically justifiable exact lower bound on the base
- * address of any flash mapping. In practice, the IO-APIC MMIO range is
- * [0xFEE00000..0xFEE01000[ -- see IO_APIC_DEFAULT_ADDRESS --, leaving free
- * only 18MB-4KB below 4G. For now, restrict the cumulative mapping to 8MB in
- * size.
- */
-#define FLASH_MAP_BASE_MIN ((hwaddr)(4 * GiB - 8 * MiB))
+    qdev_prop_set_uint64(dev, "sector-length", FLASH_SECTOR_SIZE);
+    qdev_prop_set_uint8(dev, "width", 1);
+    qdev_prop_set_string(dev, "name", name);
+    return PFLASH_CFI01(dev);
+}
 
-/* This function maps flash drives from 4G downward, in order of their unit
- * numbers. The mapping starts at unit#0, with unit number increments of 1, and
- * stops before the first missing flash drive, or before
- * unit#FLASH_MAP_UNIT_MAX, whichever is reached first.
- *
- * Addressing within one flash drive is of course not reversed.
- *
- * An error message is printed and the process exits if:
- * - the size of the backing file for a flash drive is non-positive, or not a
- *   multiple of the required sector size, or
- * - the current mapping's base address would fall below FLASH_MAP_BASE_MIN.
+void pc_system_flash_create(PCMachineState *pcms)
+{
+    PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms);
+
+    if (pcmc->pci_enabled) {
+        pcms->flash[0] = pc_pflash_create("system.flash0");
+        pcms->flash[1] = pc_pflash_create("system.flash1");
+        object_property_add_alias(OBJECT(pcms), "pflash0",
+                                  OBJECT(pcms->flash[0]), "drive",
+                                  &error_abort);
+        object_property_add_alias(OBJECT(pcms), "pflash1",
+                                  OBJECT(pcms->flash[1]), "drive",
+                                  &error_abort);
+    }
+}
+
+static void pc_system_flash_cleanup_unused(PCMachineState *pcms)
+{
+    char *prop_name;
+    int i;
+    Object *dev_obj;
+
+    assert(PC_MACHINE_GET_CLASS(pcms)->pci_enabled);
+
+    for (i = 0; i < ARRAY_SIZE(pcms->flash); i++) {
+        dev_obj = OBJECT(pcms->flash[i]);
+        if (!object_property_get_bool(dev_obj, "realized", &error_abort)) {
+            prop_name = g_strdup_printf("pflash%d", i);
+            object_property_del(OBJECT(pcms), prop_name, &error_abort);
+            g_free(prop_name);
+            /*
+             * Delete @dev_obj.  Straight object_unref() is wrong,
+             * since it leaves dangling references in the parent bus
+             * behind.  object_unparent() doesn't work, either: since
+             * @dev_obj hasn't been realized, dev_obj->parent is null,
+             * and object_unparent() does nothing.  DeviceClass method
+             * device_unparent() works, but we have to take a
+             * temporary reference, or else device_unparent() commits
+             * a use-after-free error.
+             */
+            object_ref(dev_obj);
+            object_get_class(dev_obj)->unparent(dev_obj);
+            object_unref(dev_obj);
+            pcms->flash[i] = NULL;
+        }
+    }
+}
+
+/*
+ * Map the pcms->flash[] from 4GiB downward, and realize.
+ * Map them in descending order, i.e. pcms->flash[0] at the top,
+ * without gaps.
+ * Stop at the first pcms->flash[0] lacking a block backend.
+ * Set each flash's size from its block backend.  Fatal error if the
+ * size isn't a non-zero multiple of 4KiB, or the total size exceeds
+ * FLASH_SIZE_LIMIT.
  *
- * The drive with unit#0 (if available) is mapped at the highest address, and
- * it is passed to pc_isa_bios_init(). Merging several drives for isa-bios is
+ * If pcms->flash[0] has a block backend, its memory is passed to
+ * pc_isa_bios_init().  Merging several flash devices for isa-bios is
  * not supported.
  */
-static void pc_system_flash_init(MemoryRegion *rom_memory)
+static void pc_system_flash_map(PCMachineState *pcms,
+                                MemoryRegion *rom_memory)
 {
-    int unit;
-    DriveInfo *pflash_drv;
+    hwaddr total_size = 0;
+    int i;
     BlockBackend *blk;
     int64_t size;
-    char *fatal_errmsg = NULL;
-    hwaddr phys_addr = 0x100000000ULL;
-    uint32_t sector_size = 4096;
     PFlashCFI01 *system_flash;
     MemoryRegion *flash_mem;
-    char name[64];
     void *flash_ptr;
     int ret, flash_size;
 
-    for (unit = 0;
-         (unit < FLASH_MAP_UNIT_MAX &&
-          (pflash_drv = drive_get(IF_PFLASH, 0, unit)) != NULL);
-         ++unit) {
-        blk = blk_by_legacy_dinfo(pflash_drv);
+    assert(PC_MACHINE_GET_CLASS(pcms)->pci_enabled);
+
+    for (i = 0; i < ARRAY_SIZE(pcms->flash); i++) {
+        system_flash = pcms->flash[i];
+        blk = pflash_cfi01_get_blk(system_flash);
+        if (!blk) {
+            break;
+        }
         size = blk_getlength(blk);
         if (size < 0) {
-            fatal_errmsg = g_strdup_printf("failed to get backing file size");
-        } else if (size == 0) {
-            fatal_errmsg = g_strdup_printf("PC system firmware (pflash) "
-                               "cannot have zero size");
-        } else if ((size % sector_size) != 0) {
-            fatal_errmsg = g_strdup_printf("PC system firmware (pflash) "
-                               "must be a multiple of 0x%x", sector_size);
-        } else if (phys_addr < size || phys_addr - size < FLASH_MAP_BASE_MIN) {
-            fatal_errmsg = g_strdup_printf("oversized backing file, pflash "
-                               "segments cannot be mapped under "
-                               TARGET_FMT_plx, FLASH_MAP_BASE_MIN);
+            error_report("can't get size of block device %s: %s",
+                         blk_name(blk), strerror(-size));
+            exit(1);
         }
-        if (fatal_errmsg != NULL) {
-            Location loc;
-
-            /* push a new, "none" location on the location stack; overwrite its
-             * contents with the location saved in the option; print the error
-             * (includes location); pop the top
-             */
-            loc_push_none(&loc);
-            if (pflash_drv->opts != NULL) {
-                qemu_opts_loc_restore(pflash_drv->opts);
-            }
-            error_report("%s", fatal_errmsg);
-            loc_pop(&loc);
-            g_free(fatal_errmsg);
+        if (size == 0 || size % FLASH_SECTOR_SIZE != 0) {
+            error_report("system firmware block device %s has invalid size "
+                         "%" PRId64,
+                         blk_name(blk), size);
+            info_report("its size must be a non-zero multiple of 0x%x",
+                        FLASH_SECTOR_SIZE);
+            exit(1);
+        }
+        if ((hwaddr)size != size
+            || total_size > HWADDR_MAX - size
+            || total_size + size > FLASH_SIZE_LIMIT) {
+            error_report("combined size of system firmware exceeds "
+                         "%" PRIu64 " bytes",
+                         FLASH_SIZE_LIMIT);
             exit(1);
         }
 
-        phys_addr -= size;
+        total_size += size;
+        qdev_prop_set_uint32(DEVICE(system_flash), "num-blocks",
+                             size / FLASH_SECTOR_SIZE);
+        qdev_init_nofail(DEVICE(system_flash));
+        sysbus_mmio_map(SYS_BUS_DEVICE(system_flash), 0,
+                        0x100000000ULL - total_size);
 
-        /* pflash_cfi01_register() creates a deep copy of the name */
-        snprintf(name, sizeof name, "system.flash%d", unit);
-        system_flash = pflash_cfi01_register(phys_addr, name,
-                                             size, blk, sector_size,
-                                             1      /* width */,
-                                             0x0000 /* id0 */,
-                                             0x0000 /* id1 */,
-                                             0x0000 /* id2 */,
-                                             0x0000 /* id3 */,
-                                             0      /* be */);
-        if (unit == 0) {
+        if (i == 0) {
             flash_mem = pflash_cfi01_get_memory(system_flash);
             pc_isa_bios_init(rom_memory, flash_mem, size);
 
@@ -235,23 +278,59 @@ void pc_system_firmware_init(PCMachineState *pcms,
                              MemoryRegion *rom_memory)
 {
     PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms);
-    bool isapc_ram_fw = !pcmc->pci_enabled;
+    int i;
     DriveInfo *pflash_drv;
+    BlockBackend *pflash_blk[ARRAY_SIZE(pcms->flash)];
+    Location loc;
 
-    pflash_drv = drive_get(IF_PFLASH, 0, 0);
-
-    if (isapc_ram_fw || pflash_drv == NULL) {
-        /* When a pflash drive is not found, use rom-mode */
-        old_pc_system_rom_init(rom_memory, isapc_ram_fw);
+    if (!pcmc->pci_enabled) {
+        old_pc_system_rom_init(rom_memory, true);
         return;
     }
 
-    if (kvm_enabled() && !kvm_readonly_mem_enabled()) {
-        /* Older KVM cannot execute from device memory. So, flash memory
-         * cannot be used unless the readonly memory kvm capability is present. */
-        fprintf(stderr, "qemu: pflash with kvm requires KVM readonly memory support\n");
-        exit(1);
+    /* 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);
     }
 
-    pc_system_flash_init(rom_memory);
+    /* Reject gaps */
+    for (i = 1; i < ARRAY_SIZE(pcms->flash); i++) {
+        if (pflash_blk[i] && !pflash_blk[i - 1]) {
+            error_report("pflash%d requires pflash%d", i, i - 1);
+            exit(1);
+        }
+    }
+
+    if (!pflash_blk[0]) {
+        /* Machine property pflash0 not set, use ROM mode */
+        old_pc_system_rom_init(rom_memory, false);
+    } else {
+        if (kvm_enabled() && !kvm_readonly_mem_enabled()) {
+            /*
+             * Older KVM cannot execute from device memory. So, flash
+             * memory cannot be used unless the readonly memory kvm
+             * capability is present.
+             */
+            error_report("pflash with kvm requires KVM readonly memory support");
+            exit(1);
+        }
+
+        pc_system_flash_map(pcms, rom_memory);
+    }
+
+    pc_system_flash_cleanup_unused(pcms);
 }
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index eb7360feac..266639ca8c 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -6,6 +6,7 @@
 #include "hw/boards.h"
 #include "hw/isa/isa.h"
 #include "hw/block/fdc.h"
+#include "hw/block/flash.h"
 #include "net/net.h"
 #include "hw/i386/ioapic.h"
 
@@ -39,6 +40,7 @@ struct PCMachineState {
     PCIBus *bus;
     FWCfgState *fw_cfg;
     qemu_irq *gsi;
+    PFlashCFI01 *flash[2];
 
     /* Configuration options: */
     uint64_t max_ram_below_4g;
@@ -278,6 +280,7 @@ extern PCIDevice *piix4_dev;
 int piix4_init(PCIBus *bus, ISABus **isa_bus, int devfn);
 
 /* pc_sysfw.c */
+void pc_system_flash_create(PCMachineState *pcms);
 void pc_system_firmware_init(PCMachineState *pcms, MemoryRegion *rom_memory);
 
 /* acpi-build.c */
-- 
2.17.2

Re: [Qemu-devel] [PATCH v2 11/12] pc: Support firmware configuration with -blockdev

[Philippe Mathieu-Daudé]

On 3/7/19 6:24 PM, Markus Armbruster wrote:
> The PC machines put firmware in ROM by default.  To get it put into
> flash memory (required by OVMF), you have to 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.  Below the hood, it creates
> 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.
> 
> Mapping -drive if=none,... to -blockdev is a solved problem.  With
> if=T other than if=none, -drive additionally configures a block device
> frontend.  For non-onboard devices, that part maps to -device.  Also a
> solved problem.  For onboard devices such as PC flash memory, we have
> an unsolved problem.
> 
> This is actually an instance of a wider problem: our general device
> configuration interface doesn't cover onboard devices.  Instead, we
> have a zoo of ad hoc interfaces that are much more limited.  Some of
> them we'd rather deprecate (-drive, -net), but can't until we have
> suitable replacements.
> 
> Sadly, I can't attack the wider problem today.  So back to the narrow
> problem.
> 
> My first idea was to reduce it to its solved buddy by using pluggable
> instead of onboard devices for the flash memory.  Workable, but it
> requires some extra smarts in firmware descriptors and libvirt.  Paolo
> had an idea that is simpler for libvirt: keep the devices onboard, and
> add machine properties for their block backends.
> 
> The implementation is less than straightforward, I'm afraid.
> 
> First, block backend properties are *qdev* properties.  Machines can't
> have those, as they're not devices.  I could duplicate these qdev
> properties as QOM properties, but I hate that.
> 
> 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.
> 
> Properties need to be created in .instance_init() methods.  For PC
> machines, that's pc_machine_initfn().  To make alias properties work,
> we need to create the onboard flash devices there, too.  Requires
> several bug fixes, in the previous commits.  We also have to realize
> the devices.  More on that below.
> 
> If the user sets pflash0, firmware resides in flash memory.
> pc_system_firmware_init() maps and realizes the flash devices.
> 
> Else, firmware resides in ROM.  The onboard flash devices aren't used
> then.  pc_system_firmware_init() destroys them unrealized, along with
> the alias properties.
> 
> The existing code to pick up drives defined with -drive if=pflash is
> replaced by code to desugar into the machine properties.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Oh I did not notice git publish added my S-o-b here, it was not
intentional. You can drop it, as you wish.

> ---
>  hw/i386/pc.c         |   2 +
>  hw/i386/pc_sysfw.c   | 243 ++++++++++++++++++++++++++++---------------
>  include/hw/i386/pc.h |   3 +
>  3 files changed, 166 insertions(+), 82 deletions(-)
> 
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 3cd8ed1794..420a0c5c9e 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -2671,6 +2671,8 @@ static void pc_machine_initfn(Object *obj)
>      pcms->smbus_enabled = true;
>      pcms->sata_enabled = true;
>      pcms->pit_enabled = true;
> +
> +    pc_system_flash_create(pcms);
>  }
>  
>  static void pc_machine_reset(void)
> diff --git a/hw/i386/pc_sysfw.c b/hw/i386/pc_sysfw.c
> index 785123252c..ccf2221acb 100644
> --- a/hw/i386/pc_sysfw.c
> +++ b/hw/i386/pc_sysfw.c
> @@ -40,6 +40,17 @@
>  
>  #define BIOS_FILENAME "bios.bin"
>  
> +/*
> + * We don't have a theoretically justifiable exact lower bound on the base
> + * address of any flash mapping. In practice, the IO-APIC MMIO range is
> + * [0xFEE00000..0xFEE01000] -- see IO_APIC_DEFAULT_ADDRESS --, leaving free
> + * only 18MB-4KB below 4G. For now, restrict the cumulative mapping to 8MB in
> + * size.
> + */
> +#define FLASH_SIZE_LIMIT (8 * MiB)
> +
> +#define FLASH_SECTOR_SIZE 4096
> +
>  static void pc_isa_bios_init(MemoryRegion *rom_memory,
>                               MemoryRegion *flash_mem,
>                               int ram_size)
> @@ -71,96 +82,128 @@ static void pc_isa_bios_init(MemoryRegion *rom_memory,
>      memory_region_set_readonly(isa_bios, true);
>  }
>  
> -#define FLASH_MAP_UNIT_MAX 2
> +static PFlashCFI01 *pc_pflash_create(const char *name)
> +{
> +    DeviceState *dev = qdev_create(NULL, TYPE_PFLASH_CFI01);
>  
> -/* We don't have a theoretically justifiable exact lower bound on the base
> - * address of any flash mapping. In practice, the IO-APIC MMIO range is
> - * [0xFEE00000..0xFEE01000[ -- see IO_APIC_DEFAULT_ADDRESS --, leaving free
> - * only 18MB-4KB below 4G. For now, restrict the cumulative mapping to 8MB in
> - * size.
> - */
> -#define FLASH_MAP_BASE_MIN ((hwaddr)(4 * GiB - 8 * MiB))
> +    qdev_prop_set_uint64(dev, "sector-length", FLASH_SECTOR_SIZE);
> +    qdev_prop_set_uint8(dev, "width", 1);
> +    qdev_prop_set_string(dev, "name", name);
> +    return PFLASH_CFI01(dev);
> +}
>  
> -/* This function maps flash drives from 4G downward, in order of their unit
> - * numbers. The mapping starts at unit#0, with unit number increments of 1, and
> - * stops before the first missing flash drive, or before
> - * unit#FLASH_MAP_UNIT_MAX, whichever is reached first.
> - *
> - * Addressing within one flash drive is of course not reversed.
> - *
> - * An error message is printed and the process exits if:
> - * - the size of the backing file for a flash drive is non-positive, or not a
> - *   multiple of the required sector size, or
> - * - the current mapping's base address would fall below FLASH_MAP_BASE_MIN.
> +void pc_system_flash_create(PCMachineState *pcms)
> +{
> +    PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms);
> +
> +    if (pcmc->pci_enabled) {
> +        pcms->flash[0] = pc_pflash_create("system.flash0");
> +        pcms->flash[1] = pc_pflash_create("system.flash1");
> +        object_property_add_alias(OBJECT(pcms), "pflash0",
> +                                  OBJECT(pcms->flash[0]), "drive",
> +                                  &error_abort);
> +        object_property_add_alias(OBJECT(pcms), "pflash1",
> +                                  OBJECT(pcms->flash[1]), "drive",
> +                                  &error_abort);
> +    }
> +}
> +
> +static void pc_system_flash_cleanup_unused(PCMachineState *pcms)
> +{
> +    char *prop_name;
> +    int i;
> +    Object *dev_obj;
> +
> +    assert(PC_MACHINE_GET_CLASS(pcms)->pci_enabled);
> +
> +    for (i = 0; i < ARRAY_SIZE(pcms->flash); i++) {
> +        dev_obj = OBJECT(pcms->flash[i]);
> +        if (!object_property_get_bool(dev_obj, "realized", &error_abort)) {
> +            prop_name = g_strdup_printf("pflash%d", i);
> +            object_property_del(OBJECT(pcms), prop_name, &error_abort);
> +            g_free(prop_name);
> +            /*
> +             * Delete @dev_obj.  Straight object_unref() is wrong,
> +             * since it leaves dangling references in the parent bus
> +             * behind.  object_unparent() doesn't work, either: since
> +             * @dev_obj hasn't been realized, dev_obj->parent is null,
> +             * and object_unparent() does nothing.  DeviceClass method
> +             * device_unparent() works, but we have to take a
> +             * temporary reference, or else device_unparent() commits
> +             * a use-after-free error.
> +             */
> +            object_ref(dev_obj);
> +            object_get_class(dev_obj)->unparent(dev_obj);
> +            object_unref(dev_obj);
> +            pcms->flash[i] = NULL;
> +        }
> +    }
> +}
> +
> +/*
> + * Map the pcms->flash[] from 4GiB downward, and realize.
> + * Map them in descending order, i.e. pcms->flash[0] at the top,
> + * without gaps.
> + * Stop at the first pcms->flash[0] lacking a block backend.
> + * Set each flash's size from its block backend.  Fatal error if the
> + * size isn't a non-zero multiple of 4KiB, or the total size exceeds
> + * FLASH_SIZE_LIMIT.
>   *
> - * The drive with unit#0 (if available) is mapped at the highest address, and
> - * it is passed to pc_isa_bios_init(). Merging several drives for isa-bios is
> + * If pcms->flash[0] has a block backend, its memory is passed to
> + * pc_isa_bios_init().  Merging several flash devices for isa-bios is
>   * not supported.
>   */
> -static void pc_system_flash_init(MemoryRegion *rom_memory)
> +static void pc_system_flash_map(PCMachineState *pcms,
> +                                MemoryRegion *rom_memory)
>  {
> -    int unit;
> -    DriveInfo *pflash_drv;
> +    hwaddr total_size = 0;
> +    int i;
>      BlockBackend *blk;
>      int64_t size;
> -    char *fatal_errmsg = NULL;
> -    hwaddr phys_addr = 0x100000000ULL;
> -    uint32_t sector_size = 4096;
>      PFlashCFI01 *system_flash;
>      MemoryRegion *flash_mem;
> -    char name[64];
>      void *flash_ptr;
>      int ret, flash_size;
>  
> -    for (unit = 0;
> -         (unit < FLASH_MAP_UNIT_MAX &&
> -          (pflash_drv = drive_get(IF_PFLASH, 0, unit)) != NULL);
> -         ++unit) {
> -        blk = blk_by_legacy_dinfo(pflash_drv);
> +    assert(PC_MACHINE_GET_CLASS(pcms)->pci_enabled);
> +
> +    for (i = 0; i < ARRAY_SIZE(pcms->flash); i++) {
> +        system_flash = pcms->flash[i];
> +        blk = pflash_cfi01_get_blk(system_flash);
> +        if (!blk) {
> +            break;
> +        }
>          size = blk_getlength(blk);
>          if (size < 0) {
> -            fatal_errmsg = g_strdup_printf("failed to get backing file size");
> -        } else if (size == 0) {
> -            fatal_errmsg = g_strdup_printf("PC system firmware (pflash) "
> -                               "cannot have zero size");
> -        } else if ((size % sector_size) != 0) {
> -            fatal_errmsg = g_strdup_printf("PC system firmware (pflash) "
> -                               "must be a multiple of 0x%x", sector_size);
> -        } else if (phys_addr < size || phys_addr - size < FLASH_MAP_BASE_MIN) {
> -            fatal_errmsg = g_strdup_printf("oversized backing file, pflash "
> -                               "segments cannot be mapped under "
> -                               TARGET_FMT_plx, FLASH_MAP_BASE_MIN);
> +            error_report("can't get size of block device %s: %s",
> +                         blk_name(blk), strerror(-size));
> +            exit(1);
>          }
> -        if (fatal_errmsg != NULL) {
> -            Location loc;
> -
> -            /* push a new, "none" location on the location stack; overwrite its
> -             * contents with the location saved in the option; print the error
> -             * (includes location); pop the top
> -             */
> -            loc_push_none(&loc);
> -            if (pflash_drv->opts != NULL) {
> -                qemu_opts_loc_restore(pflash_drv->opts);
> -            }
> -            error_report("%s", fatal_errmsg);
> -            loc_pop(&loc);
> -            g_free(fatal_errmsg);
> +        if (size == 0 || size % FLASH_SECTOR_SIZE != 0) {
> +            error_report("system firmware block device %s has invalid size "
> +                         "%" PRId64,
> +                         blk_name(blk), size);
> +            info_report("its size must be a non-zero multiple of 0x%x",
> +                        FLASH_SECTOR_SIZE);
> +            exit(1);
> +        }
> +        if ((hwaddr)size != size
> +            || total_size > HWADDR_MAX - size
> +            || total_size + size > FLASH_SIZE_LIMIT) {
> +            error_report("combined size of system firmware exceeds "
> +                         "%" PRIu64 " bytes",
> +                         FLASH_SIZE_LIMIT);
>              exit(1);
>          }
>  
> -        phys_addr -= size;
> +        total_size += size;
> +        qdev_prop_set_uint32(DEVICE(system_flash), "num-blocks",
> +                             size / FLASH_SECTOR_SIZE);
> +        qdev_init_nofail(DEVICE(system_flash));
> +        sysbus_mmio_map(SYS_BUS_DEVICE(system_flash), 0,
> +                        0x100000000ULL - total_size);
>  
> -        /* pflash_cfi01_register() creates a deep copy of the name */
> -        snprintf(name, sizeof name, "system.flash%d", unit);
> -        system_flash = pflash_cfi01_register(phys_addr, name,
> -                                             size, blk, sector_size,
> -                                             1      /* width */,
> -                                             0x0000 /* id0 */,
> -                                             0x0000 /* id1 */,
> -                                             0x0000 /* id2 */,
> -                                             0x0000 /* id3 */,
> -                                             0      /* be */);
> -        if (unit == 0) {
> +        if (i == 0) {
>              flash_mem = pflash_cfi01_get_memory(system_flash);
>              pc_isa_bios_init(rom_memory, flash_mem, size);
>  
> @@ -235,23 +278,59 @@ void pc_system_firmware_init(PCMachineState *pcms,
>                               MemoryRegion *rom_memory)
>  {
>      PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms);
> -    bool isapc_ram_fw = !pcmc->pci_enabled;
> +    int i;
>      DriveInfo *pflash_drv;
> +    BlockBackend *pflash_blk[ARRAY_SIZE(pcms->flash)];
> +    Location loc;
>  
> -    pflash_drv = drive_get(IF_PFLASH, 0, 0);
> -
> -    if (isapc_ram_fw || pflash_drv == NULL) {
> -        /* When a pflash drive is not found, use rom-mode */
> -        old_pc_system_rom_init(rom_memory, isapc_ram_fw);
> +    if (!pcmc->pci_enabled) {
> +        old_pc_system_rom_init(rom_memory, true);
>          return;
>      }
>  
> -    if (kvm_enabled() && !kvm_readonly_mem_enabled()) {
> -        /* Older KVM cannot execute from device memory. So, flash memory
> -         * cannot be used unless the readonly memory kvm capability is present. */
> -        fprintf(stderr, "qemu: pflash with kvm requires KVM readonly memory support\n");
> -        exit(1);
> +    /* 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);
>      }
>  
> -    pc_system_flash_init(rom_memory);
> +    /* Reject gaps */
> +    for (i = 1; i < ARRAY_SIZE(pcms->flash); i++) {
> +        if (pflash_blk[i] && !pflash_blk[i - 1]) {
> +            error_report("pflash%d requires pflash%d", i, i - 1);
> +            exit(1);
> +        }
> +    }
> +
> +    if (!pflash_blk[0]) {
> +        /* Machine property pflash0 not set, use ROM mode */
> +        old_pc_system_rom_init(rom_memory, false);
> +    } else {
> +        if (kvm_enabled() && !kvm_readonly_mem_enabled()) {
> +            /*
> +             * Older KVM cannot execute from device memory. So, flash
> +             * memory cannot be used unless the readonly memory kvm
> +             * capability is present.
> +             */
> +            error_report("pflash with kvm requires KVM readonly memory support");
> +            exit(1);
> +        }
> +
> +        pc_system_flash_map(pcms, rom_memory);
> +    }
> +
> +    pc_system_flash_cleanup_unused(pcms);
>  }
> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> index eb7360feac..266639ca8c 100644
> --- a/include/hw/i386/pc.h
> +++ b/include/hw/i386/pc.h
> @@ -6,6 +6,7 @@
>  #include "hw/boards.h"
>  #include "hw/isa/isa.h"
>  #include "hw/block/fdc.h"
> +#include "hw/block/flash.h"
>  #include "net/net.h"
>  #include "hw/i386/ioapic.h"
>  
> @@ -39,6 +40,7 @@ struct PCMachineState {
>      PCIBus *bus;
>      FWCfgState *fw_cfg;
>      qemu_irq *gsi;
> +    PFlashCFI01 *flash[2];
>  
>      /* Configuration options: */
>      uint64_t max_ram_below_4g;
> @@ -278,6 +280,7 @@ extern PCIDevice *piix4_dev;
>  int piix4_init(PCIBus *bus, ISABus **isa_bus, int devfn);
>  
>  /* pc_sysfw.c */
> +void pc_system_flash_create(PCMachineState *pcms);
>  void pc_system_firmware_init(PCMachineState *pcms, MemoryRegion *rom_memory);
>  
>  /* acpi-build.c */
> 

Re: [Qemu-devel] [PATCH v2 11/12] pc: Support firmware configuration with -blockdev

[Laszlo Ersek]

On 03/07/19 18:24, Markus Armbruster wrote:
> The PC machines put firmware in ROM by default.  To get it put into
> flash memory (required by OVMF), you have to 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.  Below the hood, it creates
> 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.
> 
> Mapping -drive if=none,... to -blockdev is a solved problem.  With
> if=T other than if=none, -drive additionally configures a block device
> frontend.  For non-onboard devices, that part maps to -device.  Also a
> solved problem.  For onboard devices such as PC flash memory, we have
> an unsolved problem.
> 
> This is actually an instance of a wider problem: our general device
> configuration interface doesn't cover onboard devices.  Instead, we
> have a zoo of ad hoc interfaces that are much more limited.  Some of
> them we'd rather deprecate (-drive, -net), but can't until we have
> suitable replacements.
> 
> Sadly, I can't attack the wider problem today.  So back to the narrow
> problem.
> 
> My first idea was to reduce it to its solved buddy by using pluggable
> instead of onboard devices for the flash memory.  Workable, but it
> requires some extra smarts in firmware descriptors and libvirt.  Paolo
> had an idea that is simpler for libvirt: keep the devices onboard, and
> add machine properties for their block backends.
> 
> The implementation is less than straightforward, I'm afraid.
> 
> First, block backend properties are *qdev* properties.  Machines can't
> have those, as they're not devices.  I could duplicate these qdev
> properties as QOM properties, but I hate that.
> 
> 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.
> 
> Properties need to be created in .instance_init() methods.  For PC
> machines, that's pc_machine_initfn().  To make alias properties work,
> we need to create the onboard flash devices there, too.  Requires
> several bug fixes, in the previous commits.  We also have to realize
> the devices.  More on that below.
> 
> If the user sets pflash0, firmware resides in flash memory.
> pc_system_firmware_init() maps and realizes the flash devices.
> 
> Else, firmware resides in ROM.  The onboard flash devices aren't used
> then.  pc_system_firmware_init() destroys them unrealized, along with
> the alias properties.
> 
> The existing code to pick up drives defined with -drive if=pflash is
> replaced by code to desugar into the machine properties.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  hw/i386/pc.c         |   2 +
>  hw/i386/pc_sysfw.c   | 243 ++++++++++++++++++++++++++++---------------
>  include/hw/i386/pc.h |   3 +
>  3 files changed, 166 insertions(+), 82 deletions(-)

Thanks for addressing my comments!

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

Laszlo

[Qemu-devel] [PATCH v2 12/12] docs/interop/firmware.json: Prefer -machine to if=pflash

[Markus Armbruster]

The previous commit added a way to configure firmware with -blockdev
rather than -drive if=pflash.  Document it as the preferred way.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 docs/interop/firmware.json | 21 +++++++++++++++------
 1 file changed, 15 insertions(+), 6 deletions(-)

diff --git a/docs/interop/firmware.json b/docs/interop/firmware.json
index 28f9bc1591..db3cb38b6a 100644
--- a/docs/interop/firmware.json
+++ b/docs/interop/firmware.json
@@ -212,9 +212,13 @@
 #
 # @executable: Identifies the firmware executable. The firmware
 #              executable may be shared by multiple virtual machine
-#              definitions. The corresponding QEMU command line option
-#              is "-drive
-#              if=pflash,unit=0,readonly=on,file=@executable.@filename,format=@executable.@format".
+#              definitions. The preferred corresponding QEMU command
+#              line option is
+#                  -drive if=none,id=pflash0,readonly=on,file=@executable.@filename,format=@executable.@format
+#                  -machine pflash0=pflash0
+#              or equivalent -blockdev.
+#              With QEMU versions older than 4.0, you have to use
+#                  -drive if=pflash,unit=0,readonly=on,file=@executable.@filename,format=@executable.@format
 #
 # @nvram-template: Identifies the NVRAM template compatible with
 #                  @executable. Management software instantiates an
@@ -225,9 +229,14 @@
 #                  individual copies of it are. An NVRAM file is
 #                  typically used for persistently storing the
 #                  non-volatile UEFI variables of a virtual machine
-#                  definition. The corresponding QEMU command line
-#                  option is "-drive
-#                  if=pflash,unit=1,readonly=off,file=FILENAME_OF_PRIVATE_NVRAM_FILE,format=@nvram-template.@format".
+#                  definition. The preferred corresponding QEMU
+#                  command line option is
+#                      -drive if=none,id=pflash1,readonly=off,file=FILENAME_OF_PRIVATE_NVRAM_FILE,format=@nvram-template.@format
+#                      -machine pflash1=pflash1
+#                  or equivalent -blockdev.
+#                  With QEMU versions older than 4.0, you have to use
+#                      -drive if=pflash,unit=1,readonly=off,file=FILENAME_OF_PRIVATE_NVRAM_FILE,format=@nvram-template.@format
+#                  
 #
 # Since: 3.0
 ##
-- 
2.17.2

Re: [Qemu-devel] [PATCH v2 12/12] docs/interop/firmware.json: Prefer -machine to if=pflash

[Laszlo Ersek]

On 03/07/19 18:24, Markus Armbruster wrote:
> The previous commit added a way to configure firmware with -blockdev
> rather than -drive if=pflash.  Document it as the preferred way.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  docs/interop/firmware.json | 21 +++++++++++++++------
>  1 file changed, 15 insertions(+), 6 deletions(-)
> 
> diff --git a/docs/interop/firmware.json b/docs/interop/firmware.json
> index 28f9bc1591..db3cb38b6a 100644
> --- a/docs/interop/firmware.json
> +++ b/docs/interop/firmware.json
> @@ -212,9 +212,13 @@
>  #
>  # @executable: Identifies the firmware executable. The firmware
>  #              executable may be shared by multiple virtual machine
> -#              definitions. The corresponding QEMU command line option
> -#              is "-drive
> -#              if=pflash,unit=0,readonly=on,file=@executable.@filename,format=@executable.@format".
> +#              definitions. The preferred corresponding QEMU command
> +#              line option is
> +#                  -drive if=none,id=pflash0,readonly=on,file=@executable.@filename,format=@executable.@format
> +#                  -machine pflash0=pflash0
> +#              or equivalent -blockdev.

If we used plural here ("options"), would that be an improvement?

  The preferred corresponding QEMU command line options are
    -drive ...
    -machine ...
  (or -blockdev equivalent to -drive).

> +#              With QEMU versions older than 4.0, you have to use

To make this easier to understand on first read, should we say

  ... you have to use the single option ...

?

(Maybe there is a better term than "single option" for the "-drive if=<not-NONE>" options, i.e. for those that configure both back-end and front-end.)

If the above over-explained things, I'd be fine with the current patch as well. I just got these ideas and wanted to run them by you.

Thanks!
Laszlo


> +#                  -drive if=pflash,unit=0,readonly=on,file=@executable.@filename,format=@executable.@format
>  #
>  # @nvram-template: Identifies the NVRAM template compatible with
>  #                  @executable. Management software instantiates an
> @@ -225,9 +229,14 @@
>  #                  individual copies of it are. An NVRAM file is
>  #                  typically used for persistently storing the
>  #                  non-volatile UEFI variables of a virtual machine
> -#                  definition. The corresponding QEMU command line
> -#                  option is "-drive
> -#                  if=pflash,unit=1,readonly=off,file=FILENAME_OF_PRIVATE_NVRAM_FILE,format=@nvram-template.@format".
> +#                  definition. The preferred corresponding QEMU
> +#                  command line option is
> +#                      -drive if=none,id=pflash1,readonly=off,file=FILENAME_OF_PRIVATE_NVRAM_FILE,format=@nvram-template.@format
> +#                      -machine pflash1=pflash1
> +#                  or equivalent -blockdev.
> +#                  With QEMU versions older than 4.0, you have to use
> +#                      -drive if=pflash,unit=1,readonly=off,file=FILENAME_OF_PRIVATE_NVRAM_FILE,format=@nvram-template.@format
> +#                  
>  #
>  # Since: 3.0
>  ##
> 

Re: [Qemu-devel] [PATCH v2 12/12] docs/interop/firmware.json: Prefer -machine to if=pflash

[Markus Armbruster]

Laszlo Ersek <lersek@redhat.com> writes:

> On 03/07/19 18:24, Markus Armbruster wrote:
>> The previous commit added a way to configure firmware with -blockdev
>> rather than -drive if=pflash.  Document it as the preferred way.
>> 
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  docs/interop/firmware.json | 21 +++++++++++++++------
>>  1 file changed, 15 insertions(+), 6 deletions(-)
>> 
>> diff --git a/docs/interop/firmware.json b/docs/interop/firmware.json
>> index 28f9bc1591..db3cb38b6a 100644
>> --- a/docs/interop/firmware.json
>> +++ b/docs/interop/firmware.json
>> @@ -212,9 +212,13 @@
>>  #
>>  # @executable: Identifies the firmware executable. The firmware
>>  #              executable may be shared by multiple virtual machine
>> -#              definitions. The corresponding QEMU command line option
>> -#              is "-drive
>> -#              if=pflash,unit=0,readonly=on,file=@executable.@filename,format=@executable.@format".
>> +#              definitions. The preferred corresponding QEMU command
>> +#              line option is
>> +#                  -drive if=none,id=pflash0,readonly=on,file=@executable.@filename,format=@executable.@format
>> +#                  -machine pflash0=pflash0
>> +#              or equivalent -blockdev.
>
> If we used plural here ("options"), would that be an improvement?
>
>   The preferred corresponding QEMU command line options are
>     -drive ...
>     -machine ...
>   (or -blockdev equivalent to -drive).

Definitely an improvement.

>> +#              With QEMU versions older than 4.0, you have to use
>
> To make this easier to understand on first read, should we say
>
>   ... you have to use the single option ...
>
> ?
>
> (Maybe there is a better term than "single option" for the "-drive if=<not-NONE>" options, i.e. for those that configure both back-end and front-end.)

I think we can leave this one to context.

> If the above over-explained things, I'd be fine with the current patch as well. I just got these ideas and wanted to run them by you.

Appreciated!

Re: [Qemu-devel] [PATCH v2 00/12] pc: Support firmware configuration with -blockdev

[no-reply]

Patchew URL: https://patchew.org/QEMU/20190307172401.29451-1-armbru@redhat.com/



Hi,

This series seems to have some coding style problems. See output below for
more information:

Type: series
Message-id: 20190307172401.29451-1-armbru@redhat.com
Subject: [Qemu-devel] [PATCH v2 00/12] pc: Support firmware configuration with -blockdev

=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 * [new tag]               patchew/20190307172401.29451-1-armbru@redhat.com -> patchew/20190307172401.29451-1-armbru@redhat.com
Switched to a new branch 'test'
c1aaf3673d docs/interop/firmware.json: Prefer -machine to if=pflash
19a2070dbe pc: Support firmware configuration with -blockdev
b5c7b84c81 pc_sysfw: Pass PCMachineState to pc_system_firmware_init()
e1c257c048 pc_sysfw: Remove unused PcSysFwDevice
fcb857ab88 pflash_cfi01: Add pflash_cfi01_get_blk() helper
1a2edd22c2 vl: Create block backends before setting machine properties
45c492a1cd vl: Factor configure_blockdev() out of main()
a21a1d9db1 vl: Improve legibility of BlockdevOptions queue
e708802553 sysbus: Fix latent bug with onboard devices
dc3441282a vl: Fix latent bug with -global and onboard devices
2fe2cce7c2 qom: Move compat_props machinery from qdev to QOM
7b82947341 qdev: Fix latent bug with compat_props and onboard devices

=== OUTPUT BEGIN ===
1/12 Checking commit 7b829473415e (qdev: Fix latent bug with compat_props and onboard devices)
2/12 Checking commit 2fe2cce7c264 (qom: Move compat_props machinery from qdev to QOM)
3/12 Checking commit dc3441282af9 (vl: Fix latent bug with -global and onboard devices)
4/12 Checking commit e7088025536e (sysbus: Fix latent bug with onboard devices)
5/12 Checking commit a21a1d9db1b5 (vl: Improve legibility of BlockdevOptions queue)
6/12 Checking commit 45c492a1cde3 (vl: Factor configure_blockdev() out of main())
WARNING: Block comments use a leading /* on a separate line
#25: FILE: vl.c:1207:
+    /* If the currently selected machine wishes to override the units-per-bus

WARNING: Block comments use a trailing */ on a separate line
#26: FILE: vl.c:1208:
+     * property of its default HBA interface type, do so now. */

total: 0 errors, 2 warnings, 83 lines checked

Patch 6/12 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
7/12 Checking commit 1a2edd22c227 (vl: Create block backends before setting machine properties)
8/12 Checking commit fcb857ab88c7 (pflash_cfi01: Add pflash_cfi01_get_blk() helper)
9/12 Checking commit e1c257c04899 (pc_sysfw: Remove unused PcSysFwDevice)
10/12 Checking commit b5c7b84c8184 (pc_sysfw: Pass PCMachineState to pc_system_firmware_init())
11/12 Checking commit 19a2070dbe4a (pc: Support firmware configuration with -blockdev)
WARNING: line over 80 characters
#372: FILE: hw/i386/pc_sysfw.c:328:
+            error_report("pflash with kvm requires KVM readonly memory support");

total: 0 errors, 1 warnings, 315 lines checked

Patch 11/12 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
12/12 Checking commit c1aaf3673dbf (docs/interop/firmware.json: Prefer -machine to if=pflash)
ERROR: trailing whitespace
#49: FILE: docs/interop/firmware.json:239:
+#                  $

total: 1 errors, 0 warnings, 33 lines checked

Patch 12/12 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

=== OUTPUT END ===

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/20190307172401.29451-1-armbru@redhat.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [Qemu-devel] [PATCH v2 00/12] pc: Support firmware configuration with -blockdev

[no-reply]

Patchew URL: https://patchew.org/QEMU/20190307172401.29451-1-armbru@redhat.com/



Hi,

This series seems to have some coding style problems. See output below for
more information:

Type: series
Message-id: 20190307172401.29451-1-armbru@redhat.com
Subject: [Qemu-devel] [PATCH v2 00/12] pc: Support firmware configuration with -blockdev

=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 t [tag update]            patchew/20190307172401.29451-1-armbru@redhat.com -> patchew/20190307172401.29451-1-armbru@redhat.com
Switched to a new branch 'test'
e159d1ed0d docs/interop/firmware.json: Prefer -machine to if=pflash
7b348eb438 pc: Support firmware configuration with -blockdev
2781f626cf pc_sysfw: Pass PCMachineState to pc_system_firmware_init()
5ed81089c8 pc_sysfw: Remove unused PcSysFwDevice
06dfc824b9 pflash_cfi01: Add pflash_cfi01_get_blk() helper
ac5ff6df76 vl: Create block backends before setting machine properties
4fd41657da vl: Factor configure_blockdev() out of main()
8e6aa0c7e8 vl: Improve legibility of BlockdevOptions queue
54f785aef6 sysbus: Fix latent bug with onboard devices
a74b064b72 vl: Fix latent bug with -global and onboard devices
004bf6fd7a qom: Move compat_props machinery from qdev to QOM
5f4c93ede2 qdev: Fix latent bug with compat_props and onboard devices

=== OUTPUT BEGIN ===
1/12 Checking commit 5f4c93ede2e9 (qdev: Fix latent bug with compat_props and onboard devices)
2/12 Checking commit 004bf6fd7af8 (qom: Move compat_props machinery from qdev to QOM)
3/12 Checking commit a74b064b72d7 (vl: Fix latent bug with -global and onboard devices)
4/12 Checking commit 54f785aef6ac (sysbus: Fix latent bug with onboard devices)
5/12 Checking commit 8e6aa0c7e85c (vl: Improve legibility of BlockdevOptions queue)
6/12 Checking commit 4fd41657da38 (vl: Factor configure_blockdev() out of main())
WARNING: Block comments use a leading /* on a separate line
#25: FILE: vl.c:1207:
+    /* If the currently selected machine wishes to override the units-per-bus

WARNING: Block comments use a trailing */ on a separate line
#26: FILE: vl.c:1208:
+     * property of its default HBA interface type, do so now. */

total: 0 errors, 2 warnings, 83 lines checked

Patch 6/12 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
7/12 Checking commit ac5ff6df764f (vl: Create block backends before setting machine properties)
8/12 Checking commit 06dfc824b9ea (pflash_cfi01: Add pflash_cfi01_get_blk() helper)
9/12 Checking commit 5ed81089c86b (pc_sysfw: Remove unused PcSysFwDevice)
10/12 Checking commit 2781f626cf40 (pc_sysfw: Pass PCMachineState to pc_system_firmware_init())
11/12 Checking commit 7b348eb43865 (pc: Support firmware configuration with -blockdev)
WARNING: line over 80 characters
#372: FILE: hw/i386/pc_sysfw.c:328:
+            error_report("pflash with kvm requires KVM readonly memory support");

total: 0 errors, 1 warnings, 315 lines checked

Patch 11/12 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
12/12 Checking commit e159d1ed0de2 (docs/interop/firmware.json: Prefer -machine to if=pflash)
ERROR: trailing whitespace
#49: FILE: docs/interop/firmware.json:239:
+#                  $

total: 1 errors, 0 warnings, 33 lines checked

Patch 12/12 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

=== OUTPUT END ===

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/20190307172401.29451-1-armbru@redhat.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [Qemu-devel] [PATCH v2 00/12] pc: Support firmware configuration with -blockdev

[no-reply]

Patchew URL: https://patchew.org/QEMU/20190307172401.29451-1-armbru@redhat.com/



Hi,

This series seems to have some coding style problems. See output below for
more information:

Type: series
Message-id: 20190307172401.29451-1-armbru@redhat.com
Subject: [Qemu-devel] [PATCH v2 00/12] pc: Support firmware configuration with -blockdev

=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 t [tag update]            patchew/20190307172401.29451-1-armbru@redhat.com -> patchew/20190307172401.29451-1-armbru@redhat.com
Switched to a new branch 'test'
6c5d25d47a docs/interop/firmware.json: Prefer -machine to if=pflash
6067eed70b pc: Support firmware configuration with -blockdev
12eb362604 pc_sysfw: Pass PCMachineState to pc_system_firmware_init()
0681a43d40 pc_sysfw: Remove unused PcSysFwDevice
48fd47a85e pflash_cfi01: Add pflash_cfi01_get_blk() helper
ed5dc8dcd3 vl: Create block backends before setting machine properties
257e2ea345 vl: Factor configure_blockdev() out of main()
72732fcc5b vl: Improve legibility of BlockdevOptions queue
fbc43b7cdc sysbus: Fix latent bug with onboard devices
f9b5a0c27e vl: Fix latent bug with -global and onboard devices
3c85d24fe2 qom: Move compat_props machinery from qdev to QOM
a2cd7f244b qdev: Fix latent bug with compat_props and onboard devices

=== OUTPUT BEGIN ===
1/12 Checking commit a2cd7f244b98 (qdev: Fix latent bug with compat_props and onboard devices)
2/12 Checking commit 3c85d24fe25c (qom: Move compat_props machinery from qdev to QOM)
3/12 Checking commit f9b5a0c27e89 (vl: Fix latent bug with -global and onboard devices)
4/12 Checking commit fbc43b7cdc64 (sysbus: Fix latent bug with onboard devices)
5/12 Checking commit 72732fcc5b55 (vl: Improve legibility of BlockdevOptions queue)
6/12 Checking commit 257e2ea345c7 (vl: Factor configure_blockdev() out of main())
WARNING: Block comments use a leading /* on a separate line
#26: FILE: vl.c:1207:
+    /* If the currently selected machine wishes to override the units-per-bus

WARNING: Block comments use a trailing */ on a separate line
#27: FILE: vl.c:1208:
+     * property of its default HBA interface type, do so now. */

total: 0 errors, 2 warnings, 83 lines checked

Patch 6/12 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
7/12 Checking commit ed5dc8dcd393 (vl: Create block backends before setting machine properties)
8/12 Checking commit 48fd47a85ef9 (pflash_cfi01: Add pflash_cfi01_get_blk() helper)
9/12 Checking commit 0681a43d40ab (pc_sysfw: Remove unused PcSysFwDevice)
10/12 Checking commit 12eb3626047c (pc_sysfw: Pass PCMachineState to pc_system_firmware_init())
11/12 Checking commit 6067eed70b5c (pc: Support firmware configuration with -blockdev)
WARNING: line over 80 characters
#372: FILE: hw/i386/pc_sysfw.c:328:
+            error_report("pflash with kvm requires KVM readonly memory support");

total: 0 errors, 1 warnings, 315 lines checked

Patch 11/12 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
12/12 Checking commit 6c5d25d47a92 (docs/interop/firmware.json: Prefer -machine to if=pflash)
ERROR: trailing whitespace
#49: FILE: docs/interop/firmware.json:239:
+#                  $

total: 1 errors, 0 warnings, 33 lines checked

Patch 12/12 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

=== OUTPUT END ===

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/20190307172401.29451-1-armbru@redhat.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [Qemu-devel] [PATCH v2 00/12] pc: Support firmware configuration with -blockdev

[no-reply]

Patchew URL: https://patchew.org/QEMU/20190307172401.29451-1-armbru@redhat.com/



Hi,

This series seems to have some coding style problems. See output below for
more information:

Type: series
Message-id: 20190307172401.29451-1-armbru@redhat.com
Subject: [Qemu-devel] [PATCH v2 00/12] pc: Support firmware configuration with -blockdev

=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 t [tag update]            patchew/20190307172401.29451-1-armbru@redhat.com -> patchew/20190307172401.29451-1-armbru@redhat.com
Switched to a new branch 'test'
b8003b82de docs/interop/firmware.json: Prefer -machine to if=pflash
4694b306ec pc: Support firmware configuration with -blockdev
93bc292290 pc_sysfw: Pass PCMachineState to pc_system_firmware_init()
c3d5fb15cd pc_sysfw: Remove unused PcSysFwDevice
3787c376e2 pflash_cfi01: Add pflash_cfi01_get_blk() helper
4fbc3cfda2 vl: Create block backends before setting machine properties
63a9dfee66 vl: Factor configure_blockdev() out of main()
5578280abb vl: Improve legibility of BlockdevOptions queue
0fddf3eb0e sysbus: Fix latent bug with onboard devices
915ec2186e vl: Fix latent bug with -global and onboard devices
a6cf369849 qom: Move compat_props machinery from qdev to QOM
facb673f93 qdev: Fix latent bug with compat_props and onboard devices

=== OUTPUT BEGIN ===
1/12 Checking commit facb673f9389 (qdev: Fix latent bug with compat_props and onboard devices)
2/12 Checking commit a6cf3698492d (qom: Move compat_props machinery from qdev to QOM)
3/12 Checking commit 915ec2186e86 (vl: Fix latent bug with -global and onboard devices)
4/12 Checking commit 0fddf3eb0e35 (sysbus: Fix latent bug with onboard devices)
5/12 Checking commit 5578280abbef (vl: Improve legibility of BlockdevOptions queue)
6/12 Checking commit 63a9dfee6675 (vl: Factor configure_blockdev() out of main())
WARNING: Block comments use a leading /* on a separate line
#26: FILE: vl.c:1207:
+    /* If the currently selected machine wishes to override the units-per-bus

WARNING: Block comments use a trailing */ on a separate line
#27: FILE: vl.c:1208:
+     * property of its default HBA interface type, do so now. */

total: 0 errors, 2 warnings, 83 lines checked

Patch 6/12 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
7/12 Checking commit 4fbc3cfda201 (vl: Create block backends before setting machine properties)
8/12 Checking commit 3787c376e2ce (pflash_cfi01: Add pflash_cfi01_get_blk() helper)
9/12 Checking commit c3d5fb15cd25 (pc_sysfw: Remove unused PcSysFwDevice)
10/12 Checking commit 93bc29229007 (pc_sysfw: Pass PCMachineState to pc_system_firmware_init())
11/12 Checking commit 4694b306ec66 (pc: Support firmware configuration with -blockdev)
WARNING: line over 80 characters
#372: FILE: hw/i386/pc_sysfw.c:328:
+            error_report("pflash with kvm requires KVM readonly memory support");

total: 0 errors, 1 warnings, 315 lines checked

Patch 11/12 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
12/12 Checking commit b8003b82deab (docs/interop/firmware.json: Prefer -machine to if=pflash)
ERROR: trailing whitespace
#49: FILE: docs/interop/firmware.json:239:
+#                  $

total: 1 errors, 0 warnings, 33 lines checked

Patch 12/12 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

=== OUTPUT END ===

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/20190307172401.29451-1-armbru@redhat.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [Qemu-devel] [PATCH v2 00/12] pc: Support firmware configuration with -blockdev

[no-reply]

Patchew URL: https://patchew.org/QEMU/20190307172401.29451-1-armbru@redhat.com/



Hi,

This series seems to have some coding style problems. See output below for
more information:

Type: series
Message-id: 20190307172401.29451-1-armbru@redhat.com
Subject: [Qemu-devel] [PATCH v2 00/12] pc: Support firmware configuration with -blockdev

=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 t [tag update]            patchew/20190307172401.29451-1-armbru@redhat.com -> patchew/20190307172401.29451-1-armbru@redhat.com
Switched to a new branch 'test'
3ea929de65 docs/interop/firmware.json: Prefer -machine to if=pflash
b2f120dc53 pc: Support firmware configuration with -blockdev
e20c52727e pc_sysfw: Pass PCMachineState to pc_system_firmware_init()
d51f290e9e pc_sysfw: Remove unused PcSysFwDevice
96565445f8 pflash_cfi01: Add pflash_cfi01_get_blk() helper
bb44a08024 vl: Create block backends before setting machine properties
3806557e1f vl: Factor configure_blockdev() out of main()
984f18284e vl: Improve legibility of BlockdevOptions queue
ee5d063c25 sysbus: Fix latent bug with onboard devices
1e5f89d716 vl: Fix latent bug with -global and onboard devices
19954ff0fa qom: Move compat_props machinery from qdev to QOM
27f1c4f184 qdev: Fix latent bug with compat_props and onboard devices

=== OUTPUT BEGIN ===
1/12 Checking commit 27f1c4f18410 (qdev: Fix latent bug with compat_props and onboard devices)
2/12 Checking commit 19954ff0fa94 (qom: Move compat_props machinery from qdev to QOM)
3/12 Checking commit 1e5f89d716e8 (vl: Fix latent bug with -global and onboard devices)
4/12 Checking commit ee5d063c25a6 (sysbus: Fix latent bug with onboard devices)
5/12 Checking commit 984f18284e05 (vl: Improve legibility of BlockdevOptions queue)
6/12 Checking commit 3806557e1f00 (vl: Factor configure_blockdev() out of main())
WARNING: Block comments use a leading /* on a separate line
#26: FILE: vl.c:1207:
+    /* If the currently selected machine wishes to override the units-per-bus

WARNING: Block comments use a trailing */ on a separate line
#27: FILE: vl.c:1208:
+     * property of its default HBA interface type, do so now. */

total: 0 errors, 2 warnings, 83 lines checked

Patch 6/12 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
7/12 Checking commit bb44a0802451 (vl: Create block backends before setting machine properties)
8/12 Checking commit 96565445f821 (pflash_cfi01: Add pflash_cfi01_get_blk() helper)
9/12 Checking commit d51f290e9e75 (pc_sysfw: Remove unused PcSysFwDevice)
10/12 Checking commit e20c52727e5d (pc_sysfw: Pass PCMachineState to pc_system_firmware_init())
11/12 Checking commit b2f120dc5319 (pc: Support firmware configuration with -blockdev)
WARNING: line over 80 characters
#373: FILE: hw/i386/pc_sysfw.c:328:
+            error_report("pflash with kvm requires KVM readonly memory support");

total: 0 errors, 1 warnings, 315 lines checked

Patch 11/12 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
12/12 Checking commit 3ea929de6531 (docs/interop/firmware.json: Prefer -machine to if=pflash)
ERROR: trailing whitespace
#49: FILE: docs/interop/firmware.json:239:
+#                  $

total: 1 errors, 0 warnings, 33 lines checked

Patch 12/12 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

=== OUTPUT END ===

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/20190307172401.29451-1-armbru@redhat.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [Qemu-devel] [PATCH v2 00/12] pc: Support firmware configuration with -blockdev

[no-reply]

Patchew URL: https://patchew.org/QEMU/20190307172401.29451-1-armbru@redhat.com/



Hi,

This series failed the docker-mingw@fedora build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.

=== TEST SCRIPT BEGIN ===
#!/bin/bash
time make docker-test-mingw@fedora SHOW_ENV=1 J=14 NETWORK=1
=== TEST SCRIPT END ===

  CC      hw/char/nrf51_uart.o
  CC      hw/char/parallel.o
/tmp/qemu-test/src/hw/block/pflash_cfi02.c: In function 'pflash_cfi02_unrealize':
/tmp/qemu-test/src/hw/block/pflash_cfi02.c:698:5: error: unknown type name 'pflash_t'; did you mean 'fpos_t'?
     pflash_t *pfl = CFI_PFLASH02(dev);
     ^~~~~~~~
     fpos_t
/tmp/qemu-test/src/hw/block/pflash_cfi02.c:698:21: error: implicit declaration of function 'CFI_PFLASH02'; did you mean 'HW_FLASH_H'? [-Werror=implicit-function-declaration]
     pflash_t *pfl = CFI_PFLASH02(dev);
                     ^~~~~~~~~~~~
                     HW_FLASH_H
/tmp/qemu-test/src/hw/block/pflash_cfi02.c:698:21: error: nested extern declaration of 'CFI_PFLASH02' [-Werror=nested-externs]
/tmp/qemu-test/src/hw/block/pflash_cfi02.c:698:21: error: initialization of 'int *' from 'int' makes pointer from integer without a cast [-Werror=int-conversion]
/tmp/qemu-test/src/hw/block/pflash_cfi02.c:699:19: error: request for member 'timer' in something not a structure or union
     timer_del(&pfl->timer);
                   ^~
cc1: all warnings being treated as errors


The full log is available at
http://patchew.org/logs/20190307172401.29451-1-armbru@redhat.com/testing.docker-mingw@fedora/?type=message.
---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@redhat.com