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

PATCH Christoffer Dall 6 participants 9 messages May 26, 2014 KVM

Share & tools

Christoffer Dall

author

2014-05-26 09:18 UTC #0 lore raw

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:

target-arm/cpu.c +1 -1

index 6c6f2b3..794dcb9 100644
@@ -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

Paolo Bonzini

maintainer Acked-by

2014-05-26 09:55 UTC #1 lore raw

Il 26/05/2014 11:18, Christoffer Dall ha scritto:

Quoted 30 lines

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:
target-arm/cpu.c +1 -1
index 6c6f2b3..794dcb9 100644
@@ -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);
 }

Quoted 1 line

Please adivce :)

I looked at the kvm_arch_init_vcpu implementation and it looks good to me.

Acked-by Paolo Bonzini <[email protected]>

Paolo

Alexander Graf

2014-05-26 09:57 UTC #2 lore raw

Quoted 32 lines · Paolo Bonzini

Il 26/05/2014 11:18, Christoffer Dall ha scritto:
Quoted 31 lines
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:
target-arm/cpu.c +1 -1
index 6c6f2b3..794dcb9 100644
@@ -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);
}

Quoted 5 lines

Quoted 1 line
Please adivce :)
I looked at the kvm_arch_init_vcpu implementation and it looks good to me.
Acked-by Paolo Bonzini <[email protected]>

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

Andreas Färber

2014-05-26 10:20 UTC #3 lore raw

Am 26.05.2014 11:57, schrieb Alexander Graf:

Quoted 8 lines

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

Quoted 8 lines

$ 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

Quoted 8 lines

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

Signature · 2 lines

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

Alexander Graf

2014-05-26 10:31 UTC #4 lore raw

Quoted 22 lines · Andreas Färber

Am 26.05.2014 11:57, schrieb Alexander Graf:
Quoted 7 lines
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
Quoted 8 lines
$ 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.

Quoted 6 lines

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

Andreas Färber

2014-05-26 12:36 UTC #5 lore raw

Am 26.05.2014 12:31, schrieb Alexander Graf:

Quoted 32 lines · Andreas Färber

Quoted 23 lines
Am 26.05.2014 11:57, schrieb Alexander Graf:
Quoted 7 lines
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
Quoted 8 lines
$ 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.
Quoted 6 lines
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.

Quoted 6 lines

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

Quoted 6 lines

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

Signature · 2 lines

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

Alexander Graf

2014-05-26 21:04 UTC #6 lore raw

Quoted 31 lines · Andreas Färber

Am 26.05.2014 12:31, schrieb Alexander Graf:
Quoted 30 lines · Andreas Färber
Quoted 29 lines
Am 26.05.2014 11:57, schrieb Alexander Graf:
Quoted 7 lines
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
Quoted 8 lines
$ 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.

Quoted 7 lines

Quoted 6 lines
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

管雪涛

2014-05-26 12:39 UTC #7 lore raw

In reply to Andreas Färber

Quoted 24 lines · ----- Andreas Färber

Am 26.05.2014 11:57, schrieb Alexander Graf:
Quoted 8 lines
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
Quoted 8 lines
$ 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.

Quoted 6 lines

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

Quoted 16 lines

Regards,
Andreas
Quoted 8 lines
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-}
Signature · 2 lines
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Richard Henderson

2014-05-26 17:29 UTC #8 lore raw

In reply to Andreas Färber

Quoted 9 lines · Andreas Färber

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

target-alpha/cpu.c +14 -3

index 6ab31a1..cbad6fa 100644
@@ -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)