Bug 1135772

Confirmed. I have posted a patch to upstream qemu:

http://lists.nongnu.org/archive/html/qemu-devel/2014-10/msg00794.html

-Wei

While the patch is being reviewed by the upstream, here are the detailed findings:

1. First of all, in my opinion this isn't a bug. Even though users can specifify "threads=n" for AMD CPU, QEMU converts such settings into a proper CPUID. Here are some examples:

** "-smp 8,cores=2,threads=2,sockets=2" will be converted to "2 sockets, 4 cores/socket" for guest VM;
** "-smp 8,cores=4,threads=2" will be converted to "1 socket, 8 cores/socket".

2. QEMU takes care of the conversion of threads=n to proper CPUID_0000_0001_EBX[LogicalProcCount] and CPUID_8000_0008_ECX[NC] for AMD CPU. Please refer to http://support.amd.com/TechDocs/25481.pdf for details. Per Paolo, the following patch was original patch to address this problem:

===========
commit 400281af34e5ee6aa9f5496b53d8f82c6fef9319
Author: Andre Przywara <andre.przywara>
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>
    Signed-off-by: Anthony Liguori <aliguori.com>
============

For this bug, 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? I think either way is fine. That is why I called it a non-bug.

-Wei

(In reply to Wei Huang from comment #4)
> For this bug, 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? I think either way is fine. That is why I
> called it a non-bug.

I believe QEMU should either: 1) do exactly what the user asked for; or 2) reject the configuration from the user; and never tweak the settings on behalf of the user.

But we have another question: is the current behavior really "tweaking"? The number of VCPUs per socket must match nr_cores*nr_threads, otherwise the guest will believe some VCPUs are on the wrong socket. We are just not telling the guest that the user also wanted the VCPUs inside each socket to be grouped as "threads" inside "cores" (to be more accurate, we not even hiding that information from the guest completely, as CPUID[4] has it. It is just that the guest chooses to ignore those bits because they are reserved on AMD docs).

And, even if the current behavior is considered "tweaking": keeping compatibility is a strong enough reason to not start rejecting configurations that work today, even if they look odd. Probably the best we can do is to generate a warning on libvirt and/or QEMU. Or, if we decide to really reject some configurations, we can do that only on newer machine-types.

(In reply to Eduardo Habkost from comment #5)
> (In reply to Wei Huang from comment #4)
> > For this bug, 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? I think either way is fine. That is why I
> > called it a non-bug.
> 
> I believe QEMU should either: 1) do exactly what the user asked for; or 2)
> reject the configuration from the user; and never tweak the settings on
> behalf of the user.
That is what the patch does.

> 
> But we have another question: is the current behavior really "tweaking"? The
> number of VCPUs per socket must match nr_cores*nr_threads, otherwise the
> guest will believe some VCPUs are on the wrong socket. We are just not
> telling the guest that the user also wanted the VCPUs inside each socket to
> be grouped as "threads" inside "cores" (to be more accurate, we not even

Current behavior still groups nr_cores*nr_threads on the same socket. But it doesn't honor the request of "threads inside cores". QEMU sliently drops the notion of threads and bumps up nr_cores instead.

> hiding that information from the guest completely, as CPUID[4] has it. It is
> just that the guest chooses to ignore those bits because they are reserved
> on AMD docs).

I think this behavior is legistimate. AMD choose to ignore it because HyperThreading doesn't exist on their CPUs.

> 
> And, even if the current behavior is considered "tweaking": keeping
> compatibility is a strong enough reason to not start rejecting
> configurations that work today, even if they look odd. Probably the best we
> can do is to generate a warning on libvirt and/or QEMU. Or, if we decide to
> really reject some configurations, we can do that only on newer
> machine-types.

Paolo has concerns regarding compability issue as well. If the patch was rejected by the upstream, I think your first suggestion would be a good choice (at least some level of warning). 

Regarding the new machine types, could you elaborate? I am not quite sure about this appraoch. Right now we have various AMD types (Opteron G1, Opteron G2, ...). A new machine type might really confuse users.

(In reply to Wei Huang from comment #6)
> > And, even if the current behavior is considered "tweaking": keeping
> > compatibility is a strong enough reason to not start rejecting
> > configurations that work today, even if they look odd. Probably the best we
> > can do is to generate a warning on libvirt and/or QEMU. Or, if we decide to
> > really reject some configurations, we can do that only on newer
> > machine-types.
> 
> Paolo has concerns regarding compability issue as well. If the patch was
> rejected by the upstream, I think your first suggestion would be a good
> choice (at least some level of warning). 
> 
> Regarding the new machine types, could you elaborate? I am not quite sure
> about this appraoch. Right now we have various AMD types (Opteron G1,
> Opteron G2, ...). A new machine type might really confuse users.

Opteron_G[1234] are CPU models, not machine-types. machine-types are the mechanism we use to introduce guest-visible changes while keeping compatibility of existing configurations. See the QEMUMachine structs, compat_props arrays, and pc_compat_*() functions on hw/i386/pc_piix.c and hw/i386/pc_q35.c. For an example of a compatibility bit directly related to CPU topology, see enable_compat_apic_id_mode().

Here is a quick update:

1) The consensus from QEMU community is to give a warning, instead of stopping QEMU. 
2) Based on the idea of (1), the proposal is to have warnings in two components: QEMU and virt-manager. 

The patch to print warning in QEMU has been accepted: http://lists.nongnu.org/archive/html/qemu-devel/2014-10/msg02275.html.

The virt-manager patches have been posted to its mailing list:
https://www.redhat.com/archives/virt-tools-list/2014-October/msg00135.html

patch introduced by commit
e48638f target-i386: warns users when CPU threads>1 for non-Intel CPUs

[root@dhcp-11-50 qemu-kvm-rhev7]# rpm -q qemu-img-rhev
qemu-img-rhev-2.3.0-6.el7.x86_64
[root@dhcp-11-50 qemu-kvm-rhev7]# /usr/libexec/qemu-kvm -S -monitor stdio -smp 4,cores=1,threads=2,sockets=2 
QEMU 2.3.0 monitor - type 'help' for more information
(qemu) qemu-kvm: AMD CPU doesn't support hyperthreading. Please configure -smp options properly.
VNC server running on `::1:5900'

(qemu) q

Since the problem described in this bug report should be
resolved in a recent advisory, it has been closed with a
resolution of ERRATA.

For information on the advisory, and where to find the updated
files, follow the link below.

If the solution does not work for you, open a new bug report.

https://rhn.redhat.com/errata/RHBA-2015-2546.html