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

[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 <address@hidden>
> 
> 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