Re: [Qemu-devel] [PATCH] i386: forward CPUID cache leaves when -cpu host is used

Re: [Qemu-devel] [PATCH] i386: forward CPUID cache leaves when -cpu host is used

[Andreas Färber]

Hi,

"target-i386:" please.

Am 27.08.2013 22:38, schrieb Benoît Canet:
> Some users running cpu intensive tasks checking the cache CPUID leaves at
> startup and making decisions based on the result reported that the guest was
> not reflecting the host CPUID leaves when -cpu host is used.
> 
> This patch fix this.
> 
> Signed-off-by: Benoit Canet <benoit@irqsave.net>
> ---
>  target-i386/cpu.c |   19 +++++++++++++++++++
>  target-i386/cpu.h |    1 +
>  2 files changed, 20 insertions(+)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 42c5de0..2c8eaf7 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -374,6 +374,7 @@ typedef struct x86_def_t {
>      int stepping;
>      FeatureWordArray features;
>      char model_id[48];
> +    bool fwd_cpuid_cache_leaves;
>  } x86_def_t;
>  
>  #define I486_FEATURES (CPUID_FP87 | CPUID_VME | CPUID_PSE)
> @@ -1027,6 +1028,7 @@ static void kvm_cpu_fill_host(x86_def_t *x86_cpu_def)
>      assert(kvm_enabled());
>  
>      x86_cpu_def->name = "host";
> +    x86_cpu_def->fwd_cpuid_cache_leaves = true;
>      host_cpuid(0x0, 0, &eax, &ebx, &ecx, &edx);
>      x86_cpu_vendor_words2str(x86_cpu_def->vendor, ebx, edx, ecx);
>  
> @@ -1776,6 +1778,7 @@ static void cpu_x86_register(X86CPU *cpu, const char *name, Error **errp)
>      env->features[FEAT_C000_0001_EDX] = def->features[FEAT_C000_0001_EDX];
>      env->features[FEAT_7_0_EBX] = def->features[FEAT_7_0_EBX];
>      env->cpuid_xlevel2 = def->xlevel2;
> +    env->fwd_cpuid_cache_leaves = def->fwd_cpuid_cache_leaves;
>  
>      object_property_set_str(OBJECT(cpu), def->model_id, "model-id", errp);
>  }
> @@ -1949,6 +1952,10 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
>          }
>          break;
>      case 2:
> +        if (env->fwd_cpuid_cache_leaves) {
> +            host_cpuid(0x2, 0, eax, ebx, ecx, edx);
> +            break;
> +        }
>          /* cache info: needed for Pentium Pro compatibility */
>          *eax = 1;
>          *ebx = 0;
> @@ -1956,6 +1963,10 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
>          *edx = 0x2c307d;
>          break;
>      case 4:
> +        if (env->fwd_cpuid_cache_leaves) {
> +            host_cpuid(0x4, count, eax, ebx, ecx, edx);
> +            break;
> +        }
>          /* cache info: needed for Core compatibility */
>          if (cs->nr_cores > 1) {
>              *eax = (cs->nr_cores - 1) << 26;
> @@ -2102,6 +2113,10 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
>          break;
>      case 0x80000005:
>          /* cache info (L1 cache) */
> +        if (env->fwd_cpuid_cache_leaves) {
> +            host_cpuid(0x80000005, 0, eax, ebx, ecx, edx);
> +            break;
> +        }
>          *eax = 0x01ff01ff;
>          *ebx = 0x01ff01ff;
>          *ecx = 0x40020140;
> @@ -2109,6 +2124,10 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
>          break;
>      case 0x80000006:
>          /* cache info (L2 cache) */
> +        if (env->fwd_cpuid_cache_leaves) {
> +            host_cpuid(0x80000006, 0, eax, ebx, ecx, edx);
> +            break;
> +        }
>          *eax = 0;
>          *ebx = 0x42004200;
>          *ecx = 0x02008140;

This hunk may trivially conflict with Eduardo's cache flags cleanup.

> diff --git a/target-i386/cpu.h b/target-i386/cpu.h
> index 8a3d0fd..1ec32fa 100644
> --- a/target-i386/cpu.h
> +++ b/target-i386/cpu.h
> @@ -865,6 +865,7 @@ typedef struct CPUX86State {
>      bool tsc_valid;
>      int tsc_khz;
>      void *kvm_xsave_buf;
> +    bool fwd_cpuid_cache_leaves;
>  
>      /* in order to simplify APIC support, we leave this pointer to the
>         user */

Please place the field in X86CPU instead and document it.

Otherwise patch looks okay to me on a brief sight; but since this is
about -cpu host I would prefer this to go through uq/master once fixed
or at least to get some acks.

Regards,
Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH] i386: forward CPUID cache leaves when -cpu host is used

[Eduardo Habkost]

On Mon, Sep 02, 2013 at 02:55:36PM +0200, Andreas Färber wrote:
[...]
> > diff --git a/target-i386/cpu.h b/target-i386/cpu.h
> > index 8a3d0fd..1ec32fa 100644
> > --- a/target-i386/cpu.h
> > +++ b/target-i386/cpu.h
> > @@ -865,6 +865,7 @@ typedef struct CPUX86State {
> >      bool tsc_valid;
> >      int tsc_khz;
> >      void *kvm_xsave_buf;
> > +    bool fwd_cpuid_cache_leaves;
> >  
> >      /* in order to simplify APIC support, we leave this pointer to the
> >         user */
> 
> Please place the field in X86CPU instead and document it.

While moving it, I believe the name can be made clearer. I would name it
"fwd_host_cache_info" or something like that, to make it clear that it
will expose the _host CPU_ cache information to the guest.

-- 
Eduardo