[Qemu-devel] [RFC PATCH 0/6] pc: Support firmware configuration with -blockdev

[Qemu-devel] [RFC PATCH 0/6] pc: Support firmware configuration with -blockdev

[Markus Armbruster]

This is RFC mostly due to the last patch.  I'm not 100% sure my
approach is sane, and there's also an unresolved FIXME.  Please
review.

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

Markus Armbruster (6):
  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: Create block backends before setting machine properties
  pc: Support firmware configuration with -blockdev

 accel/accel.c            |   1 +
 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       | 230 ++++++++++++++++++++++++++-------------
 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                     |  90 +++++++--------
 12 files changed, 255 insertions(+), 150 deletions(-)

-- 
2.17.2

[Qemu-devel] [RFC PATCH 1/6] 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>
---
 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 0a84c42756..bced1f2666 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 502857a176..c50c2d6178 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

Re: [Qemu-devel] [RFC PATCH 1/6] qdev: Fix latent bug with compat_props and onboard devices

[Marc-André Lureau]

Hi

On Mon, Feb 25, 2019 at 7:38 PM Markus Armbruster <armbru@redhat.com> wrote:
>
> 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 0a84c42756..bced1f2666 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 502857a176..c50c2d6178 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] [RFC PATCH 2/6] 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>
---
 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 bced1f2666..f2f0006234 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 b8c732063b..adb9b7fe91 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

Re: [Qemu-devel] [RFC PATCH 2/6] qom: Move compat_props machinery from qdev to QOM

[Marc-André Lureau]

Hi

On Mon, Feb 25, 2019 at 7:46 PM Markus Armbruster <armbru@redhat.com> wrote:
>
> See the previous commit for rationale.
>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

Reviewed-by: Marc-André Lureau <marcandre.lureau@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 bced1f2666..f2f0006234 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 b8c732063b..adb9b7fe91 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
>
>


-- 
Marc-André Lureau

Re: [Qemu-devel] [RFC PATCH 2/6] qom: Move compat_props machinery from qdev to QOM

[Philippe Mathieu-Daudé]

On 2/25/19 7:37 PM, Markus Armbruster wrote:
> See the previous commit for rationale.
> 
> Signed-off-by: Markus Armbruster <armbru@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 bced1f2666..f2f0006234 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);

Documentation comments are in .c, OK :(

>  
>  /**
>   * object_set_props:
> diff --git a/qom/object.c b/qom/object.c
> index b8c732063b..adb9b7fe91 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;
> +}
> +

Can you add a comment for this one too? (no need to respin)

> +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;
> 

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

Re: [Qemu-devel] [RFC PATCH 2/6] qom: Move compat_props machinery from qdev to QOM

[Markus Armbruster]

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

> On 2/25/19 7:37 PM, Markus Armbruster wrote:
>> See the previous commit for rationale.
>> 
>> Signed-off-by: Markus Armbruster <armbru@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 bced1f2666..f2f0006234 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);
>
> Documentation comments are in .c, OK :(

I chose to move the comments along with the code.  If we want the
function comments in object.h for local consistency, we should also
rewrite to conform to the style used there.  Separate patch, so this one
can remain pure code motion.

>>  
>>  /**
>>   * object_set_props:
>> diff --git a/qom/object.c b/qom/object.c
>> index b8c732063b..adb9b7fe91 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;
>> +}
>> +
>
> Can you add a comment for this one too? (no need to respin)

Separate patch, so this one can remain pure code motion.  If we decide
to move (reformatted) function comments to object.h, the additional
comment can be added in that patch.

>> +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;
>> 
>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Thanks!

[Qemu-devel] [RFC PATCH 3/6] 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>
---
 vl.c | 19 ++-----------------
 1 file changed, 2 insertions(+), 17 deletions(-)

diff --git a/vl.c b/vl.c
index c50c2d6178..e3fdce410f 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) {
@@ -4248,12 +4239,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

Re: [Qemu-devel] [RFC PATCH 3/6] vl: Fix latent bug with -global and onboard devices

[Marc-André Lureau]

On Mon, Feb 25, 2019 at 7:41 PM Markus Armbruster <armbru@redhat.com> wrote:
>
> 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 c50c2d6178..e3fdce410f 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) {
> @@ -4248,12 +4239,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
>
>


-- 
Marc-André Lureau

[Qemu-devel] [RFC PATCH 4/6] 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>
---
 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 e3fdce410f..6ce3d2d448 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

Re: [Qemu-devel] [RFC PATCH 4/6] sysbus: Fix latent bug with onboard devices

[Marc-André Lureau]

Hi

On Mon, Feb 25, 2019 at 7:40 PM Markus Armbruster <armbru@redhat.com> wrote:
>
> 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>

makes sense to me, but I might miss some subtleties..

Reviewed-by: Marc-André Lureau <marcandre.lureau@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 e3fdce410f..6ce3d2d448 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
>
>


-- 
Marc-André Lureau

Re: [Qemu-devel] [RFC PATCH 4/6] sysbus: Fix latent bug with onboard devices

[Philippe Mathieu-Daudé]

On 2/25/19 7:37 PM, Markus Armbruster wrote:
> 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>
> ---
>  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 e3fdce410f..6ce3d2d448 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 SYSTEM_BUS doesn't exist, sysbus_get_default() creates it.

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

>  
>      if (machine_class->minimum_page_bits) {
>          if (!set_preferred_target_page_bits(machine_class->minimum_page_bits)) {
> 

Re: [Qemu-devel] [RFC PATCH 4/6] sysbus: Fix latent bug with onboard devices

[Thomas Huth]

On 25/02/2019 19.37, Markus Armbruster wrote:
> 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>
> ---
>  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 e3fdce410f..6ce3d2d448 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)) {
> 

Looks right. Especially, a device should also not add itself to a
parent, so this definitely should not be done in sysbus.c

Reviewed-by: Thomas Huth <thuth@redhat.com>


PS: Not directly related to your patch, but in a separate patch we
should also object_unref(current_machine) here to drop the superfluous
second reference to current_machine after we added it as a child of the
root object.

Re: [Qemu-devel] [RFC PATCH 4/6] sysbus: Fix latent bug with onboard devices

[Markus Armbruster]

Thomas Huth <thuth@redhat.com> writes:

> On 25/02/2019 19.37, Markus Armbruster wrote:
>> 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>
>> ---
>>  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 e3fdce410f..6ce3d2d448 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)) {
>> 
>
> Looks right. Especially, a device should also not add itself to a
> parent, so this definitely should not be done in sysbus.c
>
> Reviewed-by: Thomas Huth <thuth@redhat.com>
>
>
> PS: Not directly related to your patch, but in a separate patch we
> should also object_unref(current_machine) here to drop the superfluous
> second reference to current_machine after we added it as a child of the
> root object.

Just for cleanliness.  Makes sense.

Thanks!

Re: [Qemu-devel] [RFC PATCH 4/6] sysbus: Fix latent bug with onboard devices

[Thomas Huth]

On 05/03/2019 07.54, Markus Armbruster wrote:
> Thomas Huth <thuth@redhat.com> writes:
> 
>> On 25/02/2019 19.37, Markus Armbruster wrote:
[...]
>>> diff --git a/vl.c b/vl.c
>>> index e3fdce410f..6ce3d2d448 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);
[...]
>>
>> PS: Not directly related to your patch, but in a separate patch we
>> should also object_unref(current_machine) here to drop the superfluous
>> second reference to current_machine after we added it as a child of the
>> root object.
> 
> Just for cleanliness.  Makes sense.

Not only for cleanliness ... there is a TODO at the very end of vl.c
which we should fix one day, and for that we'd need the unref here.

 Thomas

[Qemu-devel] [RFC PATCH 5/6] 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 | 66 ++++++++++++++++++++++++++++++------------------------------
 1 file changed, 33 insertions(+), 33 deletions(-)

diff --git a/vl.c b/vl.c
index 6ce3d2d448..5cb0810ffa 100644
--- a/vl.c
+++ b/vl.c
@@ -4232,6 +4232,39 @@ int main(int argc, char **argv, char **envp)
         exit(0);
     }
 
+    /* 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)) {
+        BlockdevOptions_queue *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);
+
     machine_opts = qemu_get_machine_opts();
     qemu_opt_foreach(machine_opts, machine_set_property, current_machine,
                      &error_fatal);
@@ -4355,39 +4388,6 @@ 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)) {
-        BlockdevOptions_queue *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);
-
     qemu_opts_foreach(qemu_find_opts("mon"),
                       mon_init_func, NULL, &error_fatal);
 
-- 
2.17.2

Re: [Qemu-devel] [RFC PATCH 5/6] vl: Create block backends before setting machine properties

[Philippe Mathieu-Daudé]

On 2/25/19 7:37 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 | 66 ++++++++++++++++++++++++++++++------------------------------
>  1 file changed, 33 insertions(+), 33 deletions(-)
> 
> diff --git a/vl.c b/vl.c
> index 6ce3d2d448..5cb0810ffa 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -4232,6 +4232,39 @@ int main(int argc, char **argv, char **envp)
>          exit(0);
>      }
>  

Can we extract this from the main, maybe as "parse_blockdev()"? This
will ease further rejigger :)

> +    /* 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)) {
> +        BlockdevOptions_queue *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);
> +

Can you add a comment here such:

   /*
    * Arguments setting properties on devices has to be processed before
    * the following qemu_opt_foreach(...machine_set_property...) call.
    */

>      machine_opts = qemu_get_machine_opts();
>      qemu_opt_foreach(machine_opts, machine_set_property, current_machine,
>                       &error_fatal);
> @@ -4355,39 +4388,6 @@ 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)) {
> -        BlockdevOptions_queue *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);
> -
>      qemu_opts_foreach(qemu_find_opts("mon"),
>                        mon_init_func, NULL, &error_fatal);
>  

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

Re: [Qemu-devel] [RFC PATCH 5/6] vl: Create block backends before setting machine properties

[Markus Armbruster]

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

> On 2/25/19 7:37 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 | 66 ++++++++++++++++++++++++++++++------------------------------
>>  1 file changed, 33 insertions(+), 33 deletions(-)
>> 
>> diff --git a/vl.c b/vl.c
>> index 6ce3d2d448..5cb0810ffa 100644
>> --- a/vl.c
>> +++ b/vl.c
>> @@ -4232,6 +4232,39 @@ int main(int argc, char **argv, char **envp)
>>          exit(0);
>>      }
>>  
>
> Can we extract this from the main, maybe as "parse_blockdev()"? This
> will ease further rejigger :)

I can try and see whether I like it.

>> +    /* 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)) {
>> +        BlockdevOptions_queue *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);
>> +
>
> Can you add a comment here such:
>
>    /*
>     * Arguments setting properties on devices has to be processed before
>     * the following qemu_opt_foreach(...machine_set_property...) call.
>     */

Hmm, is this accurate?  The issue this patch addresses is

    "arguments creating block backends have to be processed before
    machine_set_property()",

which is an instance of

    "arguments creating backends ...",

which is an instance of

    "arguments creating stuff machine property values can reference ..."

The patch only addresses the instance "block backends".  I have no
appetite for attacking more general problems right now.

See also

    Subject: Re: [Qemu-devel] [PATCH RFC 0/3] qdev: order devices by priority before creating them
    Date: Wed, 11 May 2016 09:51:39 +0200
    Message-ID: <87y47hhw1g.fsf@dusky.pond.sub.org>
    https://lists.nongnu.org/archive/html/qemu-devel/2016-05/msg01589.html

    Subject: Re: [Qemu-devel] [PATCH] vl: Delay initialization of memory backends
    Date: Fri, 02 Sep 2016 08:13:17 +0200
    Message-ID: <87poomg77m.fsf@dusky.pond.sub.org>
    https://lists.nongnu.org/archive/html/qemu-devel/2016-09/msg00163.html

That said, I'm fine with adding a comment explaining the more general
mess.  Can you give me some hints on what future readers will find
useful?

>>      machine_opts = qemu_get_machine_opts();
>>      qemu_opt_foreach(machine_opts, machine_set_property, current_machine,
>>                       &error_fatal);
>> @@ -4355,39 +4388,6 @@ 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)) {
>> -        BlockdevOptions_queue *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);
>> -
>>      qemu_opts_foreach(qemu_find_opts("mon"),
>>                        mon_init_func, NULL, &error_fatal);
>>  
>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Thanks!

Re: [Qemu-devel] [RFC PATCH 5/6] vl: Create block backends before setting machine properties

[Philippe Mathieu-Daudé]

