[Qemu-devel] [PATCH 1/1] target-i386: prevent users from setting threads>1 for AMD CPUs

[Qemu-devel] [PATCH 1/1] target-i386: prevent users from setting threads>1 for AMD CPUs

[Wei Huang]

AMD CPU doesn't support hyperthreading. Even though QEMU fixes
this issue by setting CPUID_0000_0001_EBX and CPUID_8000_0008_ECX
via conversion, it is better to stop end-users in the first place
with a warning message.

Signed-off-by: Wei Huang <wei@redhat.com>
---
 target-i386/cpu.c | 23 ++++++++++++++++++++---
 1 file changed, 20 insertions(+), 3 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index e7bf9de..c377cd2 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -2696,6 +2696,10 @@ static void x86_cpu_apic_realize(X86CPU *cpu, Error **errp)
 }
 #endif
 
+#define IS_AMD_CPU(env) ((env)->cpuid_vendor1 == CPUID_VENDOR_AMD_1 && \
+                         (env)->cpuid_vendor2 == CPUID_VENDOR_AMD_2 && \
+                         (env)->cpuid_vendor3 == CPUID_VENDOR_AMD_3)
+
 static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
 {
     CPUState *cs = CPU(dev);
@@ -2711,9 +2715,7 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
     /* On AMD CPUs, some CPUID[8000_0001].EDX bits must match the bits on
      * CPUID[1].EDX.
      */
-    if (env->cpuid_vendor1 == CPUID_VENDOR_AMD_1 &&
-        env->cpuid_vendor2 == CPUID_VENDOR_AMD_2 &&
-        env->cpuid_vendor3 == CPUID_VENDOR_AMD_3) {
+    if (IS_AMD_CPU(env)) {
         env->features[FEAT_8000_0001_EDX] &= ~CPUID_EXT2_AMD_ALIASES;
         env->features[FEAT_8000_0001_EDX] |= (env->features[FEAT_1_EDX]
            & CPUID_EXT2_AMD_ALIASES);
@@ -2742,6 +2744,21 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
     mce_init(cpu);
     qemu_init_vcpu(cs);
 
+    /* AMD CPU doesn't support hyperthreading. Even though QEMU does fix
+     * this issue by setting CPUID_0000_0001_EBX and CPUID_8000_0008_ECX
+     * correctly, it is still better to stop end-users in the first place
+     * by giving out a warning message.
+     *
+     * NOTE: cs->nr_threads is initialized in qemu_init_vcpu(). So the
+     * following code has to follow qemu_init_vcpu().
+     */
+    if (IS_AMD_CPU(env) && (cs->nr_threads > 1)) {
+        error_setg(&local_err,
+                   "AMD CPU doesn't support hyperthreading. Please configure "
+                   "-smp options correctly.");
+        goto out;
+    }
+
     x86_cpu_apic_realize(cpu, &local_err);
     if (local_err != NULL) {
         goto out;
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 1/1] target-i386: prevent users from setting threads>1 for AMD CPUs

[Paolo Bonzini]

Il 07/10/2014 21:44, Wei Huang ha scritto:
> AMD CPU doesn't support hyperthreading. Even though QEMU fixes
> this issue by setting CPUID_0000_0001_EBX and CPUID_8000_0008_ECX
> via conversion, it is better to stop end-users in the first place
> with a warning message.

Hi Wei,

what exactly breaks if you try creating an AMD VM with hyperthreading?

I am worried that the default CPU is an AMD one when KVM is disabled,
and thus "qemu-system-x86_64 -smp threads=2" will likely be broken.

Paolo

Re: [Qemu-devel] [PATCH 1/1] target-i386: prevent users from setting threads>1 for AMD CPUs

[Wei Huang]

On 10/07/2014 03:58 PM, Paolo Bonzini wrote:
> Il 07/10/2014 21:44, Wei Huang ha scritto:
>> AMD CPU doesn't support hyperthreading. Even though QEMU fixes
>> this issue by setting CPUID_0000_0001_EBX and CPUID_8000_0008_ECX
>> via conversion, it is better to stop end-users in the first place
>> with a warning message.
>
> Hi Wei,
>
> what exactly breaks if you try creating an AMD VM with hyperthreading?
Hi Paolo,

It isn't a bug IMO. I tested various combinations; and current QEMU 
handles it very well. It converts threads=n to proper 
CPUID_0000_0001_EBX[LogicalProcCount] and CPUID_8000_0008_ECX[NC] 
accordingly for AMD.

There is a bugzilla reported for such configuration: 
https://bugzilla.redhat.com/show_bug.cgi?id=1135772. So I thought such 
checking might be a good thing to do.
>
> I am worried that the default CPU is an AMD one when KVM is disabled,
> and thus "qemu-system-x86_64 -smp threads=2" will likely be broken.

"-smp threads=2" will be rejected by the patch. Unless the meaning of 
threads is not limited to threads-per-core, shouldn't end users use 
"-smp 2" in this case or something like "-smp 2,cores=2,sockets=1"?

>
> Paolo
>

Re: [Qemu-devel] [PATCH 1/1] target-i386: prevent users from setting threads>1 for AMD CPUs

[Paolo Bonzini]

Il 07/10/2014 23:16, Wei Huang ha scritto:
> It isn't a bug IMO. I tested various combinations; and current QEMU
> handles it very well. It converts threads=n to proper
> CPUID_0000_0001_EBX[LogicalProcCount] and CPUID_8000_0008_ECX[NC]
> accordingly for AMD.

So if it ain't broken, don't fix it. :)

>> I am worried that the default CPU is an AMD one when KVM is disabled,
>> and thus "qemu-system-x86_64 -smp threads=2" will likely be broken.
> 
> "-smp threads=2" will be rejected by the patch.

Yeah, I am afraid that is an incompatible change, and one more reason
not to do this.  AMD not selling hyperthreaded processors does not mean
that they could not be represented properly with the CPUID leaves that
AMD processors support.

Paolo

> Unless the meaning of
> threads is not limited to threads-per-core, shouldn't end users use
> "-smp 2" in this case or something like "-smp 2,cores=2,sockets=1"?

Re: [Qemu-devel] [PATCH 1/1] target-i386: prevent users from setting threads>1 for AMD CPUs

[Wei Huang]

On 10/07/2014 04:36 PM, Paolo Bonzini wrote:
> Il 07/10/2014 23:16, Wei Huang ha scritto:
>> It isn't a bug IMO. I tested various combinations; and current QEMU
>> handles it very well. It converts threads=n to proper
>> CPUID_0000_0001_EBX[LogicalProcCount] and CPUID_8000_0008_ECX[NC]
>> accordingly for AMD.
>
> So if it ain't broken, don't fix it. :)
>
>>> I am worried that the default CPU is an AMD one when KVM is disabled,
>>> and thus "qemu-system-x86_64 -smp threads=2" will likely be broken.
>>
>> "-smp threads=2" will be rejected by the patch.
>
> Yeah, I am afraid that is an incompatible change, and one more reason
> not to do this.  AMD not selling hyperthreaded processors does not mean
> that they could not be represented properly with the CPUID leaves that
> AMD processors support.
I am OK with either way. The key question is: should QEMU presents 
CPUIDs strictly as specified by the command line or QEMU can tweak a 
little bit on behalf of end-users? For instance, if end-users say "-smp 
8,cores=2,threads=2,sockets=2", they meant "two socket, each has two 
2-hyperthread cores". Current QEMU will convert CPUID as "two socket, 
each has 4 cores". My patch will forbid the tweaking...

-Wei

>
> Paolo
>
>> Unless the meaning of
>> threads is not limited to threads-per-core, shouldn't end users use
>> "-smp 2" in this case or something like "-smp 2,cores=2,sockets=1"?
>

Re: [Qemu-devel] [PATCH 1/1] target-i386: prevent users from setting threads>1 for AMD CPUs

[Paolo Bonzini]

Il 08/10/2014 02:41, Wei Huang ha scritto:
> I am OK with either way. The key question is: should QEMU presents
> CPUIDs strictly as specified by the command line or QEMU can tweak a
> little bit on behalf of end-users? For instance, if end-users say "-smp
> 8,cores=2,threads=2,sockets=2", they meant "two socket, each has two
> 2-hyperthread cores". Current QEMU will convert CPUID as "two socket,
> each has 4 cores". My patch will forbid the tweaking...

Understood---it actually looks like it was intentional:

commit 400281af34e5ee6aa9f5496b53d8f82c6fef9319
Author: Andre Przywara <andre.przywara@amd.com>
Date:   Wed Aug 19 15:42:42 2009 +0200

    set CPUID bits to present cores and threads topology
    
    Controlled by the enhanced -smp option set the CPUID bits to present the
    guest the desired topology. This is vendor specific, but (with the exception
    of the CMP_LEGACY bit) not conflicting, so we set all bits everytime.
    There is no real multithreading support for AMD CPUs, so report cores
    instead.
    
    Signed-off-by: Andre Przywara <andre.przywara@amd.com>
    Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>

Paolo

Re: [Qemu-devel] [PATCH 1/1] target-i386: prevent users from setting threads>1 for AMD CPUs

[Wei Huang]

On 10/08/2014 02:47 AM, Paolo Bonzini wrote:
> Il 08/10/2014 02:41, Wei Huang ha scritto:
>> I am OK with either way. The key question is: should QEMU presents
>> CPUIDs strictly as specified by the command line or QEMU can tweak a
>> little bit on behalf of end-users? For instance, if end-users say "-smp
>> 8,cores=2,threads=2,sockets=2", they meant "two socket, each has two
>> 2-hyperthread cores". Current QEMU will convert CPUID as "two socket,
>> each has 4 cores". My patch will forbid the tweaking...
>
> Understood---it actually looks like it was intentional:
>
> commit 400281af34e5ee6aa9f5496b53d8f82c6fef9319
> Author: Andre Przywara <andre.przywara@amd.com>
> Date:   Wed Aug 19 15:42:42 2009 +0200
>
>      set CPUID bits to present cores and threads topology
>
>      Controlled by the enhanced -smp option set the CPUID bits to present the
>      guest the desired topology. This is vendor specific, but (with the exception
>      of the CMP_LEGACY bit) not conflicting, so we set all bits everytime.
>      There is no real multithreading support for AMD CPUs, so report cores
>      instead.
>
>      Signed-off-by: Andre Przywara <andre.przywara@amd.com>
>      Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>

Given that back-ward compatibility is a concern, will the following work?

1. Instead of bailing out, print a warning message (e.g. to log file via 
error_report) in QEMU.
2. [optional] Eduardo Habkost suggested that we can create a new machine 
model which more strictly checks threads=n option for AMD. For any 
existing machine config, we don't force it; but warning message still 
applies. This is optional because it is a bit over-killed IMO.
3. Gives out a warning in virt-manager as well. This is similar to 
"Overcomming CPUs will slow down performance" in current virt-manager 
screen. The message will read "Chosen CPU model doesn't support 
hyperthreading" or something similar.

-Wei

>
> Paolo
>

Re: [Qemu-devel] [PATCH 1/1] target-i386: prevent users from setting threads>1 for AMD CPUs

[Paolo Bonzini]

Il 09/10/2014 22:22, Wei Huang ha scritto:
> 
> Given that back-ward compatibility is a concern, will the following work?
> 
> 1. Instead of bailing out, print a warning message (e.g. to log file via
> error_report) in QEMU.
> 2. [optional] Eduardo Habkost suggested that we can create a new machine
> model which more strictly checks threads=n option for AMD. For any
> existing machine config, we don't force it; but warning message still
> applies. This is optional because it is a bit over-killed IMO.
> 3. Gives out a warning in virt-manager as well. This is similar to
> "Overcomming CPUs will slow down performance" in current virt-manager
> screen. The message will read "Chosen CPU model doesn't support
> hyperthreading" or something similar.

I like (1) and (3).

Paolo

Re: [Qemu-devel] [PATCH 1/1] target-i386: prevent users from setting threads>1 for AMD CPUs

[Eduardo Habkost]

On Thu, Oct 09, 2014 at 11:08:03PM +0200, Paolo Bonzini wrote:
> Il 09/10/2014 22:22, Wei Huang ha scritto:
> > 
> > Given that back-ward compatibility is a concern, will the following work?
> > 
> > 1. Instead of bailing out, print a warning message (e.g. to log file via
> > error_report) in QEMU.
> > 2. [optional] Eduardo Habkost suggested that we can create a new machine
> > model which more strictly checks threads=n option for AMD. For any
> > existing machine config, we don't force it; but warning message still
> > applies. This is optional because it is a bit over-killed IMO.
> > 3. Gives out a warning in virt-manager as well. This is similar to
> > "Overcomming CPUs will slow down performance" in current virt-manager
> > screen. The message will read "Chosen CPU model doesn't support
> > hyperthreading" or something similar.
> 
> I like (1) and (3).

I don't think we really need (2), either. The current problem is just
user confusion, so properly warning the user is the best thing to do.

-- 
Eduardo

Re: [Qemu-devel] [PATCH 1/1] target-i386: prevent users from setting threads>1 for AMD CPUs

[Wei Huang]

Patch posted. I will post another one for virt-manager.

-Wei

On 10/09/2014 05:16 PM, Eduardo Habkost wrote:
> On Thu, Oct 09, 2014 at 11:08:03PM +0200, Paolo Bonzini wrote:
>> Il 09/10/2014 22:22, Wei Huang ha scritto:
>>>
>>> Given that back-ward compatibility is a concern, will the following work?
>>>
>>> 1. Instead of bailing out, print a warning message (e.g. to log file via
>>> error_report) in QEMU.
>>> 2. [optional] Eduardo Habkost suggested that we can create a new machine
>>> model which more strictly checks threads=n option for AMD. For any
>>> existing machine config, we don't force it; but warning message still
>>> applies. This is optional because it is a bit over-killed IMO.
>>> 3. Gives out a warning in virt-manager as well. This is similar to
>>> "Overcomming CPUs will slow down performance" in current virt-manager
>>> screen. The message will read "Chosen CPU model doesn't support
>>> hyperthreading" or something similar.
>>
>> I like (1) and (3).
> 
> I don't think we really need (2), either. The current problem is just
> user confusion, so properly warning the user is the best thing to do.
> 

[Qemu-devel] [PATCH 1/1] target-i386: prevent users from setting threads>1 for AMD CPUs

[Wei Huang]

AMD CPU doesn't support hyperthreading. Even though QEMU fixes
this issue by setting CPUID_0000_0001_EBX and CPUID_8000_0008_ECX
via conversion, it is better to stop end-users in the first place
with a warning message.

Signed-off-by: Wei Huang <wei@redhat.com>
---
 target-i386/cpu.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index e7bf9de..01bbcaf 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -2742,6 +2742,24 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
     mce_init(cpu);
     qemu_init_vcpu(cs);
 
+    /* AMD CPU doesn't support hyperthreading. Even though QEMU does fix
+     * this issue by setting CPUID_0000_0001_EBX and CPUID_8000_0008_ECX
+     * correctly, it is still better to stop end-users in the first place
+     * by giving out a warning message.
+     *
+     * NOTE: cs->nr_threads is initialized in qemu_init_vcpu(). So the
+     * following code has to follow qemu_init_vcpu().
+     */
+    if (env->cpuid_vendor1 == CPUID_VENDOR_AMD_1 &&
+        env->cpuid_vendor2 == CPUID_VENDOR_AMD_2 &&
+        env->cpuid_vendor3 == CPUID_VENDOR_AMD_3 &&
+        (cs->nr_threads > 1)) {
+        error_setg(&local_err,
+                   "AMD CPU doesn't support hyperthreading. Please configure "
+                   "-smp options correctly.");
+        goto out;
+    }
+
     x86_cpu_apic_realize(cpu, &local_err);
     if (local_err != NULL) {
         goto out;
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 1/1] target-i386: prevent users from setting threads>1 for AMD CPUs

[Wei Huang]

Sorry, please skip this version. I am sending out a updated one.

-Wei

On 10/07/2014 02:17 PM, Wei Huang wrote:
> AMD CPU doesn't support hyperthreading. Even though QEMU fixes
> this issue by setting CPUID_0000_0001_EBX and CPUID_8000_0008_ECX
> via conversion, it is better to stop end-users in the first place
> with a warning message.
>
> Signed-off-by: Wei Huang <wei@redhat.com>
> ---
>   target-i386/cpu.c | 18 ++++++++++++++++++
>   1 file changed, 18 insertions(+)
>
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index e7bf9de..01bbcaf 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -2742,6 +2742,24 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
>       mce_init(cpu);
>       qemu_init_vcpu(cs);
>
> +    /* AMD CPU doesn't support hyperthreading. Even though QEMU does fix
> +     * this issue by setting CPUID_0000_0001_EBX and CPUID_8000_0008_ECX
> +     * correctly, it is still better to stop end-users in the first place
> +     * by giving out a warning message.
> +     *
> +     * NOTE: cs->nr_threads is initialized in qemu_init_vcpu(). So the
> +     * following code has to follow qemu_init_vcpu().
> +     */
> +    if (env->cpuid_vendor1 == CPUID_VENDOR_AMD_1 &&
> +        env->cpuid_vendor2 == CPUID_VENDOR_AMD_2 &&
> +        env->cpuid_vendor3 == CPUID_VENDOR_AMD_3 &&
> +        (cs->nr_threads > 1)) {
> +        error_setg(&local_err,
> +                   "AMD CPU doesn't support hyperthreading. Please configure "
> +                   "-smp options correctly.");
> +        goto out;
> +    }
> +
>       x86_cpu_apic_realize(cpu, &local_err);
>       if (local_err != NULL) {
>           goto out;
>