From mboxrd@z Thu Jan 1 00:00:00 1970 From: Alexander Graf Subject: Re: BUG: commit 50a2c6e breaks KVM/ARM (reset/init vcpu order) Date: Mon, 26 May 2014 11:57:32 +0200 Message-ID: <5383100C.3030807@suse.de> References: <20140526091813.GA31431@lvm> <53830F7A.3060306@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Cc: kvm@vger.kernel.org, Peter Maydell , afaerber@suse.de To: Paolo Bonzini , Christoffer Dall , qemu-devel@nongnu.org Return-path: Received: from cantor2.suse.de ([195.135.220.15]:38558 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751515AbaEZJ5f (ORCPT ); Mon, 26 May 2014 05:57:35 -0400 In-Reply-To: <53830F7A.3060306@redhat.com> Sender: kvm-owner@vger.kernel.org List-ID: 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 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-} From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:52428) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Worf4-0001sc-8z for qemu-devel@nongnu.org; Mon, 26 May 2014 05:57:49 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Worew-0002pc-Oq for qemu-devel@nongnu.org; Mon, 26 May 2014 05:57:42 -0400 Received: from cantor2.suse.de ([195.135.220.15]:50627 helo=mx2.suse.de) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Worew-0002p7-Dr for qemu-devel@nongnu.org; Mon, 26 May 2014 05:57:34 -0400 Message-ID: <5383100C.3030807@suse.de> Date: Mon, 26 May 2014 11:57:32 +0200 From: Alexander Graf MIME-Version: 1.0 References: <20140526091813.GA31431@lvm> <53830F7A.3060306@redhat.com> In-Reply-To: <53830F7A.3060306@redhat.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] BUG: commit 50a2c6e breaks KVM/ARM (reset/init vcpu order) List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Paolo Bonzini , Christoffer Dall , qemu-devel@nongnu.org Cc: Peter Maydell , afaerber@suse.de, kvm@vger.kernel.org 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 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-}