Message ID | 1412711059-12524-1-git-send-email-wei@redhat.com |
---|---|
State | New |
Headers | show |
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
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 >
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"?
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"? >
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
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 >
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
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.
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. >
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;
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(-)