| Message ID | 20170419195413.30141-2-bobby.prani@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Apr 19, 2017 at 3:54 PM, Pranith Kumar <bobby.prani@gmail.com> wrote: > When we enable hyperthreading (using threads smp argument), we warn > the user if the cpu is an AMD cpu. This does not make sense on TCG and > is also obsolete now that AMD Ryzen support hyperthreading. > > Fix this by adding CPUID_HT bit to the TCG features and explicitly > checking this bit in the cpuid. > > Signed-off-by: Pranith Kumar <bobby.prani@gmail.com> > --- > target/i386/cpu.c | 10 +++++----- > 1 file changed, 5 insertions(+), 5 deletions(-) > > diff --git a/target/i386/cpu.c b/target/i386/cpu.c > index 13c0985f11..f34bb5ead7 100644 > --- a/target/i386/cpu.c > +++ b/target/i386/cpu.c > @@ -202,12 +202,12 @@ static void x86_cpu_vendor_words2str(char *dst, uint32_t vendor1, > #define TCG_FEATURES (CPUID_FP87 | CPUID_PSE | CPUID_TSC | CPUID_MSR | \ > CPUID_PAE | CPUID_MCE | CPUID_CX8 | CPUID_APIC | CPUID_SEP | \ > CPUID_MTRR | CPUID_PGE | CPUID_MCA | CPUID_CMOV | CPUID_PAT | \ > - CPUID_PSE36 | CPUID_CLFLUSH | CPUID_ACPI | CPUID_MMX | \ > + CPUID_PSE36 | CPUID_CLFLUSH | CPUID_ACPI | CPUID_MMX | CPUID_HT | \ > CPUID_FXSR | CPUID_SSE | CPUID_SSE2 | CPUID_SS | CPUID_DE) > /* partly implemented: > CPUID_MTRR, CPUID_MCA, CPUID_CLFLUSH (needed for Win64) */ > /* missing: > - CPUID_VME, CPUID_DTS, CPUID_SS, CPUID_HT, CPUID_TM, CPUID_PBE */ > + CPUID_VME, CPUID_DTS, CPUID_SS, CPUID_TM, CPUID_PBE */ > #define TCG_EXT_FEATURES (CPUID_EXT_SSE3 | CPUID_EXT_PCLMULQDQ | \ > CPUID_EXT_MONITOR | CPUID_EXT_SSSE3 | CPUID_EXT_CX16 | \ > CPUID_EXT_SSE41 | CPUID_EXT_SSE42 | CPUID_EXT_POPCNT | \ > @@ -3643,7 +3643,7 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp) > > qemu_init_vcpu(cs); > > - /* Only Intel CPUs support hyperthreading. Even though QEMU fixes this > + /* Only some CPUs support hyperthreading. Even though QEMU fixes this > * issue by adjusting CPUID_0000_0001_EBX and CPUID_8000_0008_ECX > * based on inputs (sockets,cores,threads), it is still better to gives > * users a warning. > @@ -3651,8 +3651,8 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp) > * NOTE: the following code has to follow qemu_init_vcpu(). Otherwise > * cs->nr_threads hasn't be populated yet and the checking is incorrect. > */ > - if (!IS_INTEL_CPU(env) && cs->nr_threads > 1 && !ht_warned) { > - error_report("AMD CPU doesn't support hyperthreading. Please configure" > + if ((env->features[FEAT_1_EDX] & CPUID_HT) && (cs->nr_threads > 1) && !ht_warned) { I missed a '!' here. We need to warn if CPUID_HT is not set. But I see that it is not being set even on HT enabled processors (i7-3770). How do I test for it?
On Wed, Apr 19, 2017 at 04:00:49PM -0400, Pranith Kumar wrote: > On Wed, Apr 19, 2017 at 3:54 PM, Pranith Kumar <bobby.prani@gmail.com> wrote: > > When we enable hyperthreading (using threads smp argument), we warn > > the user if the cpu is an AMD cpu. This does not make sense on TCG and > > is also obsolete now that AMD Ryzen support hyperthreading. > > > > Fix this by adding CPUID_HT bit to the TCG features and explicitly > > checking this bit in the cpuid. > > > > Signed-off-by: Pranith Kumar <bobby.prani@gmail.com> > > --- > > target/i386/cpu.c | 10 +++++----- > > 1 file changed, 5 insertions(+), 5 deletions(-) > > > > diff --git a/target/i386/cpu.c b/target/i386/cpu.c > > index 13c0985f11..f34bb5ead7 100644 > > --- a/target/i386/cpu.c > > +++ b/target/i386/cpu.c > > @@ -202,12 +202,12 @@ static void x86_cpu_vendor_words2str(char *dst, uint32_t vendor1, > > #define TCG_FEATURES (CPUID_FP87 | CPUID_PSE | CPUID_TSC | CPUID_MSR | \ > > CPUID_PAE | CPUID_MCE | CPUID_CX8 | CPUID_APIC | CPUID_SEP | \ > > CPUID_MTRR | CPUID_PGE | CPUID_MCA | CPUID_CMOV | CPUID_PAT | \ > > - CPUID_PSE36 | CPUID_CLFLUSH | CPUID_ACPI | CPUID_MMX | \ > > + CPUID_PSE36 | CPUID_CLFLUSH | CPUID_ACPI | CPUID_MMX | CPUID_HT | \ > > CPUID_FXSR | CPUID_SSE | CPUID_SSE2 | CPUID_SS | CPUID_DE) > > /* partly implemented: > > CPUID_MTRR, CPUID_MCA, CPUID_CLFLUSH (needed for Win64) */ > > /* missing: > > - CPUID_VME, CPUID_DTS, CPUID_SS, CPUID_HT, CPUID_TM, CPUID_PBE */ > > + CPUID_VME, CPUID_DTS, CPUID_SS, CPUID_TM, CPUID_PBE */ > > #define TCG_EXT_FEATURES (CPUID_EXT_SSE3 | CPUID_EXT_PCLMULQDQ | \ > > CPUID_EXT_MONITOR | CPUID_EXT_SSSE3 | CPUID_EXT_CX16 | \ > > CPUID_EXT_SSE41 | CPUID_EXT_SSE42 | CPUID_EXT_POPCNT | \ > > @@ -3643,7 +3643,7 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp) > > > > qemu_init_vcpu(cs); > > > > - /* Only Intel CPUs support hyperthreading. Even though QEMU fixes this > > + /* Only some CPUs support hyperthreading. Even though QEMU fixes this > > * issue by adjusting CPUID_0000_0001_EBX and CPUID_8000_0008_ECX > > * based on inputs (sockets,cores,threads), it is still better to gives > > * users a warning. > > @@ -3651,8 +3651,8 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp) > > * NOTE: the following code has to follow qemu_init_vcpu(). Otherwise > > * cs->nr_threads hasn't be populated yet and the checking is incorrect. > > */ > > - if (!IS_INTEL_CPU(env) && cs->nr_threads > 1 && !ht_warned) { > > - error_report("AMD CPU doesn't support hyperthreading. Please configure" > > + if ((env->features[FEAT_1_EDX] & CPUID_HT) && (cs->nr_threads > 1) && !ht_warned) { > > I missed a '!' here. We need to warn if CPUID_HT is not set. But I see > that it is not being set even on HT enabled processors (i7-3770). How > do I test for it? CPUID_HT is automatically set on the CPU if (nr_threads * nr_cores) > 1. See the "case 1:" block in cpu_x86_cpuid(). AFAIK, the point of the warning is to let the user know that the guest OS is likely to ignore thread topology information if CPUID vendor is not Intel, and has nothing to do with TCG or KVM capabilities. Maybe the warning is obsolete today and guests won't be confused by a HT AMD CPU, but we need to confirm that.
On Wed, Apr 19, 2017 at 4:13 PM, Eduardo Habkost <ehabkost@redhat.com> wrote: > On Wed, Apr 19, 2017 at 04:00:49PM -0400, Pranith Kumar wrote: >> On Wed, Apr 19, 2017 at 3:54 PM, Pranith Kumar <bobby.prani@gmail.com> wrote: >> > When we enable hyperthreading (using threads smp argument), we warn >> > the user if the cpu is an AMD cpu. This does not make sense on TCG and >> > is also obsolete now that AMD Ryzen support hyperthreading. >> > >> > Fix this by adding CPUID_HT bit to the TCG features and explicitly >> > checking this bit in the cpuid. >> > >> > Signed-off-by: Pranith Kumar <bobby.prani@gmail.com> >> > --- >> > target/i386/cpu.c | 10 +++++----- >> > 1 file changed, 5 insertions(+), 5 deletions(-) >> > >> > diff --git a/target/i386/cpu.c b/target/i386/cpu.c >> > index 13c0985f11..f34bb5ead7 100644 >> > --- a/target/i386/cpu.c >> > +++ b/target/i386/cpu.c >> > @@ -202,12 +202,12 @@ static void x86_cpu_vendor_words2str(char *dst, uint32_t vendor1, >> > #define TCG_FEATURES (CPUID_FP87 | CPUID_PSE | CPUID_TSC | CPUID_MSR | \ >> > CPUID_PAE | CPUID_MCE | CPUID_CX8 | CPUID_APIC | CPUID_SEP | \ >> > CPUID_MTRR | CPUID_PGE | CPUID_MCA | CPUID_CMOV | CPUID_PAT | \ >> > - CPUID_PSE36 | CPUID_CLFLUSH | CPUID_ACPI | CPUID_MMX | \ >> > + CPUID_PSE36 | CPUID_CLFLUSH | CPUID_ACPI | CPUID_MMX | CPUID_HT | \ >> > CPUID_FXSR | CPUID_SSE | CPUID_SSE2 | CPUID_SS | CPUID_DE) >> > /* partly implemented: >> > CPUID_MTRR, CPUID_MCA, CPUID_CLFLUSH (needed for Win64) */ >> > /* missing: >> > - CPUID_VME, CPUID_DTS, CPUID_SS, CPUID_HT, CPUID_TM, CPUID_PBE */ >> > + CPUID_VME, CPUID_DTS, CPUID_SS, CPUID_TM, CPUID_PBE */ >> > #define TCG_EXT_FEATURES (CPUID_EXT_SSE3 | CPUID_EXT_PCLMULQDQ | \ >> > CPUID_EXT_MONITOR | CPUID_EXT_SSSE3 | CPUID_EXT_CX16 | \ >> > CPUID_EXT_SSE41 | CPUID_EXT_SSE42 | CPUID_EXT_POPCNT | \ >> > @@ -3643,7 +3643,7 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp) >> > >> > qemu_init_vcpu(cs); >> > >> > - /* Only Intel CPUs support hyperthreading. Even though QEMU fixes this >> > + /* Only some CPUs support hyperthreading. Even though QEMU fixes this >> > * issue by adjusting CPUID_0000_0001_EBX and CPUID_8000_0008_ECX >> > * based on inputs (sockets,cores,threads), it is still better to gives >> > * users a warning. >> > @@ -3651,8 +3651,8 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp) >> > * NOTE: the following code has to follow qemu_init_vcpu(). Otherwise >> > * cs->nr_threads hasn't be populated yet and the checking is incorrect. >> > */ >> > - if (!IS_INTEL_CPU(env) && cs->nr_threads > 1 && !ht_warned) { >> > - error_report("AMD CPU doesn't support hyperthreading. Please configure" >> > + if ((env->features[FEAT_1_EDX] & CPUID_HT) && (cs->nr_threads > 1) && !ht_warned) { >> >> I missed a '!' here. We need to warn if CPUID_HT is not set. But I see >> that it is not being set even on HT enabled processors (i7-3770). How >> do I test for it? > > CPUID_HT is automatically set on the CPU if > (nr_threads * nr_cores) > 1. See the "case 1:" block in > cpu_x86_cpuid(). > > AFAIK, the point of the warning is to let the user know that the > guest OS is likely to ignore thread topology information if CPUID > vendor is not Intel, and has nothing to do with TCG or KVM > capabilities. Maybe the warning is obsolete today and guests > won't be confused by a HT AMD CPU, but we need to confirm that. We assume an AMD cpu for x86 TCG and it prints this warning when an smp guest is run with (cores=2,threads=2) argument. The main point of this patch is to remove that warning when using TCG.
On Wed, Apr 19, 2017 at 04:16:53PM -0400, Pranith Kumar wrote: > On Wed, Apr 19, 2017 at 4:13 PM, Eduardo Habkost <ehabkost@redhat.com> wrote: > > On Wed, Apr 19, 2017 at 04:00:49PM -0400, Pranith Kumar wrote: > >> On Wed, Apr 19, 2017 at 3:54 PM, Pranith Kumar <bobby.prani@gmail.com> wrote: > >> > When we enable hyperthreading (using threads smp argument), we warn > >> > the user if the cpu is an AMD cpu. This does not make sense on TCG and > >> > is also obsolete now that AMD Ryzen support hyperthreading. > >> > > >> > Fix this by adding CPUID_HT bit to the TCG features and explicitly > >> > checking this bit in the cpuid. > >> > > >> > Signed-off-by: Pranith Kumar <bobby.prani@gmail.com> > >> > --- > >> > target/i386/cpu.c | 10 +++++----- > >> > 1 file changed, 5 insertions(+), 5 deletions(-) > >> > > >> > diff --git a/target/i386/cpu.c b/target/i386/cpu.c > >> > index 13c0985f11..f34bb5ead7 100644 > >> > --- a/target/i386/cpu.c > >> > +++ b/target/i386/cpu.c > >> > @@ -202,12 +202,12 @@ static void x86_cpu_vendor_words2str(char *dst, uint32_t vendor1, > >> > #define TCG_FEATURES (CPUID_FP87 | CPUID_PSE | CPUID_TSC | CPUID_MSR | \ > >> > CPUID_PAE | CPUID_MCE | CPUID_CX8 | CPUID_APIC | CPUID_SEP | \ > >> > CPUID_MTRR | CPUID_PGE | CPUID_MCA | CPUID_CMOV | CPUID_PAT | \ > >> > - CPUID_PSE36 | CPUID_CLFLUSH | CPUID_ACPI | CPUID_MMX | \ > >> > + CPUID_PSE36 | CPUID_CLFLUSH | CPUID_ACPI | CPUID_MMX | CPUID_HT | \ > >> > CPUID_FXSR | CPUID_SSE | CPUID_SSE2 | CPUID_SS | CPUID_DE) > >> > /* partly implemented: > >> > CPUID_MTRR, CPUID_MCA, CPUID_CLFLUSH (needed for Win64) */ > >> > /* missing: > >> > - CPUID_VME, CPUID_DTS, CPUID_SS, CPUID_HT, CPUID_TM, CPUID_PBE */ > >> > + CPUID_VME, CPUID_DTS, CPUID_SS, CPUID_TM, CPUID_PBE */ > >> > #define TCG_EXT_FEATURES (CPUID_EXT_SSE3 | CPUID_EXT_PCLMULQDQ | \ > >> > CPUID_EXT_MONITOR | CPUID_EXT_SSSE3 | CPUID_EXT_CX16 | \ > >> > CPUID_EXT_SSE41 | CPUID_EXT_SSE42 | CPUID_EXT_POPCNT | \ > >> > @@ -3643,7 +3643,7 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp) > >> > > >> > qemu_init_vcpu(cs); > >> > > >> > - /* Only Intel CPUs support hyperthreading. Even though QEMU fixes this > >> > + /* Only some CPUs support hyperthreading. Even though QEMU fixes this > >> > * issue by adjusting CPUID_0000_0001_EBX and CPUID_8000_0008_ECX > >> > * based on inputs (sockets,cores,threads), it is still better to gives > >> > * users a warning. > >> > @@ -3651,8 +3651,8 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp) > >> > * NOTE: the following code has to follow qemu_init_vcpu(). Otherwise > >> > * cs->nr_threads hasn't be populated yet and the checking is incorrect. > >> > */ > >> > - if (!IS_INTEL_CPU(env) && cs->nr_threads > 1 && !ht_warned) { > >> > - error_report("AMD CPU doesn't support hyperthreading. Please configure" > >> > + if ((env->features[FEAT_1_EDX] & CPUID_HT) && (cs->nr_threads > 1) && !ht_warned) { > >> > >> I missed a '!' here. We need to warn if CPUID_HT is not set. But I see > >> that it is not being set even on HT enabled processors (i7-3770). How > >> do I test for it? > > > > CPUID_HT is automatically set on the CPU if > > (nr_threads * nr_cores) > 1. See the "case 1:" block in > > cpu_x86_cpuid(). > > > > AFAIK, the point of the warning is to let the user know that the > > guest OS is likely to ignore thread topology information if CPUID > > vendor is not Intel, and has nothing to do with TCG or KVM > > capabilities. Maybe the warning is obsolete today and guests > > won't be confused by a HT AMD CPU, but we need to confirm that. > > We assume an AMD cpu for x86 TCG and it prints this warning when an > smp guest is run with (cores=2,threads=2) argument. The main point of > this patch is to remove that warning when using TCG. What is different under TCG? If guests were confused with threads=2 + vendor=AMD using KVM, what exactly would make them not confused by threads=2 + vendor=AMD when using TCG? I just ran a Fedora 25 guest using "-machine accel=tcg -smp 2,threads=2", and it still thinks it's running on a 2-core CPU instead of a 2-thread CPU when CPU vendor is AMD.
On Wed, Apr 19, 2017 at 4:57 PM, Eduardo Habkost <ehabkost@redhat.com> wrote: > On Wed, Apr 19, 2017 at 04:16:53PM -0400, Pranith Kumar wrote: >> On Wed, Apr 19, 2017 at 4:13 PM, Eduardo Habkost <ehabkost@redhat.com> wrote: >> > On Wed, Apr 19, 2017 at 04:00:49PM -0400, Pranith Kumar wrote: >> >> On Wed, Apr 19, 2017 at 3:54 PM, Pranith Kumar <bobby.prani@gmail.com> wrote: >> >> > When we enable hyperthreading (using threads smp argument), we warn >> >> > the user if the cpu is an AMD cpu. This does not make sense on TCG and >> >> > is also obsolete now that AMD Ryzen support hyperthreading. >> >> > >> >> > Fix this by adding CPUID_HT bit to the TCG features and explicitly >> >> > checking this bit in the cpuid. >> >> > >> >> > Signed-off-by: Pranith Kumar <bobby.prani@gmail.com> >> >> > --- >> >> > target/i386/cpu.c | 10 +++++----- >> >> > 1 file changed, 5 insertions(+), 5 deletions(-) >> >> > >> >> > diff --git a/target/i386/cpu.c b/target/i386/cpu.c >> >> > index 13c0985f11..f34bb5ead7 100644 >> >> > --- a/target/i386/cpu.c >> >> > +++ b/target/i386/cpu.c >> >> > @@ -202,12 +202,12 @@ static void x86_cpu_vendor_words2str(char *dst, uint32_t vendor1, >> >> > #define TCG_FEATURES (CPUID_FP87 | CPUID_PSE | CPUID_TSC | CPUID_MSR | \ >> >> > CPUID_PAE | CPUID_MCE | CPUID_CX8 | CPUID_APIC | CPUID_SEP | \ >> >> > CPUID_MTRR | CPUID_PGE | CPUID_MCA | CPUID_CMOV | CPUID_PAT | \ >> >> > - CPUID_PSE36 | CPUID_CLFLUSH | CPUID_ACPI | CPUID_MMX | \ >> >> > + CPUID_PSE36 | CPUID_CLFLUSH | CPUID_ACPI | CPUID_MMX | CPUID_HT | \ >> >> > CPUID_FXSR | CPUID_SSE | CPUID_SSE2 | CPUID_SS | CPUID_DE) >> >> > /* partly implemented: >> >> > CPUID_MTRR, CPUID_MCA, CPUID_CLFLUSH (needed for Win64) */ >> >> > /* missing: >> >> > - CPUID_VME, CPUID_DTS, CPUID_SS, CPUID_HT, CPUID_TM, CPUID_PBE */ >> >> > + CPUID_VME, CPUID_DTS, CPUID_SS, CPUID_TM, CPUID_PBE */ >> >> > #define TCG_EXT_FEATURES (CPUID_EXT_SSE3 | CPUID_EXT_PCLMULQDQ | \ >> >> > CPUID_EXT_MONITOR | CPUID_EXT_SSSE3 | CPUID_EXT_CX16 | \ >> >> > CPUID_EXT_SSE41 | CPUID_EXT_SSE42 | CPUID_EXT_POPCNT | \ >> >> > @@ -3643,7 +3643,7 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp) >> >> > >> >> > qemu_init_vcpu(cs); >> >> > >> >> > - /* Only Intel CPUs support hyperthreading. Even though QEMU fixes this >> >> > + /* Only some CPUs support hyperthreading. Even though QEMU fixes this >> >> > * issue by adjusting CPUID_0000_0001_EBX and CPUID_8000_0008_ECX >> >> > * based on inputs (sockets,cores,threads), it is still better to gives >> >> > * users a warning. >> >> > @@ -3651,8 +3651,8 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp) >> >> > * NOTE: the following code has to follow qemu_init_vcpu(). Otherwise >> >> > * cs->nr_threads hasn't be populated yet and the checking is incorrect. >> >> > */ >> >> > - if (!IS_INTEL_CPU(env) && cs->nr_threads > 1 && !ht_warned) { >> >> > - error_report("AMD CPU doesn't support hyperthreading. Please configure" >> >> > + if ((env->features[FEAT_1_EDX] & CPUID_HT) && (cs->nr_threads > 1) && !ht_warned) { >> >> >> >> I missed a '!' here. We need to warn if CPUID_HT is not set. But I see >> >> that it is not being set even on HT enabled processors (i7-3770). How >> >> do I test for it? >> > >> > CPUID_HT is automatically set on the CPU if >> > (nr_threads * nr_cores) > 1. See the "case 1:" block in >> > cpu_x86_cpuid(). >> > >> > AFAIK, the point of the warning is to let the user know that the >> > guest OS is likely to ignore thread topology information if CPUID >> > vendor is not Intel, and has nothing to do with TCG or KVM >> > capabilities. Maybe the warning is obsolete today and guests >> > won't be confused by a HT AMD CPU, but we need to confirm that. >> >> We assume an AMD cpu for x86 TCG and it prints this warning when an >> smp guest is run with (cores=2,threads=2) argument. The main point of >> this patch is to remove that warning when using TCG. > > What is different under TCG? If guests were confused with > threads=2 + vendor=AMD using KVM, what exactly would make them > not confused by threads=2 + vendor=AMD when using TCG? I am not sure why we chose AMD to be the vendor for TCG since, as you note, it does not really matter if it's AMD or Intel in TCG mode. > > I just ran a Fedora 25 guest using "-machine accel=tcg > -smp 2,threads=2", and it still thinks it's running on a 2-core > CPU instead of a 2-thread CPU when CPU vendor is AMD. This all started with using Windows XP. It has an arbitrary limit on the number of cores it can support to 2 cores. So using -smp 4 will still only show 2 cores. However, we can overcome this by using -smp cores=2,threads=2 and 4 cores show up in the guest. But we see the ugly warning that HT is not supported on AMD cpus. So we can either: * Change the vendor to Intel for TCG, or * Warn based on the HT bit being set Thanks,
On Wed, Apr 19, 2017 at 05:25:23PM -0400, Pranith Kumar wrote: > On Wed, Apr 19, 2017 at 4:57 PM, Eduardo Habkost <ehabkost@redhat.com> wrote: > > On Wed, Apr 19, 2017 at 04:16:53PM -0400, Pranith Kumar wrote: > >> On Wed, Apr 19, 2017 at 4:13 PM, Eduardo Habkost <ehabkost@redhat.com> wrote: > >> > On Wed, Apr 19, 2017 at 04:00:49PM -0400, Pranith Kumar wrote: > >> >> On Wed, Apr 19, 2017 at 3:54 PM, Pranith Kumar <bobby.prani@gmail.com> wrote: > >> >> > When we enable hyperthreading (using threads smp argument), we warn > >> >> > the user if the cpu is an AMD cpu. This does not make sense on TCG and > >> >> > is also obsolete now that AMD Ryzen support hyperthreading. > >> >> > > >> >> > Fix this by adding CPUID_HT bit to the TCG features and explicitly > >> >> > checking this bit in the cpuid. > >> >> > > >> >> > Signed-off-by: Pranith Kumar <bobby.prani@gmail.com> > >> >> > --- > >> >> > target/i386/cpu.c | 10 +++++----- > >> >> > 1 file changed, 5 insertions(+), 5 deletions(-) > >> >> > > >> >> > diff --git a/target/i386/cpu.c b/target/i386/cpu.c > >> >> > index 13c0985f11..f34bb5ead7 100644 > >> >> > --- a/target/i386/cpu.c > >> >> > +++ b/target/i386/cpu.c > >> >> > @@ -202,12 +202,12 @@ static void x86_cpu_vendor_words2str(char *dst, uint32_t vendor1, > >> >> > #define TCG_FEATURES (CPUID_FP87 | CPUID_PSE | CPUID_TSC | CPUID_MSR | \ > >> >> > CPUID_PAE | CPUID_MCE | CPUID_CX8 | CPUID_APIC | CPUID_SEP | \ > >> >> > CPUID_MTRR | CPUID_PGE | CPUID_MCA | CPUID_CMOV | CPUID_PAT | \ > >> >> > - CPUID_PSE36 | CPUID_CLFLUSH | CPUID_ACPI | CPUID_MMX | \ > >> >> > + CPUID_PSE36 | CPUID_CLFLUSH | CPUID_ACPI | CPUID_MMX | CPUID_HT | \ > >> >> > CPUID_FXSR | CPUID_SSE | CPUID_SSE2 | CPUID_SS | CPUID_DE) > >> >> > /* partly implemented: > >> >> > CPUID_MTRR, CPUID_MCA, CPUID_CLFLUSH (needed for Win64) */ > >> >> > /* missing: > >> >> > - CPUID_VME, CPUID_DTS, CPUID_SS, CPUID_HT, CPUID_TM, CPUID_PBE */ > >> >> > + CPUID_VME, CPUID_DTS, CPUID_SS, CPUID_TM, CPUID_PBE */ > >> >> > #define TCG_EXT_FEATURES (CPUID_EXT_SSE3 | CPUID_EXT_PCLMULQDQ | \ > >> >> > CPUID_EXT_MONITOR | CPUID_EXT_SSSE3 | CPUID_EXT_CX16 | \ > >> >> > CPUID_EXT_SSE41 | CPUID_EXT_SSE42 | CPUID_EXT_POPCNT | \ > >> >> > @@ -3643,7 +3643,7 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp) > >> >> > > >> >> > qemu_init_vcpu(cs); > >> >> > > >> >> > - /* Only Intel CPUs support hyperthreading. Even though QEMU fixes this > >> >> > + /* Only some CPUs support hyperthreading. Even though QEMU fixes this > >> >> > * issue by adjusting CPUID_0000_0001_EBX and CPUID_8000_0008_ECX > >> >> > * based on inputs (sockets,cores,threads), it is still better to gives > >> >> > * users a warning. > >> >> > @@ -3651,8 +3651,8 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp) > >> >> > * NOTE: the following code has to follow qemu_init_vcpu(). Otherwise > >> >> > * cs->nr_threads hasn't be populated yet and the checking is incorrect. > >> >> > */ > >> >> > - if (!IS_INTEL_CPU(env) && cs->nr_threads > 1 && !ht_warned) { > >> >> > - error_report("AMD CPU doesn't support hyperthreading. Please configure" > >> >> > + if ((env->features[FEAT_1_EDX] & CPUID_HT) && (cs->nr_threads > 1) && !ht_warned) { > >> >> > >> >> I missed a '!' here. We need to warn if CPUID_HT is not set. But I see > >> >> that it is not being set even on HT enabled processors (i7-3770). How > >> >> do I test for it? > >> > > >> > CPUID_HT is automatically set on the CPU if > >> > (nr_threads * nr_cores) > 1. See the "case 1:" block in > >> > cpu_x86_cpuid(). > >> > > >> > AFAIK, the point of the warning is to let the user know that the > >> > guest OS is likely to ignore thread topology information if CPUID > >> > vendor is not Intel, and has nothing to do with TCG or KVM > >> > capabilities. Maybe the warning is obsolete today and guests > >> > won't be confused by a HT AMD CPU, but we need to confirm that. > >> > >> We assume an AMD cpu for x86 TCG and it prints this warning when an > >> smp guest is run with (cores=2,threads=2) argument. The main point of > >> this patch is to remove that warning when using TCG. > > > > What is different under TCG? If guests were confused with > > threads=2 + vendor=AMD using KVM, what exactly would make them > > not confused by threads=2 + vendor=AMD when using TCG? > > I am not sure why we chose AMD to be the vendor for TCG since, as you > note, it does not really matter if it's AMD or Intel in TCG mode. I don't know why the default on TCG is AMD, either. > > > > > I just ran a Fedora 25 guest using "-machine accel=tcg > > -smp 2,threads=2", and it still thinks it's running on a 2-core > > CPU instead of a 2-thread CPU when CPU vendor is AMD. > > This all started with using Windows XP. It has an arbitrary limit on > the number of cores it can support to 2 cores. So using -smp 4 will > still only show 2 cores. However, we can overcome this by using -smp > cores=2,threads=2 and 4 cores show up in the guest. But we see the > ugly warning that HT is not supported on AMD cpus. Is Windows XP able to detect 2 threads * 2 cores even if CPUID vendor is AMD? > So we can either: > > * Change the vendor to Intel for TCG, or > * Warn based on the HT bit being set Could you clarify what you mean by the second item? The HT bit is always set when nr_cores*nr_threads > 1.
On Wed, Apr 19, 2017 at 5:33 PM, Eduardo Habkost <ehabkost@redhat.com> wrote: > On Wed, Apr 19, 2017 at 05:25:23PM -0400, Pranith Kumar wrote: >> On Wed, Apr 19, 2017 at 4:57 PM, Eduardo Habkost <ehabkost@redhat.com> wrote: >> > On Wed, Apr 19, 2017 at 04:16:53PM -0400, Pranith Kumar wrote: >> >> On Wed, Apr 19, 2017 at 4:13 PM, Eduardo Habkost <ehabkost@redhat.com> wrote: >> >> > On Wed, Apr 19, 2017 at 04:00:49PM -0400, Pranith Kumar wrote: >> >> >> On Wed, Apr 19, 2017 at 3:54 PM, Pranith Kumar <bobby.prani@gmail.com> wrote: >> >> >> > When we enable hyperthreading (using threads smp argument), we warn >> >> >> > the user if the cpu is an AMD cpu. This does not make sense on TCG and >> >> >> > is also obsolete now that AMD Ryzen support hyperthreading. >> >> >> > >> >> >> > Fix this by adding CPUID_HT bit to the TCG features and explicitly >> >> >> > checking this bit in the cpuid. >> >> >> > >> >> >> > Signed-off-by: Pranith Kumar <bobby.prani@gmail.com> >> >> >> > --- >> >> >> > target/i386/cpu.c | 10 +++++----- >> >> >> > 1 file changed, 5 insertions(+), 5 deletions(-) >> >> >> > >> >> >> > diff --git a/target/i386/cpu.c b/target/i386/cpu.c >> >> >> > index 13c0985f11..f34bb5ead7 100644 >> >> >> > --- a/target/i386/cpu.c >> >> >> > +++ b/target/i386/cpu.c >> >> >> > @@ -202,12 +202,12 @@ static void x86_cpu_vendor_words2str(char *dst, uint32_t vendor1, >> >> >> > #define TCG_FEATURES (CPUID_FP87 | CPUID_PSE | CPUID_TSC | CPUID_MSR | \ >> >> >> > CPUID_PAE | CPUID_MCE | CPUID_CX8 | CPUID_APIC | CPUID_SEP | \ >> >> >> > CPUID_MTRR | CPUID_PGE | CPUID_MCA | CPUID_CMOV | CPUID_PAT | \ >> >> >> > - CPUID_PSE36 | CPUID_CLFLUSH | CPUID_ACPI | CPUID_MMX | \ >> >> >> > + CPUID_PSE36 | CPUID_CLFLUSH | CPUID_ACPI | CPUID_MMX | CPUID_HT | \ >> >> >> > CPUID_FXSR | CPUID_SSE | CPUID_SSE2 | CPUID_SS | CPUID_DE) >> >> >> > /* partly implemented: >> >> >> > CPUID_MTRR, CPUID_MCA, CPUID_CLFLUSH (needed for Win64) */ >> >> >> > /* missing: >> >> >> > - CPUID_VME, CPUID_DTS, CPUID_SS, CPUID_HT, CPUID_TM, CPUID_PBE */ >> >> >> > + CPUID_VME, CPUID_DTS, CPUID_SS, CPUID_TM, CPUID_PBE */ >> >> >> > #define TCG_EXT_FEATURES (CPUID_EXT_SSE3 | CPUID_EXT_PCLMULQDQ | \ >> >> >> > CPUID_EXT_MONITOR | CPUID_EXT_SSSE3 | CPUID_EXT_CX16 | \ >> >> >> > CPUID_EXT_SSE41 | CPUID_EXT_SSE42 | CPUID_EXT_POPCNT | \ >> >> >> > @@ -3643,7 +3643,7 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp) >> >> >> > >> >> >> > qemu_init_vcpu(cs); >> >> >> > >> >> >> > - /* Only Intel CPUs support hyperthreading. Even though QEMU fixes this >> >> >> > + /* Only some CPUs support hyperthreading. Even though QEMU fixes this >> >> >> > * issue by adjusting CPUID_0000_0001_EBX and CPUID_8000_0008_ECX >> >> >> > * based on inputs (sockets,cores,threads), it is still better to gives >> >> >> > * users a warning. >> >> >> > @@ -3651,8 +3651,8 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp) >> >> >> > * NOTE: the following code has to follow qemu_init_vcpu(). Otherwise >> >> >> > * cs->nr_threads hasn't be populated yet and the checking is incorrect. >> >> >> > */ >> >> >> > - if (!IS_INTEL_CPU(env) && cs->nr_threads > 1 && !ht_warned) { >> >> >> > - error_report("AMD CPU doesn't support hyperthreading. Please configure" >> >> >> > + if ((env->features[FEAT_1_EDX] & CPUID_HT) && (cs->nr_threads > 1) && !ht_warned) { >> >> >> >> >> >> I missed a '!' here. We need to warn if CPUID_HT is not set. But I see >> >> >> that it is not being set even on HT enabled processors (i7-3770). How >> >> >> do I test for it? >> >> > >> >> > CPUID_HT is automatically set on the CPU if >> >> > (nr_threads * nr_cores) > 1. See the "case 1:" block in >> >> > cpu_x86_cpuid(). >> >> > >> >> > AFAIK, the point of the warning is to let the user know that the >> >> > guest OS is likely to ignore thread topology information if CPUID >> >> > vendor is not Intel, and has nothing to do with TCG or KVM >> >> > capabilities. Maybe the warning is obsolete today and guests >> >> > won't be confused by a HT AMD CPU, but we need to confirm that. >> >> >> >> We assume an AMD cpu for x86 TCG and it prints this warning when an >> >> smp guest is run with (cores=2,threads=2) argument. The main point of >> >> this patch is to remove that warning when using TCG. >> > >> > What is different under TCG? If guests were confused with >> > threads=2 + vendor=AMD using KVM, what exactly would make them >> > not confused by threads=2 + vendor=AMD when using TCG? >> >> I am not sure why we chose AMD to be the vendor for TCG since, as you >> note, it does not really matter if it's AMD or Intel in TCG mode. > > I don't know why the default on TCG is AMD, either. > >> >> > >> > I just ran a Fedora 25 guest using "-machine accel=tcg >> > -smp 2,threads=2", and it still thinks it's running on a 2-core >> > CPU instead of a 2-thread CPU when CPU vendor is AMD. >> >> This all started with using Windows XP. It has an arbitrary limit on >> the number of cores it can support to 2 cores. So using -smp 4 will >> still only show 2 cores. However, we can overcome this by using -smp >> cores=2,threads=2 and 4 cores show up in the guest. But we see the >> ugly warning that HT is not supported on AMD cpus. > > Is Windows XP able to detect 2 threads * 2 cores even if CPUID > vendor is AMD? > Yep. I don't think it cares for vendor id. >> So we can either: >> >> * Change the vendor to Intel for TCG, or >> * Warn based on the HT bit being set > > Could you clarify what you mean by the second item? The HT bit is > always set when nr_cores*nr_threads > 1. Hmm, I see it now. We just warn the user if the host processor does not support HT. So how about setting the HT bit only if the host processor supports it? Or since it apparently does not matter anyway, remove the warning entirely?
On Wed, Apr 19, 2017 at 06:03:01PM -0400, Pranith Kumar wrote: > On Wed, Apr 19, 2017 at 5:33 PM, Eduardo Habkost <ehabkost@redhat.com> wrote: > > On Wed, Apr 19, 2017 at 05:25:23PM -0400, Pranith Kumar wrote: > >> On Wed, Apr 19, 2017 at 4:57 PM, Eduardo Habkost <ehabkost@redhat.com> wrote: > >> > On Wed, Apr 19, 2017 at 04:16:53PM -0400, Pranith Kumar wrote: > >> >> On Wed, Apr 19, 2017 at 4:13 PM, Eduardo Habkost <ehabkost@redhat.com> wrote: > >> >> > On Wed, Apr 19, 2017 at 04:00:49PM -0400, Pranith Kumar wrote: > >> >> >> On Wed, Apr 19, 2017 at 3:54 PM, Pranith Kumar <bobby.prani@gmail.com> wrote: > >> >> >> > When we enable hyperthreading (using threads smp argument), we warn > >> >> >> > the user if the cpu is an AMD cpu. This does not make sense on TCG and > >> >> >> > is also obsolete now that AMD Ryzen support hyperthreading. > >> >> >> > > >> >> >> > Fix this by adding CPUID_HT bit to the TCG features and explicitly > >> >> >> > checking this bit in the cpuid. > >> >> >> > > >> >> >> > Signed-off-by: Pranith Kumar <bobby.prani@gmail.com> > >> >> >> > --- > >> >> >> > target/i386/cpu.c | 10 +++++----- > >> >> >> > 1 file changed, 5 insertions(+), 5 deletions(-) > >> >> >> > > >> >> >> > diff --git a/target/i386/cpu.c b/target/i386/cpu.c > >> >> >> > index 13c0985f11..f34bb5ead7 100644 > >> >> >> > --- a/target/i386/cpu.c > >> >> >> > +++ b/target/i386/cpu.c > >> >> >> > @@ -202,12 +202,12 @@ static void x86_cpu_vendor_words2str(char *dst, uint32_t vendor1, > >> >> >> > #define TCG_FEATURES (CPUID_FP87 | CPUID_PSE | CPUID_TSC | CPUID_MSR | \ > >> >> >> > CPUID_PAE | CPUID_MCE | CPUID_CX8 | CPUID_APIC | CPUID_SEP | \ > >> >> >> > CPUID_MTRR | CPUID_PGE | CPUID_MCA | CPUID_CMOV | CPUID_PAT | \ > >> >> >> > - CPUID_PSE36 | CPUID_CLFLUSH | CPUID_ACPI | CPUID_MMX | \ > >> >> >> > + CPUID_PSE36 | CPUID_CLFLUSH | CPUID_ACPI | CPUID_MMX | CPUID_HT | \ > >> >> >> > CPUID_FXSR | CPUID_SSE | CPUID_SSE2 | CPUID_SS | CPUID_DE) > >> >> >> > /* partly implemented: > >> >> >> > CPUID_MTRR, CPUID_MCA, CPUID_CLFLUSH (needed for Win64) */ > >> >> >> > /* missing: > >> >> >> > - CPUID_VME, CPUID_DTS, CPUID_SS, CPUID_HT, CPUID_TM, CPUID_PBE */ > >> >> >> > + CPUID_VME, CPUID_DTS, CPUID_SS, CPUID_TM, CPUID_PBE */ > >> >> >> > #define TCG_EXT_FEATURES (CPUID_EXT_SSE3 | CPUID_EXT_PCLMULQDQ | \ > >> >> >> > CPUID_EXT_MONITOR | CPUID_EXT_SSSE3 | CPUID_EXT_CX16 | \ > >> >> >> > CPUID_EXT_SSE41 | CPUID_EXT_SSE42 | CPUID_EXT_POPCNT | \ > >> >> >> > @@ -3643,7 +3643,7 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp) > >> >> >> > > >> >> >> > qemu_init_vcpu(cs); > >> >> >> > > >> >> >> > - /* Only Intel CPUs support hyperthreading. Even though QEMU fixes this > >> >> >> > + /* Only some CPUs support hyperthreading. Even though QEMU fixes this > >> >> >> > * issue by adjusting CPUID_0000_0001_EBX and CPUID_8000_0008_ECX > >> >> >> > * based on inputs (sockets,cores,threads), it is still better to gives > >> >> >> > * users a warning. > >> >> >> > @@ -3651,8 +3651,8 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp) > >> >> >> > * NOTE: the following code has to follow qemu_init_vcpu(). Otherwise > >> >> >> > * cs->nr_threads hasn't be populated yet and the checking is incorrect. > >> >> >> > */ > >> >> >> > - if (!IS_INTEL_CPU(env) && cs->nr_threads > 1 && !ht_warned) { > >> >> >> > - error_report("AMD CPU doesn't support hyperthreading. Please configure" > >> >> >> > + if ((env->features[FEAT_1_EDX] & CPUID_HT) && (cs->nr_threads > 1) && !ht_warned) { > >> >> >> > >> >> >> I missed a '!' here. We need to warn if CPUID_HT is not set. But I see > >> >> >> that it is not being set even on HT enabled processors (i7-3770). How > >> >> >> do I test for it? > >> >> > > >> >> > CPUID_HT is automatically set on the CPU if > >> >> > (nr_threads * nr_cores) > 1. See the "case 1:" block in > >> >> > cpu_x86_cpuid(). > >> >> > > >> >> > AFAIK, the point of the warning is to let the user know that the > >> >> > guest OS is likely to ignore thread topology information if CPUID > >> >> > vendor is not Intel, and has nothing to do with TCG or KVM > >> >> > capabilities. Maybe the warning is obsolete today and guests > >> >> > won't be confused by a HT AMD CPU, but we need to confirm that. > >> >> > >> >> We assume an AMD cpu for x86 TCG and it prints this warning when an > >> >> smp guest is run with (cores=2,threads=2) argument. The main point of > >> >> this patch is to remove that warning when using TCG. > >> > > >> > What is different under TCG? If guests were confused with > >> > threads=2 + vendor=AMD using KVM, what exactly would make them > >> > not confused by threads=2 + vendor=AMD when using TCG? > >> > >> I am not sure why we chose AMD to be the vendor for TCG since, as you > >> note, it does not really matter if it's AMD or Intel in TCG mode. > > > > I don't know why the default on TCG is AMD, either. > > > >> > >> > > >> > I just ran a Fedora 25 guest using "-machine accel=tcg > >> > -smp 2,threads=2", and it still thinks it's running on a 2-core > >> > CPU instead of a 2-thread CPU when CPU vendor is AMD. > >> > >> This all started with using Windows XP. It has an arbitrary limit on > >> the number of cores it can support to 2 cores. So using -smp 4 will > >> still only show 2 cores. However, we can overcome this by using -smp > >> cores=2,threads=2 and 4 cores show up in the guest. But we see the > >> ugly warning that HT is not supported on AMD cpus. > > > > Is Windows XP able to detect 2 threads * 2 cores even if CPUID > > vendor is AMD? > > > > Yep. I don't think it cares for vendor id. What happens if you use -smp 4,cores=4 ? > > >> So we can either: > >> > >> * Change the vendor to Intel for TCG, or > >> * Warn based on the HT bit being set > > > > Could you clarify what you mean by the second item? The HT bit is > > always set when nr_cores*nr_threads > 1. > > Hmm, I see it now. We just warn the user if the host processor does > not support HT. We don't look at the host processor at all: the warn depends only on the CPUID data we're showing to the guest. > > So how about setting the HT bit only if the host processor supports > it? Or since it apparently does not matter anyway, remove the warning > entirely? The host processor doesn't matter. What matters is the CPUID data seen by the guest, and it doesn't matter if we're using TCG or KVM. Linux guests seem to ignore the hyperthreading information if vendor ID is AMD, and that seems to be the original reason for the warning. I would like to remove the warning, too, but it looks like this fact hasn't changed.
On Wed, Apr 19, 2017 at 10:26 PM, Eduardo Habkost <ehabkost@redhat.com> wrote: > On Wed, Apr 19, 2017 at 06:03:01PM -0400, Pranith Kumar wrote: >> On Wed, Apr 19, 2017 at 5:33 PM, Eduardo Habkost <ehabkost@redhat.com> wrote: >> > On Wed, Apr 19, 2017 at 05:25:23PM -0400, Pranith Kumar wrote: >> >> On Wed, Apr 19, 2017 at 4:57 PM, Eduardo Habkost <ehabkost@redhat.com> wrote: >> >> > On Wed, Apr 19, 2017 at 04:16:53PM -0400, Pranith Kumar wrote: >> >> >> On Wed, Apr 19, 2017 at 4:13 PM, Eduardo Habkost <ehabkost@redhat.com> wrote: >> >> >> > On Wed, Apr 19, 2017 at 04:00:49PM -0400, Pranith Kumar wrote: >> >> >> >> On Wed, Apr 19, 2017 at 3:54 PM, Pranith Kumar <bobby.prani@gmail.com> wrote: >> >> >> >> > When we enable hyperthreading (using threads smp argument), we warn >> >> >> >> > the user if the cpu is an AMD cpu. This does not make sense on TCG and >> >> >> >> > is also obsolete now that AMD Ryzen support hyperthreading. >> >> >> >> > >> >> >> >> > Fix this by adding CPUID_HT bit to the TCG features and explicitly >> >> >> >> > checking this bit in the cpuid. >> >> >> >> > >> >> >> >> > Signed-off-by: Pranith Kumar <bobby.prani@gmail.com> >> >> >> >> > --- >> >> >> >> > target/i386/cpu.c | 10 +++++----- >> >> >> >> > 1 file changed, 5 insertions(+), 5 deletions(-) >> >> >> >> > >> >> >> >> > diff --git a/target/i386/cpu.c b/target/i386/cpu.c >> >> >> >> > index 13c0985f11..f34bb5ead7 100644 >> >> >> >> > --- a/target/i386/cpu.c >> >> >> >> > +++ b/target/i386/cpu.c >> >> >> >> > @@ -202,12 +202,12 @@ static void x86_cpu_vendor_words2str(char *dst, uint32_t vendor1, >> >> >> >> > #define TCG_FEATURES (CPUID_FP87 | CPUID_PSE | CPUID_TSC | CPUID_MSR | \ >> >> >> >> > CPUID_PAE | CPUID_MCE | CPUID_CX8 | CPUID_APIC | CPUID_SEP | \ >> >> >> >> > CPUID_MTRR | CPUID_PGE | CPUID_MCA | CPUID_CMOV | CPUID_PAT | \ >> >> >> >> > - CPUID_PSE36 | CPUID_CLFLUSH | CPUID_ACPI | CPUID_MMX | \ >> >> >> >> > + CPUID_PSE36 | CPUID_CLFLUSH | CPUID_ACPI | CPUID_MMX | CPUID_HT | \ >> >> >> >> > CPUID_FXSR | CPUID_SSE | CPUID_SSE2 | CPUID_SS | CPUID_DE) >> >> >> >> > /* partly implemented: >> >> >> >> > CPUID_MTRR, CPUID_MCA, CPUID_CLFLUSH (needed for Win64) */ >> >> >> >> > /* missing: >> >> >> >> > - CPUID_VME, CPUID_DTS, CPUID_SS, CPUID_HT, CPUID_TM, CPUID_PBE */ >> >> >> >> > + CPUID_VME, CPUID_DTS, CPUID_SS, CPUID_TM, CPUID_PBE */ >> >> >> >> > #define TCG_EXT_FEATURES (CPUID_EXT_SSE3 | CPUID_EXT_PCLMULQDQ | \ >> >> >> >> > CPUID_EXT_MONITOR | CPUID_EXT_SSSE3 | CPUID_EXT_CX16 | \ >> >> >> >> > CPUID_EXT_SSE41 | CPUID_EXT_SSE42 | CPUID_EXT_POPCNT | \ >> >> >> >> > @@ -3643,7 +3643,7 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp) >> >> >> >> > >> >> >> >> > qemu_init_vcpu(cs); >> >> >> >> > >> >> >> >> > - /* Only Intel CPUs support hyperthreading. Even though QEMU fixes this >> >> >> >> > + /* Only some CPUs support hyperthreading. Even though QEMU fixes this >> >> >> >> > * issue by adjusting CPUID_0000_0001_EBX and CPUID_8000_0008_ECX >> >> >> >> > * based on inputs (sockets,cores,threads), it is still better to gives >> >> >> >> > * users a warning. >> >> >> >> > @@ -3651,8 +3651,8 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp) >> >> >> >> > * NOTE: the following code has to follow qemu_init_vcpu(). Otherwise >> >> >> >> > * cs->nr_threads hasn't be populated yet and the checking is incorrect. >> >> >> >> > */ >> >> >> >> > - if (!IS_INTEL_CPU(env) && cs->nr_threads > 1 && !ht_warned) { >> >> >> >> > - error_report("AMD CPU doesn't support hyperthreading. Please configure" >> >> >> >> > + if ((env->features[FEAT_1_EDX] & CPUID_HT) && (cs->nr_threads > 1) && !ht_warned) { >> >> >> >> >> >> >> >> I missed a '!' here. We need to warn if CPUID_HT is not set. But I see >> >> >> >> that it is not being set even on HT enabled processors (i7-3770). How >> >> >> >> do I test for it? >> >> >> > >> >> >> > CPUID_HT is automatically set on the CPU if >> >> >> > (nr_threads * nr_cores) > 1. See the "case 1:" block in >> >> >> > cpu_x86_cpuid(). >> >> >> > >> >> >> > AFAIK, the point of the warning is to let the user know that the >> >> >> > guest OS is likely to ignore thread topology information if CPUID >> >> >> > vendor is not Intel, and has nothing to do with TCG or KVM >> >> >> > capabilities. Maybe the warning is obsolete today and guests >> >> >> > won't be confused by a HT AMD CPU, but we need to confirm that. >> >> >> >> >> >> We assume an AMD cpu for x86 TCG and it prints this warning when an >> >> >> smp guest is run with (cores=2,threads=2) argument. The main point of >> >> >> this patch is to remove that warning when using TCG. >> >> > >> >> > What is different under TCG? If guests were confused with >> >> > threads=2 + vendor=AMD using KVM, what exactly would make them >> >> > not confused by threads=2 + vendor=AMD when using TCG? >> >> >> >> I am not sure why we chose AMD to be the vendor for TCG since, as you >> >> note, it does not really matter if it's AMD or Intel in TCG mode. >> > >> > I don't know why the default on TCG is AMD, either. >> > >> >> >> >> > >> >> > I just ran a Fedora 25 guest using "-machine accel=tcg >> >> > -smp 2,threads=2", and it still thinks it's running on a 2-core >> >> > CPU instead of a 2-thread CPU when CPU vendor is AMD. >> >> >> >> This all started with using Windows XP. It has an arbitrary limit on >> >> the number of cores it can support to 2 cores. So using -smp 4 will >> >> still only show 2 cores. However, we can overcome this by using -smp >> >> cores=2,threads=2 and 4 cores show up in the guest. But we see the >> >> ugly warning that HT is not supported on AMD cpus. >> > >> > Is Windows XP able to detect 2 threads * 2 cores even if CPUID >> > vendor is AMD? >> > >> >> Yep. I don't think it cares for vendor id. > > What happens if you use -smp 4,cores=4 ? This is interesting. The above shows 4 cores in the guest. I then tried various combinations: -smp 4 shows 2 cores -smp 4,cores=4 shows 4 cores -smp 4,threads=4 shows 4 cores -smp 4,sockets=1 shows 4 cores I am a bit confused why smp 4 is showing only 2 cores? and why the rest are showing 4 cores > >> >> >> So we can either: >> >> >> >> * Change the vendor to Intel for TCG, or >> >> * Warn based on the HT bit being set >> > >> > Could you clarify what you mean by the second item? The HT bit is >> > always set when nr_cores*nr_threads > 1. >> >> Hmm, I see it now. We just warn the user if the host processor does >> not support HT. > > We don't look at the host processor at all: the warn depends only > on the CPUID data we're showing to the guest. > >> >> So how about setting the HT bit only if the host processor supports >> it? Or since it apparently does not matter anyway, remove the warning >> entirely? > > The host processor doesn't matter. What matters is the CPUID > data seen by the guest, and it doesn't matter if we're using TCG > or KVM. > > Linux guests seem to ignore the hyperthreading information if > vendor ID is AMD, and that seems to be the original reason for > the warning. I would like to remove the warning, too, but it > looks like this fact hasn't changed.
On Thu, Apr 20, 2017 at 12:14:18AM -0400, Pranith Kumar wrote: > On Wed, Apr 19, 2017 at 10:26 PM, Eduardo Habkost <ehabkost@redhat.com> wrote: > > On Wed, Apr 19, 2017 at 06:03:01PM -0400, Pranith Kumar wrote: > >> On Wed, Apr 19, 2017 at 5:33 PM, Eduardo Habkost <ehabkost@redhat.com> wrote: > >> > On Wed, Apr 19, 2017 at 05:25:23PM -0400, Pranith Kumar wrote: > >> >> On Wed, Apr 19, 2017 at 4:57 PM, Eduardo Habkost <ehabkost@redhat.com> wrote: > >> >> > On Wed, Apr 19, 2017 at 04:16:53PM -0400, Pranith Kumar wrote: > >> >> >> On Wed, Apr 19, 2017 at 4:13 PM, Eduardo Habkost <ehabkost@redhat.com> wrote: > >> >> >> > On Wed, Apr 19, 2017 at 04:00:49PM -0400, Pranith Kumar wrote: > >> >> >> >> On Wed, Apr 19, 2017 at 3:54 PM, Pranith Kumar <bobby.prani@gmail.com> wrote: > >> >> >> >> > When we enable hyperthreading (using threads smp argument), we warn > >> >> >> >> > the user if the cpu is an AMD cpu. This does not make sense on TCG and > >> >> >> >> > is also obsolete now that AMD Ryzen support hyperthreading. > >> >> >> >> > > >> >> >> >> > Fix this by adding CPUID_HT bit to the TCG features and explicitly > >> >> >> >> > checking this bit in the cpuid. > >> >> >> >> > > >> >> >> >> > Signed-off-by: Pranith Kumar <bobby.prani@gmail.com> > >> >> >> >> > --- > >> >> >> >> > target/i386/cpu.c | 10 +++++----- > >> >> >> >> > 1 file changed, 5 insertions(+), 5 deletions(-) > >> >> >> >> > > >> >> >> >> > diff --git a/target/i386/cpu.c b/target/i386/cpu.c > >> >> >> >> > index 13c0985f11..f34bb5ead7 100644 > >> >> >> >> > --- a/target/i386/cpu.c > >> >> >> >> > +++ b/target/i386/cpu.c > >> >> >> >> > @@ -202,12 +202,12 @@ static void x86_cpu_vendor_words2str(char *dst, uint32_t vendor1, > >> >> >> >> > #define TCG_FEATURES (CPUID_FP87 | CPUID_PSE | CPUID_TSC | CPUID_MSR | \ > >> >> >> >> > CPUID_PAE | CPUID_MCE | CPUID_CX8 | CPUID_APIC | CPUID_SEP | \ > >> >> >> >> > CPUID_MTRR | CPUID_PGE | CPUID_MCA | CPUID_CMOV | CPUID_PAT | \ > >> >> >> >> > - CPUID_PSE36 | CPUID_CLFLUSH | CPUID_ACPI | CPUID_MMX | \ > >> >> >> >> > + CPUID_PSE36 | CPUID_CLFLUSH | CPUID_ACPI | CPUID_MMX | CPUID_HT | \ > >> >> >> >> > CPUID_FXSR | CPUID_SSE | CPUID_SSE2 | CPUID_SS | CPUID_DE) > >> >> >> >> > /* partly implemented: > >> >> >> >> > CPUID_MTRR, CPUID_MCA, CPUID_CLFLUSH (needed for Win64) */ > >> >> >> >> > /* missing: > >> >> >> >> > - CPUID_VME, CPUID_DTS, CPUID_SS, CPUID_HT, CPUID_TM, CPUID_PBE */ > >> >> >> >> > + CPUID_VME, CPUID_DTS, CPUID_SS, CPUID_TM, CPUID_PBE */ > >> >> >> >> > #define TCG_EXT_FEATURES (CPUID_EXT_SSE3 | CPUID_EXT_PCLMULQDQ | \ > >> >> >> >> > CPUID_EXT_MONITOR | CPUID_EXT_SSSE3 | CPUID_EXT_CX16 | \ > >> >> >> >> > CPUID_EXT_SSE41 | CPUID_EXT_SSE42 | CPUID_EXT_POPCNT | \ > >> >> >> >> > @@ -3643,7 +3643,7 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp) > >> >> >> >> > > >> >> >> >> > qemu_init_vcpu(cs); > >> >> >> >> > > >> >> >> >> > - /* Only Intel CPUs support hyperthreading. Even though QEMU fixes this > >> >> >> >> > + /* Only some CPUs support hyperthreading. Even though QEMU fixes this > >> >> >> >> > * issue by adjusting CPUID_0000_0001_EBX and CPUID_8000_0008_ECX > >> >> >> >> > * based on inputs (sockets,cores,threads), it is still better to gives > >> >> >> >> > * users a warning. > >> >> >> >> > @@ -3651,8 +3651,8 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp) > >> >> >> >> > * NOTE: the following code has to follow qemu_init_vcpu(). Otherwise > >> >> >> >> > * cs->nr_threads hasn't be populated yet and the checking is incorrect. > >> >> >> >> > */ > >> >> >> >> > - if (!IS_INTEL_CPU(env) && cs->nr_threads > 1 && !ht_warned) { > >> >> >> >> > - error_report("AMD CPU doesn't support hyperthreading. Please configure" > >> >> >> >> > + if ((env->features[FEAT_1_EDX] & CPUID_HT) && (cs->nr_threads > 1) && !ht_warned) { > >> >> >> >> > >> >> >> >> I missed a '!' here. We need to warn if CPUID_HT is not set. But I see > >> >> >> >> that it is not being set even on HT enabled processors (i7-3770). How > >> >> >> >> do I test for it? > >> >> >> > > >> >> >> > CPUID_HT is automatically set on the CPU if > >> >> >> > (nr_threads * nr_cores) > 1. See the "case 1:" block in > >> >> >> > cpu_x86_cpuid(). > >> >> >> > > >> >> >> > AFAIK, the point of the warning is to let the user know that the > >> >> >> > guest OS is likely to ignore thread topology information if CPUID > >> >> >> > vendor is not Intel, and has nothing to do with TCG or KVM > >> >> >> > capabilities. Maybe the warning is obsolete today and guests > >> >> >> > won't be confused by a HT AMD CPU, but we need to confirm that. > >> >> >> > >> >> >> We assume an AMD cpu for x86 TCG and it prints this warning when an > >> >> >> smp guest is run with (cores=2,threads=2) argument. The main point of > >> >> >> this patch is to remove that warning when using TCG. > >> >> > > >> >> > What is different under TCG? If guests were confused with > >> >> > threads=2 + vendor=AMD using KVM, what exactly would make them > >> >> > not confused by threads=2 + vendor=AMD when using TCG? > >> >> > >> >> I am not sure why we chose AMD to be the vendor for TCG since, as you > >> >> note, it does not really matter if it's AMD or Intel in TCG mode. > >> > > >> > I don't know why the default on TCG is AMD, either. > >> > > >> >> > >> >> > > >> >> > I just ran a Fedora 25 guest using "-machine accel=tcg > >> >> > -smp 2,threads=2", and it still thinks it's running on a 2-core > >> >> > CPU instead of a 2-thread CPU when CPU vendor is AMD. > >> >> > >> >> This all started with using Windows XP. It has an arbitrary limit on > >> >> the number of cores it can support to 2 cores. So using -smp 4 will > >> >> still only show 2 cores. However, we can overcome this by using -smp > >> >> cores=2,threads=2 and 4 cores show up in the guest. But we see the > >> >> ugly warning that HT is not supported on AMD cpus. > >> > > >> > Is Windows XP able to detect 2 threads * 2 cores even if CPUID > >> > vendor is AMD? > >> > > >> > >> Yep. I don't think it cares for vendor id. > > > > What happens if you use -smp 4,cores=4 ? > > This is interesting. The above shows 4 cores in the guest. I then > tried various combinations: > > -smp 4 shows 2 cores This is a shortcut for 4 sockets, 1 core per socket, 1 thread per core. > -smp 4,cores=4 shows 4 cores This is a shortcut for 1 socket with 4 cores, 1 thread per core. > -smp 4,threads=4 shows 4 cores This means 1 socket with 1 core, 4 threads per core. But if vendor ID is AMD, guests will probably ignore the hyperthreading info and treat each thread as a separate core. That's why we have a warning. > -smp 4,sockets=1 shows 4 cores This is a shortcut for 1 socket with 4 cores, 1 thread per core. > > I am a bit confused why smp 4 is showing only 2 cores? and why the > rest are showing 4 cores Probably because Windows XP is limited to 2 sockets.
On 20/04/2017 20:42, Eduardo Habkost wrote: >> -smp 4,threads=4 shows 4 cores > This means 1 socket with 1 core, 4 threads per core. But if > vendor ID is AMD, guests will probably ignore the hyperthreading > info and treat each thread as a separate core. That's why we have > a warning. I'm not sure about the "probably". As long as the HT bit is 1 and the number of threads is > 1 in CPUID[EAX=1].EBX[bits 23:16], I am not sure why the OS would care about the vendor. x86info does care (it just does not try to detect the topology for !Intel processors), but Linux seems not to. detect_ht is called unconditionally. >> I am a bit confused why smp 4 is showing only 2 cores? and why the >> rest are showing 4 cores > Probably because Windows XP is limited to 2 sockets. This is correct. Paolo
On Fri, Apr 21, 2017 at 11:38:30AM +0200, Paolo Bonzini wrote: > > > On 20/04/2017 20:42, Eduardo Habkost wrote: > >> -smp 4,threads=4 shows 4 cores > > This means 1 socket with 1 core, 4 threads per core. But if > > vendor ID is AMD, guests will probably ignore the hyperthreading > > info and treat each thread as a separate core. That's why we have > > a warning. > > I'm not sure about the "probably". As long as the HT bit is 1 and the > number of threads is > 1 in CPUID[EAX=1].EBX[bits 23:16], I am not sure > why the OS would care about the vendor. > On my testing, "-cpu qemu64,vendor=AuthenticAMD" and "-cpu qemu64,vendor=GenuineIntel" gave different results on lscpu and /proc/cpuinfo in a Fedora 25 guest (Linux 4.8.6-300.fc25). > x86info does care (it just does not try to detect the topology for > !Intel processors), but Linux seems not to. detect_ht is called > unconditionally. I see detect_ht() inside a "#ifdef CONFIG_X86_32" in amd_cpu_dev.c_init (init_amd()), and intel_cpudev.c_init (init_intel()). It is inside a "#ifdef CONFIG_X86_64" in identify_cpu(), though, so it looks like it is called at different places, but always called. But the logic becomes very confusing after that: the core/sibling CPU masks are initialized based on phys_proc_id and cpu_core_id, and detect_ht() isn't the only place where those fields are initialized (see amd_get_topology(), amd_detect_cmp(), detect_extended_topology()). It looks like amd_detect_cmp() and/or amd_get_topology() will undo what detect_ht() did.
diff --git a/target/i386/cpu.c b/target/i386/cpu.c index 13c0985f11..f34bb5ead7 100644 --- a/target/i386/cpu.c +++ b/target/i386/cpu.c @@ -202,12 +202,12 @@ static void x86_cpu_vendor_words2str(char *dst, uint32_t vendor1, #define TCG_FEATURES (CPUID_FP87 | CPUID_PSE | CPUID_TSC | CPUID_MSR | \ CPUID_PAE | CPUID_MCE | CPUID_CX8 | CPUID_APIC | CPUID_SEP | \ CPUID_MTRR | CPUID_PGE | CPUID_MCA | CPUID_CMOV | CPUID_PAT | \ - CPUID_PSE36 | CPUID_CLFLUSH | CPUID_ACPI | CPUID_MMX | \ + CPUID_PSE36 | CPUID_CLFLUSH | CPUID_ACPI | CPUID_MMX | CPUID_HT | \ CPUID_FXSR | CPUID_SSE | CPUID_SSE2 | CPUID_SS | CPUID_DE) /* partly implemented: CPUID_MTRR, CPUID_MCA, CPUID_CLFLUSH (needed for Win64) */ /* missing: - CPUID_VME, CPUID_DTS, CPUID_SS, CPUID_HT, CPUID_TM, CPUID_PBE */ + CPUID_VME, CPUID_DTS, CPUID_SS, CPUID_TM, CPUID_PBE */ #define TCG_EXT_FEATURES (CPUID_EXT_SSE3 | CPUID_EXT_PCLMULQDQ | \ CPUID_EXT_MONITOR | CPUID_EXT_SSSE3 | CPUID_EXT_CX16 | \ CPUID_EXT_SSE41 | CPUID_EXT_SSE42 | CPUID_EXT_POPCNT | \ @@ -3643,7 +3643,7 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp) qemu_init_vcpu(cs); - /* Only Intel CPUs support hyperthreading. Even though QEMU fixes this + /* Only some CPUs support hyperthreading. Even though QEMU fixes this * issue by adjusting CPUID_0000_0001_EBX and CPUID_8000_0008_ECX * based on inputs (sockets,cores,threads), it is still better to gives * users a warning. @@ -3651,8 +3651,8 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp) * NOTE: the following code has to follow qemu_init_vcpu(). Otherwise * cs->nr_threads hasn't be populated yet and the checking is incorrect. */ - if (!IS_INTEL_CPU(env) && cs->nr_threads > 1 && !ht_warned) { - error_report("AMD CPU doesn't support hyperthreading. Please configure" + if ((env->features[FEAT_1_EDX] & CPUID_HT) && (cs->nr_threads > 1) && !ht_warned) { + error_report("This CPU doesn't support hyperthreading. Please configure" " -smp options properly."); ht_warned = true; }
When we enable hyperthreading (using threads smp argument), we warn the user if the cpu is an AMD cpu. This does not make sense on TCG and is also obsolete now that AMD Ryzen support hyperthreading. Fix this by adding CPUID_HT bit to the TCG features and explicitly checking this bit in the cpuid. Signed-off-by: Pranith Kumar <bobby.prani@gmail.com> --- target/i386/cpu.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)