From mboxrd@z Thu Jan  1 00:00:00 1970
From: Andre Przywara <andre.przywara@amd.com>
Subject: Re: [PATCH] Add cpu model configuration support.. (resend)
Date: Tue, 2 Feb 2010 12:07:09 +0100
Message-ID: <4B68075D.2030701@amd.com>
References: <4B672535.5050303@redhat.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="ISO-8859-1"; format=flowed
Content-Transfer-Encoding: 7bit
Cc: KVM list <kvm@vger.kernel.org>, qemu-devel@nongnu.org,
	donald.d.dugger@intel.com, Anthony Liguori <anthony@codemonkey.ws>
To: john cooper <john.cooper@redhat.com>
Return-path: <kvm-owner@vger.kernel.org>
Received: from va3ehsobe003.messaging.microsoft.com ([216.32.180.13]:2413 "EHLO
	VA3EHSOBE003.bigfish.com" rhost-flags-OK-OK-OK-FAIL)
	by vger.kernel.org with ESMTP id S1754865Ab0BBLHj (ORCPT
	<rfc822;kvm@vger.kernel.org>); Tue, 2 Feb 2010 06:07:39 -0500
In-Reply-To: <4B672535.5050303@redhat.com>
Sender: kvm-owner@vger.kernel.org
List-ID: <kvm.vger.kernel.org>

John,

just two comments from skimming through the patch:

> diff --git a/sysconfigs/target/target-x86_64.conf b/sysconfigs/target/target-x86_64.conf
> new file mode 100644
> index 0000000..43ad282
> --- /dev/null
> +++ b/sysconfigs/target/target-x86_64.conf
> @@ -0,0 +1,86 @@
> +# x86 CPU MODELS
> +
> +[cpudef]
> +   name = "Conroe"
> +   level = "2"
> +   vendor = "GenuineIntel"
> +   family = "6"
> +   model = "2"
> +   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"
> +   extfeature_edx = "fxsr mmx pat cmov pge apic cx8 mce pae msr tsc pse de fpu    lm syscall nx"
> +   extfeature_ecx = "lahf_lm"
Wouldn't it be much more user friendly to merge them all into one 
string? Just from the feature names it is quite obscure to guess which 
flag belongs into which string (especially since we lack the EXTn_ 
prefix we had in helper.c). I haven't tried it, but the parsing code 
looks like this shouldn't be too hard.
To avoid overlong lines one could think about a += operator.

> @@ -129,7 +201,14 @@ 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)
> -static x86_def_t x86_defs[] = {
> +
> +/* maintains list of cpu model definitions
> + */
> +static x86_def_t *x86_defs = {NULL};
> +
> +/* built-in cpu model definitions (deprecated)
> + */
> +static x86_def_t builtin_x86_defs[] = {
>  #ifdef TARGET_X86_64
>      {
>          .name = "qemu64",

I would just drop all definitions here except qemu{32,64} and 
kvm{32,64}. The other models should be described in the config file.

Regards,
Andre.

-- 
Andre Przywara
AMD-Operating System Research Center (OSRC), Dresden, Germany
Tel: +49 351 448 3567 12
----to satisfy European Law for business letters:
Advanced Micro Devices GmbH
Karl-Hammerschmidt-Str. 34, 85609 Dornach b. Muenchen
Geschaeftsfuehrer: Andrew Bowd; Thomas M. McCoy; Giuliano Meroni
Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632