diff mbox

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

Message ID 20140526091813.GA31431@lvm
State Superseded
Headers show

Commit Message

Christoffer Dall May 26, 2014, 9:18 a.m. UTC 
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:





Please adivce :)

Thanks,
-Christoffer

Comments

Paolo Bonzini May 26, 2014, 9:55 a.m. UTC | #1 
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
Alexander Graf May 26, 2014, 9:57 a.m. UTC | #2 
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-}
Andreas Färber May 26, 2014, 10:20 a.m. UTC | #3 
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-}
Alexander Graf May 26, 2014, 10:31 a.m. UTC | #4 
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
Andreas Färber May 26, 2014, 12:36 p.m. UTC | #5 
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
>
Guan Xuetao May 26, 2014, 12:39 p.m. UTC | #6 
----- 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
Alexander Graf May 26, 2014, 9:04 p.m. UTC | #7 
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
diff mbox

Patch

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