[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 1/1] cpu: Correct cpu-hotplug failure
From: |
chenfan |
Subject: |
Re: [Qemu-devel] [PATCH 1/1] cpu: Correct cpu-hotplug failure |
Date: |
Thu, 25 Jul 2013 17:13:18 +0800 |
On Thu, 2013-07-25 at 10:39 +0200, Andreas Färber wrote:
> Hi,
>
> Am 25.07.2013 08:27, schrieb Chen Fan:
> > When useing x86_64-softmmu --enable-kvm boot qemu, cpu-add command fails to
> > add a vcpu,
> > there show (KVM: setting VAPIC address failed).
> >
> > The reason is that we use an uninitialized cpu->kvm-fd to ioctl.
> > so we move realizing apic to the back of qemu_init_vcpu.
> >
> > Signed-off-by: Chen Fan <address@hidden>
> > ---
> > include/qom/cpu.h | 2 ++
> > qom/cpu.c | 13 +++++++++++++
> > target-i386/cpu.c | 10 ++++------
> > 3 files changed, 19 insertions(+), 6 deletions(-)
>
> First, please CC the maintainer - in this case me.
>
Ok.
> The referenced commit is this one:
> http://git.qemu.org/?p=qemu.git;a=commit;h=c643bed99
>
> Sorry for regressing.
>
> > diff --git a/include/qom/cpu.h b/include/qom/cpu.h
> > index daf1835..487a808 100644
> > --- a/include/qom/cpu.h
> > +++ b/include/qom/cpu.h
> > @@ -80,6 +80,7 @@ struct TranslationBlock;
> > * @synchronize_from_tb: Callback for synchronizing state from a TCG
> > * #TranslationBlock.
> > * @get_phys_page_debug: Callback for obtaining a physical address.
> > + * @apic_realize: Callback for realizing apic.
> > * @vmsd: State description for migration.
> > *
> > * Represents a CPU family or model.
> > @@ -108,6 +109,7 @@ typedef struct CPUClass {
> > void (*set_pc)(CPUState *cpu, vaddr value);
> > void (*synchronize_from_tb)(CPUState *cpu, struct TranslationBlock
> > *tb);
> > hwaddr (*get_phys_page_debug)(CPUState *cpu, vaddr addr);
> > + void (*apic_realize)(CPUState *cpu, Error **errp);
> >
> > const struct VMStateDescription *vmsd;
> > int (*write_elf64_note)(WriteCoreDumpFunction f, CPUState *cpu,
> > diff --git a/qom/cpu.c b/qom/cpu.c
> > index 5c45ab5..88c6028 100644
> > --- a/qom/cpu.c
> > +++ b/qom/cpu.c
> > @@ -213,12 +213,25 @@ static ObjectClass *cpu_common_class_by_name(const
> > char *cpu_model)
> > return NULL;
> > }
> >
> > +static void cpu_apic_realize(CPUState *cpu, Error **errp)
> > +{
> > + CPUClass *cc = CPU_GET_CLASS(cpu);
> > + if (cc->apic_realize != NULL) {
> > + (*cc->apic_realize)(cpu, errp);
> > + }
> > +}
> > +
> > static void cpu_common_realizefn(DeviceState *dev, Error **errp)
> > {
> > CPUState *cpu = CPU(dev);
> >
> > qemu_init_vcpu(cpu);
> >
> > + cpu_apic_realize(cpu, errp);
> > + if (error_is_set(errp)) {
> > + return;
> > + }
> > +
> > if (dev->hotplugged) {
> > cpu_synchronize_post_init(cpu);
> > notifier_list_notify(&cpu_added_notifiers, dev);
>
> This is bogus since APIC is an x86 thing. It should stay in
> target-i386/cpu.c. We could generalize it as a post-vCPU-init hook, but
> I don't think that's needed here:
>
> > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > index cd350cb..916d69e 100644
> > --- a/target-i386/cpu.c
> > +++ b/target-i386/cpu.c
> > @@ -2311,8 +2311,9 @@ static void x86_cpu_apic_create(X86CPU *cpu, Error
> > **errp)
> > apic->cpu = cpu;
> > }
> >
> > -static void x86_cpu_apic_realize(X86CPU *cpu, Error **errp)
> > +static void x86_cpu_apic_realize(CPUState *s, Error **errp)
> > {
> > + X86CPU *cpu = X86_CPU(s);
> > CPUX86State *env = &cpu->env;
> >
> > if (env->apic_state == NULL) {
> > @@ -2326,7 +2327,7 @@ static void x86_cpu_apic_realize(X86CPU *cpu, Error
> > **errp)
> > }
> > }
> > #else
> > -static void x86_cpu_apic_realize(X86CPU *cpu, Error **errp)
> > +static void x86_cpu_apic_realize(CPUState *s, Error **errp)
> > {
> > }
> > #endif
> > @@ -2388,10 +2389,6 @@ static void x86_cpu_realizefn(DeviceState *dev,
> > Error **errp)
> >
> > mce_init(cpu);
> >
> > - x86_cpu_apic_realize(cpu, &local_err);
> > - if (local_err != NULL) {
> > - goto out;
> > - }
> > cpu_reset(CPU(cpu));
> >
> > xcc->parent_realize(dev, &local_err);
>
> I think all that really needs to happen is to move the above four lines
> to below xcc->parent_realize(). I doubt that a hot-add notifier is going
> to fiddle with the APIC - we're just incrementing the number of CPUs in
> the RTC today.
>
> Can you send a v2 doing so please? Thanks.
>
I have already done so, unfortunately,when cpu-add, showing "smpboot:
CPU1: Not responding."
Maybe we need to use the post-vCPU-init hook to do this.
Thanks,
Chen
> Regards,
> Andreas
>
> > @@ -2540,6 +2537,7 @@ static void x86_cpu_common_class_init(ObjectClass
> > *oc, void *data)
> > cc->synchronize_from_tb = x86_cpu_synchronize_from_tb;
> > cc->get_arch_id = x86_cpu_get_arch_id;
> > cc->get_paging_enabled = x86_cpu_get_paging_enabled;
> > + cc->apic_realize = x86_cpu_apic_realize;
> > #ifndef CONFIG_USER_ONLY
> > cc->get_memory_mapping = x86_cpu_get_memory_mapping;
> > cc->get_phys_page_debug = x86_cpu_get_phys_page_debug;
> >
>
>