Re: BUG: commit 50a2c6e breaks KVM/ARM (reset/init vcpu order)

[Alexander Graf <agraf@suse.de>]

On 26.05.14 11:55, Paolo Bonzini wrote:
> Il 26/05/2014 11:18, Christoffer Dall ha scritto:
>> Hi,
>>
>> I noticed that commit 50a2c6e55fa2ce5a2916a2c206bad2c6b0e06df1 broke
>> KVM/ARM, because the realize function (arm_cpu_realizefn()) now calls
>> cpu_reset() before qemu_init_vcpu(), which causes kvm_arm_reset_cpu() to
>> segfault because it dereferences cpu->cpreg_reset_values, which is not
>> allocated before kvm_arch_init_vcpu().
>>
>> Simply changing the order of the reset/init calls (see the tiny patch
>> below) seems to fix it, but I'm not completely sure this is a clean and
>> correct fix:
>>
>>
>> diff --git a/target-arm/cpu.c b/target-arm/cpu.c
>> index 6c6f2b3..794dcb9 100644
>> --- a/target-arm/cpu.c
>> +++ b/target-arm/cpu.c
>> @@ -370,8 +370,8 @@ static void arm_cpu_realizefn(DeviceState *dev, 
>> Error **errp)
>>
>>      init_cpreg_list(cpu);
>>
>> -    cpu_reset(cs);
>>      qemu_init_vcpu(cs);
>> +    cpu_reset(cs);
>>
>>      acc->parent_realize(dev, errp);
>>  }
>>
>>
>>
>> Please adivce :)
>
> I looked at the kvm_arch_init_vcpu implementation and it looks good to 
> me.
>
> Acked-by: Paolo Bonzini <pbonzini@redhat.com>

Any reason we're so incredibly inconsistent in what we do during realize 
with reset? I would really prefer to ensure we're doing the same thing 
on all targets.


Alex

