Re: [Qemu-devel] [PATCH 3/9] target-i386: check/enforce: Fix CPUID leaf numbers on error messages

[Blue Swirl]

On Fri, Jan 4, 2013 at 3:37 PM, Eduardo Habkost <address@hidden> wrote:
> The -cpu check/enforce warnings are printing incorrect information about the
> missing flags. There are no feature flags on CPUID leaves 0 and 0x80000000, 
> but
> there were references to 0 and 0x80000000 in the table at
> kvm_check_features_against_host().
>
> This changes the model_features_t struct to contain the register number as
> well, so the error messages print the correct CPUID leaf+register information,
> instead of wrong CPUID leaf numbers.
>
> This also changes the format of the error messages, so they follow the
> "CPUID.<leaf>.<register>.<name> [bit <offset>]" convention used on Intel
> documentation. Example output:
>
>     $ qemu-system-x86_64 -machine pc-1.0,accel=kvm -cpu 
> Opteron_G4,+ia64,enforce
>     warning: host doesn't support requested feature: CPUID.01H:EDX.ia64 [bit 
> 30]
>     warning: host doesn't support requested feature: CPUID.01H:ECX.xsave [bit 
> 26]
>     warning: host doesn't support requested feature: CPUID.01H:ECX.avx [bit 
> 28]
>     warning: host doesn't support requested feature: CPUID.80000001H:ECX.abm 
> [bit 5]
>     warning: host doesn't support requested feature: 
> CPUID.80000001H:ECX.sse4a [bit 6]
>     warning: host doesn't support requested feature: 
> CPUID.80000001H:ECX.misalignsse [bit 7]
>     warning: host doesn't support requested feature: 
> CPUID.80000001H:ECX.3dnowprefetch [bit 8]
>     warning: host doesn't support requested feature: CPUID.80000001H:ECX.xop 
> [bit 11]
>     warning: host doesn't support requested feature: CPUID.80000001H:ECX.fma4 
> [bit 16]
>     Unable to find x86 CPU definition
>     $
>
> Signed-off-by: Eduardo Habkost <address@hidden>
> ---
> Cc: Gleb Natapov <address@hidden>
> Cc: Marcelo Tosatti <address@hidden>
> Cc: address@hidden
> ---
>  target-i386/cpu.c | 38 +++++++++++++++++++++++++++++---------
>  target-i386/cpu.h |  3 +++
>  2 files changed, 32 insertions(+), 9 deletions(-)
>
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 4e26b11..6c43ace 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -124,6 +124,24 @@ static const char *cpuid_7_0_ebx_feature_name[] = {
>      NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL,
>  };
>
> +const char *get_register_name_32(unsigned int reg)
> +{
> +    static const char *reg_names[CPU_NB_REGS32] = {
> +        [R_EAX] = "EAX",
> +        [R_ECX] = "ECX",
> +        [R_EDX] = "EDX",
> +        [R_EBX] = "EBX",
> +        [R_ESP] = "ESP",
> +        [R_EBP] = "EBP",
> +        [R_ESI] = "ESI",
> +        [R_EDI] = "EDI",
> +    };
> +
> +    if (reg > CPU_NB_REGS32)

Missing braces.

> +        return NULL;
> +    return reg_names[reg];
> +}
> +
>  /* collects per-function cpuid data
>   */
>  typedef struct model_features_t {
> @@ -132,7 +150,8 @@ typedef struct model_features_t {
>      uint32_t check_feat;
>      const char **flag_names;
>      uint32_t cpuid;
> -    } model_features_t;
> +    int reg;
> +} model_features_t;
>
>  int check_cpuid = 0;
>  int enforce_cpuid = 0;
> @@ -921,10 +940,11 @@ static int unavailable_host_feature(struct 
> model_features_t *f, uint32_t mask)
>
>      for (i = 0; i < 32; ++i)
>          if (1 << i & mask) {
> -            fprintf(stderr, "warning: host cpuid %04x_%04x lacks requested"
> -                " flag '%s' [0x%08x]\n",
> -                f->cpuid >> 16, f->cpuid & 0xffff,
> -                f->flag_names[i] ? f->flag_names[i] : "[reserved]", mask);
> +            fprintf(stderr, "warning: host doesn't support requested 
> feature: "
> +                "CPUID.%02XH:%s%s%s [bit %d]\n",
> +                f->cpuid, get_register_name_32(f->reg),

This could attempt to print NULL via %s format, which is not OK with
all C libraries. If we trust that the callers always pass valid
numbers, the check above could be turned into assert().

> +                f->flag_names[i] ? "." : "",
> +                f->flag_names[i] ? f->flag_names[i] : "", i);
>              break;
>          }
>      return 0;
> @@ -943,13 +963,13 @@ static int kvm_check_features_against_host(x86_def_t 
> *guest_def)
>      int rv, i;
>      struct model_features_t ft[] = {
>          {&guest_def->features, &host_def.features,
> -            ~0, feature_name, 0x00000000},
> +            ~0, feature_name, 0x00000001, R_EDX},
>          {&guest_def->ext_features, &host_def.ext_features,
> -            ~CPUID_EXT_HYPERVISOR, ext_feature_name, 0x00000001},
> +            ~CPUID_EXT_HYPERVISOR, ext_feature_name, 0x00000001, R_ECX},
>          {&guest_def->ext2_features, &host_def.ext2_features,
> -            ~PPRO_FEATURES, ext2_feature_name, 0x80000000},
> +            ~PPRO_FEATURES, ext2_feature_name, 0x80000001, R_EDX},
>          {&guest_def->ext3_features, &host_def.ext3_features,
> -            ~CPUID_EXT3_SVM, ext3_feature_name, 0x80000001}};
> +            ~CPUID_EXT3_SVM, ext3_feature_name, 0x80000001, R_ECX}};
>
>      assert(kvm_enabled());
>
> diff --git a/target-i386/cpu.h b/target-i386/cpu.h
> index 27c8d0c..ab81a5c 100644
> --- a/target-i386/cpu.h
> +++ b/target-i386/cpu.h
> @@ -1221,4 +1221,7 @@ void cpu_report_tpr_access(CPUX86State *env, TPRAccess 
> access);
>  void enable_kvm_pv_eoi(void);
>  void disable_kvm_mmu_op(void);
>
> +/* Return name of 32-bit register, from a R_* constant */
> +const char *get_register_name_32(unsigned int reg);
> +
>  #endif /* CPU_I386_H */
> --
> 1.7.11.7
>
>