[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 1/2] vl: Fix to create accel compat props before
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-devel] [PATCH 1/2] vl: Fix to create accel compat props before migration object again |
Date: |
Mon, 01 Apr 2019 08:25:15 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/26.1 (gnu/linux) |
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.
[Prev in Thread] |
Current Thread |
[Next in Thread] |
- Re: [Qemu-devel] [PATCH 1/2] vl: Fix to create accel compat props before migration object again,
Markus Armbruster <=