[Qemu-devel] BUG: commit 50a2c6e breaks KVM/ARM (reset/init vcpu order)

[Qemu-devel] BUG: commit 50a2c6e breaks KVM/ARM (reset/init vcpu order)

[Christoffer Dall]

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 :)

Thanks,
-Christoffer

Re: [Qemu-devel] BUG: commit 50a2c6e breaks KVM/ARM (reset/init vcpu order)

[Paolo Bonzini]

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>

Paolo

Re: [Qemu-devel] BUG: commit 50a2c6e breaks KVM/ARM (reset/init vcpu order)

[Alexander Graf]

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

Re: [Qemu-devel] BUG: commit 50a2c6e breaks KVM/ARM (reset/init vcpu order)

[Andreas Färber]

Am 26.05.2014 11:57, schrieb Alexander Graf:
> 
> 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-}

Alpha is the main blocker for unifying CPU reset iirc. It does not
implement reset at all and thus is not calling it. The struct was not
designed for zero'ing things, so there's a mix of data fields and
pointers without clear separation to allow memset(), and I have neither
a working alpha test image nor the time to investigate this at the moment.

WIP here:
https://github.com/afaerber/qemu-cpu/commits/qom-cpu-alpha
https://github.com/afaerber/qemu-cpu/commits/qom-cpu-reset

According to my commit unicore32 is another odd sock that doesn't reset
the CPU - despite implemented iirc.

Regards,
Andreas

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

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] BUG: commit 50a2c6e breaks KVM/ARM (reset/init vcpu order)

[Alexander Graf]

On 26.05.14 12:20, Andreas Färber wrote:
> Am 26.05.2014 11:57, schrieb Alexander Graf:
>> 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-}
> Alpha is the main blocker for unifying CPU reset iirc. It does not
> implement reset at all and thus is not calling it. The struct was not
> designed for zero'ing things, so there's a mix of data fields and
> pointers without clear separation to allow memset(), and I have neither
> a working alpha test image nor the time to investigate this at the moment.
>
> WIP here:
> https://github.com/afaerber/qemu-cpu/commits/qom-cpu-alpha
> https://github.com/afaerber/qemu-cpu/commits/qom-cpu-reset
>
> According to my commit unicore32 is another odd sock that doesn't reset
> the CPU - despite implemented iirc.

So if we had reset, we could call

   qemu_init_vcpu();
   cpu_reset()

inside parent_realize(), right?

Then let's prepare for that step and make at least all targets that do 
call cpu_reset call it after init_vcpu().


Alex