Re: [Qemu-devel] [PATCH] Add definitions for current cpu models..

[Jamie Lokier]

john cooper wrote:
>      {
> +        .name = "Merom",
> +        .level = 2,
> +        .vendor1 = CPUID_VENDOR_INTEL_1,
> +        .vendor2 = CPUID_VENDOR_INTEL_2,
> +        .vendor3 = CPUID_VENDOR_INTEL_3,
> +        .family = 6, /* P6 */
> +        .model = 2,
> +        .stepping = 3,
> +        .features = PPRO_FEATURES | 
> +        /* these features are needed for Win64 and aren't fully implemented 
> */
> +            CPUID_MTRR | CPUID_CLFLUSH | CPUID_MCA |
> +        /* this feature is needed for Solaris and isn't fully implemented */
> +            CPUID_PSE36,
> +        .ext_features = CPUID_EXT_SSE3,              /* from qemu64 */

Isn't SSE3 a generic feature on these Intel CPUs, so this comment is 
unnecessary?
Or is SSE3 not present on a real Merom?  If so, wouldn't it be better to omit 
it?

> +        .ext2_features = (PPRO_FEATURES & 0x0183F3FF) | 

Could we have a meaningful name for the magic number, please?
Maybe even a:

   #define PPRO_EXT2_FEATURES (PPRO_FEATURES & PPRO_EXT2_MASK)
   #define PPRO_EXT2_MASK     (CPUID_... | CPUID_... | ...) /* Fill in. */

> +            CPUID_EXT2_LM | CPUID_EXT2_SYSCALL | CPUID_EXT2_NX,
> +        .ext3_features = CPUID_EXT3_SVM,     /* from qemu64 */
> +        .xlevel = 0x8000000A,
> +        .model_id = "Intel Merom Core 2",
> +    },

Does this mean requesting an Intel Merom will give the guest AMD's SVM
capability?  That's handy for virtualisation, but not an accurate CPU
model.  It seems inappropriate to name it "Merom", with model_id
"Intel Merom Core 2", if it's adding extra qemu-specific capabilities.

I would think few guests are likely to need the nested-SVM capability,
so it could be omitted when "Merom" is requested, and added as an
additional feature on request from the command line, just like other
cpuid features can be added.

> +    {
> +        .name = "Penryn",
> +        .level = 2,
> +        .vendor1 = CPUID_VENDOR_INTEL_1,
> +        .vendor2 = CPUID_VENDOR_INTEL_2,
> +        .vendor3 = CPUID_VENDOR_INTEL_3,
> +        .family = 6, /* P6 */
> +        .model = 2,
> +        .stepping = 3,
> +        .features = PPRO_FEATURES | 
> +        /* these features are needed for Win64 and aren't fully implemented 
> */
> +            CPUID_MTRR | CPUID_CLFLUSH | CPUID_MCA |
> +        /* this feature is needed for Solaris and isn't fully implemented */
> +            CPUID_PSE36,
> +        .ext_features = CPUID_EXT_SSE3 |
> +            CPUID_EXT_CX16 | CPUID_EXT_SSSE3 | CPUID_EXT_SSE41,
> +        .ext2_features = (PPRO_FEATURES & 0x0183F3FF) | 
> +            CPUID_EXT2_LM | CPUID_EXT2_SYSCALL | CPUID_EXT2_NX,
> +        .ext3_features = CPUID_EXT3_SVM,
> +        .xlevel = 0x8000000A,
> +        .model_id = "Intel Penryn Core 2",
> +    },

Same comments as above for Merom about SVM and the PPRO_FEATURES mask.

You don't include the "from qemu64" comments this time.  Is there a reason?

> +    {
> +        .name = "Nehalem",
> +        .level = 2,
> +        .vendor1 = CPUID_VENDOR_INTEL_1,
> +        .vendor2 = CPUID_VENDOR_INTEL_2,
> +        .vendor3 = CPUID_VENDOR_INTEL_3,
> +        .family = 6, /* P6 */
> +        .model = 2,
> +        .stepping = 3,
> +        .features = PPRO_FEATURES | 
> +        /* these features are needed for Win64 and aren't fully implemented 
> */
> +            CPUID_MTRR | CPUID_CLFLUSH | CPUID_MCA |
> +        /* this feature is needed for Solaris and isn't fully implemented */
> +            CPUID_PSE36,
> +        .ext_features = CPUID_EXT_SSE3 |
> +            CPUID_EXT_CX16 | CPUID_EXT_SSSE3 | CPUID_EXT_SSE41 |
> +            CPUID_EXT_SSE42 | CPUID_EXT_POPCNT,
> +        .ext2_features = (PPRO_FEATURES & 0x0183F3FF) | 
> +            CPUID_EXT2_LM | CPUID_EXT2_SYSCALL | CPUID_EXT2_NX,
> +        .ext3_features = CPUID_EXT3_SVM,
> +        .xlevel = 0x8000000A,
> +        .model_id = "Intel Nehalem Core i7",
> +    },