$ grep -R -A 3 -B 3 qemu_init_vcpu target-*
target-alpha/cpu.c-    CPUState *cs = CPU(dev);
target-alpha/cpu.c-    AlphaCPUClass *acc = ALPHA_CPU_GET_CLASS(dev);
target-alpha/cpu.c-
target-alpha/cpu.c:    qemu_init_vcpu(cs);
target-alpha/cpu.c-
target-alpha/cpu.c-    acc->parent_realize(dev, errp);
target-alpha/cpu.c-}
--
target-arm/cpu.c-    init_cpreg_list(cpu);
target-arm/cpu.c-
target-arm/cpu.c-    cpu_reset(cs);
target-arm/cpu.c:    qemu_init_vcpu(cs);
target-arm/cpu.c-
target-arm/cpu.c-    acc->parent_realize(dev, errp);
target-arm/cpu.c-}
--
target-cris/cpu.c-    CRISCPUClass *ccc = CRIS_CPU_GET_CLASS(dev);
target-cris/cpu.c-
target-cris/cpu.c-    cpu_reset(cs);
target-cris/cpu.c:    qemu_init_vcpu(cs);
target-cris/cpu.c-
target-cris/cpu.c-    ccc->parent_realize(dev, errp);
target-cris/cpu.c-}
--
target-i386/cpu.c-#endif
target-i386/cpu.c-
target-i386/cpu.c-    mce_init(cpu);
target-i386/cpu.c:    qemu_init_vcpu(cs);
target-i386/cpu.c-
target-i386/cpu.c-    x86_cpu_apic_realize(cpu, &local_err);
target-i386/cpu.c-    if (local_err != NULL) {
--
target-lm32/cpu.c-
target-lm32/cpu.c-    cpu_reset(cs);
target-lm32/cpu.c-
target-lm32/cpu.c:    qemu_init_vcpu(cs);
target-lm32/cpu.c-
target-lm32/cpu.c-    lcc->parent_realize(dev, errp);
target-lm32/cpu.c-}
--
target-m68k/cpu.c-    m68k_cpu_init_gdb(cpu);
target-m68k/cpu.c-
target-m68k/cpu.c-    cpu_reset(cs);
target-m68k/cpu.c:    qemu_init_vcpu(cs);
target-m68k/cpu.c-
target-m68k/cpu.c-    mcc->parent_realize(dev, errp);
target-m68k/cpu.c-}
--
target-microblaze/cpu.c-    MicroBlazeCPUClass *mcc = 
MICROBLAZE_CPU_GET_CLASS(dev);
target-microblaze/cpu.c-
target-microblaze/cpu.c-    cpu_reset(cs);
target-microblaze/cpu.c:    qemu_init_vcpu(cs);
target-microblaze/cpu.c-
target-microblaze/cpu.c-    mcc->parent_realize(dev, errp);
target-microblaze/cpu.c-}
--
target-mips/cpu.c-    MIPSCPUClass *mcc = MIPS_CPU_GET_CLASS(dev);
target-mips/cpu.c-
target-mips/cpu.c-    cpu_reset(cs);
target-mips/cpu.c:    qemu_init_vcpu(cs);
target-mips/cpu.c-
target-mips/cpu.c-    mcc->parent_realize(dev, errp);
target-mips/cpu.c-}
--
target-moxie/cpu.c-    CPUState *cs = CPU(dev);
target-moxie/cpu.c-    MoxieCPUClass *mcc = MOXIE_CPU_GET_CLASS(dev);
target-moxie/cpu.c-
target-moxie/cpu.c:    qemu_init_vcpu(cs);
target-moxie/cpu.c-    cpu_reset(cs);
target-moxie/cpu.c-
target-moxie/cpu.c-    mcc->parent_realize(dev, errp);
--
target-openrisc/cpu.c-    CPUState *cs = CPU(dev);
target-openrisc/cpu.c-    OpenRISCCPUClass *occ = 
OPENRISC_CPU_GET_CLASS(dev);
target-openrisc/cpu.c-
target-openrisc/cpu.c:    qemu_init_vcpu(cs);
target-openrisc/cpu.c-    cpu_reset(cs);
target-openrisc/cpu.c-
target-openrisc/cpu.c-    occ->parent_realize(dev, errp);
Binary file target-ppc/.translate_init.c.swp matches
--
target-ppc/translate_init.c-                                 34, 
"power-spe.xml", 0);
target-ppc/translate_init.c-    }
target-ppc/translate_init.c-
target-ppc/translate_init.c:    qemu_init_vcpu(cs);
target-ppc/translate_init.c-
target-ppc/translate_init.c-    pcc->parent_realize(dev, errp);
target-ppc/translate_init.c-
--
target-s390x/cpu.c-    CPUState *cs = CPU(dev);
target-s390x/cpu.c-    S390CPUClass *scc = S390_CPU_GET_CLASS(dev);
target-s390x/cpu.c-
target-s390x/cpu.c:    qemu_init_vcpu(cs);
target-s390x/cpu.c-    cpu_reset(cs);
target-s390x/cpu.c-
target-s390x/cpu.c-    scc->parent_realize(dev, errp);
--
target-sh4/cpu.c-    SuperHCPUClass *scc = SUPERH_CPU_GET_CLASS(dev);
target-sh4/cpu.c-
target-sh4/cpu.c-    cpu_reset(cs);
target-sh4/cpu.c:    qemu_init_vcpu(cs);
target-sh4/cpu.c-
target-sh4/cpu.c-    scc->parent_realize(dev, errp);
target-sh4/cpu.c-}
--
target-sparc/cpu.c-    }
target-sparc/cpu.c-#endif
target-sparc/cpu.c-
target-sparc/cpu.c:    qemu_init_vcpu(CPU(dev));
target-sparc/cpu.c-
target-sparc/cpu.c-    scc->parent_realize(dev, errp);
target-sparc/cpu.c-}
--
target-unicore32/cpu.c-{
target-unicore32/cpu.c-    UniCore32CPUClass *ucc = 
UNICORE32_CPU_GET_CLASS(dev);
target-unicore32/cpu.c-
target-unicore32/cpu.c:    qemu_init_vcpu(CPU(dev));
target-unicore32/cpu.c-
target-unicore32/cpu.c-    ucc->parent_realize(dev, errp);
target-unicore32/cpu.c-}
--
target-xtensa/cpu.c-
target-xtensa/cpu.c-    cs->gdb_num_regs = xcc->config->gdb_regmap.num_regs;
target-xtensa/cpu.c-
target-xtensa/cpu.c:    qemu_init_vcpu(cs);
target-xtensa/cpu.c-
target-xtensa/cpu.c-    xcc->parent_realize(dev, errp);
target-xtensa/cpu.c-}