This is a cleanup that tries to solve two small issues: - We don't need a separate kvm_pv_eoi_features variable just to keep a constant calculated at compile-time, and this style would require adding a separate variable (that's declared twice because of the CONFIG_KVM ifdef) for each feature that's going to be enabled/disable by machine-type compat code. - The pc-1.3 code is setting the kvm_pv_eoi flag on cpuid_kvm_features even when KVM is disabled at runtime. This small incosistency in the cpuid_kvm_features field isn't a problem today because cpuid_kvm_features is ignored by the TCG code, but it may cause unexpected problems later when refactoring the CPUID handling code. This patch eliminates the kvm_pv_eoi_features variable and simply uses kvm_enabled() inside the enable_kvm_pv_eoi() compat function, so it enables kvm_pv_eoi only if KVM is enabled. I believe this makes the behavior of enable_kvm_pv_eoi() clearer and easier to understand. Signed-off-by: Eduardo Habkost <ehabkost(a)redhat.com&gt; --- Cc: kvm(a)vger.kernel.org Cc: Michael S. Tsirkin <mst(a)redhat.com&gt; Cc: Gleb Natapov <gleb(a)redhat.com&gt; Cc: Marcelo Tosatti <mtosatti(a)redhat.com&gt; Changes v2: - Coding style fix Changes v3: - Eliminate #ifdef by using the fake KVM_FEATURE_PV_EOI #define --- target-i386/cpu.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/target-i386/cpu.c b/target-i386/cpu.c index 951e206..40400ac 100644 --- a/target-i386/cpu.c +++ b/target-i386/cpu.c @@ -164,15 +164,15 @@ static uint32_t kvm_default_features = (1 << KVM_FEATURE_CLOCKSOURCE) | (1 << KVM_FEATURE_ASYNC_PF) | (1 << KVM_FEATURE_STEAL_TIME) | (1 << KVM_FEATURE_CLOCKSOURCE_STABLE_BIT); -static const uint32_t kvm_pv_eoi_features = (0x1 << KVM_FEATURE_PV_EOI); #else static uint32_t kvm_default_features = 0; -static const uint32_t kvm_pv_eoi_features = 0; #endif void enable_kvm_pv_eoi(void) { - kvm_default_features |= kvm_pv_eoi_features; + if (kvm_enabled()) { + kvm_default_features |= (1UL << KVM_FEATURE_PV_EOI); + } } void host_cpuid(uint32_t function, uint32_t count, -- 1.7.11.7

