Re: [Qemu-devel] [PATCH 5/7] cpu model bug fixes and definition corrections

["Daniel P. Berrange" <berrange@redhat.com>]

On Mon, May 23, 2011 at 05:47:19PM -0400, john cooper wrote:
> This patch was intended to address the replicated feature
> flags in cpuid 8000_0001:edx from cpuid 0000_0001:edx.
> This is due to AMD's definition where these flags are
> mostly cloned in the 8000_0001:edx cpuid function.
> qemu64 attempted to glue together the respective Intel
> and AMD nearly disjoint features and this propagated to
> the new Intel models as doing so was believed conservative
> at the time.  However after further soak and test lugging
> around this cruft doesn't provide any value, could
> conceivably confuse a guest, and has confused users trying
> to maintain/add cpu definitions.  This also caused issues
> for libvirt attempting to track this mis-encoding.
> 
> So we've here tossed out the AMD replicated definitions
> from the Intel models, added a few replications into AMD
> definitions which were missing according to AMD's latest
> CPUID document, and reordered the config file flags to
> follow intuitive sequential bit ordering.  Also two flag
> name aliases were added for clarity to Intel models.  The
> end result being the models definitions now conform to
> their respective cpuid specifications sans x2apic which is
> emulated by kvm.
> 
> This was tested with the following combinations:
> 
>     [Conroe, Penryn, Nehalem] x [F12-64, win64, win32] -- Intel host
>     [Opteron_G1, Opteron_G2, Opteron_G3] x [F12-64, win64, win32] -- AMD host
> 
> Yielding successful boots in all cases.
> 
> Signed-off-by: john cooper <john.cooper@redhat.com>
> ---
> 
> diff --git a/sysconfigs/target/target-x86_64.conf b/sysconfigs/target/target-x86_64.conf
> index 3b85b2e..3874ff1 100644
> --- a/sysconfigs/target/target-x86_64.conf
> +++ b/sysconfigs/target/target-x86_64.conf
> @@ -7,9 +7,9 @@
>     family = "6"
>     model = "15"
>     stepping = "3"
> -   feature_edx = "sse2 sse fxsr mmx pat cmov pge sep apic cx8 mce pae msr tsc pse de fpu    mtrr clflush mca pse36"
> -   feature_ecx = "sse3 ssse3 x2apic"
> -   extfeature_edx = "fxsr mmx pat cmov pge apic cx8 mce pae msr tsc pse de fpu    lm syscall nx"
> +   feature_edx = "sse2 sse fxsr mmx clflush pse36 pat cmov mca pge mtrr sep apic cx8 mce pae msr tsc pse de fpu"
> +   feature_ecx = "x2apic ssse3 sse3"
> +   extfeature_edx = "i64 syscall xd"
>     extfeature_ecx = "lahf_lm"
>     xlevel = "0x8000000A"
>     model_id = "Intel Celeron_4x0 (Conroe/Merom Class Core 2)"
> @@ -21,9 +21,9 @@
>     family = "6"
>     model = "23"
>     stepping = "3"
> -   feature_edx = "sse2 sse fxsr mmx pat cmov pge sep apic cx8 mce pae msr tsc pse de fpu    mtrr clflush mca pse36"
> -   feature_ecx = "sse3 cx16 ssse3 sse4.1 x2apic"
> -   extfeature_edx = "fxsr mmx pat cmov pge apic cx8 mce pae msr tsc pse de fpu    lm syscall nx"
> +   feature_edx = "sse2 sse fxsr mmx clflush pse36 pat cmov mca pge mtrr sep apic cx8 mce pae msr tsc pse de fpu"
> +   feature_ecx = "x2apic sse4.1 cx16 ssse3 sse3"
> +   extfeature_edx = "i64 syscall xd"
>     extfeature_ecx = "lahf_lm"
>     xlevel = "0x8000000A"
>     model_id = "Intel Core 2 Duo P9xxx (Penryn Class Core 2)"
> @@ -35,9 +35,9 @@
>     family = "6"
>     model = "26"
>     stepping = "3"
> -   feature_edx = "sse2 sse fxsr mmx pat cmov pge sep apic cx8 mce pae msr tsc pse de fpu    mtrr clflush mca pse36"
> -   feature_ecx = "sse3 cx16 ssse3 sse4.1 sse4.2 popcnt x2apic"
> -   extfeature_edx = "fxsr mmx pat cmov pge apic cx8 mce pae msr tsc pse de fpu    lm syscall nx"
> +   feature_edx = "sse2 sse fxsr mmx clflush pse36 pat cmov mca pge mtrr sep apic cx8 mce pae msr tsc pse de fpu"
> +   feature_ecx = "popcnt x2apic sse4.2 sse4.1 cx16 ssse3 sse3"
> +   extfeature_edx = "i64 syscall xd"
>     extfeature_ecx = "lahf_lm"
>     xlevel = "0x8000000A"
>     model_id = "Intel Core i7 9xx (Nehalem Class Core i7)"
> @@ -49,10 +49,10 @@
>     family = "15"
>     model = "6"
>     stepping = "1"
> -   feature_edx = "sse2 sse fxsr mmx pat cmov pge sep apic cx8 mce pae msr tsc pse de fpu    mtrr clflush mca pse36"
> -   feature_ecx = "sse3 x2apic"
> -   extfeature_edx = "fxsr mmx pat cmov pge apic cx8 mce pae msr tsc pse de fpu    lm syscall nx"
> -#   extfeature_ecx = ""
> +   feature_edx = "sse2 sse fxsr mmx clflush pse36 pat cmov mca pge mtrr sep apic cx8 mce pae msr tsc pse de fpu"
> +   feature_ecx = "x2apic sse3"	# x2apic kvm emulated
> +   extfeature_edx = "lm fxsr mmx nx pse36 pat cmov mca pge mtrr syscall apic cx8 mce pae msr tsc pse de fpu"
> +   extfeature_ecx = " "
>     xlevel = "0x80000008"
>     model_id = "AMD Opteron 240 (Gen 1 Class Opteron)"
>  
> @@ -63,9 +63,9 @@
>     family = "15"
>     model = "6"
>     stepping = "1"
> -   feature_edx = "sse2 sse fxsr mmx pat cmov pge sep apic cx8 mce pae msr tsc pse de fpu    mtrr clflush mca pse36"
> -   feature_ecx = "sse3 cx16 x2apic"
> -   extfeature_edx = "fxsr mmx pat cmov pge apic cx8 mce pae msr tsc pse de fpu    lm syscall nx rdtscp"
> +   feature_edx = "sse2 sse fxsr mmx clflush pse36 pat cmov mca pge mtrr sep apic cx8 mce pae msr tsc pse de fpu"
> +   feature_ecx = "x2apic cx16 sse3"	# x2apic kvm emulated
> +   extfeature_edx = "lm rdtscp fxsr mmx nx pse36 pat cmov mca pge mtrr syscall apic cx8 mce pae msr tsc pse de fpu"
>     extfeature_ecx = "svm lahf_lm"
>     xlevel = "0x80000008"
>     model_id = "AMD Opteron 22xx (Gen 2 Class Opteron)"
> @@ -77,10 +77,10 @@
>     family = "15"
>     model = "6"
>     stepping = "1"
> -   feature_edx = "sse2 sse fxsr mmx pat cmov pge sep apic cx8 mce pae msr tsc pse de fpu    mtrr clflush mca pse36"
> -   feature_ecx = "sse3 cx16 monitor popcnt x2apic"
> -   extfeature_edx = "fxsr mmx pat cmov pge apic cx8 mce pae msr tsc pse de fpu    lm syscall nx rdtscp"
> -   extfeature_ecx = "svm sse4a  abm misalignsse lahf_lm"
> +   feature_edx = "sse2 sse fxsr mmx clflush pse36 pat cmov mca pge mtrr sep apic cx8 mce pae msr tsc pse de fpu"
> +   feature_ecx = "popcnt x2apic cx16 monitor sse3"	# x2apic kvm emulated
> +   extfeature_edx = "lm rdtscp fxsr mmx nx pse36 pat cmov mca pge mtrr syscall apic cx8 mce pae msr tsc pse de fpu"
> +   extfeature_ecx = "misalignsse sse4a abm svm lahf_lm"
>     xlevel = "0x80000008"
>     model_id = "AMD Opteron 23xx (Gen 3 Class Opteron)"
>  
> diff --git a/target-i386/cpuid.c b/target-i386/cpuid.c
> index c151f12..fc72f7b 100644
> --- a/target-i386/cpuid.c
> +++ b/target-i386/cpuid.c
> @@ -57,9 +57,9 @@ static const char *ext2_feature_name[] = {
>      "cx8" /* AMD CMPXCHG8B */, "apic", NULL, "syscall",
>      "mtrr", "pge", "mca", "cmov",
>      "pat", "pse36", NULL, NULL /* Linux mp */,
> -    "nx" /* Intel xd */, NULL, "mmxext", "mmx",
> +    "nx|xd", NULL, "mmxext", "mmx",
>      "fxsr", "fxsr_opt" /* AMD ffxsr */, "pdpe1gb" /* AMD Page1GB */, "rdtscp",
> -    NULL, "lm" /* Intel 64 */, "3dnowext", "3dnow",
> +    NULL, "lm|i64", "3dnowext", "3dnow",
>  };
>  static const char *ext3_feature_name[] = {
>      "lahf_lm" /* AMD LahfSahf */, "cmp_legacy", "svm", "extapic" /* AMD ExtApicSpace */,

This diff seems to have a huge, arbitrary re-ordering of existing
flags, combined with a tiny number of changes. It would be really
nice if the arbitrary re-ordering was removed, leaving just the
functional changes clearly visible. Alternatively split the patch
into 2, the first doing a no-functional-change reordering, and the
second doing the actual fixes.

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|