Re: [Qemu-devel] [PATCH 1/2] vl: Fix to create accel compat props before migration object again

[Markus Armbruster]

Igor Mammedov <address@hidden> writes:

> On Fri, 29 Mar 2019 15:34:59 +0100
> Markus Armbruster <address@hidden> wrote:
>
>> Igor Mammedov <address@hidden> writes:
>> 
>> > On Wed, 27 Mar 2019 16:03:46 +0100
>> > Markus Armbruster <address@hidden> wrote:
>> >  
>> >> 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.  Fix by calling configure_accelerator() earlier, right before
>> >> migration_object_init().
>> >> 
>> >> Fixes: e60483f2f8498ae08ae79ca4c6fb03a3317f5e1e
>> >> Reported-by: Anthony PERARD <address@hidden>
>> >> Signed-off-by: Markus Armbruster <address@hidden>
>> >> ---
>> >>  vl.c | 8 ++++++--
>> >>  1 file changed, 6 insertions(+), 2 deletions(-)
>> >> 
>> >> 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]);
>> >> +  
>> > Setting machine properties with this change happens after
>> > accelerator was initialized.
>> >
>> > It probably will break initialization of accelerator features
>> > (to name a couple kernel-irqchip, memory-encryption) that depend
>> > on machine properties being set before accelerator is initialized.  
>> 
>> I'm blind.  Can you suggest a potential reproducer for me to try?
> sure (it' difficult to move things around in vl.c and spot implicit
> dependency or not to break something while at it)
>
>
> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
> index 241db49..9b0d89a 100644
> --- a/accel/kvm/kvm-all.c
> +++ b/accel/kvm/kvm-all.c
> @@ -1724,6 +1724,7 @@ static int kvm_init(MachineState *ms)
>      }
>  
>      if (machine_kernel_irqchip_allowed(ms)) {
> +fprintf(stderr, "kvm_irqchip_create\n");
>          kvm_irqchip_create(ms, s);
>      }
>
> with:
>
> qemu-system-x86_64 -machine accel=kvm,kernel-irqchip=off
>
> prints "kvm_irqchip_create" after patch and doesn't with current master

Now I see.  Thank you!

machine_kernel_irqchip_allowed(m) returns m->kernel_irqchip_allowed.
machine_initfn() sets the default value, true.  -machine
kernel-irqchip=off sets it to false.

To make the latter stick, kvm_init() must run after
machine_set_property().

kvm_init() is AccelClass method init_machine(), called by
configure_accelerator() via accel_init_machine().

We have a depencency cycle:

    configure_accelerator()
    before migration_object_init()
        to make Xen's accel compat props stick
    before configure_blockdev()
        so we can create migration blockers
    before machine_set_property()
        so machine props can refer to blockdevs
    before configure_accelerator()
        to make -machine kvm-irqchip=off and similar stick

To break it, we need to split one of them.

I've long had a queasy feeling about configure_accelerator().  It needs
to run early so accelerator properties work, and it needs to run late so
it can use machine properties.

Can we split it into

    part 1: runs early, can fail, can't use machine properties
    part 2: runs late, can't fail, can use machine properties

?

If we can't, or can't for 4.0, then I guess we need to split
migration_object_init() along similar lines:

    part 1: runs early, makes migration blockers usable,
            can't use migration properties
    part 2: runs late, can use migration properties

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