On Wed, Jan 09, 2013 at 10:46:12AM +0100, Igor Mammedov wrote: True. The other flags in kvm_default_features may be already set and will be copied to cpuid_kvm_features even if kvm_enabled() is false. I had a previous version of this patch that also changed the code using kvm_default_features to check kvm_enabled(), but I removed that part and kept only the kvm_pv_eoi_features variable removal. Andreas, please ignore this patch (it is not necessary anymore as this series doesn't include machine-type compatibility code for kvm_mmu). Patches 2-7 don't depend on this patch and should apply cleanly without it. I will send a new version later, probably with a separate patch to ignore kvm_default_features if kvm_enabled() is false (so there's no need to even check kvm_enabled() inside enable_kvm_pv_eoi()). -- Eduardo

KVM_CAP_PV_MMU capability reporting was removed from the kernel since v2.6.33 (see commit a68a6a7282373), and was completely removed from the kernel since v3.3 (see commit fb92045843). It doesn't make sense to keep it enabled by default, as it would cause unnecessary hassle when using the "enforce" flag. This disables kvm_mmu on all machine-types. With this fix, the possible scenarios when migrating from QEMU <= 1.3 to QEMU 1.4 are; ------------+------------+---------------------------------------------------- src kernel | dst kernel | Result ------------+------------+---------------------------------------------------- <= 2.6.32 | >= 3.3 | correct live migration is impossible <= 2.6.32 | <= 3.2 | kvm_mmu will be disabled on next guest reboot * ------------+------------+---------------------------------------------------- * If they are running kernel <= 2.6.32 and want kvm_mmu to be kept enabled on guest reboot, they can explicitly add +kvm_mmu to the QEMU command-line. Using 2.6.33 and higher, it is not possible to enable kvm_mmu explicitly anymore. Signed-off-by: Eduardo Habkost <ehabkost(a)redhat.com&gt; --- Cc: kvm(a)vger.kernel.org Cc: Michael S. Tsirkin <mst(a)redhat.com&gt; Cc: Gleb Natapov <gleb(a)redhat.com&gt; Cc: Marcelo Tosatti <mtosatti(a)redhat.com&gt; Cc: libvir-list(a)redhat.com Cc: Jiri Denemark <jdenemar(a)redhat.com&gt; Changes v2: - Coding style fix - Removed redundant comments above machine init functions Changes v3: - Eliminate per-machine-type compatibility code --- target-i386/cpu.c | 1 - 1 file changed, 1 deletion(-) diff --git a/target-i386/cpu.c b/target-i386/cpu.c index 40400ac..b09b625 100644 --- a/target-i386/cpu.c +++ b/target-i386/cpu.c @@ -159,7 +159,6 @@ int enforce_cpuid = 0; #if defined(CONFIG_KVM) static uint32_t kvm_default_features = (1 << KVM_FEATURE_CLOCKSOURCE) | (1 << KVM_FEATURE_NOP_IO_DELAY) | - (1 << KVM_FEATURE_MMU_OP) | (1 << KVM_FEATURE_CLOCKSOURCE2) | (1 << KVM_FEATURE_ASYNC_PF) | (1 << KVM_FEATURE_STEAL_TIME) | -- 1.7.11.7

This introduces a FeatureWord enum, FeatureWordInfo struct (with generation information about a feature word), and a FeatureWordArray typedef, and changes add_flagname_to_bitmaps() code and cpu_x86_parse_featurestr() to use the new typedefs instead of separate variables for each feature word. This will help us keep the code at kvm_check_features_against_host(), cpu_x86_parse_featurestr() and add_flagname_to_bitmaps() sane while adding new feature name arrays. Signed-off-by: Eduardo Habkost <ehabkost(a)redhat.com&gt; --- target-i386/cpu.c | 97 +++++++++++++++++++++++++++---------------------------- target-i386/cpu.h | 15 +++++++++ 2 files changed, 63 insertions(+), 49 deletions(-) diff --git a/target-i386/cpu.c b/target-i386/cpu.c index b09b625..7d62d48 100644 --- a/target-i386/cpu.c +++ b/target-i386/cpu.c @@ -124,6 +124,20 @@ static const char *cpuid_7_0_ebx_feature_name[] = { NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, }; +typedef struct FeatureWordInfo { + const char **feat_names; +} FeatureWordInfo; + +static FeatureWordInfo feature_word_info[FEATURE_WORDS] = { + [FEAT_1_EDX] = { .feat_names = feature_name }, + [FEAT_1_ECX] = { .feat_names = ext_feature_name }, + [FEAT_8000_0001_EDX] = { .feat_names = ext2_feature_name }, + [FEAT_8000_0001_ECX] = { .feat_names = ext3_feature_name }, + [FEAT_KVM] = { .feat_names = kvm_feature_name }, + [FEAT_SVM] = { .feat_names = svm_feature_name }, + [FEAT_7_0_EBX] = { .feat_names = cpuid_7_0_ebx_feature_name }, +}; + const char *get_register_name_32(unsigned int reg) { static const char *reg_names[CPU_NB_REGS32] = { @@ -271,23 +285,20 @@ static bool lookup_feature(uint32_t *pval, const char *s, const char *e, return found; } -static void add_flagname_to_bitmaps(const char *flagname, uint32_t *features, - uint32_t *ext_features, - uint32_t *ext2_features, - uint32_t *ext3_features, - uint32_t *kvm_features, - uint32_t *svm_features, - uint32_t *cpuid_7_0_ebx_features) +static void add_flagname_to_bitmaps(const char *flagname, + FeatureWordArray words) { - if (!lookup_feature(features, flagname, NULL, feature_name) && - !lookup_feature(ext_features, flagname, NULL, ext_feature_name) && - !lookup_feature(ext2_features, flagname, NULL, ext2_feature_name) && - !lookup_feature(ext3_features, flagname, NULL, ext3_feature_name) && - !lookup_feature(kvm_features, flagname, NULL, kvm_feature_name) && - !lookup_feature(svm_features, flagname, NULL, svm_feature_name) && - !lookup_feature(cpuid_7_0_ebx_features, flagname, NULL, - cpuid_7_0_ebx_feature_name)) - fprintf(stderr, "CPU feature %s not found\n", flagname); + FeatureWord w; + for (w = 0; w < FEATURE_WORDS; w++) { + FeatureWordInfo *wi = &feature_word_info[w]; + if (wi->feat_names && + lookup_feature(&words[w], flagname, NULL, wi->feat_names)) { + break; + } + } + if (w == FEATURE_WORDS) { + fprintf(stderr, "CPU feature %s not found\n", flagname); + } } typedef struct x86_def_t { @@ -1256,35 +1267,23 @@ static int cpu_x86_parse_featurestr(x86_def_t *x86_cpu_def, char *features) unsigned int i; char *featurestr; /* Single 'key=value" string being parsed */ /* Features to be added */ - uint32_t plus_features = 0, plus_ext_features = 0; - uint32_t plus_ext2_features = 0, plus_ext3_features = 0; - uint32_t plus_kvm_features = kvm_default_features, plus_svm_features = 0; - uint32_t plus_7_0_ebx_features = 0; + FeatureWordArray plus_features = { + [FEAT_KVM] = kvm_default_features, + }; /* Features to be removed */ - uint32_t minus_features = 0, minus_ext_features = 0; - uint32_t minus_ext2_features = 0, minus_ext3_features = 0; - uint32_t minus_kvm_features = 0, minus_svm_features = 0; - uint32_t minus_7_0_ebx_features = 0; + FeatureWordArray minus_features = { 0 }; uint32_t numvalue; - add_flagname_to_bitmaps("hypervisor", &plus_features, - &plus_ext_features, &plus_ext2_features, &plus_ext3_features, - &plus_kvm_features, &plus_svm_features, &plus_7_0_ebx_features); + add_flagname_to_bitmaps("hypervisor", plus_features); featurestr = features ? strtok(features, ",") : NULL; while (featurestr) { char *val; if (featurestr[0] == '+') { - add_flagname_to_bitmaps(featurestr + 1, &plus_features, - &plus_ext_features, &plus_ext2_features, - &plus_ext3_features, &plus_kvm_features, - &plus_svm_features, &plus_7_0_ebx_features); + add_flagname_to_bitmaps(featurestr + 1, plus_features); } else if (featurestr[0] == '-') { - add_flagname_to_bitmaps(featurestr + 1, &minus_features, - &minus_ext_features, &minus_ext2_features, - &minus_ext3_features, &minus_kvm_features, - &minus_svm_features, &minus_7_0_ebx_features); + add_flagname_to_bitmaps(featurestr + 1, minus_features); } else if ((val = strchr(featurestr, '='))) { *val = 0; val++; if (!strcmp(featurestr, "family")) { @@ -1384,20 +1383,20 @@ static int cpu_x86_parse_featurestr(x86_def_t *x86_cpu_def, char *features) } featurestr = strtok(NULL, ","); } - x86_cpu_def->features |= plus_features; - x86_cpu_def->ext_features |= plus_ext_features; - x86_cpu_def->ext2_features |= plus_ext2_features; - x86_cpu_def->ext3_features |= plus_ext3_features; - x86_cpu_def->kvm_features |= plus_kvm_features; - x86_cpu_def->svm_features |= plus_svm_features; - x86_cpu_def->cpuid_7_0_ebx_features |= plus_7_0_ebx_features; - x86_cpu_def->features &= ~minus_features; - x86_cpu_def->ext_features &= ~minus_ext_features; - x86_cpu_def->ext2_features &= ~minus_ext2_features; - x86_cpu_def->ext3_features &= ~minus_ext3_features; - x86_cpu_def->kvm_features &= ~minus_kvm_features; - x86_cpu_def->svm_features &= ~minus_svm_features; - x86_cpu_def->cpuid_7_0_ebx_features &= ~minus_7_0_ebx_features; + x86_cpu_def->features |= plus_features[FEAT_1_EDX]; + x86_cpu_def->ext_features |= plus_features[FEAT_1_ECX]; + x86_cpu_def->ext2_features |= plus_features[FEAT_8000_0001_EDX]; + x86_cpu_def->ext3_features |= plus_features[FEAT_8000_0001_ECX]; + x86_cpu_def->kvm_features |= plus_features[FEAT_KVM]; + x86_cpu_def->svm_features |= plus_features[FEAT_SVM]; + x86_cpu_def->cpuid_7_0_ebx_features |= plus_features[FEAT_7_0_EBX]; + x86_cpu_def->features &= ~minus_features[FEAT_1_EDX]; + x86_cpu_def->ext_features &= ~minus_features[FEAT_1_ECX]; + x86_cpu_def->ext2_features &= ~minus_features[FEAT_8000_0001_EDX]; + x86_cpu_def->ext3_features &= ~minus_features[FEAT_8000_0001_ECX]; + x86_cpu_def->kvm_features &= ~minus_features[FEAT_KVM]; + x86_cpu_def->svm_features &= ~minus_features[FEAT_SVM]; + x86_cpu_def->cpuid_7_0_ebx_features &= ~minus_features[FEAT_7_0_EBX]; if (check_cpuid && kvm_enabled()) { if (kvm_check_features_against_host(x86_cpu_def) && enforce_cpuid) goto error; diff --git a/target-i386/cpu.h b/target-i386/cpu.h index e56921b..e4a7c50 100644 --- a/target-i386/cpu.h +++ b/target-i386/cpu.h @@ -361,6 +361,21 @@ #define MSR_VM_HSAVE_PA 0xc0010117 +/* CPUID feature words */ +typedef enum FeatureWord { + FEAT_1_EDX, /* CPUID[1].EDX */ + FEAT_1_ECX, /* CPUID[1].ECX */ + FEAT_7_0_EBX, /* CPUID[EAX=7,ECX=0].EBX */ + FEAT_8000_0001_EDX, /* CPUID[8000_0001].EDX */ + FEAT_8000_0001_ECX, /* CPUID[8000_0001].ECX */ + FEAT_C000_0001_EDX, /* CPUID[C000_0001].EDX */ + FEAT_KVM, /* CPUID[4000_0001].EAX (KVM_CPUID_FEATURES) */ + FEAT_SVM, /* CPUID[8000_000A].EDX */ + FEATURE_WORDS, +} FeatureWord; + +typedef uint32_t FeatureWordArray[FEATURE_WORDS]; + /* cpuid_features bits */ #define CPUID_FP87 (1 << 0) #define CPUID_VME (1 << 1) -- 1.7.11.7

Instead of carrying the CPUID leaf/register and feature name array on the model_features_t struct, move that information into feature_word_info so it can be reused by other functions. The goal is to eventually kill model_features_t entirely, but to do that we have to either convert x86_def_t.features to an array or use offsetof() inside FeatureWordInfo (to replace the pointers inside model_features_t). So by now just move most of the model_features_t fields to FeatureWordInfo except for the two pointers to local arguments. Signed-off-by: Eduardo Habkost <ehabkost(a)redhat.com&gt; --- target-i386/cpu.c | 73 +++++++++++++++++++++++++++++++++++++------------------ 1 file changed, 49 insertions(+), 24 deletions(-) diff --git a/target-i386/cpu.c b/target-i386/cpu.c index 7d62d48..4b3ee63 100644 --- a/target-i386/cpu.c +++ b/target-i386/cpu.c @@ -126,16 +126,39 @@ static const char *cpuid_7_0_ebx_feature_name[] = { typedef struct FeatureWordInfo { const char **feat_names; + uint32_t cpuid_eax; /* Input EAX for CPUID */ + int cpuid_reg; /* R_* register constant */ } FeatureWordInfo; static FeatureWordInfo feature_word_info[FEATURE_WORDS] = { - [FEAT_1_EDX] = { .feat_names = feature_name }, - [FEAT_1_ECX] = { .feat_names = ext_feature_name }, - [FEAT_8000_0001_EDX] = { .feat_names = ext2_feature_name }, - [FEAT_8000_0001_ECX] = { .feat_names = ext3_feature_name }, - [FEAT_KVM] = { .feat_names = kvm_feature_name }, - [FEAT_SVM] = { .feat_names = svm_feature_name }, - [FEAT_7_0_EBX] = { .feat_names = cpuid_7_0_ebx_feature_name }, + [FEAT_1_EDX] = { + .feat_names = feature_name, + .cpuid_eax = 1, .cpuid_reg = R_EDX, + }, + [FEAT_1_ECX] = { + .feat_names = ext_feature_name, + .cpuid_eax = 1, .cpuid_reg = R_ECX, + }, + [FEAT_8000_0001_EDX] = { + .feat_names = ext2_feature_name, + .cpuid_eax = 0x80000001, .cpuid_reg = R_EDX, + }, + [FEAT_8000_0001_ECX] = { + .feat_names = ext3_feature_name, + .cpuid_eax = 0x80000001, .cpuid_reg = R_ECX, + }, + [FEAT_KVM] = { + .feat_names = kvm_feature_name, + .cpuid_eax = KVM_CPUID_FEATURES, .cpuid_reg = R_EAX, + }, + [FEAT_SVM] = { + .feat_names = svm_feature_name, + .cpuid_eax = 0x8000000A, .cpuid_reg = R_EDX, + }, + [FEAT_7_0_EBX] = { + .feat_names = cpuid_7_0_ebx_feature_name, + .cpuid_eax = 7, .cpuid_reg = R_EBX, + }, }; const char *get_register_name_32(unsigned int reg) @@ -162,9 +185,7 @@ const char *get_register_name_32(unsigned int reg) typedef struct model_features_t { uint32_t *guest_feat; uint32_t *host_feat; - const char **flag_names; - uint32_t cpuid; - int reg; + FeatureWord feat_word; } model_features_t; int check_cpuid = 0; @@ -935,19 +956,19 @@ static void kvm_cpu_fill_host(x86_def_t *x86_cpu_def) #endif /* CONFIG_KVM */ } -static int unavailable_host_feature(struct model_features_t *f, uint32_t mask) +static int unavailable_host_feature(FeatureWordInfo *f, uint32_t mask) { int i; for (i = 0; i < 32; ++i) if (1 << i & mask) { - const char *reg = get_register_name_32(f->reg); + const char *reg = get_register_name_32(f->cpuid_reg); assert(reg); fprintf(stderr, "warning: host doesn't support requested feature: " "CPUID.%02XH:%s%s%s [bit %d]\n", - f->cpuid, reg, - f->flag_names[i] ? "." : "", - f->flag_names[i] ? f->flag_names[i] : "", i); + f->cpuid_eax, reg, + f->feat_names[i] ? "." : "", + f->feat_names[i] ? f->feat_names[i] : "", i); break; } return 0; @@ -965,25 +986,29 @@ static int kvm_check_features_against_host(x86_def_t *guest_def) int rv, i; struct model_features_t ft[] = { {&guest_def->features, &host_def.features, - feature_name, 0x00000001, R_EDX}, + FEAT_1_EDX }, {&guest_def->ext_features, &host_def.ext_features, - ext_feature_name, 0x00000001, R_ECX}, + FEAT_1_ECX }, {&guest_def->ext2_features, &host_def.ext2_features, - ext2_feature_name, 0x80000001, R_EDX}, + FEAT_8000_0001_EDX }, {&guest_def->ext3_features, &host_def.ext3_features, - ext3_feature_name, 0x80000001, R_ECX} + FEAT_8000_0001_ECX }, }; assert(kvm_enabled()); kvm_cpu_fill_host(&host_def); - for (rv = 0, i = 0; i < ARRAY_SIZE(ft); ++i) - for (mask = 1; mask; mask <<= 1) + for (rv = 0, i = 0; i < ARRAY_SIZE(ft); ++i) { + FeatureWord w = ft[i].feat_word; + FeatureWordInfo *wi = &feature_word_info[w]; + for (mask = 1; mask; mask <<= 1) { if (*ft[i].guest_feat & mask && !(*ft[i].host_feat & mask)) { - unavailable_host_feature(&ft[i], mask); - rv = 1; - } + unavailable_host_feature(wi, mask); + rv = 1; + } + } + } return rv; } -- 1.7.11.7

Am 07.01.2013 19:20, schrieb Eduardo Habkost: Missing spaces, fixed. Andreas [snip] -- SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg