[libvirt] [PATCH 0/9] target-i386: make "enforce" flag work as it should
This changes the -cpu check/enforce code to work as it should: it will check every single CPUID bit to make sure it is supported by the host. The changes are a bit intrusive, but: - The longer we take to make "enforce" strict as it should (and make libvirt finally use it), more users will have VMs with migration-unsafe unpredictable guest ABIs. For this reason, I would like to get this into QEMU 1.4. - The changes in this series should affect only users that are already using the "enforce" flag, and I believe whoever is using the "enforce" flag really want the strict behavior introduced by this series. This series is based on Andreas' qom-cpu branch, and depends on the series: Subject: [PATCH 0/2] Disable kvm_mmu_op by default on pc-1.4 Message-Id: <1357311145-16410-1-git-send-email-ehabkost@redhat.com> Git tree for reference: git://github.com/ehabkost/qemu-hacks.git cpu-enforce-all.v1 https://github.com/ehabkost/qemu-hacks/tree/cpu-enforce-all.v1 Eduardo Habkost (9): target-i386: kvm: -cpu host: Use GET_SUPPORTED_CPUID for SVM features target-i386: kvm: Enable all supported KVM features for -cpu host target-i386: check/enforce: Fix CPUID leaf numbers on error messages target-i386: check/enforce: Do not ignore "hypervisor" flag target-i386: check/enforce: Check all CPUID.80000001H.EDX bits target-i386: check/enforce: Check SVM flag support as well target-i386: check/enforce: Eliminate check_feat field target-i386: Call kvm_check_features_against_host() only if CONFIG_KVM is set target-i386: check/enforce: Check all feature words target-i386/cpu.c | 73 +++++++++++++++++++++++++++++++++++++++---------------- target-i386/cpu.h | 3 +++ 2 files changed, 55 insertions(+), 21 deletions(-) -- 1.7.11.7
The existing -cpu host code simply set every bit inside svm_features (initializing it to -1), and that makes it impossible to make the enforce/check options work properly when the user asks for SVM features explicitly in the command-line. So, instead of initializing svm_features to -1, use GET_SUPPORTED_CPUID to fill only the bits that are supported by the host (just like we do for all other CPUID feature words inside kvm_cpu_fill_host()). This will keep the existing behavior (as filter_features_for_kvm() already uses GET_SUPPORTED_CPUID to filter svm_features), but will allow us to properly check for KVM features inside kvm_check_features_against_host() later. For example, we will be able to make this: $ qemu-system-x86_64 -cpu ...,+pfthreshold,enforce refuse to start if the SVM "pfthreshold" feature is not supported by the host (after we fix kvm_check_features_against_host() to check SVM flags as well). Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> --- Changes v2: - Coding style (indentation) fix Cc: Gleb Natapov <gleb@redhat.com> Cc: Marcelo Tosatti <mtosatti@redhat.com> Cc: Joerg Roedel <joro@8bytes.org> Cc: kvm@vger.kernel.org --- target-i386/cpu.c | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/target-i386/cpu.c b/target-i386/cpu.c index ec877c7..649cfb2 100644 --- a/target-i386/cpu.c +++ b/target-i386/cpu.c @@ -906,13 +906,10 @@ static void kvm_cpu_fill_host(x86_def_t *x86_cpu_def) } } - /* - * Every SVM feature requires emulation support in KVM - so we can't just - * read the host features here. KVM might even support SVM features not - * available on the host hardware. Just set all bits and mask out the - * unsupported ones later. - */ - x86_cpu_def->svm_features = -1; + /* Other KVM-specific feature fields: */ + x86_cpu_def->svm_features = + kvm_arch_get_supported_cpuid(s, 0x8000000A, 0, R_EDX); + #endif /* CONFIG_KVM */ } -- 1.7.11.7
When using -cpu host, we don't need to use the kvm_default_features variable, as the user is explicitly asking QEMU to enable all feature supported by the host. This changes the kvm_cpu_fill_host() code to use GET_SUPPORTED_CPUID to initialize the kvm_features field, so we get all host KVM features enabled. This will also allow use to properly check/enforce KVM features inside kvm_check_features_against_host() later. For example, we will be able to make this: $ qemu-system-x86_64 -cpu ...,+kvm_pv_eoi,enforce refuse to start if kvm_pv_eoi is not supported by the host (after we fix kvm_check_features_against_host() to check KVM flags as well). Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> --- Changes v2: - Coding style (indentation) fix Cc: Gleb Natapov <gleb@redhat.com> Cc: Michael S. Tsirkin <mst@redhat.com> Cc: Marcelo Tosatti <mtosatti@redhat.com> Cc: kvm@vger.kernel.org --- target-i386/cpu.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/target-i386/cpu.c b/target-i386/cpu.c index 649cfb2..4e26b11 100644 --- a/target-i386/cpu.c +++ b/target-i386/cpu.c @@ -909,6 +909,8 @@ static void kvm_cpu_fill_host(x86_def_t *x86_cpu_def) /* Other KVM-specific feature fields: */ x86_cpu_def->svm_features = kvm_arch_get_supported_cpuid(s, 0x8000000A, 0, R_EDX); + x86_cpu_def->kvm_features = + kvm_arch_get_supported_cpuid(s, KVM_CPUID_FEATURES, 0, R_EAX); #endif /* CONFIG_KVM */ } -- 1.7.11.7
The -cpu check/enforce warnings are printing incorrect information about the missing flags. There are no feature flags on CPUID leaves 0 and 0x80000000, but there were references to 0 and 0x80000000 in the table at kvm_check_features_against_host(). This changes the model_features_t struct to contain the register number as well, so the error messages print the correct CPUID leaf+register information, instead of wrong CPUID leaf numbers. This also changes the format of the error messages, so they follow the "CPUID.<leaf>.<register>.<name> [bit <offset>]" convention used on Intel documentation. Example output: $ qemu-system-x86_64 -machine pc-1.0,accel=kvm -cpu Opteron_G4,+ia64,enforce warning: host doesn't support requested feature: CPUID.01H:EDX.ia64 [bit 30] warning: host doesn't support requested feature: CPUID.01H:ECX.xsave [bit 26] warning: host doesn't support requested feature: CPUID.01H:ECX.avx [bit 28] warning: host doesn't support requested feature: CPUID.80000001H:ECX.abm [bit 5] warning: host doesn't support requested feature: CPUID.80000001H:ECX.sse4a [bit 6] warning: host doesn't support requested feature: CPUID.80000001H:ECX.misalignsse [bit 7] warning: host doesn't support requested feature: CPUID.80000001H:ECX.3dnowprefetch [bit 8] warning: host doesn't support requested feature: CPUID.80000001H:ECX.xop [bit 11] warning: host doesn't support requested feature: CPUID.80000001H:ECX.fma4 [bit 16] Unable to find x86 CPU definition $ Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> --- Cc: Gleb Natapov <gleb@redhat.com> Cc: Marcelo Tosatti <mtosatti@redhat.com> Cc: kvm@vger.kernel.org --- target-i386/cpu.c | 38 +++++++++++++++++++++++++++++--------- target-i386/cpu.h | 3 +++ 2 files changed, 32 insertions(+), 9 deletions(-) diff --git a/target-i386/cpu.c b/target-i386/cpu.c index 4e26b11..6c43ace 100644 --- a/target-i386/cpu.c +++ b/target-i386/cpu.c @@ -124,6 +124,24 @@ static const char *cpuid_7_0_ebx_feature_name[] = { NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, }; +const char *get_register_name_32(unsigned int reg) +{ + static const char *reg_names[CPU_NB_REGS32] = { + [R_EAX] = "EAX", + [R_ECX] = "ECX", + [R_EDX] = "EDX", + [R_EBX] = "EBX", + [R_ESP] = "ESP", + [R_EBP] = "EBP", + [R_ESI] = "ESI", + [R_EDI] = "EDI", + }; + + if (reg > CPU_NB_REGS32) + return NULL; + return reg_names[reg]; +} + /* collects per-function cpuid data */ typedef struct model_features_t { @@ -132,7 +150,8 @@ typedef struct model_features_t { uint32_t check_feat; const char **flag_names; uint32_t cpuid; - } model_features_t; + int reg; +} model_features_t; int check_cpuid = 0; int enforce_cpuid = 0; @@ -921,10 +940,11 @@ static int unavailable_host_feature(struct model_features_t *f, uint32_t mask) for (i = 0; i < 32; ++i) if (1 << i & mask) { - fprintf(stderr, "warning: host cpuid %04x_%04x lacks requested" - " flag '%s' [0x%08x]\n", - f->cpuid >> 16, f->cpuid & 0xffff, - f->flag_names[i] ? f->flag_names[i] : "[reserved]", mask); + fprintf(stderr, "warning: host doesn't support requested feature: " + "CPUID.%02XH:%s%s%s [bit %d]\n", + f->cpuid, get_register_name_32(f->reg), + f->flag_names[i] ? "." : "", + f->flag_names[i] ? f->flag_names[i] : "", i); break; } return 0; @@ -943,13 +963,13 @@ 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, - ~0, feature_name, 0x00000000}, + ~0, feature_name, 0x00000001, R_EDX}, {&guest_def->ext_features, &host_def.ext_features, - ~CPUID_EXT_HYPERVISOR, ext_feature_name, 0x00000001}, + ~CPUID_EXT_HYPERVISOR, ext_feature_name, 0x00000001, R_ECX}, {&guest_def->ext2_features, &host_def.ext2_features, - ~PPRO_FEATURES, ext2_feature_name, 0x80000000}, + ~PPRO_FEATURES, ext2_feature_name, 0x80000001, R_EDX}, {&guest_def->ext3_features, &host_def.ext3_features, - ~CPUID_EXT3_SVM, ext3_feature_name, 0x80000001}}; + ~CPUID_EXT3_SVM, ext3_feature_name, 0x80000001, R_ECX}}; assert(kvm_enabled()); diff --git a/target-i386/cpu.h b/target-i386/cpu.h index 27c8d0c..ab81a5c 100644 --- a/target-i386/cpu.h +++ b/target-i386/cpu.h @@ -1221,4 +1221,7 @@ void cpu_report_tpr_access(CPUX86State *env, TPRAccess access); void enable_kvm_pv_eoi(void); void disable_kvm_mmu_op(void); +/* Return name of 32-bit register, from a R_* constant */ +const char *get_register_name_32(unsigned int reg); + #endif /* CPU_I386_H */ -- 1.7.11.7
On Fri, Jan 4, 2013 at 3:37 PM, Eduardo Habkost <ehabkost@redhat.com> wrote:
The -cpu check/enforce warnings are printing incorrect information about the missing flags. There are no feature flags on CPUID leaves 0 and 0x80000000, but there were references to 0 and 0x80000000 in the table at kvm_check_features_against_host().
This changes the model_features_t struct to contain the register number as well, so the error messages print the correct CPUID leaf+register information, instead of wrong CPUID leaf numbers.
This also changes the format of the error messages, so they follow the "CPUID.<leaf>.<register>.<name> [bit <offset>]" convention used on Intel documentation. Example output:
$ qemu-system-x86_64 -machine pc-1.0,accel=kvm -cpu Opteron_G4,+ia64,enforce warning: host doesn't support requested feature: CPUID.01H:EDX.ia64 [bit 30] warning: host doesn't support requested feature: CPUID.01H:ECX.xsave [bit 26] warning: host doesn't support requested feature: CPUID.01H:ECX.avx [bit 28] warning: host doesn't support requested feature: CPUID.80000001H:ECX.abm [bit 5] warning: host doesn't support requested feature: CPUID.80000001H:ECX.sse4a [bit 6] warning: host doesn't support requested feature: CPUID.80000001H:ECX.misalignsse [bit 7] warning: host doesn't support requested feature: CPUID.80000001H:ECX.3dnowprefetch [bit 8] warning: host doesn't support requested feature: CPUID.80000001H:ECX.xop [bit 11] warning: host doesn't support requested feature: CPUID.80000001H:ECX.fma4 [bit 16] Unable to find x86 CPU definition $
Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> --- Cc: Gleb Natapov <gleb@redhat.com> Cc: Marcelo Tosatti <mtosatti@redhat.com> Cc: kvm@vger.kernel.org --- target-i386/cpu.c | 38 +++++++++++++++++++++++++++++--------- target-i386/cpu.h | 3 +++ 2 files changed, 32 insertions(+), 9 deletions(-)
diff --git a/target-i386/cpu.c b/target-i386/cpu.c index 4e26b11..6c43ace 100644 --- a/target-i386/cpu.c +++ b/target-i386/cpu.c @@ -124,6 +124,24 @@ static const char *cpuid_7_0_ebx_feature_name[] = { NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, };
+const char *get_register_name_32(unsigned int reg) +{ + static const char *reg_names[CPU_NB_REGS32] = { + [R_EAX] = "EAX", + [R_ECX] = "ECX", + [R_EDX] = "EDX", + [R_EBX] = "EBX", + [R_ESP] = "ESP", + [R_EBP] = "EBP", + [R_ESI] = "ESI", + [R_EDI] = "EDI", + }; + + if (reg > CPU_NB_REGS32)
Missing braces.
+ return NULL; + return reg_names[reg]; +} + /* collects per-function cpuid data */ typedef struct model_features_t { @@ -132,7 +150,8 @@ typedef struct model_features_t { uint32_t check_feat; const char **flag_names; uint32_t cpuid; - } model_features_t; + int reg; +} model_features_t;
int check_cpuid = 0; int enforce_cpuid = 0; @@ -921,10 +940,11 @@ static int unavailable_host_feature(struct model_features_t *f, uint32_t mask)
for (i = 0; i < 32; ++i) if (1 << i & mask) { - fprintf(stderr, "warning: host cpuid %04x_%04x lacks requested" - " flag '%s' [0x%08x]\n", - f->cpuid >> 16, f->cpuid & 0xffff, - f->flag_names[i] ? f->flag_names[i] : "[reserved]", mask); + fprintf(stderr, "warning: host doesn't support requested feature: " + "CPUID.%02XH:%s%s%s [bit %d]\n", + f->cpuid, get_register_name_32(f->reg),
This could attempt to print NULL via %s format, which is not OK with all C libraries. If we trust that the callers always pass valid numbers, the check above could be turned into assert().
+ f->flag_names[i] ? "." : "", + f->flag_names[i] ? f->flag_names[i] : "", i); break; } return 0; @@ -943,13 +963,13 @@ 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, - ~0, feature_name, 0x00000000}, + ~0, feature_name, 0x00000001, R_EDX}, {&guest_def->ext_features, &host_def.ext_features, - ~CPUID_EXT_HYPERVISOR, ext_feature_name, 0x00000001}, + ~CPUID_EXT_HYPERVISOR, ext_feature_name, 0x00000001, R_ECX}, {&guest_def->ext2_features, &host_def.ext2_features, - ~PPRO_FEATURES, ext2_feature_name, 0x80000000}, + ~PPRO_FEATURES, ext2_feature_name, 0x80000001, R_EDX}, {&guest_def->ext3_features, &host_def.ext3_features, - ~CPUID_EXT3_SVM, ext3_feature_name, 0x80000001}}; + ~CPUID_EXT3_SVM, ext3_feature_name, 0x80000001, R_ECX}};
assert(kvm_enabled());
diff --git a/target-i386/cpu.h b/target-i386/cpu.h index 27c8d0c..ab81a5c 100644 --- a/target-i386/cpu.h +++ b/target-i386/cpu.h @@ -1221,4 +1221,7 @@ void cpu_report_tpr_access(CPUX86State *env, TPRAccess access); void enable_kvm_pv_eoi(void); void disable_kvm_mmu_op(void);
+/* Return name of 32-bit register, from a R_* constant */ +const char *get_register_name_32(unsigned int reg); + #endif /* CPU_I386_H */ -- 1.7.11.7
We don't need any hack to ignore CPUID_EXT_HYPERVISOR anymore, because kvm_arch_get_supported_cpuid() now set CPUID_EXT_HYPERVISOR properly. So, this shouldn't introduce any behavior change, but it makes the code simpler. Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> --- My goal is to eliminate the check_feat field completely, as kvm_arch_get_supported_cpuid() should now really return all the bits we can set on all CPUID leaves. --- target-i386/cpu.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/target-i386/cpu.c b/target-i386/cpu.c index 6c43ace..f1a21cf 100644 --- a/target-i386/cpu.c +++ b/target-i386/cpu.c @@ -965,7 +965,7 @@ static int kvm_check_features_against_host(x86_def_t *guest_def) {&guest_def->features, &host_def.features, ~0, feature_name, 0x00000001, R_EDX}, {&guest_def->ext_features, &host_def.ext_features, - ~CPUID_EXT_HYPERVISOR, ext_feature_name, 0x00000001, R_ECX}, + ~0, ext_feature_name, 0x00000001, R_ECX}, {&guest_def->ext2_features, &host_def.ext2_features, ~PPRO_FEATURES, ext2_feature_name, 0x80000001, R_EDX}, {&guest_def->ext3_features, &host_def.ext3_features, -- 1.7.11.7
I have no idea why PPRO_FEATURES was being ignored on the check of the CPUID.80000001H.EDX bits. I believe it was a mistake, and it was supposed to be ~(PPRO_FEATURES & CPUID_EXT2_AMD_ALIASES) or just ~CPUID_EXT2_AMD_ALIASES, because some time ago kvm_cpu_fill_host() used the CPUID instruction directly (instead of kvm_arch_get_supported_cpuid()). But now kvm_cpu_fill_host() use kvm_arch_get_supported_cpuid(), and kvm_arch_get_supported_cpuid() returns all supported bits for CPUID.80000001H.EDX, even the AMD aliases (that are explicitly copied from CPUID.01H.EDX), so we can make the code check/enforce all the CPUID.80000001H.EDX bits. Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> --- target-i386/cpu.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/target-i386/cpu.c b/target-i386/cpu.c index f1a21cf..13075c7 100644 --- a/target-i386/cpu.c +++ b/target-i386/cpu.c @@ -967,7 +967,7 @@ static int kvm_check_features_against_host(x86_def_t *guest_def) {&guest_def->ext_features, &host_def.ext_features, ~0, ext_feature_name, 0x00000001, R_ECX}, {&guest_def->ext2_features, &host_def.ext2_features, - ~PPRO_FEATURES, ext2_feature_name, 0x80000001, R_EDX}, + ~0, ext2_feature_name, 0x80000001, R_EDX}, {&guest_def->ext3_features, &host_def.ext3_features, ~CPUID_EXT3_SVM, ext3_feature_name, 0x80000001, R_ECX}}; -- 1.7.11.7
When nested SVM is supported, the kernel returns the SVM flag on GET_SUPPORTED_CPUID[1], so we can check the SVM flag safely on kvm_check_features_against_host(). I don't know why the original code ignored the SVM flag. Maybe it was because kvm_cpu_fill_host() used the CPUID instruction directly instead of GET_SUPPORTED_CPUID [1] Older kernels (before v2.6.37) returned the SVM flag even if nested SVM was _not_ supported. So the only cases where this patch should change behavior is when SVM is being requested by the user or the CPU model, but not supported by the host. And on these cases we really want QEMU to abort if the "enforce" option is set. Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> --- Cc: Joerg Roedel <joro@8bytes.org> Cc: kvm@vger.kernel.org Cc: libvir-list@redhat.com Cc: Jiri Denemark <jdenemar@redhat.com> I'm CCing libvirt people in case having SVM enabled by default may cause trouble when libvirt starts using the "enforce" flag. I don't know if libvirt expects most of the QEMU CPU models to have nested SVM enabled. --- target-i386/cpu.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/target-i386/cpu.c b/target-i386/cpu.c index 13075c7..21aceed 100644 --- a/target-i386/cpu.c +++ b/target-i386/cpu.c @@ -969,7 +969,7 @@ static int kvm_check_features_against_host(x86_def_t *guest_def) {&guest_def->ext2_features, &host_def.ext2_features, ~0, ext2_feature_name, 0x80000001, R_EDX}, {&guest_def->ext3_features, &host_def.ext3_features, - ~CPUID_EXT3_SVM, ext3_feature_name, 0x80000001, R_ECX}}; + ~0, ext3_feature_name, 0x80000001, R_ECX}}; assert(kvm_enabled()); -- 1.7.11.7
Now that all entries have check_feat=~0 on kvm_check_features_against_host(), we can eliminate check_feat entirely and make the code check all bits. This patch shouldn't introduce any behavior change, as check_feat is set to ~0 on all entries. Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> --- target-i386/cpu.c | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/target-i386/cpu.c b/target-i386/cpu.c index 21aceed..05134a8 100644 --- a/target-i386/cpu.c +++ b/target-i386/cpu.c @@ -147,7 +147,6 @@ const char *get_register_name_32(unsigned int reg) typedef struct model_features_t { uint32_t *guest_feat; uint32_t *host_feat; - uint32_t check_feat; const char **flag_names; uint32_t cpuid; int reg; @@ -951,8 +950,7 @@ static int unavailable_host_feature(struct model_features_t *f, uint32_t mask) } /* 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. + * their way to the guest. * * This function may be called only if KVM is enabled. */ @@ -963,20 +961,20 @@ 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, - ~0, feature_name, 0x00000001, R_EDX}, + feature_name, 0x00000001, R_EDX}, {&guest_def->ext_features, &host_def.ext_features, - ~0, ext_feature_name, 0x00000001, R_ECX}, + ext_feature_name, 0x00000001, R_ECX}, {&guest_def->ext2_features, &host_def.ext2_features, - ~0, ext2_feature_name, 0x80000001, R_EDX}, + ext2_feature_name, 0x80000001, R_EDX}, {&guest_def->ext3_features, &host_def.ext3_features, - ~0, ext3_feature_name, 0x80000001, R_ECX}}; + ext3_feature_name, 0x80000001, R_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) - if (ft[i].check_feat & mask && *ft[i].guest_feat & mask && + if (*ft[i].guest_feat & mask && !(*ft[i].host_feat & mask)) { unavailable_host_feature(&ft[i], mask); rv = 1; -- 1.7.11.7
This will be necessary once kvm_check_features_against_host() starts using KVM-specific definitions (so it won't compile anymore if CONFIG_KVM is not set). Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> --- target-i386/cpu.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/target-i386/cpu.c b/target-i386/cpu.c index 05134a8..02c00bf 100644 --- a/target-i386/cpu.c +++ b/target-i386/cpu.c @@ -933,6 +933,7 @@ static void kvm_cpu_fill_host(x86_def_t *x86_cpu_def) #endif /* CONFIG_KVM */ } +#ifdef CONFIG_KVM static int unavailable_host_feature(struct model_features_t *f, uint32_t mask) { int i; @@ -981,6 +982,7 @@ static int kvm_check_features_against_host(x86_def_t *guest_def) } return rv; } +#endif static void x86_cpuid_version_get_family(Object *obj, Visitor *v, void *opaque, const char *name, Error **errp) @@ -1404,10 +1406,12 @@ static int cpu_x86_parse_featurestr(x86_def_t *x86_cpu_def, char *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; +#ifdef CONFIG_KVM if (check_cpuid && kvm_enabled()) { if (kvm_check_features_against_host(x86_cpu_def) && enforce_cpuid) goto error; } +#endif return 0; error: -- 1.7.11.7
This adds the following feature words to the list of flags to be checked by kvm_check_features_against_host(): - cpuid_7_0_ebx_features - ext4_features - kvm_features - svm_features This will ensure the "enforce" flag works as it should: it won't allow QEMU to be started unless every flag that was requested by the user or defined in the CPU model is supported by the host. This patch may cause existing configurations where "enforce" wasn't preventing QEMU from being started to abort QEMU. But that's exactly the point of this patch: if a flag was not supported by the host and QEMU wasn't aborting, it was a bug in the "enforce" code. Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> --- Cc: Gleb Natapov <gleb@redhat.com> Cc: Marcelo Tosatti <mtosatti@redhat.com> Cc: kvm@vger.kernel.org Cc: libvir-list@redhat.com Cc: Jiri Denemark <jdenemar@redhat.com> CCing libvirt people, as this is directly related to the planned usage of the "enforce" flag by libvirt. The libvirt team probably has a problem in their hands: libvirt should use "enforce" to make sure all requested flags are making their way into the guest (so the resulting CPU is always the same, on any host), but users may have existing working configurations where a flag is not supported by the guest and the user really doesn't care about it. Those configurations will necessarily break when libvirt starts using "enforce". One example where it may cause trouble for common setups: pc-1.3 wants the kvm_pv_eoi flag enabled by default (so "enforce" will make sure it is enabled), but the user may have an existing VM running on a host without pv_eoi support. That setup is unsafe today because live-migration between different host kernel versions may enable/disable pv_eoi silently (that's why we need the "enforce" flag to be used by libvirt), but the user probably would like to be able to live-migrate that VM anyway (and have libvirt to "just do the right thing"). One possible solution to libvirt is to use "enforce" only on newer machine-types, so existing machines with older machine-types will keep the unsafe host-dependent-ABI behavior, but at least would keep live-migration working in case the user is careful. I really don't know what the libvirt team prefers, but that's the situation today. The longer we take to make "enforce" strict as it should and make libvirt finally use it, more users will have VMs with migration-unsafe unpredictable guest ABIs. --- target-i386/cpu.c | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/target-i386/cpu.c b/target-i386/cpu.c index 02c00bf..543ca34 100644 --- a/target-i386/cpu.c +++ b/target-i386/cpu.c @@ -950,8 +950,9 @@ static int unavailable_host_feature(struct model_features_t *f, uint32_t mask) return 0; } -/* best effort attempt to inform user requested cpu flags aren't making - * their way to the guest. +/* Check if all requested cpu flags are making their way to the guest + * + * Returns 0 if all flags are supported by the host, non-zero otherwise. * * This function may be called only if KVM is enabled. */ @@ -968,7 +969,16 @@ static int kvm_check_features_against_host(x86_def_t *guest_def) {&guest_def->ext2_features, &host_def.ext2_features, ext2_feature_name, 0x80000001, R_EDX}, {&guest_def->ext3_features, &host_def.ext3_features, - ext3_feature_name, 0x80000001, R_ECX}}; + ext3_feature_name, 0x80000001, R_ECX}, + {&guest_def->ext4_features, &host_def.ext4_features, + NULL, 0xC0000001, R_EDX}, + {&guest_def->cpuid_7_0_ebx_features, &host_def.cpuid_7_0_ebx_features, + cpuid_7_0_ebx_feature_name, 7, R_EBX}, + {&guest_def->svm_features, &host_def.svm_features, + svm_feature_name, 0x8000000A, R_EDX}, + {&guest_def->kvm_features, &host_def.kvm_features, + kvm_feature_name, KVM_CPUID_FEATURES, R_EAX}, + }; assert(kvm_enabled()); -- 1.7.11.7
Hi, This is an automated message generated from the QEMU Patches. Thank you for submitting this patch. This patch no longer applies to qemu.git. This may have occurred due to: 1) Changes in mainline requiring your patch to be rebased and re-tested. 2) Sending the mail using a tool other than git-send-email. Please use git-send-email to send patches to QEMU. 3) Basing this patch off of a branch that isn't tracking the QEMU master branch. If that was done purposefully, please include the name of the tree in the subject line in the future to prevent this message. For instance: "[PATCH block-next 1/10] qcow3: add fancy new feature" 4) You no longer wish for this patch to be applied to QEMU. No additional action is required on your part. Nacked-by: QEMU Patches <aliguori@us.ibm.com> Below is the output from git-am: Applying: target-i386: kvm: -cpu host: Use GET_SUPPORTED_CPUID for SVM features Applying: target-i386: kvm: Enable all supported KVM features for -cpu host Applying: target-i386: check/enforce: Fix CPUID leaf numbers on error messages fatal: sha1 information is lacking or useless (target-i386/cpu.c). Repository lacks necessary blobs to fall back on 3-way merge. Cannot fall back to three-way merge. Patch failed at 0003 target-i386: check/enforce: Fix CPUID leaf numbers on error messages The copy of the patch that failed is found in: /home/aliguori/.patches/qemu-working/.git/rebase-apply/patch When you have resolved this problem run "git am --resolved". If you would prefer to skip this patch, instead run "git am --skip". To restore the original branch and stop patching run "git am --abort".
Hi, This is an automated message generated from the QEMU Patches. Thank you for submitting this patch. This patch no longer applies to qemu.git. This may have occurred due to: 1) Changes in mainline requiring your patch to be rebased and re-tested. 2) Sending the mail using a tool other than git-send-email. Please use git-send-email to send patches to QEMU. 3) Basing this patch off of a branch that isn't tracking the QEMU master branch. If that was done purposefully, please include the name of the tree in the subject line in the future to prevent this message. For instance: "[PATCH block-next 1/10] qcow3: add fancy new feature" 4) You no longer wish for this patch to be applied to QEMU. No additional action is required on your part. Nacked-by: QEMU Patches <aliguori@us.ibm.com> Below is the output from git-am: Applying: target-i386: kvm: -cpu host: Use GET_SUPPORTED_CPUID for SVM features Applying: target-i386: kvm: Enable all supported KVM features for -cpu host Applying: target-i386: check/enforce: Fix CPUID leaf numbers on error messages fatal: sha1 information is lacking or useless (target-i386/cpu.c). Repository lacks necessary blobs to fall back on 3-way merge. Cannot fall back to three-way merge. Patch failed at 0003 target-i386: check/enforce: Fix CPUID leaf numbers on error messages The copy of the patch that failed is found in: /home/aliguori/.patches/git-working/.git/rebase-apply/patch When you have resolved this problem run "git am --resolved". If you would prefer to skip this patch, instead run "git am --skip". To restore the original branch and stop patching run "git am --abort".
participants (3)
-
Anthony Liguori -
Blue Swirl -
Eduardo Habkost