On 3/5/19 11:38 AM, Markus Armbruster wrote:
> Philippe Mathieu-Daudé <philmd@redhat.com> writes:
>> On 2/25/19 7:37 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 | 66 ++++++++++++++++++++++++++++++------------------------------
>>>  1 file changed, 33 insertions(+), 33 deletions(-)
>>>
>>> diff --git a/vl.c b/vl.c
>>> index 6ce3d2d448..5cb0810ffa 100644
>>> --- a/vl.c
>>> +++ b/vl.c
>>> @@ -4232,6 +4232,39 @@ int main(int argc, char **argv, char **envp)
>>>          exit(0);
>>>      }
>>>  
>>
>> Can we extract this from the main, maybe as "parse_blockdev()"? This
>> will ease further 	 :)
> 
> I can try and see whether I like it.
> 
>>> +    /* 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)) {
>>> +        BlockdevOptions_queue *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);
>>> +
>>
>> Can you add a comment here such:
>>
>>    /*
>>     * Arguments setting properties on devices has to be processed before
>>     * the following qemu_opt_foreach(...machine_set_property...) call.
>>     */
> 
> Hmm, is this accurate?  The issue this patch addresses is
> 
>     "arguments creating block backends have to be processed before
>     machine_set_property()",
> 
> which is an instance of
> 
>     "arguments creating backends ...",
> 
> which is an instance of
> 
>     "arguments creating stuff machine property values can reference ..."
> 
> The patch only addresses the instance "block backends".  I have no
> appetite for attacking more general problems right now.
> 
> See also
> 
>     Subject: Re: [Qemu-devel] [PATCH RFC 0/3] qdev: order devices by priority before creating them
>     Date: Wed, 11 May 2016 09:51:39 +0200
>     Message-ID: <87y47hhw1g.fsf@dusky.pond.sub.org>
>     https://lists.nongnu.org/archive/html/qemu-devel/2016-05/msg01589.html
> 
>     Subject: Re: [Qemu-devel] [PATCH] vl: Delay initialization of memory backends
>     Date: Fri, 02 Sep 2016 08:13:17 +0200
>     Message-ID: <87poomg77m.fsf@dusky.pond.sub.org>
>     https://lists.nongnu.org/archive/html/qemu-devel/2016-09/msg00163.html
> 
> That said, I'm fine with adding a comment explaining the more general
> mess.  Can you give me some hints on what future readers will find
> useful?

This can be as simple as:

     /*
      * Arguments creating block backends have to be processed before
      * machine_set_property().
      */
     parse_blockdev(...);

So if I have to hack around in the future I'd look at the commit
introducing this comment:

  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.

So I won't rejig this at his previous place :)

> 
>>>      machine_opts = qemu_get_machine_opts();
>>>      qemu_opt_foreach(machine_opts, machine_set_property, current_machine,
>>>                       &error_fatal);
>>> @@ -4355,39 +4388,6 @@ 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)) {
>>> -        BlockdevOptions_queue *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);
>>> -
>>>      qemu_opts_foreach(qemu_find_opts("mon"),
>>>                        mon_init_func, NULL, &error_fatal);
>>>  
>>
>> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> 
> Thanks!
> 

Re: [Qemu-devel] [RFC PATCH 5/6] vl: Create block backends before setting machine properties

[Markus Armbruster]

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

> On 3/5/19 11:38 AM, Markus Armbruster wrote:
>> Philippe Mathieu-Daudé <philmd@redhat.com> writes:
>>> On 2/25/19 7:37 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 | 66 ++++++++++++++++++++++++++++++------------------------------
>>>>  1 file changed, 33 insertions(+), 33 deletions(-)
>>>>
>>>> diff --git a/vl.c b/vl.c
>>>> index 6ce3d2d448..5cb0810ffa 100644
>>>> --- a/vl.c
>>>> +++ b/vl.c
>>>> @@ -4232,6 +4232,39 @@ int main(int argc, char **argv, char **envp)
>>>>          exit(0);
>>>>      }
>>>>  
>>>
>>> Can we extract this from the main, maybe as "parse_blockdev()"? This
>>> will ease further 	 :)
>> 
>> I can try and see whether I like it.
>> 
>>>> +    /* 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)) {
>>>> +        BlockdevOptions_queue *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);
>>>> +
>>>
>>> Can you add a comment here such:
>>>
>>>    /*
>>>     * Arguments setting properties on devices has to be processed before
>>>     * the following qemu_opt_foreach(...machine_set_property...) call.
>>>     */
>> 
>> Hmm, is this accurate?  The issue this patch addresses is
>> 
>>     "arguments creating block backends have to be processed before
>>     machine_set_property()",
>> 
>> which is an instance of
>> 
>>     "arguments creating backends ...",
>> 
>> which is an instance of
>> 
>>     "arguments creating stuff machine property values can reference ..."
>> 
>> The patch only addresses the instance "block backends".  I have no
>> appetite for attacking more general problems right now.
>> 
>> See also
>> 
>>     Subject: Re: [Qemu-devel] [PATCH RFC 0/3] qdev: order devices by priority before creating them
>>     Date: Wed, 11 May 2016 09:51:39 +0200
>>     Message-ID: <87y47hhw1g.fsf@dusky.pond.sub.org>
>>     https://lists.nongnu.org/archive/html/qemu-devel/2016-05/msg01589.html
>> 
>>     Subject: Re: [Qemu-devel] [PATCH] vl: Delay initialization of memory backends
>>     Date: Fri, 02 Sep 2016 08:13:17 +0200
>>     Message-ID: <87poomg77m.fsf@dusky.pond.sub.org>
>>     https://lists.nongnu.org/archive/html/qemu-devel/2016-09/msg00163.html
>> 
>> That said, I'm fine with adding a comment explaining the more general
>> mess.  Can you give me some hints on what future readers will find
>> useful?
>
> This can be as simple as:
>
>      /*
>       * Arguments creating block backends have to be processed before
>       * machine_set_property().
>       */
>      parse_blockdev(...);
>
> So if I have to hack around in the future I'd look at the commit
> introducing this comment:
>
>   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.
>
> So I won't rejig this at his previous place :)

Makes sense, thanks.

[...]

[Qemu-devel] [RFC PATCH 6/6] 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.  Except I can't figure out how to destroy the
devices correctly.  Marked FIXME.

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>
---
 hw/block/pflash_cfi01.c  |   5 +
 hw/i386/pc.c             |   4 +-
 hw/i386/pc_sysfw.c       | 230 ++++++++++++++++++++++++++-------------
 include/hw/block/flash.h |   1 +
 include/hw/i386/pc.h     |   6 +-
 5 files changed, 168 insertions(+), 78 deletions(-)

diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
index 6ad27f4472..7c428bbf50 100644
--- a/hw/block/pflash_cfi01.c
+++ b/hw/block/pflash_cfi01.c
@@ -959,6 +959,11 @@ MemoryRegion *pflash_cfi01_get_memory(PFlashCFI01 *fl)
     return &fl->mem;
 }
 
