[Qemu-devel] [PATCH v2 0/5] Compat props bug fixes

[Qemu-devel] [PATCH v2 0/5] Compat props bug fixes

[Markus Armbruster]

v2:
* PATCH 1-4: Complete rewrite of fatally flawed PATCH v1 1/2 [Igor]

Markus Armbruster (5):
  Revert "vl: Fix to create migration object before block backends
    again"
  Revert "migration: move only_migratable to MigrationState"
  migration: Support adding migration blockers earlier
  vl: Document dependencies hiding in global and compat props
  accel: Unbreak accelerator fallback

 accel/accel.c           |  3 ++-
 include/sysemu/sysemu.h |  1 +
 migration/migration.c   | 11 +++++++----
 migration/migration.h   |  3 ---
 migration/savevm.c      |  2 +-
 vl.c                    | 44 +++++++++++++++++++++++++++--------------
 6 files changed, 40 insertions(+), 24 deletions(-)

-- 
2.17.2

[Qemu-devel] [PATCH v2 1/5] Revert "vl: Fix to create migration object before block backends again"

[Markus Armbruster]

This reverts commit e60483f2f8498ae08ae79ca4c6fb03a3317f5e1e.

Recent commit cda4aa9a5a0 moved block backend creation before machine
property evaluation.  This broke block backends registering migration
blockers.  Commit e60483f2f84 fixed it by moving migration object
creation before block backend creation.  This broke migration with
Xen.  Turns out we need to configure the accelerator before we create
the migration object so that Xen's accelerator compat properties get
applied.  Revert the flawed commit.  This fixes the Xen regression,
but brings back the block backend regression.  The next commits will
fix it again.

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

diff --git a/vl.c b/vl.c
index d61d5604e5..c1d5484e12 100644
--- a/vl.c
+++ b/vl.c
@@ -4276,17 +4276,10 @@ int main(int argc, char **argv, char **envp)
         exit(0);
     }
 
-    /*
-     * Migration object can only be created after global properties
-     * are applied correctly.
-     */
-    migration_object_init();
-
     /*
      * Note: we need to create block backends before
      * machine_set_property(), so machine properties can refer to
-     * them, and after migration_object_init(), so we can create
-     * migration blockers.
+     * them.
      */
     configure_blockdev(&bdo_queue, machine_class, snapshot);
 
@@ -4304,6 +4297,12 @@ int main(int argc, char **argv, char **envp)
                      machine_class->name, machine_class->deprecation_reason);
     }
 
+    /*
+     * Migration object can only be created after global properties
+     * are applied correctly.
+     */
+    migration_object_init();
+
     if (qtest_chrdev) {
         qtest_init(qtest_chrdev, qtest_log, &error_fatal);
     }
-- 
2.17.2

Re: [Qemu-devel] [PATCH v2 1/5] Revert "vl: Fix to create migration object before block backends again"

[Igor Mammedov]

On Mon,  1 Apr 2019 11:08:23 +0200
Markus Armbruster <armbru@redhat.com> wrote:

> This reverts commit e60483f2f8498ae08ae79ca4c6fb03a3317f5e1e.
> 
> Recent commit cda4aa9a5a0 moved block backend creation before machine
> property evaluation.

Side note:
I've also looked at
 "[PATCH v3 11/12] pc: Support firmware configuration  with -blockdev"
that caused movement and have a similar issue with hostmem backends
where I wanted to set machine.inital-ram-memdev property pointing to
backend. But hostmem backends are created, since they have to be created
after machine_set_properties/configure_accelerator due to dependencies.

After some playing around with link property on machine and trying
to untangle dependency chain I gave up (doesn't look feasible).
Idea with aliasing like with blockdev won't work either due to
dependencies (also it looks to me to be quite complicated one).
So I've resorted to "usual" machine practice 'machine.set_string_property'
and deal with 'id' referred by it later at machine_run_board_init() time to
instantiate initial RAM, when we already created all backends.

> This broke block backends registering migration
> blockers.  Commit e60483f2f84 fixed it by moving migration object
> creation before block backend creation.  This broke migration with
> Xen.  Turns out we need to configure the accelerator before we create
> the migration object so that Xen's accelerator compat properties get
> applied.  Revert the flawed commit.  This fixes the Xen regression,
> but brings back the block backend regression.  The next commits will
> fix it again.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

Reviewed-by: Igor Mammedov <imammedo@redhat.com>

> ---
>  vl.c | 15 +++++++--------
>  1 file changed, 7 insertions(+), 8 deletions(-)
> 
> diff --git a/vl.c b/vl.c
> index d61d5604e5..c1d5484e12 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -4276,17 +4276,10 @@ int main(int argc, char **argv, char **envp)
>          exit(0);
>      }
>  
> -    /*
> -     * Migration object can only be created after global properties
> -     * are applied correctly.
> -     */
> -    migration_object_init();
> -
>      /*
>       * Note: we need to create block backends before
>       * machine_set_property(), so machine properties can refer to
> -     * them, and after migration_object_init(), so we can create
> -     * migration blockers.
> +     * them.
>       */
>      configure_blockdev(&bdo_queue, machine_class, snapshot);
>  
> @@ -4304,6 +4297,12 @@ int main(int argc, char **argv, char **envp)
>                       machine_class->name, machine_class->deprecation_reason);
>      }
>  
> +    /*
> +     * Migration object can only be created after global properties
> +     * are applied correctly.
> +     */
> +    migration_object_init();
> +
>      if (qtest_chrdev) {
>          qtest_init(qtest_chrdev, qtest_log, &error_fatal);
>      }

Re: [Qemu-devel] [PATCH v2 1/5] Revert "vl: Fix to create migration object before block backends again"

[Markus Armbruster]

Igor Mammedov <imammedo@redhat.com> writes:

