Re: [Qemu-devel] [PATCH] tcg/i386: Do not display HT warning for TCG

[Eduardo Habkost]

On Thu, Apr 20, 2017 at 12:14:18AM -0400, Pranith Kumar wrote:
> On Wed, Apr 19, 2017 at 10:26 PM, Eduardo Habkost <address@hidden> 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 <address@hidden> 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 <address@hidden> 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 <address@hidden> 
> >> >> >> 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 <address@hidden> 
> >> >> >> >> 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 <address@hidden>
> >> >> >> >> > ---
> >> >> >> >> >  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.

-- 
Eduardo