+BlockBackend *pflash_cfi01_get_blk(PFlashCFI01 *fl)
+{
+    return fl->blk;
+}
+
 static void postload_update_cb(void *opaque, int running, RunState state)
 {
     PFlashCFI01 *pfl = opaque;
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 3889eccdc3..420a0c5c9e 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,
@@ -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 34727c5b1f..98998e1ba8 100644
--- a/hw/i386/pc_sysfw.c
+++ b/hw/i386/pc_sysfw.c
@@ -76,7 +76,58 @@ 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_CFI_PFLASH01);
+
+    qdev_prop_set_uint64(dev, "sector-length", 4096);
+    qdev_prop_set_uint8(dev, "width", 1);
+    qdev_prop_set_string(dev, "name", name);
+    return CFI_PFLASH01(dev);
+}
+
+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);
+            /*
+             * FIXME 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.
+             */
+            pcms->flash[i] = NULL;
+        }
+    }
+}
 
 /* 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
@@ -84,88 +135,78 @@ static void pc_isa_bios_init(MemoryRegion *rom_memory,
  * 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))
+#define FLASH_SIZE_LIMIT (8 * MiB)
 
-/* 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.
+/*
+ * 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 multiples of 4KiB, or the total size exceeds
+ * FLASH_SIZE_LIMIT.
  *
- * 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.
- *
- * 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) {
+            error_report("system firmware block device %s is empty",
+                         blk_name(blk));
+            exit(1);
+        }
+        if (size == 0 || size % 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",
+                        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 / 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);
 
@@ -236,24 +277,63 @@ 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);
+    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/block/flash.h b/include/hw/block/flash.h
index 24b13eb525..91e0f8dd6e 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 */
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index 3ff127ebd0..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,8 +280,8 @@ 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_flash_create(PCMachineState *pcms);
+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] [RFC PATCH 6/6] pc: Support firmware configuration with -blockdev

[Laszlo Ersek]

Hi Markus,

On 02/25/19 19:37, 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.  Except I can't figure out how to destroy the
> devices correctly.  Marked FIXME.
> 
> 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>
> ---
>  hw/block/pflash_cfi01.c  |   5 +
>  hw/i386/pc.c             |   4 +-
>  hw/i386/pc_sysfw.c       | 230 ++++++++++++++++++++++++++-------------
>  include/hw/block/flash.h |   1 +
>  include/hw/i386/pc.h     |   6 +-
>  5 files changed, 168 insertions(+), 78 deletions(-)

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

In general, the patch looks fine to me.


> diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
> index 6ad27f4472..7c428bbf50 100644
> --- a/hw/block/pflash_cfi01.c
> +++ b/hw/block/pflash_cfi01.c
> @@ -959,6 +959,11 @@ MemoryRegion *pflash_cfi01_get_memory(PFlashCFI01 *fl)
>      return &fl->mem;
>  }
>  
> +BlockBackend *pflash_cfi01_get_blk(PFlashCFI01 *fl)
> +{
> +    return fl->blk;
> +}
> +
>  static void postload_update_cb(void *opaque, int running, RunState state)
>  {
>      PFlashCFI01 *pfl = opaque;
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 3889eccdc3..420a0c5c9e 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,
> @@ -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 34727c5b1f..98998e1ba8 100644
> --- a/hw/i386/pc_sysfw.c
> +++ b/hw/i386/pc_sysfw.c
> @@ -76,7 +76,58 @@ 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_CFI_PFLASH01);
> +
> +    qdev_prop_set_uint64(dev, "sector-length", 4096);
> +    qdev_prop_set_uint8(dev, "width", 1);
> +    qdev_prop_set_string(dev, "name", name);
> +    return CFI_PFLASH01(dev);
> +}
> +
> +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);
> +            /*
> +             * FIXME 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.
> +             */
> +            pcms->flash[i] = NULL;
> +        }
> +    }
> +}

I totally can't recommend a way to resolve this FIXME, alas.

>  
>  /* 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
> @@ -84,88 +135,78 @@ static void pc_isa_bios_init(MemoryRegion *rom_memory,
>   * 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))
> +#define FLASH_SIZE_LIMIT (8 * MiB)
>  
> -/* 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.
> +/*
> + * 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 multiples of 4KiB, or the total size exceeds
> + * FLASH_SIZE_LIMIT.
>   *
> - * 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.
> - *
> - * 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;

(1) Rather than duplicate this constant here, can we get it inside the
loop, via the "sector-length" property? We set the property in
pc_pflash_create() (identically for both chips, but that's not a problem
I think).

>      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) {
> +            error_report("system firmware block device %s is empty",
> +                         blk_name(blk));
> +            exit(1);
> +        }
> +        if (size == 0 || size % sector_size != 0) {

(2) The (size == 0) condition is superfluous here, it's been checked
just above.


> +            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",
> +                        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 / sector_size);

OK, this division looks safe (the quotient should fit in a uint32_t
property) due to the FLASH_SIZE_LIMIT check above.

> +        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);
>  
> @@ -236,24 +277,63 @@ 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);
> +    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/block/flash.h b/include/hw/block/flash.h
> index 24b13eb525..91e0f8dd6e 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 */
> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> index 3ff127ebd0..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,8 +280,8 @@ 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_flash_create(PCMachineState *pcms);
> +void pc_system_firmware_init(PCMachineState *pcms, MemoryRegion *rom_memory);
>  
>  /* acpi-build.c */
>  void pc_madt_cpu_entry(AcpiDeviceIf *adev, int uid,
> 

Looks like a careful conversion to me (although I certainly miss the
finer aspects of DriveInfo vs. BlockBackend, etc).

Thanks,
Laszlo

Re: [Qemu-devel] [RFC PATCH 6/6] pc: Support firmware configuration with -blockdev

[Markus Armbruster]

Laszlo Ersek <lersek@redhat.com> writes:

> Hi Markus,
>
> On 02/25/19 19:37, 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.  Except I can't figure out how to destroy the
>> devices correctly.  Marked FIXME.
>> 
>> 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>
>> ---
>>  hw/block/pflash_cfi01.c  |   5 +
>>  hw/i386/pc.c             |   4 +-
>>  hw/i386/pc_sysfw.c       | 230 ++++++++++++++++++++++++++-------------
>>  include/hw/block/flash.h |   1 +
>>  include/hw/i386/pc.h     |   6 +-
>>  5 files changed, 168 insertions(+), 78 deletions(-)
>
> I don't know enough to understand a large part of the commit message. I
> can make some (superficial) comments on the code.

Appreciated!

> In general, the patch looks fine to me.
>
>
>> diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
>> index 6ad27f4472..7c428bbf50 100644
>> --- a/hw/block/pflash_cfi01.c
>> +++ b/hw/block/pflash_cfi01.c
>> @@ -959,6 +959,11 @@ MemoryRegion *pflash_cfi01_get_memory(PFlashCFI01 *fl)
>>      return &fl->mem;
>>  }
>>  
>> +BlockBackend *pflash_cfi01_get_blk(PFlashCFI01 *fl)
>> +{
>> +    return fl->blk;
>> +}
>> +
>>  static void postload_update_cb(void *opaque, int running, RunState state)
>>  {
>>      PFlashCFI01 *pfl = opaque;
>> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
>> index 3889eccdc3..420a0c5c9e 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,
>> @@ -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 34727c5b1f..98998e1ba8 100644
>> --- a/hw/i386/pc_sysfw.c
>> +++ b/hw/i386/pc_sysfw.c
>> @@ -76,7 +76,58 @@ 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_CFI_PFLASH01);
>> +
>> +    qdev_prop_set_uint64(dev, "sector-length", 4096);
>> +    qdev_prop_set_uint8(dev, "width", 1);
>> +    qdev_prop_set_string(dev, "name", name);
>> +    return CFI_PFLASH01(dev);
>> +}
>> +
>> +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);
>> +            /*
>> +             * FIXME 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.
>> +             */
>> +            pcms->flash[i] = NULL;
>> +        }
>> +    }
>> +}
>
> I totally can't recommend a way to resolve this FIXME, alas.

