Re: [Qemu-devel] [PATCH 1/3] target-i386: add EXT2_PPRO_FEATURES #define

2012-12-14 Thread Igor Mammedov 
On Wed, 12 Dec 2012 20:22:24 -0200
Eduardo Habkost ehabk...@redhat.com wrote:

 Instead of repeating the (PPRO_FEATURES  CPUID_EXT2_AMD_ALIASES)
 expression everywhere, use EXT2_PPRO_FEATURES.
 
 Signed-off-by: Eduardo Habkost ehabk...@redhat.com
 ---
  target-i386/cpu.c | 13 +++--
  1 file changed, 7 insertions(+), 6 deletions(-)

Reviewed-by: Igor Mammedov imamm...@redhat.com

 
 diff --git a/target-i386/cpu.c b/target-i386/cpu.c
 index 546c86a..a2ee8bb 100644
 --- a/target-i386/cpu.c
 +++ b/target-i386/cpu.c
 @@ -303,6 +303,7 @@ typedef struct x86_def_t {
CPUID_MSR | CPUID_MCE | CPUID_CX8 | CPUID_PGE | CPUID_CMOV | \
CPUID_PAT | CPUID_FXSR | CPUID_MMX | CPUID_SSE | CPUID_SSE2 | \
CPUID_PAE | CPUID_SEP | CPUID_APIC)
 +#define EXT2_PPRO_FEATURES (PPRO_FEATURES  CPUID_EXT2_AMD_ALIASES)
  
  #define TCG_FEATURES (CPUID_FP87 | CPUID_PSE | CPUID_TSC | CPUID_MSR | \
