[1/3] target-i386: add EXT2_PPRO_FEATURES #define

Instead of repeating the (PPRO_FEATURES & CPUID_EXT2_AMD_ALIASES)
expression everywhere, use EXT2_PPRO_FEATURES.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 target-i386/cpu.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

On Wed, 12 Dec 2012 20:22:24 -0200
Eduardo Habkost <ehabkost@redhat.com> wrote:

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

Reviewed-by: Igor Mammedov <imammedo@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 = 0x80000008,
>          .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 = 0x80000008,
>      },
> @@ -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 = 0x8000000A,
> -- 
> 1.7.11.7
> 
>

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 <ehabkost@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

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 <ehabkost@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.