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

[Markus Armbruster]

Markus Armbruster <address@hidden> writes:

> 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
>
> ?

Too risky for 4.0.

> 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

No dice: what we need to make migration blockers usable is the exactly
what may only run after configure_accelerator(): object_new().

We need to decouple migration blockers from the migration object.  I'll
post patches later today.