I'm hoping for Paolo to don his shining armor and rescue me ;)

>>  
>>  /* 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
>> @@ -84,88 +135,78 @@ static void pc_isa_bios_init(MemoryRegion *rom_memory,
>>   * 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))
>> +#define FLASH_SIZE_LIMIT (8 * MiB)
>>  
>> -/* 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.
>> +/*
>> + * 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 multiples of 4KiB, or the total size exceeds
>> + * FLASH_SIZE_LIMIT.
>>   *
>> - * 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.
>> - *
>> - * 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;
>
> (1) Rather than duplicate this constant here, can we get it inside the
> loop, via the "sector-length" property? We set the property in
> pc_pflash_create() (identically for both chips, but that's not a problem
> I think).

We can.  It's slightly bothersome, since PFlashCFI01 is incomplete here.

Simpler: #define FLASH_SECTOR_SIZE 4096, and use it in both places.  Not
quite as clean, because while the property value cannot change now, we
can't completely exclude the possibility for the future.

>>      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) {
>> +            error_report("system firmware block device %s is empty",
>> +                         blk_name(blk));
>> +            exit(1);
>> +        }
>> +        if (size == 0 || size % sector_size != 0) {
>
> (2) The (size == 0) condition is superfluous here, it's been checked
> just above.

Editing accident!  I meant to delete the conditional above.

>> +            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",
>> +                        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 / sector_size);
>
> OK, this division looks safe (the quotient should fit in a uint32_t
> property) due to the FLASH_SIZE_LIMIT check above.

Precisely.

>> +        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);
>>  
>> @@ -236,24 +277,63 @@ 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);
>> +    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/block/flash.h b/include/hw/block/flash.h
>> index 24b13eb525..91e0f8dd6e 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 */
>> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
>> index 3ff127ebd0..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,8 +280,8 @@ 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_flash_create(PCMachineState *pcms);
>> +void pc_system_firmware_init(PCMachineState *pcms, MemoryRegion *rom_memory);
>>  
>>  /* acpi-build.c */
>>  void pc_madt_cpu_entry(AcpiDeviceIf *adev, int uid,
>> 
>
> Looks like a careful conversion to me (although I certainly miss the
> finer aspects of DriveInfo vs. BlockBackend, etc).

Thank you!

Re: [Qemu-devel] [RFC PATCH 6/6] pc: Support firmware configuration with -blockdev

[Laszlo Ersek]

On 02/26/19 13:35, Markus Armbruster wrote:
> Laszlo Ersek <lersek@redhat.com> writes:

>>> -#define FLASH_MAP_UNIT_MAX 2
>>> +static PFlashCFI01 *pc_pflash_create(const char *name)
>>> +{
>>> +    DeviceState *dev = qdev_create(NULL, TYPE_CFI_PFLASH01);
>>> +
>>> +    qdev_prop_set_uint64(dev, "sector-length", 4096);
>>> +    qdev_prop_set_uint8(dev, "width", 1);
>>> +    qdev_prop_set_string(dev, "name", name);
>>> +    return CFI_PFLASH01(dev);
>>> +}
>>> +
>>> +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);
>>> +            /*
>>> +             * FIXME 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.
>>> +             */
>>> +            pcms->flash[i] = NULL;
>>> +        }
>>> +    }
>>> +}
>>
>> I totally can't recommend a way to resolve this FIXME, alas.
> 
> I'm hoping for Paolo to don his shining armor and rescue me ;)
> 
>>>  
>>>  /* 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
>>> @@ -84,88 +135,78 @@ static void pc_isa_bios_init(MemoryRegion *rom_memory,
>>>   * 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))
>>> +#define FLASH_SIZE_LIMIT (8 * MiB)
>>>  
>>> -/* 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.
>>> +/*
>>> + * 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 multiples of 4KiB, or the total size exceeds
>>> + * FLASH_SIZE_LIMIT.
>>>   *
>>> - * 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.
>>> - *
>>> - * 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;
>>
>> (1) Rather than duplicate this constant here, can we get it inside the
>> loop, via the "sector-length" property? We set the property in
>> pc_pflash_create() (identically for both chips, but that's not a problem
>> I think).
> 
> We can.  It's slightly bothersome, since PFlashCFI01 is incomplete here.
> 
> Simpler: #define FLASH_SECTOR_SIZE 4096, and use it in both places.  Not
> quite as clean, because while the property value cannot change now, we
> can't completely exclude the possibility for the future.

I agree, a macro works too.

Thanks,
Laszlo

Re: [Qemu-devel] [RFC PATCH 6/6] pc: Support firmware configuration with -blockdev

[Markus Armbruster]

The problem at hand is how to destroy a device created with
qdev_create() without ever realizing it.

This hack passes tests:

diff --git a/hw/i386/pc_sysfw.c b/hw/i386/pc_sysfw.c
index ed608a53d3..1bd538796b 100644
--- a/hw/i386/pc_sysfw.c
+++ b/hw/i386/pc_sysfw.c
@@ -116,14 +116,9 @@ static void pc_system_flash_cleanup_unused(PCMachineState *pcms)
             prop_name = g_strdup_printf("pflash%d", i);
             object_property_del(OBJECT(pcms), prop_name, &error_abort);
             g_free(prop_name);
-            /*
-             * FIXME 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.
-             */
+            object_ref(dev_obj);
+            object_get_class(dev_obj)->unparent(dev_obj);
+            object_unref(dev_obj);
             pcms->flash[i] = NULL;
         }
     }

If you have a better idea, please let me know.

Here's how I arrived at my hack.  We need to undo qdev_create().
qdev_create() is qdev_try_create() plus error handling.  The latter is
uninteresting here, so let's have a look at just the former:

    DeviceState *qdev_try_create(BusState *bus, const char *type)
    {
        DeviceState *dev;

        if (object_class_by_name(type) == NULL) {
            return NULL;
        }
        dev = DEVICE(object_new(type));
        if (!dev) {
            return NULL;
        }

        if (!bus) {
            /* Assert that the device really is a SysBusDevice before
             * we put it onto the sysbus. Non-sysbus devices which aren't
             * being put onto a bus should be created with object_new(TYPE_FOO),
             * not qdev_create(NULL, TYPE_FOO).
             */
            g_assert(object_dynamic_cast(OBJECT(dev), TYPE_SYS_BUS_DEVICE));
            bus = sysbus_get_default();
        }

--->    qdev_set_parent_bus(dev, bus);
        object_unref(OBJECT(dev));
        return dev;
    }

