[Qemu-devel] [PATCH v2] Fix X86 CPU topology in KVM mode

[Qemu-devel] [PATCH v2] Fix X86 CPU topology in KVM mode

[bharata.rao]

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.

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

Re: [Qemu-devel] [PATCH v2] Fix X86 CPU topology in KVM mode

[Jan Kiszka]

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

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] [PATCH v2] Fix X86 CPU topology in KVM mode

[Anthony Liguori]

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

Re: [Qemu-devel] [PATCH v2] Fix X86 CPU topology in KVM mode

[Bharata B Rao]

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.

Re: [Qemu-devel] [PATCH v2] Fix X86 CPU topology in KVM mode

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 1370 bytes --]

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]