Re: [libvirt] [PATCH qom-cpu 05/11] target-i386: check/enforce: Fix CPUID leaf numbers on error messages

2013-01-07 Thread Eduardo Habkost 
On Sun, Jan 06, 2013 at 04:12:54PM +0200, Gleb Natapov wrote:
 On Fri, Jan 04, 2013 at 08:01:06PM -0200, Eduardo Habkost wrote:
  The -cpu check/enforce warnings are printing incorrect information about the
  missing flags. There are no feature flags on CPUID leaves 0 and 0x8000, 
  but
  there were references to 0 and 0x8000 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.8001H:ECX.abm [bit 5]
  warning: host doesn't support requested feature: 
  CPUID.8001H:ECX.sse4a [bit 6]
  warning: host doesn't support requested feature: 
  CPUID.8001H:ECX.misalignsse [bit 7]
  warning: host doesn't support requested feature: 
  CPUID.8001H:ECX.3dnowprefetch [bit 8]
  warning: host doesn't support requested feature: 
  CPUID.8001H:ECX.xop [bit 11]
  warning: host doesn't support requested feature: 
  CPUID.8001H:ECX.fma4 [bit 16]
  Unable to find x86 CPU definition
  $
  
  Signed-off-by: Eduardo Habkost ehabk...@redhat.com
 Reviewed-by: Gleb Natapov g...@redhat.com
 But see the question below.
 
  ---
  Cc: Gleb Natapov g...@redhat.com
  Cc: Marcelo Tosatti mtosa...@redhat.com
  Cc: k...@vger.kernel.org
  
  Changes v2:
   - Coding style fixes
   - Add assert() for invalid register numbers on
 unavailable_host_feature()
  ---
   target-i386/cpu.c | 42 +-
   target-i386/cpu.h |  3 +++
   2 files changed, 36 insertions(+), 9 deletions(-)
  
  diff --git a/target-i386/cpu.c b/target-i386/cpu.c
  index e916ae0..c3e5db8 100644
  --- a/target-i386/cpu.c
  +++ b/target-i386/cpu.c
  @@ -124,6 +124,25 @@ 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) {
  +return NULL;
  +}
  +return reg_names[reg];
  +}
  +
   /* collects per-function cpuid data
*/
   typedef struct model_features_t {
  @@ -132,7 +151,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;
  @@ -923,10 +943,13 @@ 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  0x,
  -f-flag_names[i] ? f-flag_names[i] : [reserved], mask);
  +const char *reg = get_register_name_32(f-reg);
  +assert(reg);
  +fprintf(stderr, warning: host doesn't support requested 
  feature: 
  +CPUID.%02XH:%s%s%s [bit %d]\n,
  +f-cpuid, reg,
  +f-flag_names[i] ? . : ,
  +f-flag_names[i] ? f-flag_names[i] : , i);
   break;
   }
   return 0;
  @@ -945,13 +968,14 @@ 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, 0x},
  +~0, feature_name, 0x0001, R_EDX},
   {guest_def-ext_features, host_def.ext_features,
  -~CPUID_EXT_HYPERVISOR, ext_feature_name, 0x0001},
  +~CPUID_EXT_HYPERVISOR, ext_feature_name, 0x0001, R_ECX},
   {guest_def-ext2_features, host_def.ext2_features,
  -~PPRO_FEATURES, ext2_feature_name, 0x8000},
  +~PPRO_FEATURES, ext2_feature_name, 0x8001, R_EDX},
   {guest_def-ext3_features, host_def.ext3_features,
  -~CPUID_EXT3_SVM, ext3_feature_name, 0x8001}};
  +~CPUID_EXT3_SVM,

Re: [libvirt] [PATCH qom-cpu 05/11] target-i386: check/enforce: Fix CPUID leaf numbers on error messages

2013-01-06 Thread Gleb Natapov 
On Fri, Jan 04, 2013 at 08:01:06PM -0200, Eduardo Habkost wrote:
 The -cpu check/enforce warnings are printing incorrect information about the
 missing flags. There are no feature flags on CPUID leaves 0 and 0x8000, 
 but
 there were references to 0 and 0x8000 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.8001H:ECX.abm 
 [bit 5]
 warning: host doesn't support requested feature: 
 CPUID.8001H:ECX.sse4a [bit 6]
 warning: host doesn't support requested feature: 
 CPUID.8001H:ECX.misalignsse [bit 7]
 warning: host doesn't support requested feature: 
 CPUID.8001H:ECX.3dnowprefetch [bit 8]
 warning: host doesn't support requested feature: CPUID.8001H:ECX.xop 
 [bit 11]
 warning: host doesn't support requested feature: CPUID.8001H:ECX.fma4 
 [bit 16]
 Unable to find x86 CPU definition
 $
 
 Signed-off-by: Eduardo Habkost ehabk...@redhat.com
Reviewed-by: Gleb Natapov g...@redhat.com
But see the question below.

 ---
 Cc: Gleb Natapov g...@redhat.com
 Cc: Marcelo Tosatti mtosa...@redhat.com
 Cc: k...@vger.kernel.org
 
 Changes v2:
  - Coding style fixes
  - Add assert() for invalid register numbers on