We need to undo qdev_set_parent_bus().

    void qdev_set_parent_bus(DeviceState *dev, BusState *bus)
    {
        bool replugging = dev->parent_bus != NULL;

        if (replugging) {
            /* Keep a reference to the device while it's not plugged into
             * any bus, to avoid it potentially evaporating when it is
             * dereffed in bus_remove_child().
             */
            object_ref(OBJECT(dev));
            bus_remove_child(dev->parent_bus, dev);
            object_unref(OBJECT(dev->parent_bus));
        }
        dev->parent_bus = bus;
        object_ref(OBJECT(bus));
--->    bus_add_child(bus, dev);
        if (replugging) {
            object_unref(OBJECT(dev));
        }
    }

dev->parent_bus is null, since we didn't realize the device.  All we
need to undo is bus_add_child(), with bus_remove_child().  Destruction
of realized devices does that here:

    static void device_unparent(Object *obj)
    {
        DeviceState *dev = DEVICE(obj);
        BusState *bus;

        if (dev->realized) {
            object_property_set_bool(obj, false, "realized", NULL);
        }
        while (dev->num_child_bus) {
            bus = QLIST_FIRST(&dev->child_bus);
            object_unparent(OBJECT(bus));
        }
        if (dev->parent_bus) {
            bus_remove_child(dev->parent_bus, dev);
            object_unref(OBJECT(dev->parent_bus));
            dev->parent_bus = NULL;
        }
    }

dev->realized is false, dev->num_child_bus is zero, dev->parent_bus is
main_system_bus.  Okay, this does what we need.  Now how do we call it?
It's static...  but it's a method:

    static void device_class_init(ObjectClass *class, void *data)
    {
        DeviceClass *dc = DEVICE_CLASS(class);

        class->unparent = device_unparent;
        [...]
    }

Alright, we can call object_get_class(dev_obj)->unparent(dev_obj).

Final complication: if I call just that, the device's reference counter
goes down to zero in the middle of device_unparent(), and we use after
free.  So I bracket he call with object_ref() and object_unref().

Re: [Qemu-devel] [RFC PATCH 6/6] pc: Support firmware configuration with -blockdev

[Laszlo Ersek]

On 03/04/19 18:50, Markus Armbruster wrote:

> Alright, we can call object_get_class(dev_obj)->unparent(dev_obj).
> 
> Final complication: if I call just that, the device's reference counter
> goes down to zero in the middle of device_unparent(), and we use after
> free.  So I bracket he call with object_ref() and object_unref().

I don't think that requiring such a bracketing is necessarily a problem.
I vaguely remember reviewing a kernel patch 6 or so years ago where the
patch used the same idea, with those "get" and "put" functions (the bug
the patch was fixing was that the last reference was "temporarily" lost
mid-operation).

So perhaps this can be addressed, for the general case, by extending the
documentation of device_unparent(). (The function has no documentation
at all, at the moment.)

Thanks
Laszlo

Re: [Qemu-devel] [RFC PATCH 6/6] pc: Support firmware configuration with -blockdev

[Markus Armbruster]

Laszlo Ersek <lersek@redhat.com> writes:

> On 03/04/19 18:50, Markus Armbruster wrote:
>
>> Alright, we can call object_get_class(dev_obj)->unparent(dev_obj).
>> 
>> Final complication: if I call just that, the device's reference counter
>> goes down to zero in the middle of device_unparent(), and we use after
>> free.  So I bracket he call with object_ref() and object_unref().
>
> I don't think that requiring such a bracketing is necessarily a problem.
> I vaguely remember reviewing a kernel patch 6 or so years ago where the
> patch used the same idea, with those "get" and "put" functions (the bug
> the patch was fixing was that the last reference was "temporarily" lost
> mid-operation).

I don't regard it as problem.  My voodoo coding just wasn't prepared for
it.

> So perhaps this can be addressed, for the general case, by extending the
> documentation of device_unparent(). (The function has no documentation
> at all, at the moment.)

I know just enough to be dangerous here, not enough to write
documentation.

We really need a complete life cycle diagram for devices.  The closest
we have is the section on realization in qdev-core.h, which lets me
divine only a part of the life cycle.  The missing part I struggled with
here is how to go from device state "created, not realized" to
"destroyed".

Re: [Qemu-devel] [RFC PATCH 6/6] pc: Support firmware configuration with -blockdev

[Philippe Mathieu-Daudé]

On 2/25/19 7:37 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.  Except I can't figure out how to destroy the
> devices correctly.  Marked FIXME.
> 
> 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>
> ---
>  hw/block/pflash_cfi01.c  |   5 +
>  hw/i386/pc.c             |   4 +-
>  hw/i386/pc_sysfw.c       | 230 ++++++++++++++++++++++++++-------------
>  include/hw/block/flash.h |   1 +
>  include/hw/i386/pc.h     |   6 +-
>  5 files changed, 168 insertions(+), 78 deletions(-)
> 
> diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
> index 6ad27f4472..7c428bbf50 100644
> --- a/hw/block/pflash_cfi01.c
> +++ b/hw/block/pflash_cfi01.c
> @@ -959,6 +959,11 @@ MemoryRegion *pflash_cfi01_get_memory(PFlashCFI01 *fl)
>      return &fl->mem;
>  }
>  
> +BlockBackend *pflash_cfi01_get_blk(PFlashCFI01 *fl)
> +{
> +    return fl->blk;
> +}
> +
>  static void postload_update_cb(void *opaque, int running, RunState state)
>  {
>      PFlashCFI01 *pfl = opaque;
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 3889eccdc3..420a0c5c9e 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,
> @@ -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 34727c5b1f..98998e1ba8 100644
> --- a/hw/i386/pc_sysfw.c
> +++ b/hw/i386/pc_sysfw.c
> @@ -76,7 +76,58 @@ 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_CFI_PFLASH01);
> +
> +    qdev_prop_set_uint64(dev, "sector-length", 4096);

4 * KiB

> +    qdev_prop_set_uint8(dev, "width", 1);
> +    qdev_prop_set_string(dev, "name", name);
> +    return CFI_PFLASH01(dev);
> +}
> +
> +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);

Since this assert is called unconditionally, we can move it to
pc_system_firmware_init().

