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

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

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

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

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

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

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

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

[Andreas Färber]

Am 26.05.2014 12:31, schrieb 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?

That's exactly what the single commit on qom-cpu-reset does. :)

Andreas

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


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

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

[管雪涛]

----- Andreas Färber <afaerber@suse.de> 写道：
> 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.

I'm sorry.
I haven't implemented and tested reset function for unicore32 image, but only left the codes there.
So, if any change for this function, please modify unicore32's codes correspondingly, and I'll test it later.

Regards and thanks,

Xuetao

> 
> 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: BUG: commit 50a2c6e breaks KVM/ARM (reset/init vcpu order)

[Richard Henderson]

[-- Attachment #1: Type: text/plain, Size: 950 bytes --]

On 05/26/2014 03:20 AM, Andreas Färber wrote:
> 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

Doesn't compile anymore.  I can fix that up with the attached, but we can't
actually test this without changes to the rom to actually support reset.  At
the moment, the rom will tell qemu to poweroff whether the kernel signals for
poweroff or reset.

If this is good enough to unblock you in other qom cleanups, please go ahead.
One of these days I'll get around to filling out more complete roms...


r~


[-- Attachment #2: z --]
[-- Type: text/plain, Size: 1023 bytes --]

diff --git a/target-alpha/cpu.c b/target-alpha/cpu.c
index 6ab31a1..cbad6fa 100644
--- a/target-alpha/cpu.c
+++ b/target-alpha/cpu.c
@@ -49,19 +49,30 @@ static bool alpha_cpu_has_work(CPUState *cs)
 /* CPUClass::reset() */
 static void alpha_cpu_reset(CPUState *s)
 {
+#ifdef CONFIG_SOFTMMU
     AlphaCPU *cpu = ALPHA_CPU(s);
     AlphaCPUClass *acc = ALPHA_CPU_GET_CLASS(cpu);
     CPUAlphaState *env = &cpu->env;
+    uint64_t palbr;
 
     if (qemu_loglevel_mask(CPU_LOG_RESET)) {
         qemu_log("CPU Reset (CPU %d)\n", s->cpu_index);
-        log_cpu_state(env, 0);
+        log_cpu_state(s, 0);
     }
 
     acc->parent_reset(s);
 
-    memset(env, 0, offsetof(CPUAlphaState, breakpoints));
-    tlb_flush(env, 1);
+    palbr = env->palbr;
+
+    memset(env, 0, offsetof(CPUAlphaState, error_code));
+    tlb_flush(s, 1);
+
+    /* Reset vector goes to palbr + 0.  */
+    env->palbr = palbr;
+    env->pc = palbr;
+#else
+    abort();
+#endif
 }
 
 static void alpha_cpu_realizefn(DeviceState *dev, Error **errp)

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

[Alexander Graf]

On 26.05.14 14:36, Andreas Färber wrote:
> Am 26.05.2014 12:31, schrieb 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?
> That's exactly what the single commit on qom-cpu-reset does. :)

Yeah, I was indicating that we should maybe take 2 steps:

  1) Unify all targets to call init, then reset
  2) Move init and reset into the parent

That way nothing gets blocked on the CPU QOMification, yet still we are 
consistent across all targets :). As a nice bonus, nobody can claim QOM 
broke their code because the code flow won't change with step 2 ;).


Alex