Message ID | 1315460008-22657-1-git-send-email-bharata.rao@gmail.com |
---|---|
State | New |
Headers | show |
On 2011-09-08 07:33, bharata.rao@gmail.com wrote: > From: Bharata B Rao <bharata.rao@gmail.com> > > apic id returned to guest kernel in ebx for cpuid(function=1) depends on > CPUX86State->cpuid_apic_id which gets populated after the cpuid information > is cached in the host kernel. This results in broken CPU topology in guest. > > Fix this by setting cpuid_apic_id before cpuid information is passed to > the host kernel. This is done by moving the setting of cpuid_apic_id > to cpu_x86_init() where it will work for both KVM as well as TCG modes. > > Signed-off-by: Bharata B Rao <bharata.rao@gmail.com> > --- > This is the next post of the fix that addresses Jan's comment about > bringing back (smp_cpus > 1) check. > > The previous version was posted here: > http://lists.gnu.org/archive/html/qemu-devel/2011-09/msg00892.html > > I coudn't boot a 486 kernel successfully with qemu and hence not > sure if and how this fix breaks i486. Any help from Jan or others > who might have easy means to boot 486 would be good. At least it preserves the current logic, just moves it up in the initialization path. > > hw/pc.c | 1 - > target-i386/helper.c | 5 +++++ > 2 files changed, 5 insertions(+), 1 deletions(-) > > diff --git a/hw/pc.c b/hw/pc.c > index 5bc845a..f9cca4d 100644 > --- a/hw/pc.c > +++ b/hw/pc.c > @@ -933,7 +933,6 @@ static CPUState *pc_new_cpu(const char *cpu_model) > exit(1); > } > if ((env->cpuid_features & CPUID_APIC) || smp_cpus > 1) { > - env->cpuid_apic_id = env->cpu_index; > env->apic_state = apic_init(env, env->cpuid_apic_id); > } > qemu_register_reset(pc_cpu_reset, env); > diff --git a/target-i386/helper.c b/target-i386/helper.c > index 5df40d4..139a193 100644 > --- a/target-i386/helper.c > +++ b/target-i386/helper.c > @@ -1256,6 +1256,11 @@ CPUX86State *cpu_x86_init(const char *cpu_model) > cpu_x86_close(env); > return NULL; > } > + > + if ((env->cpuid_features & CPUID_APIC) || smp_cpus > 1) { > + env->cpuid_apic_id = env->cpu_index; > + } > + > mce_init(env); > > qemu_init_vcpu(env); Tested-and-acked-by: Jan Kiszka <jan.kiszka@siemens.com> Just in time, we happen to hit this bug today too. It confused libvirt in the guest quite a bit... Jan
On 09/08/2011 12:33 AM, bharata.rao@gmail.com wrote: > From: Bharata B Rao<bharata.rao@gmail.com> > > apic id returned to guest kernel in ebx for cpuid(function=1) depends on > CPUX86State->cpuid_apic_id which gets populated after the cpuid information > is cached in the host kernel. This results in broken CPU topology in guest. > > Fix this by setting cpuid_apic_id before cpuid information is passed to > the host kernel. This is done by moving the setting of cpuid_apic_id > to cpu_x86_init() where it will work for both KVM as well as TCG modes. > > Signed-off-by: Bharata B Rao<bharata.rao@gmail.com> This breaks the build: CC i386-linux-user/helper.o /home/anthony/git/qemu/target-i386/helper.c: In function ‘cpu_x86_init’: /home/anthony/git/qemu/target-i386/helper.c:1260:47: error: ‘smp_cpus’ undeclared (first use in this function) /home/anthony/git/qemu/target-i386/helper.c:1260:47: note: each undeclared identifier is reported only once for each function it appears in make[1]: *** [helper.o] Error 1 make: *** [subdir-i386-linux-user] Error 2 Please do a full build before submitting patches. Regards, Anthony Liguori > --- > This is the next post of the fix that addresses Jan's comment about > bringing back (smp_cpus> 1) check. > > The previous version was posted here: > http://lists.gnu.org/archive/html/qemu-devel/2011-09/msg00892.html > > I coudn't boot a 486 kernel successfully with qemu and hence not > sure if and how this fix breaks i486. Any help from Jan or others > who might have easy means to boot 486 would be good. > > hw/pc.c | 1 - > target-i386/helper.c | 5 +++++ > 2 files changed, 5 insertions(+), 1 deletions(-) > > diff --git a/hw/pc.c b/hw/pc.c > index 5bc845a..f9cca4d 100644 > --- a/hw/pc.c > +++ b/hw/pc.c > @@ -933,7 +933,6 @@ static CPUState *pc_new_cpu(const char *cpu_model) > exit(1); > } > if ((env->cpuid_features& CPUID_APIC) || smp_cpus> 1) { > - env->cpuid_apic_id = env->cpu_index; > env->apic_state = apic_init(env, env->cpuid_apic_id); > } > qemu_register_reset(pc_cpu_reset, env); > diff --git a/target-i386/helper.c b/target-i386/helper.c > index 5df40d4..139a193 100644 > --- a/target-i386/helper.c > +++ b/target-i386/helper.c > @@ -1256,6 +1256,11 @@ CPUX86State *cpu_x86_init(const char *cpu_model) > cpu_x86_close(env); > return NULL; > } > + > + if ((env->cpuid_features& CPUID_APIC) || smp_cpus> 1) { > + env->cpuid_apic_id = env->cpu_index; > + } > + > mce_init(env); > > qemu_init_vcpu(env);
On Fri, Sep 9, 2011 at 10:44 PM, Anthony Liguori <anthony@codemonkey.ws> wrote: > > This breaks the build: > > CC i386-linux-user/helper.o > /home/anthony/git/qemu/target-i386/helper.c: In function ‘cpu_x86_init’: > /home/anthony/git/qemu/target-i386/helper.c:1260:47: error: ‘smp_cpus’ > undeclared (first use in this function) > /home/anthony/git/qemu/target-i386/helper.c:1260:47: note: each undeclared > identifier is reported only once for each function it appears in > make[1]: *** [helper.o] Error 1 > make: *** [subdir-i386-linux-user] Error 2 > > Please do a full build before submitting patches. Sorry about this, the complexity of qemu and its capability to support/emulate such wide range of targets is slowly sinking in :) smp_cpus is defined in vl.c which doesn't even get compiled for targets like i386-linux-user and hence we end up in above undefined reference error. I wouldn't attempt another fix until I spend some time understanding the code organization for various targets. Meanwhile since the root cause of the probem and the solution are known, if anyone more well versed with qemu code organization can fix this, please go ahead. Regards, Bharata.
On 2011-09-10 16:37, Bharata B Rao wrote: > On Fri, Sep 9, 2011 at 10:44 PM, Anthony Liguori <anthony@codemonkey.ws> wrote: >> >> This breaks the build: >> >> CC i386-linux-user/helper.o >> /home/anthony/git/qemu/target-i386/helper.c: In function ‘cpu_x86_init’: >> /home/anthony/git/qemu/target-i386/helper.c:1260:47: error: ‘smp_cpus’ >> undeclared (first use in this function) >> /home/anthony/git/qemu/target-i386/helper.c:1260:47: note: each undeclared >> identifier is reported only once for each function it appears in >> make[1]: *** [helper.o] Error 1 >> make: *** [subdir-i386-linux-user] Error 2 >> >> Please do a full build before submitting patches. > > Sorry about this, the complexity of qemu and its capability to > support/emulate such wide range of targets is slowly sinking in :) > > smp_cpus is defined in vl.c which doesn't even get compiled for > targets like i386-linux-user and hence we end up in above undefined > reference error. > > I wouldn't attempt another fix until I spend some time understanding > the code organization for various targets. Meanwhile since the root > cause of the probem and the solution are known, if anyone more well > versed with qemu code organization can fix this, please go ahead. Just assign cpuid_apic_id unconditionally in cpu_x86_init. Will have no side effects. Jan
diff --git a/hw/pc.c b/hw/pc.c index 5bc845a..f9cca4d 100644 --- a/hw/pc.c +++ b/hw/pc.c @@ -933,7 +933,6 @@ static CPUState *pc_new_cpu(const char *cpu_model) exit(1); } if ((env->cpuid_features & CPUID_APIC) || smp_cpus > 1) { - env->cpuid_apic_id = env->cpu_index; env->apic_state = apic_init(env, env->cpuid_apic_id); } qemu_register_reset(pc_cpu_reset, env); diff --git a/target-i386/helper.c b/target-i386/helper.c index 5df40d4..139a193 100644 --- a/target-i386/helper.c +++ b/target-i386/helper.c @@ -1256,6 +1256,11 @@ CPUX86State *cpu_x86_init(const char *cpu_model) cpu_x86_close(env); return NULL; } + + if ((env->cpuid_features & CPUID_APIC) || smp_cpus > 1) { + env->cpuid_apic_id = env->cpu_index; + } + mce_init(env); qemu_init_vcpu(env);