> +
> +    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);
> +            /*
> +             * FIXME 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.
> +             */
> +            pcms->flash[i] = NULL;
> +        }
> +    }
> +}
>  
>  /* 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
> @@ -84,88 +135,78 @@ static void pc_isa_bios_init(MemoryRegion *rom_memory,
>   * 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))
> +#define FLASH_SIZE_LIMIT (8 * MiB)
>  
> -/* 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.
> +/*
> + * 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 multiples of 4KiB, or the total size exceeds

Not related to this patch:

Looking at git history the commit introducing this is bd183c79b51 but
there are no details about why it is required for 'PC'.
Is it a backend requirement?
If not, here we shouldn't care about underlying layout. It is the
responsability of the pflash device to figure this out.

Somehow related to this patch:

I understand I could use 1 single flash of 16KiB, or 2 of 4KiB and this
would work, I'm not sure coz haven't tested, are these real-world use cases?
Now we have a 8MiB limit. Neither a real-world use case.
Per commit 637a5acb46b this is OVMF related, also OVMF strictly uses 2
flashes.
IMHO there are too many checks around, and we could simplify.

> + * FLASH_SIZE_LIMIT.
>   *
> - * 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.
> - *
> - * 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) {
> +            error_report("system firmware block device %s is empty",
> +                         blk_name(blk));
> +            exit(1);
> +        }
> +        if (size == 0 || size % 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",
> +                        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 / 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);
>  
> @@ -236,24 +277,63 @@ 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);
> +    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);
> +        }
> +    }

Or simpler:

       if (pflash_blk[1] && !pflash_blk[0]) {
           error_report("pflash1 requires pflash0");
           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/block/flash.h b/include/hw/block/flash.h
> index 24b13eb525..91e0f8dd6e 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 */
> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> index 3ff127ebd0..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,8 +280,8 @@ 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_flash_create(PCMachineState *pcms);
> +void pc_system_firmware_init(PCMachineState *pcms, MemoryRegion *rom_memory);
>  
>  /* acpi-build.c */
>  void pc_madt_cpu_entry(AcpiDeviceIf *adev, int uid,
> 

Re: [Qemu-devel] [RFC PATCH 6/6] pc: Support firmware configuration with -blockdev

[Philippe Mathieu-Daudé]

On 3/4/19 8:14 PM, Philippe Mathieu-Daudé wrote:
> On 2/25/19 7:37 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.  Except I can't figure out how to destroy the
>> devices correctly.  Marked FIXME.
>>
>> 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>
>> ---
>>  hw/block/pflash_cfi01.c  |   5 +
>>  hw/i386/pc.c             |   4 +-
>>  hw/i386/pc_sysfw.c       | 230 ++++++++++++++++++++++++++-------------
>>  include/hw/block/flash.h |   1 +
>>  include/hw/i386/pc.h     |   6 +-
>>  5 files changed, 168 insertions(+), 78 deletions(-)
>>
>> diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
>> index 6ad27f4472..7c428bbf50 100644
>> --- a/hw/block/pflash_cfi01.c
>> +++ b/hw/block/pflash_cfi01.c
>> @@ -959,6 +959,11 @@ MemoryRegion *pflash_cfi01_get_memory(PFlashCFI01 *fl)
>>      return &fl->mem;
>>  }
>>  
>> +BlockBackend *pflash_cfi01_get_blk(PFlashCFI01 *fl)
>> +{
>> +    return fl->blk;
>> +}
>> +
>>  static void postload_update_cb(void *opaque, int running, RunState state)
>>  {
>>      PFlashCFI01 *pfl = opaque;
>> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
>> index 3889eccdc3..420a0c5c9e 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,
>> @@ -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 34727c5b1f..98998e1ba8 100644
>> --- a/hw/i386/pc_sysfw.c
>> +++ b/hw/i386/pc_sysfw.c
>> @@ -76,7 +76,58 @@ 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_CFI_PFLASH01);
>> +
>> +    qdev_prop_set_uint64(dev, "sector-length", 4096);
> 
> 4 * KiB
> 
>> +    qdev_prop_set_uint8(dev, "width", 1);
>> +    qdev_prop_set_string(dev, "name", name);
>> +    return CFI_PFLASH01(dev);
>> +}
>> +
>> +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);
> 
> Since this assert is called unconditionally, we can move it to
> pc_system_firmware_init().
> 
>> +
>> +    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);
>> +            /*
>> +             * FIXME 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.
>> +             */

I forgot, apart from this comment and the hack patch you posted after
which I don't understand, for the rest:
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

>> +            pcms->flash[i] = NULL;
>> +        }
>> +    }
>> +}
>>  
>>  /* 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
>> @@ -84,88 +135,78 @@ static void pc_isa_bios_init(MemoryRegion *rom_memory,
>>   * 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))
>> +#define FLASH_SIZE_LIMIT (8 * MiB)
>>  
>> -/* 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.
>> +/*
>> + * 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 multiples of 4KiB, or the total size exceeds
> 
> Not related to this patch:
> 
> Looking at git history the commit introducing this is bd183c79b51 but
> there are no details about why it is required for 'PC'.
> Is it a backend requirement?
> If not, here we shouldn't care about underlying layout. It is the
> responsability of the pflash device to figure this out.
> 
> Somehow related to this patch:
> 
> I understand I could use 1 single flash of 16KiB, or 2 of 4KiB and this
> would work, I'm not sure coz haven't tested, are these real-world use cases?
> Now we have a 8MiB limit. Neither a real-world use case.
> Per commit 637a5acb46b this is OVMF related, also OVMF strictly uses 2
> flashes.
> IMHO there are too many checks around, and we could simplify.
> 
>> + * FLASH_SIZE_LIMIT.
>>   *
>> - * 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.
>> - *
>> - * 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) {
>> +            error_report("system firmware block device %s is empty",
>> +                         blk_name(blk));
>> +            exit(1);
>> +        }
>> +        if (size == 0 || size % 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",
>> +                        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 / 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);
>>  
>> @@ -236,24 +277,63 @@ 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);
>> +    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);
>> +        }
>> +    }
> 
> Or simpler:
> 
>        if (pflash_blk[1] && !pflash_blk[0]) {
>            error_report("pflash1 requires pflash0");
>            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/block/flash.h b/include/hw/block/flash.h
>> index 24b13eb525..91e0f8dd6e 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 */
>> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
>> index 3ff127ebd0..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,8 +280,8 @@ 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_flash_create(PCMachineState *pcms);
>> +void pc_system_firmware_init(PCMachineState *pcms, MemoryRegion *rom_memory);
>>  
>>  /* acpi-build.c */
>>  void pc_madt_cpu_entry(AcpiDeviceIf *adev, int uid,
>>

Re: [Qemu-devel] [RFC PATCH 6/6] pc: Support firmware configuration with -blockdev

[Markus Armbruster]

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

> On 2/25/19 7:37 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.  Except I can't figure out how to destroy the
>> devices correctly.  Marked FIXME.
>> 
>> 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>
>> ---
>>  hw/block/pflash_cfi01.c  |   5 +
>>  hw/i386/pc.c             |   4 +-
>>  hw/i386/pc_sysfw.c       | 230 ++++++++++++++++++++++++++-------------
>>  include/hw/block/flash.h |   1 +
>>  include/hw/i386/pc.h     |   6 +-
>>  5 files changed, 168 insertions(+), 78 deletions(-)
>> 
>> diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
>> index 6ad27f4472..7c428bbf50 100644
>> --- a/hw/block/pflash_cfi01.c
>> +++ b/hw/block/pflash_cfi01.c
>> @@ -959,6 +959,11 @@ MemoryRegion *pflash_cfi01_get_memory(PFlashCFI01 *fl)
>>      return &fl->mem;
>>  }
>>  
>> +BlockBackend *pflash_cfi01_get_blk(PFlashCFI01 *fl)
>> +{
>> +    return fl->blk;
>> +}
>> +
>>  static void postload_update_cb(void *opaque, int running, RunState state)
>>  {
>>      PFlashCFI01 *pfl = opaque;
>> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
>> index 3889eccdc3..420a0c5c9e 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,
>> @@ -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 34727c5b1f..98998e1ba8 100644
>> --- a/hw/i386/pc_sysfw.c
>> +++ b/hw/i386/pc_sysfw.c
>> @@ -76,7 +76,58 @@ 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_CFI_PFLASH01);
>> +
>> +    qdev_prop_set_uint64(dev, "sector-length", 4096);
>
> 4 * KiB