Same as previous.

> +    {
> +        .name = "Opteron_G1",
> +        .level = 5,
> +        .vendor1 = CPUID_VENDOR_INTEL_1,
> +        .vendor2 = CPUID_VENDOR_INTEL_2,
> +        .vendor3 = CPUID_VENDOR_INTEL_3,

Someone else has already enquired - why Intel vendor id?

> +        .family = 15,
> +        .model = 6,
> +        .stepping = 1,
> +        .features = PPRO_FEATURES | 
> +        /* these features are needed for Win64 and aren't fully implemented 
> */
> +            CPUID_MTRR | CPUID_CLFLUSH | CPUID_MCA |
> +        /* this feature is needed for Solaris and isn't fully implemented */
> +            CPUID_PSE36,
> +        .ext_features = CPUID_EXT_SSE3 | CPUID_EXT_MONITOR,
> +        .ext2_features = (PPRO_FEATURES & 0x0183F3FF) | 
> +            CPUID_EXT2_LM | CPUID_EXT2_SYSCALL | CPUID_EXT2_NX,
> +        .ext3_features = CPUID_EXT3_SVM,
> +        .xlevel = 0x80000008,
> +        .model_id = "AMD Opteron G1",
> +    },

Why do the AMD models have CPUID_EXT_MONITOR but the Intel ones don't.
Is it correct for the CPU models?  Even a lowly 32-bit Intel Core has MONITOR.
Is it omitted for performance?  In that case shouldn't it be omitted for AMD 
too?

> +    {
> +        .name = "Opteron_G2",
> +        .level = 5,
> +        .vendor1 = CPUID_VENDOR_INTEL_1,
> +        .vendor2 = CPUID_VENDOR_INTEL_2,
> +        .vendor3 = CPUID_VENDOR_INTEL_3,
> +        .family = 15,
> +        .model = 6,
> +        .stepping = 1,
> +        .features = PPRO_FEATURES | 
> +        /* these features are needed for Win64 and aren't fully implemented 
> */
> +            CPUID_MTRR | CPUID_CLFLUSH | CPUID_MCA |
> +        /* this feature is needed for Solaris and isn't fully implemented */
> +            CPUID_PSE36 | CPUID_EXT_CX16,

Solaris comment can use plurals here, like Win64 :-)

> +        .model_id = "AMD Opteron G3",
> +    },
> +        /* this feature is needed for Solaris and isn't fully implemented */
> +    {
The above Solaris comment seems out of place.

>          .name = "core2duo",

The new name style is clearly different from the old name style,
"Merom" and "Opteron_G3" vs "core2duo".  It's a bit untidy.

I'm thinking The names "Merom", "Penryn" and so on might be better as
"Intel Meron", "Intel Penryn" and so on (or perhaps with underscores),
and changing "core2duo" to "Intel Core 2 Duo" and so on.

>          .level = 10,
>          .family = 6,
> @@ -319,7 +464,7 @@ static x86_def_t x86_defs[] = {
>          /* Missing: .ext3_features = CPUID_EXT3_LAHF_LM */
>          .xlevel = 0x8000000A,
>          .model_id = "Intel(R) Atom(TM) CPU N270   @ 1.60GHz",
> -    },
> +    }
>  };

What does qemu coding style say about comma after the last element in a list?
(Not serious :-)

> +/* best effort attempt to inform user requested cpu flags aren't making
> + * their way to the guest.  Note: ft[].check_feat ideally should be
> + * specified via a guest_def field to suppress report of extraneous flags.
> + */

With the recent KVM special handling of the SYSCALL feature, so that
Intel 64-bit hosts can run 64-bit guest kernels with 32-bit userspace
without triggering SYSCALL emulation (and working on older host kernels),
how does the feature masking work out for the SYSCALL feature with "check"?

And general comments:

Thanks for adding the CPU models.  You said they've been checked by
Intel and AMD, but as you can tell from my comments, it's hard to tell
(without consulting processor manuals) if they are accurate
descriptions of real CPU models, or approximations.  What's the intention?

They are all 64-bit models.  Any plans to add contemporary 32-bit
models, or is the existing list of those complete enough already?

Thanks,
-- Jamie