CPUID_PAE | CPUID_MCE | CPUID_CX8 | CPUID_APIC | CPUID_SEP | \
 @@ -350,7 +351,7 @@ static x86_def_t builtin_x86_defs[] = {
  CPUID_MTRR | CPUID_CLFLUSH | CPUID_MCA |
  CPUID_PSE36,
  .ext_features = CPUID_EXT_SSE3 | CPUID_EXT_CX16 | CPUID_EXT_POPCNT,
 -.ext2_features = (PPRO_FEATURES  CPUID_EXT2_AMD_ALIASES) |
 +.ext2_features = EXT2_PPRO_FEATURES |
  CPUID_EXT2_LM | CPUID_EXT2_SYSCALL | CPUID_EXT2_NX,
  .ext3_features = CPUID_EXT3_LAHF_LM | CPUID_EXT3_SVM |
  CPUID_EXT3_ABM | CPUID_EXT3_SSE4A,
 @@ -370,7 +371,7 @@ static x86_def_t builtin_x86_defs[] = {
  CPUID_PSE36 | CPUID_VME | CPUID_HT,
  .ext_features = CPUID_EXT_SSE3 | CPUID_EXT_MONITOR | CPUID_EXT_CX16 |
  CPUID_EXT_POPCNT,
 -.ext2_features = (PPRO_FEATURES  CPUID_EXT2_AMD_ALIASES) |
 +.ext2_features = EXT2_PPRO_FEATURES |
  CPUID_EXT2_LM | CPUID_EXT2_SYSCALL | CPUID_EXT2_NX |
  CPUID_EXT2_3DNOW | CPUID_EXT2_3DNOWEXT | CPUID_EXT2_MMXEXT |
  CPUID_EXT2_FFXSR | CPUID_EXT2_PDPE1GB | CPUID_EXT2_RDTSCP,
 @@ -421,7 +422,7 @@ static x86_def_t builtin_x86_defs[] = {
  /* Missing: CPUID_EXT_POPCNT, CPUID_EXT_MONITOR */
  .ext_features = CPUID_EXT_SSE3 | CPUID_EXT_CX16,
  /* Missing: CPUID_EXT2_PDPE1GB, CPUID_EXT2_RDTSCP */
 -.ext2_features = (PPRO_FEATURES  CPUID_EXT2_AMD_ALIASES) |
 +.ext2_features = EXT2_PPRO_FEATURES |
  CPUID_EXT2_LM | CPUID_EXT2_SYSCALL | CPUID_EXT2_NX,
  /* Missing: CPUID_EXT3_LAHF_LM, CPUID_EXT3_CMP_LEG, 
 CPUID_EXT3_EXTAPIC,
  CPUID_EXT3_CR8LEG, CPUID_EXT3_ABM, CPUID_EXT3_SSE4A,
 @@ -456,7 +457,7 @@ static x86_def_t builtin_x86_defs[] = {
  .features = PPRO_FEATURES |
  CPUID_MTRR | CPUID_CLFLUSH | CPUID_MCA | CPUID_PSE36,
  .ext_features = CPUID_EXT_SSE3,
 -.ext2_features = PPRO_FEATURES  CPUID_EXT2_AMD_ALIASES,
 +.ext2_features = EXT2_PPRO_FEATURES,
  .ext3_features = 0,
  .xlevel = 0x8008,
  .model_id = Common 32-bit KVM processor
 @@ -538,7 +539,7 @@ static x86_def_t builtin_x86_defs[] = {
  .stepping = 3,
  .features = PPRO_FEATURES | CPUID_PSE36 | CPUID_VME | CPUID_MTRR |
  CPUID_MCA,
 -.ext2_features = (PPRO_FEATURES  CPUID_EXT2_AMD_ALIASES) |
 +.ext2_features = EXT2_PPRO_FEATURES |
  CPUID_EXT2_MMXEXT | CPUID_EXT2_3DNOW | CPUID_EXT2_3DNOWEXT,
  .xlevel = 0x8008,
  },
 @@ -558,7 +559,7 @@ static x86_def_t builtin_x86_defs[] = {
  /* Some CPUs got no CPUID_SEP */
  .ext_features = CPUID_EXT_SSE3 | CPUID_EXT_MONITOR | CPUID_EXT_SSSE3 
 |
  CPUID_EXT_DSCPL | CPUID_EXT_EST | CPUID_EXT_TM2 | CPUID_EXT_XTPR,
 -.ext2_features = (PPRO_FEATURES  CPUID_EXT2_AMD_ALIASES) |
 +.ext2_features = EXT2_PPRO_FEATURES |
  CPUID_EXT2_NX,
  .ext3_features = CPUID_EXT3_LAHF_LM,
  .xlevel = 0x800A,
 -- 
 1.7.11.7
 
 


-- 
Regards,
  Igor

Re: [Qemu-devel] [PATCH 1/3] target-i386: add EXT2_PPRO_FEATURES #define

2012-12-14 Thread Andreas Färber 
Am 12.12.2012 23:22, schrieb Eduardo Habkost:
 Instead of repeating the (PPRO_FEATURES  CPUID_EXT2_AMD_ALIASES)
 expression everywhere, use EXT2_PPRO_FEATURES.
 
 Signed-off-by: Eduardo Habkost ehabk...@redhat.com

Technically this patch looks fine. My dislike for these defines aside, I
have doubts about the semantics: This is masking out AMD_ALIASES
(whatever that is exactly I still need to look up) - doesn't that rather
call for EXT2_PPRO_INTEL_FEATURES or so? (But then again the Pentium Pro
was an Intel chip so AMD sounds confusing...) Or does no AMD model
actually inherit those AMD aliases? This at least deserves a mention in
the commit message (no need to resend then).

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 1/3] target-i386: add EXT2_PPRO_FEATURES #define

2012-12-14 Thread Eduardo Habkost 
On Fri, Dec 14, 2012 at 12:44:43PM +0100, Andreas Färber wrote:
 Am 12.12.2012 23:22, schrieb Eduardo Habkost:
  Instead of repeating the (PPRO_FEATURES  CPUID_EXT2_AMD_ALIASES)
  expression everywhere, use EXT2_PPRO_FEATURES.
  
  Signed-off-by: Eduardo Habkost ehabk...@redhat.com
 
 Technically this patch looks fine. My dislike for these defines aside, I
 have doubts about the semantics: This is masking out AMD_ALIASES
 (whatever that is exactly I still need to look up) - doesn't that rather
 call for EXT2_PPRO_INTEL_FEATURES or so? (But then again the Pentium Pro
 was an Intel chip so AMD sounds confusing...) Or does no AMD model
 actually inherit those AMD aliases? This at least deserves a mention in
 the commit message (no need to resend then).

The code is not masking out AMD_ALIASES, it is is the opposite: it's
keeping only the AMD_ALIASES bits. CPUID_EXT2_AMD_ALIASES are the bits
in cpuid_ext2_features that have to be directly duplicated from
cpuid_features, but only on AMD CPUs. Intel CPUs don't have those bit
aliases.

Anyway, you have a point: I think the #define could be named
PPRO_EXT2_FEATURES_AMD instead, to make it clear that those bits make
sense only on AMD CPU models.

Also: on the models that actually have vendor=AMD, we can probably
remove those bits entirely from the table, as we now have code that
makes sure the feature aliases on cpuid_ext2_features are consistent
with cpuid_features. But we still have a few exceptions that have
vendor=Intel (kvm64, kvm32, and n270), that have to be (carefully) fixed
later.

The main reason I sent this change was to make it easier to
automatically line-wrap the feature lists in the code to 80-columns on
patch 3/3. I think I will reorder and send the feature-array patch
first, and fix the CPUID_EXT2_AMD_ALIASES mess later.

-- 
Eduardo