Re: [Qemu-devel] [PATCH] vl: Fix to create migration object before block backends again

[Markus Armbruster]

Anthony PERARD <address@hidden> writes:

> On Tue, Mar 26, 2019 at 03:58:52PM +0000, Anthony PERARD wrote:
>> On Wed, Mar 13, 2019 at 09:43:30AM +0100, Markus Armbruster wrote:
>> > Recent commit cda4aa9a5a0 moved block backend creation before machine
>> > property evaluation.  This broke qemu-iotests 055.  Turns out we need
>> > to create the migration object before block backends, so block
>> > backends can add migration blockers.  Fix by calling
>> > migration_object_init() earlier, right before configure_blockdev().
>> > 
>> > Fixes: cda4aa9a5a08777cf13e164c0543bd4888b8adce
>> > Reported-by: Kevin Wolf <address@hidden>
>> > Signed-off-by: Markus Armbruster <address@hidden>
>> 
>> Hi,
>> 
>> After this patch is applied, migration with Xen doesn't work anymore.
>> When QEMU on the receiving end is started, it prints:
>>     qemu-system-i386: load of migration failed: Invalid argument
>
> I should have quote this from QEMU stderr instead of the single line
> abrove:
>     qemu-system-i386: Configuration section missing
>     qemu-system-i386: load of migration failed: Invalid argument
>
>> > +    /*
>> > +     * Migration object can only be created after global properties
>> > +     * are applied correctly.
>> > +     */
>> > +    migration_object_init();
>> 
>> I think it's because migration_object_init() is now called before
>> configure_accelerator(). xen_accel_class_init() have some compat_props
>> for migration.

I see.

Anything that gets created before a certain compat property becomes
known silently ignores the compat property[*].

Accelerator compat properties become known when we select the
accelerator.  This is awfully late, because we want to pick the first
accelerator that initializes, which requires a machine.

>> Any idea how to fix this?

Known dependencies:

* migration object_init() must run before configure_blockdev(), so block
  backends can register migration blockers

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

* configure_accelerator() must run after machine creation, because it
  needs a machine

* configure_accelerator() must run before migration_object_init(), so
  migration_object_init() can see accelerator compat properties.

We're currently violating the last one, because weren't aware of it.  To
fix that, rejigger some more: move configure_accelerator() before
migration_object_init() (with a suitable comment), then see what else
breaks.  Could you try the appended patch and report whether it fixes
your problem?


[*] Silent ignore is nasty.  I got experimental code to turn it into a
hard failure.


diff --git a/vl.c b/vl.c
index d61d5604e5..28b9e9a170 100644
--- a/vl.c
+++ b/vl.c
@@ -4276,6 +4276,12 @@ int main(int argc, char **argv, char **envp)
         exit(0);
     }
 
+    /*
+     * Must run before migration_object_init() to make Xen's
+     * accelerator compat properties stick
+     */
+    configure_accelerator(current_machine, argv[0]);
+
     /*
      * Migration object can only be created after global properties
      * are applied correctly.
@@ -4297,8 +4303,6 @@ int main(int argc, char **argv, char **envp)
     current_machine->maxram_size = maxram_size;
     current_machine->ram_slots = ram_slots;
 
-    configure_accelerator(current_machine, argv[0]);
-
     if (!qtest_enabled() && machine_class->deprecation_reason) {
         error_report("Machine type '%s' is deprecated: %s",
                      machine_class->name, machine_class->deprecation_reason);