Feels like a wash to me.  We use 4096 far more often than 4 * KiB.
Anyone else got a preference?

>> +    qdev_prop_set_uint8(dev, "width", 1);
>> +    qdev_prop_set_string(dev, "name", name);
>> +    return CFI_PFLASH01(dev);
>> +}
>> +
>> +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);
>
> Since this assert is called unconditionally, we can move it to
> pc_system_firmware_init().

pc_system_firmware_init() populates pcms->flash[] when ->pci_enabled.  I
systematically assert ->pci_enabled before using pcms->flash[].  The
assertion does double-duty as documentation.

>> +
>> +    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);
>> +            /*
>> +             * FIXME 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.
>> +             */
>> +            pcms->flash[i] = NULL;
>> +        }
>> +    }
>> +}
>>  
>>  /* 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
>> @@ -84,88 +135,78 @@ static void pc_isa_bios_init(MemoryRegion *rom_memory,
>>   * 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))
>> +#define FLASH_SIZE_LIMIT (8 * MiB)
>>  
>> -/* 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.
>> +/*
>> + * 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 multiples of 4KiB, or the total size exceeds

Note to self: non-zero multiple

> Not related to this patch:
>
> Looking at git history the commit introducing this is bd183c79b51 but
> there are no details about why it is required for 'PC'.

By "this" you mean "size must be a multiple of 4KiB"?

> Is it a backend requirement?
> If not, here we shouldn't care about underlying layout. It is the
> responsability of the pflash device to figure this out.
>
> Somehow related to this patch:
>
> I understand I could use 1 single flash of 16KiB, or 2 of 4KiB and this
> would work, I'm not sure coz haven't tested, are these real-world use cases?
> Now we have a 8MiB limit. Neither a real-world use case.
> Per commit 637a5acb46b this is OVMF related, also OVMF strictly uses 2
> flashes.

Actual use cases:

* Configuring flash memory backed by a firmware image

  Initial OVMF support, looks like

    -drive if=pflash,format=raw,file=/where/ever/OVMF.fd

* Configuring flash memory backed by two firmware images, one read-only,
  and one read/write

  Current OVMF support, looks like

    -drive if=pflash,format=raw,readonly,file=/usr/share/OVMF/OVMF_CODE.fd
    -drive if=pflash,format=raw,file=/where/ever/OVMF_VARS.fd

  Below the hood, this creates two separate flash chips, because we
  couldn't be bothered to implement sector protection.  I hate that.

  I figure this use case requires "no gap between the two flash chips".

I'm not aware of any other use case with PC machines.

Limiting total flash size is a good idea, because without it, users
could configure flash that overlaps other stuff, which can only end in
tears.

Note that the code can handle any number of units up to a limit set at
compile time.  That limit is #define FLASH_MAP_UNIT_MAX 2.

> IMHO there are too many checks around, and we could simplify.

I agree this magic board code is more general than necessary.  I
actually argued for picking *one* size of flash and be done with it, or
maybe a few sizes, if firmware developers really, really wants that:

    The size of the flash chip is a property of the machine.  It is *fixed*.
    Using whatever size the image has is sloppy modelling.

    A machine may come in minor variations that aren't worth their own
    machine type.  One such variation could be a different flash chip size.
    Using the image size to select one from the set of fixed sizes is
    tolerable.

    Subject: Re: [PATCH v2] hw/block: report when pflash backing file isn't aligned
    Message-ID: <87lg2hujld.fsf@dusky.pond.sub.org>
    Date: Fri, 15 Feb 2019 17:01:18 +0100
    https://lists.nongnu.org/archive/html/qemu-devel/2019-02/msg04268.html

But László objected.

I still think offering powers of two between 64KiB and 8MiB (or whatever
upper limit we can agree upon) would satisfy all practical needs and
then some.

>> + * FLASH_SIZE_LIMIT.
>>   *
>> - * 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.
>> - *
>> - * 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) {
>> +            error_report("system firmware block device %s is empty",
>> +                         blk_name(blk));
>> +            exit(1);
>> +        }
>> +        if (size == 0 || size % 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",
>> +                        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 / 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);
>>  
>> @@ -236,24 +277,63 @@ 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);
>> +    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);
>> +        }
>> +    }
>
> Or simpler:
>
>        if (pflash_blk[1] && !pflash_blk[0]) {
>            error_report("pflash1 requires pflash0");
>            exit(1);
>        }

This assumes ARRAY_SIZE(pcms->flash) == 2.  Valid, but the code avoids
the assumption everywhere else.  Similar to how the old code should work
for any FLASH_MAP_UNIT_MAX, even though it's actually 2.

If we decide two images are going to be enough for everybody, we can
simplify some.  There's more unused flexibility to discard, if we want
to.

>> +
>> +    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/block/flash.h b/include/hw/block/flash.h
>> index 24b13eb525..91e0f8dd6e 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 */
>> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
>> index 3ff127ebd0..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,8 +280,8 @@ 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_flash_create(PCMachineState *pcms);
>> +void pc_system_firmware_init(PCMachineState *pcms, MemoryRegion *rom_memory);
>>  
>>  /* acpi-build.c */
>>  void pc_madt_cpu_entry(AcpiDeviceIf *adev, int uid,
>>