unavailable_host_feature()
 ---
  target-i386/cpu.c | 42 +-
  target-i386/cpu.h |  3 +++
  2 files changed, 36 insertions(+), 9 deletions(-)
 
 diff --git a/target-i386/cpu.c b/target-i386/cpu.c
 index e916ae0..c3e5db8 100644
 --- a/target-i386/cpu.c
 +++ b/target-i386/cpu.c
 @@ -124,6 +124,25 @@ 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) {
 +return NULL;
 +}
 +return reg_names[reg];
 +}
 +
  /* collects per-function cpuid data
   */
  typedef struct model_features_t {
 @@ -132,7 +151,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;
 @@ -923,10 +943,13 @@ 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  0x,
 -f-flag_names[i] ? f-flag_names[i] : [reserved], mask);
 +const char *reg = get_register_name_32(f-reg);
 +assert(reg);
 +fprintf(stderr, warning: host doesn't support requested 
 feature: 
 +CPUID.%02XH:%s%s%s [bit %d]\n,
 +f-cpuid, reg,
 +f-flag_names[i] ? . : ,
 +f-flag_names[i] ? f-flag_names[i] : , i);
  break;
  }
  return 0;
 @@ -945,13 +968,14 @@ 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, 0x},
 +~0, feature_name, 0x0001, R_EDX},
  {guest_def-ext_features, host_def.ext_features,
 -~CPUID_EXT_HYPERVISOR, ext_feature_name, 0x0001},
 +~CPUID_EXT_HYPERVISOR, ext_feature_name, 0x0001, R_ECX},
  {guest_def-ext2_features, host_def.ext2_features,
 -~PPRO_FEATURES, ext2_feature_name, 0x8000},
 +~PPRO_FEATURES, ext2_feature_name, 0x8001, R_EDX},
  {guest_def-ext3_features, host_def.ext3_features,
 -~CPUID_EXT3_SVM, ext3_feature_name, 0x8001}};
 +~CPUID_EXT3_SVM, ext3_feature_name, 0x8001, R_ECX}
Why do we exclude PPRO_FEATURES/CPUID_EXT3_SVM from been checked?

 +};
  
  assert(kvm_enabled());
  
 diff --git a/target-i386/cpu.h b/target-i386/cpu.h

[libvirt] [PATCH qom-cpu 05/11] target-i386: check/enforce: Fix CPUID leaf numbers on error messages

2013-01-04 Thread Eduardo Habkost 
The -cpu check/enforce warnings are printing incorrect information about the
missing flags. There are no feature flags on CPUID leaves 0 and 0x8000, but
there were references to 0 and 0x8000 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.8001H:ECX.abm 
[bit 5]
warning: host doesn't support requested feature: CPUID.8001H:ECX.sse4a 
[bit 6]
warning: host doesn't support requested feature: 
CPUID.8001H:ECX.misalignsse [bit 7]
warning: host doesn't support requested feature: 
CPUID.8001H:ECX.3dnowprefetch [bit 8]
warning: host doesn't support requested feature: CPUID.8001H:ECX.xop 
[bit 11]
warning: host doesn't support requested feature: CPUID.8001H:ECX.fma4 
[bit 16]
Unable to find x86 CPU definition
$

Signed-off-by: Eduardo Habkost ehabk...@redhat.com
---
Cc: Gleb Natapov g...@redhat.com
Cc: Marcelo Tosatti mtosa...@redhat.com
Cc: k...@vger.kernel.org

Changes v2:
 - Coding style fixes
 - Add assert() for invalid register numbers on
   unavailable_host_feature()
---
 target-i386/cpu.c | 42 +-
 target-i386/cpu.h |  3 +++
 2 files changed, 36 insertions(+), 9 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index e916ae0..c3e5db8 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -124,6 +124,25 @@ 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) {
+return NULL;
+}
+return reg_names[reg];
+}
+
 /* collects per-function cpuid data
  */
 typedef struct model_features_t {
@@ -132,7 +151,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;
@@ -923,10 +943,13 @@ 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  0x,
-f-flag_names[i] ? f-flag_names[i] : [reserved], mask);
+const char *reg = get_register_name_32(f-reg);
+assert(reg);
+fprintf(stderr, warning: host doesn't support requested feature: 
+CPUID.%02XH:%s%s%s [bit %d]\n,
+f-cpuid, reg,
+f-flag_names[i] ? . : ,
+f-flag_names[i] ? f-flag_names[i] : , i);
 break;
 }
 return 0;
@@ -945,13 +968,14 @@ 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, 0x},
+~0, feature_name, 0x0001, R_EDX},
 {guest_def-ext_features, host_def.ext_features,
-~CPUID_EXT_HYPERVISOR, ext_feature_name, 0x0001},
+~CPUID_EXT_HYPERVISOR, ext_feature_name, 0x0001, R_ECX},
 {guest_def-ext2_features, host_def.ext2_features,
-~PPRO_FEATURES, ext2_feature_name, 0x8000},
+~PPRO_FEATURES, ext2_feature_name, 0x8001, R_EDX},
 {guest_def-ext3_features, host_def.ext3_features,
-~CPUID_EXT3_SVM, ext3_feature_name, 0x8001}};
+~CPUID_EXT3_SVM, ext3_feature_name, 0x8001, 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);
+