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

[Igor Mammedov]

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

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