> On Mon,  1 Apr 2019 11:08:23 +0200
> Markus Armbruster <armbru@redhat.com> wrote:
>
>> This reverts commit e60483f2f8498ae08ae79ca4c6fb03a3317f5e1e.
>> 
>> Recent commit cda4aa9a5a0 moved block backend creation before machine
>> property evaluation.
>
> Side note:
> I've also looked at
>  "[PATCH v3 11/12] pc: Support firmware configuration  with -blockdev"
> that caused movement and have a similar issue with hostmem backends
> where I wanted to set machine.inital-ram-memdev property pointing to
> backend. But hostmem backends are created, since they have to be created
> after machine_set_properties/configure_accelerator due to dependencies.
>
> After some playing around with link property on machine and trying
> to untangle dependency chain I gave up (doesn't look feasible).
> Idea with aliasing like with blockdev won't work either due to
> dependencies (also it looks to me to be quite complicated one).
> So I've resorted to "usual" machine practice 'machine.set_string_property'
> and deal with 'id' referred by it later at machine_run_board_init() time to
> instantiate initial RAM, when we already created all backends.

That's... horrible.  How many of these monstrosities have crept in?

Let's discuss once the RC circus has moved to the next town.

>> This broke block backends registering migration
>> blockers.  Commit e60483f2f84 fixed it by moving migration object
>> creation before block backend creation.  This broke migration with
>> Xen.  Turns out we need to configure the accelerator before we create
>> the migration object so that Xen's accelerator compat properties get
>> applied.  Revert the flawed commit.  This fixes the Xen regression,
>> but brings back the block backend regression.  The next commits will
>> fix it again.
>> 
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>
> Reviewed-by: Igor Mammedov <imammedo@redhat.com>

Thanks!

[Qemu-devel] [PATCH v2 2/5] Revert "migration: move only_migratable to MigrationState"

[Markus Armbruster]

This reverts commit 3df663e575f1876d7f3bc684f80e72fca0703d39.
This reverts commit b605c47b57b58e61a901a50a0762dccf43d94783.

Command line option --only-migratable is for disallowing any
configuration that can block migration.

Initially, --only-migratable set global variable @only_migratable.

Commit 3df663e575 "migration: move only_migratable to MigrationState"
replaced it by MigrationState member @only_migratable.  That was a
mistake.

First, it doesn't make sense on the design level.  MigrationState
captures the state of an individual migration, but --only-migratable
isn't a property of an individual migration, it's a restriction on
QEMU configuration.  With fault tolerance, we could have several
migrations at once.  --only-migratable would certainly protect all of
them.  Storing it in MigrationState feels inappropriate.

Second, it contributes to a dependency cycle that manifests itself as
a bug now.

Putting @only_migratable into MigrationState means its available only
after migration_object_init().

We can't set it before migration_object_init(), so we delay setting it
with a global property (this is fixup commit b605c47b57 "migration:
fix handling for --only-migratable").

We can't get it before migration_object_init(), so anything that uses
it can only run afterwards.

Since migrate_add_blocker() needs to obey --only-migratable, any code
adding migration blockers can run only afterwards.  This contributes
to the following dependency cycle:

* configure_blockdev() must run before machine_set_property()
  so machine properties can refer to block backends

* machine_set_property() before configure_accelerator()
  so machine properties like kvm-irqchip get applied

* configure_accelerator() before migration_object_init()
  so that Xen's accelerator compat properties get applied.

* migration_object_init() before configure_blockdev()
  so configure_blockdev() can add migration blockers

The cycle was closed when recent commit cda4aa9a5a0 "Create block
backends before setting machine properties" added the first
dependency, and satisfied it by violating the last one.  Broke block
backends that add migration blockers.

Moving @only_migratable into MigrationState was a mistake.  Revert it.

This doesn't quite break the "migration_object_init() before
configure_blockdev() dependency, since migrate_add_blocker() still has
another dependency on migration_object_init().  To be addressed the
next commit

Conflicts:
	include/migration/misc.h
	migration/migration.c
	migration/migration.h
	vl.c

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 include/sysemu/sysemu.h | 1 +
 migration/migration.c   | 5 ++---
 migration/migration.h   | 3 ---
 migration/savevm.c      | 2 +-
 vl.c                    | 9 ++-------
 5 files changed, 6 insertions(+), 14 deletions(-)

diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
index 6065d9e420..5f133cae83 100644
--- a/include/sysemu/sysemu.h
+++ b/include/sysemu/sysemu.h
@@ -14,6 +14,7 @@
 /* vl.c */
 
 extern const char *bios_name;
+extern int only_migratable;
 extern const char *qemu_name;
 extern QemuUUID qemu_uuid;
 extern bool qemu_uuid_set;
diff --git a/migration/migration.c b/migration/migration.c
index 69f75124c9..f6076e5295 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -1707,7 +1707,7 @@ static GSList *migration_blockers;
 
 int migrate_add_blocker(Error *reason, Error **errp)
 {
-    if (migrate_get_current()->only_migratable) {
+    if (only_migratable) {
         error_propagate_prepend(errp, error_copy(reason),
                                 "disallowing migration blocker "
                                 "(--only_migratable) for: ");
@@ -3337,7 +3337,7 @@ void migration_global_dump(Monitor *mon)
     monitor_printf(mon, "store-global-state: %s\n",
                    ms->store_global_state ? "on" : "off");
     monitor_printf(mon, "only-migratable: %s\n",
-                   ms->only_migratable ? "on" : "off");
+                   only_migratable ? "on" : "off");
     monitor_printf(mon, "send-configuration: %s\n",
                    ms->send_configuration ? "on" : "off");
     monitor_printf(mon, "send-section-footer: %s\n",
@@ -3352,7 +3352,6 @@ void migration_global_dump(Monitor *mon)
 static Property migration_properties[] = {
     DEFINE_PROP_BOOL("store-global-state", MigrationState,
                      store_global_state, true),
-    DEFINE_PROP_BOOL("only-migratable", MigrationState, only_migratable, false),
     DEFINE_PROP_BOOL("send-configuration", MigrationState,
                      send_configuration, true),
     DEFINE_PROP_BOOL("send-section-footer", MigrationState,
diff --git a/migration/migration.h b/migration/migration.h
index 0f986935e1..438f17edad 100644
--- a/migration/migration.h
+++ b/migration/migration.h
@@ -219,9 +219,6 @@ struct MigrationState
      */
     bool store_global_state;
 
-    /* Whether the VM is only allowing for migratable devices */
-    bool only_migratable;
-
     /* Whether we send QEMU_VM_CONFIGURATION during migration */
     bool send_configuration;
     /* Whether we send section footer during migration */
diff --git a/migration/savevm.c b/migration/savevm.c
index 1415001d1c..34bcad3807 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -2844,7 +2844,7 @@ void vmstate_register_ram_global(MemoryRegion *mr)
 bool vmstate_check_only_migratable(const VMStateDescription *vmsd)
 {
     /* check needed if --only-migratable is specified */
-    if (!migrate_get_current()->only_migratable) {
+    if (!only_migratable) {
         return true;
     }
 
diff --git a/vl.c b/vl.c
index c1d5484e12..e4d7ad6b85 100644
--- a/vl.c
+++ b/vl.c
@@ -185,6 +185,7 @@ const char *prom_envs[MAX_PROM_ENVS];
 int boot_menu;
 bool boot_strict;
 uint8_t *boot_splash_filedata;
+int only_migratable; /* turn it off unless user states otherwise */
 bool wakeup_suspend_enabled;
 
 int icount_align_option;
@@ -3799,13 +3800,7 @@ int main(int argc, char **argv, char **envp)
                 incoming = optarg;
                 break;
             case QEMU_OPTION_only_migratable:
-                /*
-                 * TODO: we can remove this option one day, and we
-                 * should all use:
-                 *
-                 * "-global migration.only-migratable=true"
-                 */
-                qemu_global_option("migration.only-migratable=true");
+                only_migratable = 1;
                 break;
             case QEMU_OPTION_nodefaults:
                 has_defaults = 0;
-- 
2.17.2

Re: [Qemu-devel] [PATCH v2 2/5] Revert "migration: move only_migratable to MigrationState"

[Dr. David Alan Gilbert]

* Markus Armbruster (armbru@redhat.com) wrote:
> This reverts commit 3df663e575f1876d7f3bc684f80e72fca0703d39.
> This reverts commit b605c47b57b58e61a901a50a0762dccf43d94783.
> 
> Command line option --only-migratable is for disallowing any
> configuration that can block migration.
> 
> Initially, --only-migratable set global variable @only_migratable.
> 
> Commit 3df663e575 "migration: move only_migratable to MigrationState"
> replaced it by MigrationState member @only_migratable.  That was a
> mistake.
> 
> First, it doesn't make sense on the design level.  MigrationState
> captures the state of an individual migration, but --only-migratable
> isn't a property of an individual migration, it's a restriction on
> QEMU configuration.  With fault tolerance, we could have several
> migrations at once.  --only-migratable would certainly protect all of
> them.  Storing it in MigrationState feels inappropriate.
> 
> Second, it contributes to a dependency cycle that manifests itself as
> a bug now.
> 
> Putting @only_migratable into MigrationState means its available only
> after migration_object_init().
> 
> We can't set it before migration_object_init(), so we delay setting it
> with a global property (this is fixup commit b605c47b57 "migration:
> fix handling for --only-migratable").
> 
> We can't get it before migration_object_init(), so anything that uses
> it can only run afterwards.
> 
> Since migrate_add_blocker() needs to obey --only-migratable, any code
> adding migration blockers can run only afterwards.  This contributes
> to the following dependency cycle:
> 
> * configure_blockdev() must run before machine_set_property()
>   so machine properties can refer to block backends
> 
> * machine_set_property() before configure_accelerator()
>   so machine properties like kvm-irqchip get applied
> 
> * configure_accelerator() before migration_object_init()
>   so that Xen's accelerator compat properties get applied.
> 
> * migration_object_init() before configure_blockdev()
>   so configure_blockdev() can add migration blockers
> 
> The cycle was closed when recent commit cda4aa9a5a0 "Create block
> backends before setting machine properties" added the first
> dependency, and satisfied it by violating the last one.  Broke block
> backends that add migration blockers.
> 
> Moving @only_migratable into MigrationState was a mistake.  Revert it.
> 
> This doesn't quite break the "migration_object_init() before
> configure_blockdev() dependency, since migrate_add_blocker() still has
> another dependency on migration_object_init().  To be addressed the
> next commit
> 
> Conflicts:
> 	include/migration/misc.h
> 	migration/migration.c
> 	migration/migration.h
> 	vl.c
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  include/sysemu/sysemu.h | 1 +
>  migration/migration.c   | 5 ++---
>  migration/migration.h   | 3 ---
>  migration/savevm.c      | 2 +-
>  vl.c                    | 9 ++-------
>  5 files changed, 6 insertions(+), 14 deletions(-)
> 
> diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
> index 6065d9e420..5f133cae83 100644
> --- a/include/sysemu/sysemu.h
> +++ b/include/sysemu/sysemu.h
> @@ -14,6 +14,7 @@
>  /* vl.c */
>  
>  extern const char *bios_name;
> +extern int only_migratable;
>  extern const char *qemu_name;
>  extern QemuUUID qemu_uuid;
>  extern bool qemu_uuid_set;
> diff --git a/migration/migration.c b/migration/migration.c
> index 69f75124c9..f6076e5295 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -1707,7 +1707,7 @@ static GSList *migration_blockers;
>  
>  int migrate_add_blocker(Error *reason, Error **errp)
>  {
> -    if (migrate_get_current()->only_migratable) {
> +    if (only_migratable) {
>          error_propagate_prepend(errp, error_copy(reason),
>                                  "disallowing migration blocker "
>                                  "(--only_migratable) for: ");
> @@ -3337,7 +3337,7 @@ void migration_global_dump(Monitor *mon)
>      monitor_printf(mon, "store-global-state: %s\n",
>                     ms->store_global_state ? "on" : "off");
>      monitor_printf(mon, "only-migratable: %s\n",
> -                   ms->only_migratable ? "on" : "off");
> +                   only_migratable ? "on" : "off");
>      monitor_printf(mon, "send-configuration: %s\n",
>                     ms->send_configuration ? "on" : "off");
>      monitor_printf(mon, "send-section-footer: %s\n",
> @@ -3352,7 +3352,6 @@ void migration_global_dump(Monitor *mon)
>  static Property migration_properties[] = {
>      DEFINE_PROP_BOOL("store-global-state", MigrationState,
>                       store_global_state, true),
> -    DEFINE_PROP_BOOL("only-migratable", MigrationState, only_migratable, false),

So I'm generally OK at this patch, but I'm worried whether this
is API?

Dave

>      DEFINE_PROP_BOOL("send-configuration", MigrationState,
>                       send_configuration, true),
>      DEFINE_PROP_BOOL("send-section-footer", MigrationState,
> diff --git a/migration/migration.h b/migration/migration.h
> index 0f986935e1..438f17edad 100644
> --- a/migration/migration.h
> +++ b/migration/migration.h
> @@ -219,9 +219,6 @@ struct MigrationState
>       */
>      bool store_global_state;
>  
> -    /* Whether the VM is only allowing for migratable devices */
> -    bool only_migratable;
> -
>      /* Whether we send QEMU_VM_CONFIGURATION during migration */
>      bool send_configuration;
>      /* Whether we send section footer during migration */
> diff --git a/migration/savevm.c b/migration/savevm.c
> index 1415001d1c..34bcad3807 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -2844,7 +2844,7 @@ void vmstate_register_ram_global(MemoryRegion *mr)
>  bool vmstate_check_only_migratable(const VMStateDescription *vmsd)
>  {
>      /* check needed if --only-migratable is specified */
> -    if (!migrate_get_current()->only_migratable) {
> +    if (!only_migratable) {
>          return true;
>      }
>  
> diff --git a/vl.c b/vl.c
> index c1d5484e12..e4d7ad6b85 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -185,6 +185,7 @@ const char *prom_envs[MAX_PROM_ENVS];
>  int boot_menu;
>  bool boot_strict;
>  uint8_t *boot_splash_filedata;
> +int only_migratable; /* turn it off unless user states otherwise */
>  bool wakeup_suspend_enabled;
>  
>  int icount_align_option;
> @@ -3799,13 +3800,7 @@ int main(int argc, char **argv, char **envp)
>                  incoming = optarg;
>                  break;
>              case QEMU_OPTION_only_migratable:
> -                /*
> -                 * TODO: we can remove this option one day, and we
> -                 * should all use:
> -                 *
> -                 * "-global migration.only-migratable=true"
> -                 */
> -                qemu_global_option("migration.only-migratable=true");
> +                only_migratable = 1;
>                  break;
>              case QEMU_OPTION_nodefaults:
>                  has_defaults = 0;
> -- 
> 2.17.2
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v2 2/5] Revert "migration: move only_migratable to MigrationState"

[Daniel P. Berrangé]

On Mon, Apr 01, 2019 at 10:27:49AM +0100, Dr. David Alan Gilbert wrote:
> * Markus Armbruster (armbru@redhat.com) wrote:
> > This reverts commit 3df663e575f1876d7f3bc684f80e72fca0703d39.
> > This reverts commit b605c47b57b58e61a901a50a0762dccf43d94783.
> > 
> > Command line option --only-migratable is for disallowing any
> > configuration that can block migration.
> > 
> > Initially, --only-migratable set global variable @only_migratable.
> > 
> > Commit 3df663e575 "migration: move only_migratable to MigrationState"
> > replaced it by MigrationState member @only_migratable.  That was a
> > mistake.
> > 
> > First, it doesn't make sense on the design level.  MigrationState
> > captures the state of an individual migration, but --only-migratable
> > isn't a property of an individual migration, it's a restriction on
> > QEMU configuration.  With fault tolerance, we could have several
> > migrations at once.  --only-migratable would certainly protect all of
> > them.  Storing it in MigrationState feels inappropriate.
> > 
> > Second, it contributes to a dependency cycle that manifests itself as
> > a bug now.
> > 
> > Putting @only_migratable into MigrationState means its available only
> > after migration_object_init().
> > 
> > We can't set it before migration_object_init(), so we delay setting it
> > with a global property (this is fixup commit b605c47b57 "migration:
> > fix handling for --only-migratable").
> > 
> > We can't get it before migration_object_init(), so anything that uses
> > it can only run afterwards.
> > 
> > Since migrate_add_blocker() needs to obey --only-migratable, any code
> > adding migration blockers can run only afterwards.  This contributes
> > to the following dependency cycle:
> > 
> > * configure_blockdev() must run before machine_set_property()
> >   so machine properties can refer to block backends
> > 
> > * machine_set_property() before configure_accelerator()
> >   so machine properties like kvm-irqchip get applied
> > 
> > * configure_accelerator() before migration_object_init()
> >   so that Xen's accelerator compat properties get applied.
> > 
> > * migration_object_init() before configure_blockdev()
> >   so configure_blockdev() can add migration blockers
> > 
> > The cycle was closed when recent commit cda4aa9a5a0 "Create block
> > backends before setting machine properties" added the first
> > dependency, and satisfied it by violating the last one.  Broke block
> > backends that add migration blockers.
> > 
> > Moving @only_migratable into MigrationState was a mistake.  Revert it.
> > 
> > This doesn't quite break the "migration_object_init() before
> > configure_blockdev() dependency, since migrate_add_blocker() still has
> > another dependency on migration_object_init().  To be addressed the
> > next commit
> > 
> > Conflicts:
> > 	include/migration/misc.h
> > 	migration/migration.c
> > 	migration/migration.h
> > 	vl.c
> > 
> > Signed-off-by: Markus Armbruster <armbru@redhat.com>
> > ---
> >  include/sysemu/sysemu.h | 1 +
> >  migration/migration.c   | 5 ++---
> >  migration/migration.h   | 3 ---
> >  migration/savevm.c      | 2 +-
> >  vl.c                    | 9 ++-------
> >  5 files changed, 6 insertions(+), 14 deletions(-)
> > 
> > diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
> > index 6065d9e420..5f133cae83 100644
> > --- a/include/sysemu/sysemu.h
> > +++ b/include/sysemu/sysemu.h
> > @@ -14,6 +14,7 @@
> >  /* vl.c */
> >  
> >  extern const char *bios_name;
> > +extern int only_migratable;
> >  extern const char *qemu_name;
> >  extern QemuUUID qemu_uuid;
> >  extern bool qemu_uuid_set;
> > diff --git a/migration/migration.c b/migration/migration.c
> > index 69f75124c9..f6076e5295 100644
> > --- a/migration/migration.c
> > +++ b/migration/migration.c
> > @@ -1707,7 +1707,7 @@ static GSList *migration_blockers;
> >  
> >  int migrate_add_blocker(Error *reason, Error **errp)
> >  {
> > -    if (migrate_get_current()->only_migratable) {
> > +    if (only_migratable) {
> >          error_propagate_prepend(errp, error_copy(reason),
> >                                  "disallowing migration blocker "
> >                                  "(--only_migratable) for: ");
> > @@ -3337,7 +3337,7 @@ void migration_global_dump(Monitor *mon)
> >      monitor_printf(mon, "store-global-state: %s\n",
> >                     ms->store_global_state ? "on" : "off");
> >      monitor_printf(mon, "only-migratable: %s\n",
> > -                   ms->only_migratable ? "on" : "off");
> > +                   only_migratable ? "on" : "off");
> >      monitor_printf(mon, "send-configuration: %s\n",
> >                     ms->send_configuration ? "on" : "off");
> >      monitor_printf(mon, "send-section-footer: %s\n",
> > @@ -3352,7 +3352,6 @@ void migration_global_dump(Monitor *mon)
> >  static Property migration_properties[] = {
> >      DEFINE_PROP_BOOL("store-global-state", MigrationState,
> >                       store_global_state, true),
> > -    DEFINE_PROP_BOOL("only-migratable", MigrationState, only_migratable, false),
> 
> So I'm generally OK at this patch, but I'm worried whether this
> is API?

IIUC, old code can use either

  -global migration.only-migratable=true
  -only-migratable

After this patch only

  -only-migratable

is valid. So from that POV it is an API break. 

>From libvirt's POV this is a non-issue as we never use either of these options.
We did have a BZ filed to support it, but never did.

To avoid the API break I guess there would been to be a special case early
loop iterating over argv[] looking for '-global migration.only-migratable=true',
making that set the static 'bool only_migratable', instead of letting it get
handled by the object infra.


> >      DEFINE_PROP_BOOL("send-configuration", MigrationState,
> >                       send_configuration, true),
> >      DEFINE_PROP_BOOL("send-section-footer", MigrationState,
> > diff --git a/migration/migration.h b/migration/migration.h
> > index 0f986935e1..438f17edad 100644
> > --- a/migration/migration.h
> > +++ b/migration/migration.h
> > @@ -219,9 +219,6 @@ struct MigrationState
> >       */
> >      bool store_global_state;
> >  
> > -    /* Whether the VM is only allowing for migratable devices */
> > -    bool only_migratable;
> > -
> >      /* Whether we send QEMU_VM_CONFIGURATION during migration */
> >      bool send_configuration;
> >      /* Whether we send section footer during migration */
> > diff --git a/migration/savevm.c b/migration/savevm.c
> > index 1415001d1c..34bcad3807 100644
> > --- a/migration/savevm.c
> > +++ b/migration/savevm.c
> > @@ -2844,7 +2844,7 @@ void vmstate_register_ram_global(MemoryRegion *mr)
> >  bool vmstate_check_only_migratable(const VMStateDescription *vmsd)
> >  {
> >      /* check needed if --only-migratable is specified */
> > -    if (!migrate_get_current()->only_migratable) {
> > +    if (!only_migratable) {
> >          return true;
> >      }
> >  
> > diff --git a/vl.c b/vl.c
> > index c1d5484e12..e4d7ad6b85 100644
> > --- a/vl.c
> > +++ b/vl.c
> > @@ -185,6 +185,7 @@ const char *prom_envs[MAX_PROM_ENVS];
> >  int boot_menu;
> >  bool boot_strict;
> >  uint8_t *boot_splash_filedata;
> > +int only_migratable; /* turn it off unless user states otherwise */
> >  bool wakeup_suspend_enabled;
> >  
> >  int icount_align_option;
> > @@ -3799,13 +3800,7 @@ int main(int argc, char **argv, char **envp)
> >                  incoming = optarg;
> >                  break;
> >              case QEMU_OPTION_only_migratable:
> > -                /*
> > -                 * TODO: we can remove this option one day, and we
> > -                 * should all use:
> > -                 *
> > -                 * "-global migration.only-migratable=true"
> > -                 */
> > -                qemu_global_option("migration.only-migratable=true");
> > +                only_migratable = 1;
> >                  break;
> >              case QEMU_OPTION_nodefaults:
> >                  has_defaults = 0;
> > -- 
> > 2.17.2
> > 
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
> 

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [PATCH v2 2/5] Revert "migration: move only_migratable to MigrationState"

[Markus Armbruster]

Daniel P. Berrangé <berrange@redhat.com> writes:

> On Mon, Apr 01, 2019 at 10:27:49AM +0100, Dr. David Alan Gilbert wrote:
>> * Markus Armbruster (armbru@redhat.com) wrote:
>> > This reverts commit 3df663e575f1876d7f3bc684f80e72fca0703d39.
>> > This reverts commit b605c47b57b58e61a901a50a0762dccf43d94783.
>> > 
>> > Command line option --only-migratable is for disallowing any
>> > configuration that can block migration.
>> > 
>> > Initially, --only-migratable set global variable @only_migratable.
>> > 
>> > Commit 3df663e575 "migration: move only_migratable to MigrationState"
>> > replaced it by MigrationState member @only_migratable.  That was a
>> > mistake.
>> > 
>> > First, it doesn't make sense on the design level.  MigrationState
>> > captures the state of an individual migration, but --only-migratable
>> > isn't a property of an individual migration, it's a restriction on
>> > QEMU configuration.  With fault tolerance, we could have several
>> > migrations at once.  --only-migratable would certainly protect all of
>> > them.  Storing it in MigrationState feels inappropriate.
>> > 
>> > Second, it contributes to a dependency cycle that manifests itself as
>> > a bug now.
>> > 
>> > Putting @only_migratable into MigrationState means its available only
>> > after migration_object_init().
>> > 
>> > We can't set it before migration_object_init(), so we delay setting it
>> > with a global property (this is fixup commit b605c47b57 "migration:
>> > fix handling for --only-migratable").
>> > 
>> > We can't get it before migration_object_init(), so anything that uses
>> > it can only run afterwards.
>> > 
>> > Since migrate_add_blocker() needs to obey --only-migratable, any code
>> > adding migration blockers can run only afterwards.  This contributes
>> > to the following dependency cycle:
>> > 
>> > * configure_blockdev() must run before machine_set_property()
>> >   so machine properties can refer to block backends
>> > 
>> > * machine_set_property() before configure_accelerator()
>> >   so machine properties like kvm-irqchip get applied
>> > 
>> > * configure_accelerator() before migration_object_init()
>> >   so that Xen's accelerator compat properties get applied.
>> > 
>> > * migration_object_init() before configure_blockdev()
>> >   so configure_blockdev() can add migration blockers
>> > 
>> > The cycle was closed when recent commit cda4aa9a5a0 "Create block
>> > backends before setting machine properties" added the first
>> > dependency, and satisfied it by violating the last one.  Broke block
>> > backends that add migration blockers.
>> > 
>> > Moving @only_migratable into MigrationState was a mistake.  Revert it.
>> > 
>> > This doesn't quite break the "migration_object_init() before
>> > configure_blockdev() dependency, since migrate_add_blocker() still has
>> > another dependency on migration_object_init().  To be addressed the
>> > next commit
>> > 
>> > Conflicts:
>> > 	include/migration/misc.h
>> > 	migration/migration.c
>> > 	migration/migration.h
>> > 	vl.c
>> > 
>> > Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> > ---
>> >  include/sysemu/sysemu.h | 1 +
>> >  migration/migration.c   | 5 ++---
>> >  migration/migration.h   | 3 ---
>> >  migration/savevm.c      | 2 +-
>> >  vl.c                    | 9 ++-------
>> >  5 files changed, 6 insertions(+), 14 deletions(-)
>> > 
>> > diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
>> > index 6065d9e420..5f133cae83 100644
>> > --- a/include/sysemu/sysemu.h
>> > +++ b/include/sysemu/sysemu.h
>> > @@ -14,6 +14,7 @@
>> >  /* vl.c */
>> >  
>> >  extern const char *bios_name;
>> > +extern int only_migratable;
>> >  extern const char *qemu_name;
>> >  extern QemuUUID qemu_uuid;
>> >  extern bool qemu_uuid_set;
>> > diff --git a/migration/migration.c b/migration/migration.c
>> > index 69f75124c9..f6076e5295 100644
>> > --- a/migration/migration.c
>> > +++ b/migration/migration.c
>> > @@ -1707,7 +1707,7 @@ static GSList *migration_blockers;
>> >  
>> >  int migrate_add_blocker(Error *reason, Error **errp)
>> >  {
>> > -    if (migrate_get_current()->only_migratable) {
>> > +    if (only_migratable) {
>> >          error_propagate_prepend(errp, error_copy(reason),
>> >                                  "disallowing migration blocker "
>> >                                  "(--only_migratable) for: ");
>> > @@ -3337,7 +3337,7 @@ void migration_global_dump(Monitor *mon)
>> >      monitor_printf(mon, "store-global-state: %s\n",
>> >                     ms->store_global_state ? "on" : "off");
>> >      monitor_printf(mon, "only-migratable: %s\n",
>> > -                   ms->only_migratable ? "on" : "off");
>> > +                   only_migratable ? "on" : "off");
>> >      monitor_printf(mon, "send-configuration: %s\n",
>> >                     ms->send_configuration ? "on" : "off");
>> >      monitor_printf(mon, "send-section-footer: %s\n",
>> > @@ -3352,7 +3352,6 @@ void migration_global_dump(Monitor *mon)
>> >  static Property migration_properties[] = {
>> >      DEFINE_PROP_BOOL("store-global-state", MigrationState,
>> >                       store_global_state, true),
>> > -    DEFINE_PROP_BOOL("only-migratable", MigrationState, only_migratable, false),
>> 
>> So I'm generally OK at this patch, but I'm worried whether this
>> is API?

Uh, forgot to discuss this in my commit message, sorry!

> IIUC, old code can use either
>
>   -global migration.only-migratable=true
>   -only-migratable
>
> After this patch only
>
>   -only-migratable
>
> is valid. So from that POV it is an API break. 
>
> From libvirt's POV this is a non-issue as we never use either of these options.
> We did have a BZ filed to support it, but never did.
>
> To avoid the API break I guess there would been to be a special case early
> loop iterating over argv[] looking for '-global migration.only-migratable=true',
> making that set the static 'bool only_migratable', instead of letting it get
> handled by the object infra.

Is this is worth the trouble?

-global migration.only-migratable=on is entirely undocumented.  The
property appears in -device migration,help and device-list-properties,
but that's all.

Why would any sane person risk using arcane & undocumented -global
migration.only-migratable=on instead of the obvious & documented
-only-migratable?

[...]

Re: [Qemu-devel] [PATCH v2 2/5] Revert "migration: move only_migratable to MigrationState"

[Daniel P. Berrangé]

On Mon, Apr 01, 2019 at 01:31:47PM +0200, Markus Armbruster wrote:
> Daniel P. Berrangé <berrange@redhat.com> writes:
> 
> > On Mon, Apr 01, 2019 at 10:27:49AM +0100, Dr. David Alan Gilbert wrote:
> >> * Markus Armbruster (armbru@redhat.com) wrote:
> >> > This reverts commit 3df663e575f1876d7f3bc684f80e72fca0703d39.
> >> > This reverts commit b605c47b57b58e61a901a50a0762dccf43d94783.
> >> > 
> >> > Command line option --only-migratable is for disallowing any
> >> > configuration that can block migration.
> >> > 
> >> > Initially, --only-migratable set global variable @only_migratable.
> >> > 
> >> > Commit 3df663e575 "migration: move only_migratable to MigrationState"
> >> > replaced it by MigrationState member @only_migratable.  That was a
> >> > mistake.
> >> > 
> >> > First, it doesn't make sense on the design level.  MigrationState
> >> > captures the state of an individual migration, but --only-migratable
> >> > isn't a property of an individual migration, it's a restriction on
> >> > QEMU configuration.  With fault tolerance, we could have several
> >> > migrations at once.  --only-migratable would certainly protect all of
> >> > them.  Storing it in MigrationState feels inappropriate.
> >> > 
> >> > Second, it contributes to a dependency cycle that manifests itself as
> >> > a bug now.
> >> > 
> >> > Putting @only_migratable into MigrationState means its available only
> >> > after migration_object_init().
> >> > 
> >> > We can't set it before migration_object_init(), so we delay setting it
> >> > with a global property (this is fixup commit b605c47b57 "migration:
> >> > fix handling for --only-migratable").
> >> > 
> >> > We can't get it before migration_object_init(), so anything that uses
> >> > it can only run afterwards.
> >> > 
> >> > Since migrate_add_blocker() needs to obey --only-migratable, any code
> >> > adding migration blockers can run only afterwards.  This contributes
> >> > to the following dependency cycle:
> >> > 
> >> > * configure_blockdev() must run before machine_set_property()
> >> >   so machine properties can refer to block backends
> >> > 
> >> > * machine_set_property() before configure_accelerator()
> >> >   so machine properties like kvm-irqchip get applied
> >> > 
> >> > * configure_accelerator() before migration_object_init()
> >> >   so that Xen's accelerator compat properties get applied.
> >> > 
> >> > * migration_object_init() before configure_blockdev()
> >> >   so configure_blockdev() can add migration blockers
> >> > 
> >> > The cycle was closed when recent commit cda4aa9a5a0 "Create block
> >> > backends before setting machine properties" added the first
> >> > dependency, and satisfied it by violating the last one.  Broke block
> >> > backends that add migration blockers.
> >> > 
> >> > Moving @only_migratable into MigrationState was a mistake.  Revert it.
> >> > 
> >> > This doesn't quite break the "migration_object_init() before
> >> > configure_blockdev() dependency, since migrate_add_blocker() still has
> >> > another dependency on migration_object_init().  To be addressed the
> >> > next commit
> >> > 
> >> > Conflicts:
> >> > 	include/migration/misc.h
> >> > 	migration/migration.c
> >> > 	migration/migration.h
> >> > 	vl.c
> >> > 
> >> > Signed-off-by: Markus Armbruster <armbru@redhat.com>
> >> > ---
> >> >  include/sysemu/sysemu.h | 1 +
> >> >  migration/migration.c   | 5 ++---
> >> >  migration/migration.h   | 3 ---
> >> >  migration/savevm.c      | 2 +-
> >> >  vl.c                    | 9 ++-------
> >> >  5 files changed, 6 insertions(+), 14 deletions(-)
> >> > 
> >> > diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
> >> > index 6065d9e420..5f133cae83 100644
> >> > --- a/include/sysemu/sysemu.h
> >> > +++ b/include/sysemu/sysemu.h
> >> > @@ -14,6 +14,7 @@
> >> >  /* vl.c */
> >> >  
> >> >  extern const char *bios_name;
> >> > +extern int only_migratable;
> >> >  extern const char *qemu_name;
> >> >  extern QemuUUID qemu_uuid;
> >> >  extern bool qemu_uuid_set;
> >> > diff --git a/migration/migration.c b/migration/migration.c
> >> > index 69f75124c9..f6076e5295 100644
> >> > --- a/migration/migration.c
> >> > +++ b/migration/migration.c
> >> > @@ -1707,7 +1707,7 @@ static GSList *migration_blockers;
> >> >  
> >> >  int migrate_add_blocker(Error *reason, Error **errp)
> >> >  {
> >> > -    if (migrate_get_current()->only_migratable) {
> >> > +    if (only_migratable) {
> >> >          error_propagate_prepend(errp, error_copy(reason),
> >> >                                  "disallowing migration blocker "
> >> >                                  "(--only_migratable) for: ");
> >> > @@ -3337,7 +3337,7 @@ void migration_global_dump(Monitor *mon)
> >> >      monitor_printf(mon, "store-global-state: %s\n",
> >> >                     ms->store_global_state ? "on" : "off");
> >> >      monitor_printf(mon, "only-migratable: %s\n",
> >> > -                   ms->only_migratable ? "on" : "off");
> >> > +                   only_migratable ? "on" : "off");
> >> >      monitor_printf(mon, "send-configuration: %s\n",
> >> >                     ms->send_configuration ? "on" : "off");
> >> >      monitor_printf(mon, "send-section-footer: %s\n",
> >> > @@ -3352,7 +3352,6 @@ void migration_global_dump(Monitor *mon)
> >> >  static Property migration_properties[] = {
> >> >      DEFINE_PROP_BOOL("store-global-state", MigrationState,
> >> >                       store_global_state, true),
> >> > -    DEFINE_PROP_BOOL("only-migratable", MigrationState, only_migratable, false),
> >> 
> >> So I'm generally OK at this patch, but I'm worried whether this
> >> is API?
> 
> Uh, forgot to discuss this in my commit message, sorry!
> 
> > IIUC, old code can use either
> >
> >   -global migration.only-migratable=true
> >   -only-migratable
> >
> > After this patch only
> >
> >   -only-migratable
> >
> > is valid. So from that POV it is an API break. 
> >
> > From libvirt's POV this is a non-issue as we never use either of these options.
> > We did have a BZ filed to support it, but never did.
> >
> > To avoid the API break I guess there would been to be a special case early
> > loop iterating over argv[] looking for '-global migration.only-migratable=true',
> > making that set the static 'bool only_migratable', instead of letting it get
> > handled by the object infra.
> 
> Is this is worth the trouble?

Probably not.

> -global migration.only-migratable=on is entirely undocumented.  The
> property appears in -device migration,help and device-list-properties,
> but that's all.
> 
> Why would any sane person risk using arcane & undocumented -global
> migration.only-migratable=on instead of the obvious & documented
> -only-migratable?

Given the combination of no known users, current broken impl, and its
existance not documented, I think there's a reasonable case for just
removing it.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [PATCH v2 2/5] Revert "migration: move only_migratable to MigrationState"

[Dr. David Alan Gilbert]

* Daniel P. Berrangé (berrange@redhat.com) wrote:
> On Mon, Apr 01, 2019 at 01:31:47PM +0200, Markus Armbruster wrote:
> > Daniel P. Berrangé <berrange@redhat.com> writes:
> > 
> > > On Mon, Apr 01, 2019 at 10:27:49AM +0100, Dr. David Alan Gilbert wrote:
> > >> * Markus Armbruster (armbru@redhat.com) wrote:
> > >> > This reverts commit 3df663e575f1876d7f3bc684f80e72fca0703d39.
> > >> > This reverts commit b605c47b57b58e61a901a50a0762dccf43d94783.
> > >> > 
> > >> > Command line option --only-migratable is for disallowing any
> > >> > configuration that can block migration.
> > >> > 
> > >> > Initially, --only-migratable set global variable @only_migratable.
> > >> > 
> > >> > Commit 3df663e575 "migration: move only_migratable to MigrationState"
> > >> > replaced it by MigrationState member @only_migratable.  That was a
> > >> > mistake.
> > >> > 
> > >> > First, it doesn't make sense on the design level.  MigrationState
> > >> > captures the state of an individual migration, but --only-migratable
> > >> > isn't a property of an individual migration, it's a restriction on
> > >> > QEMU configuration.  With fault tolerance, we could have several
> > >> > migrations at once.  --only-migratable would certainly protect all of
> > >> > them.  Storing it in MigrationState feels inappropriate.
> > >> > 
> > >> > Second, it contributes to a dependency cycle that manifests itself as
> > >> > a bug now.
> > >> > 
> > >> > Putting @only_migratable into MigrationState means its available only
> > >> > after migration_object_init().
> > >> > 
> > >> > We can't set it before migration_object_init(), so we delay setting it
> > >> > with a global property (this is fixup commit b605c47b57 "migration:
> > >> > fix handling for --only-migratable").
> > >> > 
> > >> > We can't get it before migration_object_init(), so anything that uses
> > >> > it can only run afterwards.
> > >> > 
> > >> > Since migrate_add_blocker() needs to obey --only-migratable, any code
> > >> > adding migration blockers can run only afterwards.  This contributes
> > >> > to the following dependency cycle:
> > >> > 
> > >> > * configure_blockdev() must run before machine_set_property()
> > >> >   so machine properties can refer to block backends
> > >> > 
> > >> > * machine_set_property() before configure_accelerator()
> > >> >   so machine properties like kvm-irqchip get applied
> > >> > 
> > >> > * configure_accelerator() before migration_object_init()
> > >> >   so that Xen's accelerator compat properties get applied.
> > >> > 
> > >> > * migration_object_init() before configure_blockdev()
> > >> >   so configure_blockdev() can add migration blockers
> > >> > 
> > >> > The cycle was closed when recent commit cda4aa9a5a0 "Create block
> > >> > backends before setting machine properties" added the first
> > >> > dependency, and satisfied it by violating the last one.  Broke block
> > >> > backends that add migration blockers.
> > >> > 
> > >> > Moving @only_migratable into MigrationState was a mistake.  Revert it.
> > >> > 
> > >> > This doesn't quite break the "migration_object_init() before
> > >> > configure_blockdev() dependency, since migrate_add_blocker() still has
> > >> > another dependency on migration_object_init().  To be addressed the
> > >> > next commit
> > >> > 
> > >> > Conflicts:
> > >> > 	include/migration/misc.h
> > >> > 	migration/migration.c
> > >> > 	migration/migration.h
> > >> > 	vl.c
> > >> > 
> > >> > Signed-off-by: Markus Armbruster <armbru@redhat.com>
> > >> > ---
> > >> >  include/sysemu/sysemu.h | 1 +
> > >> >  migration/migration.c   | 5 ++---
> > >> >  migration/migration.h   | 3 ---
> > >> >  migration/savevm.c      | 2 +-
> > >> >  vl.c                    | 9 ++-------
> > >> >  5 files changed, 6 insertions(+), 14 deletions(-)
> > >> > 
> > >> > diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
> > >> > index 6065d9e420..5f133cae83 100644
> > >> > --- a/include/sysemu/sysemu.h
> > >> > +++ b/include/sysemu/sysemu.h
> > >> > @@ -14,6 +14,7 @@
> > >> >  /* vl.c */
> > >> >  
> > >> >  extern const char *bios_name;
> > >> > +extern int only_migratable;
> > >> >  extern const char *qemu_name;
> > >> >  extern QemuUUID qemu_uuid;
> > >> >  extern bool qemu_uuid_set;
> > >> > diff --git a/migration/migration.c b/migration/migration.c
> > >> > index 69f75124c9..f6076e5295 100644
> > >> > --- a/migration/migration.c
> > >> > +++ b/migration/migration.c
> > >> > @@ -1707,7 +1707,7 @@ static GSList *migration_blockers;
> > >> >  
> > >> >  int migrate_add_blocker(Error *reason, Error **errp)
> > >> >  {
> > >> > -    if (migrate_get_current()->only_migratable) {
> > >> > +    if (only_migratable) {
> > >> >          error_propagate_prepend(errp, error_copy(reason),
> > >> >                                  "disallowing migration blocker "
> > >> >                                  "(--only_migratable) for: ");
> > >> > @@ -3337,7 +3337,7 @@ void migration_global_dump(Monitor *mon)
> > >> >      monitor_printf(mon, "store-global-state: %s\n",
> > >> >                     ms->store_global_state ? "on" : "off");
> > >> >      monitor_printf(mon, "only-migratable: %s\n",
> > >> > -                   ms->only_migratable ? "on" : "off");
> > >> > +                   only_migratable ? "on" : "off");
> > >> >      monitor_printf(mon, "send-configuration: %s\n",
> > >> >                     ms->send_configuration ? "on" : "off");
> > >> >      monitor_printf(mon, "send-section-footer: %s\n",
> > >> > @@ -3352,7 +3352,6 @@ void migration_global_dump(Monitor *mon)
> > >> >  static Property migration_properties[] = {
> > >> >      DEFINE_PROP_BOOL("store-global-state", MigrationState,
> > >> >                       store_global_state, true),
> > >> > -    DEFINE_PROP_BOOL("only-migratable", MigrationState, only_migratable, false),
> > >> 
> > >> So I'm generally OK at this patch, but I'm worried whether this
> > >> is API?
> > 
> > Uh, forgot to discuss this in my commit message, sorry!
> > 
> > > IIUC, old code can use either
> > >
> > >   -global migration.only-migratable=true
> > >   -only-migratable
> > >
> > > After this patch only
> > >
> > >   -only-migratable
> > >
> > > is valid. So from that POV it is an API break. 
> > >
> > > From libvirt's POV this is a non-issue as we never use either of these options.
> > > We did have a BZ filed to support it, but never did.
> > >
> > > To avoid the API break I guess there would been to be a special case early
> > > loop iterating over argv[] looking for '-global migration.only-migratable=true',
> > > making that set the static 'bool only_migratable', instead of letting it get
> > > handled by the object infra.
> > 
> > Is this is worth the trouble?
> 
> Probably not.
> 
> > -global migration.only-migratable=on is entirely undocumented.  The
> > property appears in -device migration,help and device-list-properties,
> > but that's all.
> > 
> > Why would any sane person risk using arcane & undocumented -global
> > migration.only-migratable=on instead of the obvious & documented
> > -only-migratable?
> 
> Given the combination of no known users, current broken impl, and its
> existance not documented, I think there's a reasonable case for just
> removing it.

Yes, agreed - it should be noted though.

Dave

> Regards,
> Daniel
> -- 
> |: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org         -o-            https://fstop138.berrange.com :|
> |: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v2 2/5] Revert "migration: move only_migratable to MigrationState"

[Igor Mammedov]

On Mon,  1 Apr 2019 11:08:24 +0200
Markus Armbruster <armbru@redhat.com> wrote:

> This reverts commit 3df663e575f1876d7f3bc684f80e72fca0703d39.
> This reverts commit b605c47b57b58e61a901a50a0762dccf43d94783.
> 
> Command line option --only-migratable is for disallowing any
> configuration that can block migration.
> 
> Initially, --only-migratable set global variable @only_migratable.
> 
> Commit 3df663e575 "migration: move only_migratable to MigrationState"
> replaced it by MigrationState member @only_migratable.  That was a
> mistake.
> 
> First, it doesn't make sense on the design level.  MigrationState
> captures the state of an individual migration, but --only-migratable
> isn't a property of an individual migration, it's a restriction on
> QEMU configuration.  With fault tolerance, we could have several
> migrations at once.  --only-migratable would certainly protect all of
> them.  Storing it in MigrationState feels inappropriate.
> 
> Second, it contributes to a dependency cycle that manifests itself as
> a bug now.
> 
> Putting @only_migratable into MigrationState means its available only
> after migration_object_init().
> 
> We can't set it before migration_object_init(), so we delay setting it
> with a global property (this is fixup commit b605c47b57 "migration:
> fix handling for --only-migratable").
> 
> We can't get it before migration_object_init(), so anything that uses
> it can only run afterwards.
> 
> Since migrate_add_blocker() needs to obey --only-migratable, any code
> adding migration blockers can run only afterwards.  This contributes
> to the following dependency cycle:
> 
> * configure_blockdev() must run before machine_set_property()
>   so machine properties can refer to block backends
> 
> * machine_set_property() before configure_accelerator()
>   so machine properties like kvm-irqchip get applied
> 
> * configure_accelerator() before migration_object_init()
>   so that Xen's accelerator compat properties get applied.
> 
> * migration_object_init() before configure_blockdev()
>   so configure_blockdev() can add migration blockers
> 
> The cycle was closed when recent commit cda4aa9a5a0 "Create block
> backends before setting machine properties" added the first
> dependency, and satisfied it by violating the last one.  Broke block
> backends that add migration blockers.
> 
> Moving @only_migratable into MigrationState was a mistake.  Revert it.
> 
> This doesn't quite break the "migration_object_init() before
> configure_blockdev() dependency, since migrate_add_blocker() still has
> another dependency on migration_object_init().  To be addressed the
> next commit
> 
> Conflicts:
> 	include/migration/misc.h
> 	migration/migration.c
> 	migration/migration.h
> 	vl.c
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

with commit message amended like mentioned by David
(wrt removed "only-migratable" property)

Reviewed-by: Igor Mammedov <imammedo@redhat.com>

> ---
>  include/sysemu/sysemu.h | 1 +
>  migration/migration.c   | 5 ++---
>  migration/migration.h   | 3 ---
>  migration/savevm.c      | 2 +-
>  vl.c                    | 9 ++-------
>  5 files changed, 6 insertions(+), 14 deletions(-)
> 
> diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
> index 6065d9e420..5f133cae83 100644
> --- a/include/sysemu/sysemu.h
> +++ b/include/sysemu/sysemu.h
> @@ -14,6 +14,7 @@
>  /* vl.c */
>  
>  extern const char *bios_name;
> +extern int only_migratable;
>  extern const char *qemu_name;
>  extern QemuUUID qemu_uuid;
>  extern bool qemu_uuid_set;
> diff --git a/migration/migration.c b/migration/migration.c
> index 69f75124c9..f6076e5295 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -1707,7 +1707,7 @@ static GSList *migration_blockers;
>  
>  int migrate_add_blocker(Error *reason, Error **errp)
>  {
> -    if (migrate_get_current()->only_migratable) {
> +    if (only_migratable) {
>          error_propagate_prepend(errp, error_copy(reason),
>                                  "disallowing migration blocker "
>                                  "(--only_migratable) for: ");
> @@ -3337,7 +3337,7 @@ void migration_global_dump(Monitor *mon)
>      monitor_printf(mon, "store-global-state: %s\n",
>                     ms->store_global_state ? "on" : "off");
>      monitor_printf(mon, "only-migratable: %s\n",
> -                   ms->only_migratable ? "on" : "off");
> +                   only_migratable ? "on" : "off");
>      monitor_printf(mon, "send-configuration: %s\n",
>                     ms->send_configuration ? "on" : "off");
>      monitor_printf(mon, "send-section-footer: %s\n",
> @@ -3352,7 +3352,6 @@ void migration_global_dump(Monitor *mon)
>  static Property migration_properties[] = {
>      DEFINE_PROP_BOOL("store-global-state", MigrationState,
>                       store_global_state, true),
> -    DEFINE_PROP_BOOL("only-migratable", MigrationState, only_migratable, false),
>      DEFINE_PROP_BOOL("send-configuration", MigrationState,
>                       send_configuration, true),
>      DEFINE_PROP_BOOL("send-section-footer", MigrationState,
> diff --git a/migration/migration.h b/migration/migration.h
> index 0f986935e1..438f17edad 100644
> --- a/migration/migration.h
> +++ b/migration/migration.h
> @@ -219,9 +219,6 @@ struct MigrationState
>       */
>      bool store_global_state;
>  
> -    /* Whether the VM is only allowing for migratable devices */
> -    bool only_migratable;
> -
>      /* Whether we send QEMU_VM_CONFIGURATION during migration */
>      bool send_configuration;
>      /* Whether we send section footer during migration */
> diff --git a/migration/savevm.c b/migration/savevm.c
> index 1415001d1c..34bcad3807 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -2844,7 +2844,7 @@ void vmstate_register_ram_global(MemoryRegion *mr)
>  bool vmstate_check_only_migratable(const VMStateDescription *vmsd)
>  {
>      /* check needed if --only-migratable is specified */
> -    if (!migrate_get_current()->only_migratable) {
> +    if (!only_migratable) {
>          return true;
>      }
>  
> diff --git a/vl.c b/vl.c
> index c1d5484e12..e4d7ad6b85 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -185,6 +185,7 @@ const char *prom_envs[MAX_PROM_ENVS];
>  int boot_menu;
>  bool boot_strict;
>  uint8_t *boot_splash_filedata;
> +int only_migratable; /* turn it off unless user states otherwise */
>  bool wakeup_suspend_enabled;
>  
>  int icount_align_option;
> @@ -3799,13 +3800,7 @@ int main(int argc, char **argv, char **envp)
>                  incoming = optarg;
>                  break;
>              case QEMU_OPTION_only_migratable:
> -                /*
> -                 * TODO: we can remove this option one day, and we
> -                 * should all use:
> -                 *
> -                 * "-global migration.only-migratable=true"
> -                 */
> -                qemu_global_option("migration.only-migratable=true");
> +                only_migratable = 1;
>                  break;
>              case QEMU_OPTION_nodefaults:
>                  has_defaults = 0;

Re: [Qemu-devel] [PATCH v2 2/5] Revert "migration: move only_migratable to MigrationState"

[Markus Armbruster]

Igor Mammedov <imammedo@redhat.com> writes:

> On Mon,  1 Apr 2019 11:08:24 +0200
> Markus Armbruster <armbru@redhat.com> wrote:
>
>> This reverts commit 3df663e575f1876d7f3bc684f80e72fca0703d39.
>> This reverts commit b605c47b57b58e61a901a50a0762dccf43d94783.
>> 
>> Command line option --only-migratable is for disallowing any
>> configuration that can block migration.
>> 
>> Initially, --only-migratable set global variable @only_migratable.
>> 
>> Commit 3df663e575 "migration: move only_migratable to MigrationState"
>> replaced it by MigrationState member @only_migratable.  That was a
>> mistake.
>> 
>> First, it doesn't make sense on the design level.  MigrationState
>> captures the state of an individual migration, but --only-migratable
>> isn't a property of an individual migration, it's a restriction on
>> QEMU configuration.  With fault tolerance, we could have several
>> migrations at once.  --only-migratable would certainly protect all of
>> them.  Storing it in MigrationState feels inappropriate.
>> 
>> Second, it contributes to a dependency cycle that manifests itself as
>> a bug now.
>> 
>> Putting @only_migratable into MigrationState means its available only
>> after migration_object_init().
>> 
>> We can't set it before migration_object_init(), so we delay setting it
>> with a global property (this is fixup commit b605c47b57 "migration:
>> fix handling for --only-migratable").
>> 
>> We can't get it before migration_object_init(), so anything that uses
>> it can only run afterwards.
>> 
>> Since migrate_add_blocker() needs to obey --only-migratable, any code
>> adding migration blockers can run only afterwards.  This contributes
>> to the following dependency cycle:
>> 
>> * configure_blockdev() must run before machine_set_property()
>>   so machine properties can refer to block backends
>> 
>> * machine_set_property() before configure_accelerator()
>>   so machine properties like kvm-irqchip get applied
>> 
>> * configure_accelerator() before migration_object_init()
>>   so that Xen's accelerator compat properties get applied.
>> 
>> * migration_object_init() before configure_blockdev()
>>   so configure_blockdev() can add migration blockers
>> 
>> The cycle was closed when recent commit cda4aa9a5a0 "Create block
>> backends before setting machine properties" added the first
>> dependency, and satisfied it by violating the last one.  Broke block
>> backends that add migration blockers.
>> 
>> Moving @only_migratable into MigrationState was a mistake.  Revert it.
>> 
>> This doesn't quite break the "migration_object_init() before
>> configure_blockdev() dependency, since migrate_add_blocker() still has
>> another dependency on migration_object_init().  To be addressed the
>> next commit
>> 
>> Conflicts:
>> 	include/migration/misc.h
>> 	migration/migration.c
>> 	migration/migration.h
>> 	vl.c
>> 
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>
> with commit message amended like mentioned by David
> (wrt removed "only-migratable" property)

Inserting right before the "Conflicts:" line:

    Note that the reverted commit made -only-migratable sugar for -global
    migration.only-migratable=on below the hood.  Documentation has only
    ever mentioned -only-migratable.  This commit removes the arcane &
    undocumented alternative to -only-migratable again.  Nobody should be
    using it.

> Reviewed-by: Igor Mammedov <imammedo@redhat.com>

Thanks!

Re: [Qemu-devel] [PATCH v2 2/5] Revert "migration: move only_migratable to MigrationState"

[Dr. David Alan Gilbert]

* Markus Armbruster (armbru@redhat.com) wrote:
> Igor Mammedov <imammedo@redhat.com> writes:
> 
> > On Mon,  1 Apr 2019 11:08:24 +0200
> > Markus Armbruster <armbru@redhat.com> wrote:
> >
> >> This reverts commit 3df663e575f1876d7f3bc684f80e72fca0703d39.
> >> This reverts commit b605c47b57b58e61a901a50a0762dccf43d94783.
> >> 
> >> Command line option --only-migratable is for disallowing any
> >> configuration that can block migration.
> >> 
> >> Initially, --only-migratable set global variable @only_migratable.
> >> 
> >> Commit 3df663e575 "migration: move only_migratable to MigrationState"
> >> replaced it by MigrationState member @only_migratable.  That was a
> >> mistake.
> >> 
> >> First, it doesn't make sense on the design level.  MigrationState
> >> captures the state of an individual migration, but --only-migratable
> >> isn't a property of an individual migration, it's a restriction on
> >> QEMU configuration.  With fault tolerance, we could have several
> >> migrations at once.  --only-migratable would certainly protect all of
> >> them.  Storing it in MigrationState feels inappropriate.
> >> 
> >> Second, it contributes to a dependency cycle that manifests itself as
> >> a bug now.
> >> 
> >> Putting @only_migratable into MigrationState means its available only
> >> after migration_object_init().
> >> 
> >> We can't set it before migration_object_init(), so we delay setting it
> >> with a global property (this is fixup commit b605c47b57 "migration:
> >> fix handling for --only-migratable").
> >> 
> >> We can't get it before migration_object_init(), so anything that uses
> >> it can only run afterwards.
> >> 
> >> Since migrate_add_blocker() needs to obey --only-migratable, any code
> >> adding migration blockers can run only afterwards.  This contributes
> >> to the following dependency cycle:
> >> 
> >> * configure_blockdev() must run before machine_set_property()
> >>   so machine properties can refer to block backends
> >> 
> >> * machine_set_property() before configure_accelerator()
> >>   so machine properties like kvm-irqchip get applied
> >> 
> >> * configure_accelerator() before migration_object_init()
> >>   so that Xen's accelerator compat properties get applied.
> >> 
> >> * migration_object_init() before configure_blockdev()
> >>   so configure_blockdev() can add migration blockers
> >> 
> >> The cycle was closed when recent commit cda4aa9a5a0 "Create block
> >> backends before setting machine properties" added the first
> >> dependency, and satisfied it by violating the last one.  Broke block
> >> backends that add migration blockers.
> >> 
> >> Moving @only_migratable into MigrationState was a mistake.  Revert it.
> >> 
> >> This doesn't quite break the "migration_object_init() before
> >> configure_blockdev() dependency, since migrate_add_blocker() still has
> >> another dependency on migration_object_init().  To be addressed the
> >> next commit
> >> 
> >> Conflicts:
> >> 	include/migration/misc.h
> >> 	migration/migration.c
> >> 	migration/migration.h
> >> 	vl.c
> >> 
> >> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> >
> > with commit message amended like mentioned by David
> > (wrt removed "only-migratable" property)
> 
> Inserting right before the "Conflicts:" line:
> 
>     Note that the reverted commit made -only-migratable sugar for -global
>     migration.only-migratable=on below the hood.  Documentation has only
>     ever mentioned -only-migratable.  This commit removes the arcane &
>     undocumented alternative to -only-migratable again.  Nobody should be
>     using it.

Yes, that's fine:

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

> > Reviewed-by: Igor Mammedov <imammedo@redhat.com>
> 
> Thanks!
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

[Qemu-devel] [PATCH v2 3/5] migration: Support adding migration blockers earlier

[Markus Armbruster]

migrate_add_blocker() asserts we have a current_migration object, in
migrate_get_current().  We do only after migration_object_init().

This contributes to the following dependency cycle:

* configure_blockdev() must run before machine_set_property()
  so machine properties can refer to block backends

* machine_set_property() before configure_accelerator()
  so machine properties like kvm-irqchip get applied

* configure_accelerator() before migration_object_init()
  so that Xen's accelerator compat properties get applied.

* migration_object_init() before configure_blockdev()
  so configure_blockdev() can add migration blockers

The cycle was closed when recent commit cda4aa9a5a0 "Create block
backends before setting machine properties" added the first
dependency, and satisfied it by violating the last one.  Broke block
backends that add migration blockers, as demonstrated by qemu-iotests
055.

To fix it, break the last dependency: make migrate_add_blocker()
usable before migration_object_init().

The previous commit already removed the use of migrate_get_current()
from migrate_add_blocker() itself.  Didn't quite do the trick, as
there's another one hiding in migration_is_idle().

The use there isn't actually necessary: when no migration object has
been created yet, migration is surely idle.  Make migration_is_idle()
return true then.

Fixes: cda4aa9a5a08777cf13e164c0543bd4888b8adce
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 migration/migration.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/migration/migration.c b/migration/migration.c
index f6076e5295..609e0df5d0 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -1646,7 +1646,11 @@ bool migration_in_postcopy_after_devices(MigrationState *s)
 
 bool migration_is_idle(void)
 {
-    MigrationState *s = migrate_get_current();
+    MigrationState *s = current_migration;
+
+    if (!s) {
+        return true;
+    }
 
     switch (s->state) {
     case MIGRATION_STATUS_NONE:
-- 
2.17.2

Re: [Qemu-devel] [PATCH v2 3/5] migration: Support adding migration blockers earlier

[Igor Mammedov]

On Mon,  1 Apr 2019 11:08:25 +0200
Markus Armbruster <armbru@redhat.com> wrote:

> migrate_add_blocker() asserts we have a current_migration object, in
> migrate_get_current().  We do only after migration_object_init().
> 
> This contributes to the following dependency cycle:
> 
> * configure_blockdev() must run before machine_set_property()
>   so machine properties can refer to block backends
> 
> * machine_set_property() before configure_accelerator()
>   so machine properties like kvm-irqchip get applied
> 
> * configure_accelerator() before migration_object_init()
>   so that Xen's accelerator compat properties get applied.
> 
> * migration_object_init() before configure_blockdev()
>   so configure_blockdev() can add migration blockers
> 
> The cycle was closed when recent commit cda4aa9a5a0 "Create block
> backends before setting machine properties" added the first
> dependency, and satisfied it by violating the last one.  Broke block
> backends that add migration blockers, as demonstrated by qemu-iotests
> 055.
> 
> To fix it, break the last dependency: make migrate_add_blocker()
> usable before migration_object_init().
> 
> The previous commit already removed the use of migrate_get_current()
> from migrate_add_blocker() itself.  Didn't quite do the trick, as
> there's another one hiding in migration_is_idle().
> 
> The use there isn't actually necessary: when no migration object has
> been created yet, migration is surely idle.  Make migration_is_idle()
> return true then.
> 
> Fixes: cda4aa9a5a08777cf13e164c0543bd4888b8adce
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

Reviewed-by: Igor Mammedov <imammedo@redhat.com>

> ---
>  migration/migration.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/migration/migration.c b/migration/migration.c
> index f6076e5295..609e0df5d0 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -1646,7 +1646,11 @@ bool migration_in_postcopy_after_devices(MigrationState *s)
>  
>  bool migration_is_idle(void)
>  {
> -    MigrationState *s = migrate_get_current();
> +    MigrationState *s = current_migration;
> +
> +    if (!s) {
> +        return true;
> +    }
>  
>      switch (s->state) {
>      case MIGRATION_STATUS_NONE:

[Qemu-devel] [PATCH v2 4/5] vl: Document dependencies hiding in global and compat props

[Markus Armbruster]

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

diff --git a/vl.c b/vl.c
index e4d7ad6b85..8c6d06b01d 100644
--- a/vl.c
+++ b/vl.c
@@ -4285,16 +4285,36 @@ int main(int argc, char **argv, char **envp)
     current_machine->maxram_size = maxram_size;
     current_machine->ram_slots = ram_slots;
 
+    /*
+     * Note: uses machine properties such as kernel-irqchip, must run
+     * after machine_set_property().
+     */
     configure_accelerator(current_machine, argv[0]);
 
+    /*
+     * Beware, QOM objects created before this point miss global and
+     * compat properties.
+     *
+     * Global properties get set up by qdev_prop_register_global(),
+     * called from user_register_global_props(), and certain option
+     * desugaring.  Also in CPU feature desugaring (buried in
+     * parse_cpu_model()), which happens below this point, but may
+     * only target the CPU type, which can only be created after
+     * parse_cpu_model() returned the type.
+     *
+     * Machine compat properties: object_set_machine_compat_props().
+     * Accelerator compat props: object_set_accelerator_compat_props(),
+     * called from configure_accelerator().
+     */
+
     if (!qtest_enabled() && machine_class->deprecation_reason) {
         error_report("Machine type '%s' is deprecated: %s",
                      machine_class->name, machine_class->deprecation_reason);
     }
 
     /*
-     * Migration object can only be created after global properties
-     * are applied correctly.
+     * Note: creates a QOM object, must run only after global and
+     * compat properties have been set up.
      */
     migration_object_init();
 
-- 
2.17.2

Re: [Qemu-devel] [PATCH v2 4/5] vl: Document dependencies hiding in global and compat props

[Igor Mammedov]

On Mon,  1 Apr 2019 11:08:26 +0200
Markus Armbruster <armbru@redhat.com> wrote:

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

Reviewed-by: Igor Mammedov <imammedo@redhat.com>

> ---
>  vl.c | 24 ++++++++++++++++++++++--
>  1 file changed, 22 insertions(+), 2 deletions(-)
> 
> diff --git a/vl.c b/vl.c
> index e4d7ad6b85..8c6d06b01d 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -4285,16 +4285,36 @@ int main(int argc, char **argv, char **envp)
>      current_machine->maxram_size = maxram_size;
>      current_machine->ram_slots = ram_slots;
>  
> +    /*
> +     * Note: uses machine properties such as kernel-irqchip, must run
> +     * after machine_set_property().
> +     */
>      configure_accelerator(current_machine, argv[0]);
>  
> +    /*
> +     * Beware, QOM objects created before this point miss global and
> +     * compat properties.
> +     *
> +     * Global properties get set up by qdev_prop_register_global(),
> +     * called from user_register_global_props(), and certain option
> +     * desugaring.  Also in CPU feature desugaring (buried in
> +     * parse_cpu_model()), which happens below this point, but may
> +     * only target the CPU type, which can only be created after
> +     * parse_cpu_model() returned the type.
> +     *
> +     * Machine compat properties: object_set_machine_compat_props().
> +     * Accelerator compat props: object_set_accelerator_compat_props(),
> +     * called from configure_accelerator().
> +     */
> +
>      if (!qtest_enabled() && machine_class->deprecation_reason) {
>          error_report("Machine type '%s' is deprecated: %s",
>                       machine_class->name, machine_class->deprecation_reason);
>      }
>  
>      /*
> -     * Migration object can only be created after global properties
> -     * are applied correctly.
> +     * Note: creates a QOM object, must run only after global and
> +     * compat properties have been set up.
>       */
>      migration_object_init();
>  

Re: [Qemu-devel] [PATCH v2 4/5] vl: Document dependencies hiding in global and compat props

[Dr. David Alan Gilbert]

* Markus Armbruster (armbru@redhat.com) wrote:
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  vl.c | 24 ++++++++++++++++++++++--
>  1 file changed, 22 insertions(+), 2 deletions(-)
> 
> diff --git a/vl.c b/vl.c
> index e4d7ad6b85..8c6d06b01d 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -4285,16 +4285,36 @@ int main(int argc, char **argv, char **envp)
>      current_machine->maxram_size = maxram_size;
>      current_machine->ram_slots = ram_slots;
>  
> +    /*
> +     * Note: uses machine properties such as kernel-irqchip, must run
> +     * after machine_set_property().
> +     */
>      configure_accelerator(current_machine, argv[0]);
>  
> +    /*
> +     * Beware, QOM objects created before this point miss global and
> +     * compat properties.
> +     *
> +     * Global properties get set up by qdev_prop_register_global(),
> +     * called from user_register_global_props(), and certain option
> +     * desugaring.

Not to stop this patch series; but could we add a way to actually
detect when this has happened?  Like setting a flag on QOM objects
created before this point for example?

>  Also in CPU feature desugaring (buried in
> +     * parse_cpu_model()), which happens below this point, but may
> +     * only target the CPU type, which can only be created after
> +     * parse_cpu_model() returned the type.
> +     *
> +     * Machine compat properties: object_set_machine_compat_props().
> +     * Accelerator compat props: object_set_accelerator_compat_props(),
> +     * called from configure_accelerator().
> +     */
> +
>      if (!qtest_enabled() && machine_class->deprecation_reason) {
>          error_report("Machine type '%s' is deprecated: %s",
>                       machine_class->name, machine_class->deprecation_reason);
>      }
>  
>      /*
> -     * Migration object can only be created after global properties
> -     * are applied correctly.
> +     * Note: creates a QOM object, must run only after global and
> +     * compat properties have been set up.
>       */
>      migration_object_init();
>  
> -- 
> 2.17.2
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v2 4/5] vl: Document dependencies hiding in global and compat props

[Markus Armbruster]

"Dr. David Alan Gilbert" <dgilbert@redhat.com> writes:

> * Markus Armbruster (armbru@redhat.com) wrote:
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  vl.c | 24 ++++++++++++++++++++++--
>>  1 file changed, 22 insertions(+), 2 deletions(-)
>> 
>> diff --git a/vl.c b/vl.c
>> index e4d7ad6b85..8c6d06b01d 100644
>> --- a/vl.c
>> +++ b/vl.c
>> @@ -4285,16 +4285,36 @@ int main(int argc, char **argv, char **envp)
>>      current_machine->maxram_size = maxram_size;
>>      current_machine->ram_slots = ram_slots;
>>  
>> +    /*
>> +     * Note: uses machine properties such as kernel-irqchip, must run
>> +     * after machine_set_property().
>> +     */
>>      configure_accelerator(current_machine, argv[0]);
>>  
>> +    /*
>> +     * Beware, QOM objects created before this point miss global and
>> +     * compat properties.
>> +     *
>> +     * Global properties get set up by qdev_prop_register_global(),
>> +     * called from user_register_global_props(), and certain option
>> +     * desugaring.
>
> Not to stop this patch series; but could we add a way to actually
> detect when this has happened?  Like setting a flag on QOM objects
> created before this point for example?

I started to work on something like this, but had to switch to more
urgent work.

[Qemu-devel] [PATCH v2 5/5] accel: Unbreak accelerator fallback

[Markus Armbruster]

When the user specifies a list of accelerators, we pick the first one
that initializes successfully.  Recent commit 1a3ec8c1564 broke that.
Reproducer:

    $ qemu-system-x86_64 --machine accel=xen:tcg
    xencall: error: Could not obtain handle on privileged command interface: No such file or directory
    xen be core: xen be core: can't open xen interface
    can't open xen interface
    qemu-system-x86_64: failed to initialize Xen: Operation not permitted
    qemu-system-x86_64: /home/armbru/work/qemu/qom/object.c:436: object_set_accelerator_compat_props: Assertion `!object_compat_props[0]' failed.

Root cause: we register accelerator compat properties even when the
accelerator fails.  The failed assertion is
object_set_accelerator_compat_props() telling us off.  Fix by calling
it only for the accelerator that succeeded.

Fixes: 1a3ec8c1564f51628cce10d435a2e22559ea29fd
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Igor Mammedov <imammedo@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 accel/accel.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/accel/accel.c b/accel/accel.c
index 8deb475b5d..454fef9d92 100644
--- a/accel/accel.c
+++ b/accel/accel.c
@@ -65,8 +65,9 @@ static int accel_init_machine(AccelClass *acc, MachineState *ms)
         ms->accelerator = NULL;
         *(acc->allowed) = false;
         object_unref(OBJECT(accel));
+    } else {
+        object_set_accelerator_compat_props(acc->compat_props);
     }
-    object_set_accelerator_compat_props(acc->compat_props);
     return ret;
 }
 
-- 
2.17.2