On Fri, Jan 04, 2013 at 08:01:02PM -0200, Eduardo Habkost wrote: > 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 > CONFIG_KVM and 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 > --- > target-i386/cpu.c | 8 +++++--- > 1 file changed, 5 insertions(+), 3 deletions(-) > > diff --git a/target-i386/cpu.c b/target-i386/cpu.c > index 82685dc..e6435da 100644 > --- a/target-i386/cpu.c > +++ b/target-i386/cpu.c > @@ -145,15 +145,17 @@ 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; > +#ifdef CONFIG_KVM You do not need ifdef here.

> + if (kvm_enabled()) { > + kvm_default_features |= (1UL << KVM_FEATURE_PV_EOI); > + } > +#endif > } > > void host_cpuid(uint32_t function, uint32_t count, > -- > 1.7.11.7 -- Gleb.

On Mon, Jan 07, 2013 at 09:42:36AM -0200, Eduardo Habkost wrote: > On Sun, Jan 06, 2013 at 01:32:34PM +0200, Gleb Natapov wrote: > > On Fri, Jan 04, 2013 at 08:01:02PM -0200, Eduardo Habkost wrote: > > > 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 > > > CONFIG_KVM and 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 > > > --- > > > target-i386/cpu.c | 8 +++++--- > > > 1 file changed, 5 insertions(+), 3 deletions(-) > > > > > > diff --git a/target-i386/cpu.c b/target-i386/cpu.c > > > index 82685dc..e6435da 100644 > > > --- a/target-i386/cpu.c > > > +++ b/target-i386/cpu.c > > > @@ -145,15 +145,17 @@ 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; > > > +#ifdef CONFIG_KVM > > You do not need ifdef here. > > We need it because KVM_FEATURE_PV_EOI is available only if CONFIG_KVM is > set. > > I could also write it as: > > if (kvm_enabled()) { > #ifdef CONFIG_KVM > kvm_default_features |= (1UL << KVM_FEATURE_PV_EOI); > #endif > } > > But I find it less readable. > > Why not define KVM_FEATURE_PV_EOI unconditionally?

> > > > > + if (kvm_enabled()) { > > > + kvm_default_features |= (1UL << KVM_FEATURE_PV_EOI); > > > + } > > > +#endif > > > } > > > > > > void host_cpuid(uint32_t function, uint32_t count, > > > -- > > > 1.7.11.7 > > > > -- > > Gleb. > > -- > Eduardo -- Gleb.

On Mon, Jan 07, 2013 at 10:09:24AM -0200, Eduardo Habkost wrote: > On Mon, Jan 07, 2013 at 01:42:53PM +0200, Gleb Natapov wrote: > > On Mon, Jan 07, 2013 at 09:42:36AM -0200, Eduardo Habkost wrote: > > > On Sun, Jan 06, 2013 at 01:32:34PM +0200, Gleb Natapov wrote: > > > > On Fri, Jan 04, 2013 at 08:01:02PM -0200, Eduardo Habkost wrote: > > > > > 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 > > > > > CONFIG_KVM and 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 > > > > > --- > > > > > target-i386/cpu.c | 8 +++++--- > > > > > 1 file changed, 5 insertions(+), 3 deletions(-) > > > > > > > > > > diff --git a/target-i386/cpu.c b/target-i386/cpu.c > > > > > index 82685dc..e6435da 100644 > > > > > --- a/target-i386/cpu.c > > > > > +++ b/target-i386/cpu.c > > > > > @@ -145,15 +145,17 @@ 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; > > > > > +#ifdef CONFIG_KVM > > > > You do not need ifdef here. > > > > > > We need it because KVM_FEATURE_PV_EOI is available only if CONFIG_KVM is > > > set. > > > > > > I could also write it as: > > > > > > if (kvm_enabled()) { > > > #ifdef CONFIG_KVM > > > kvm_default_features |= (1UL << KVM_FEATURE_PV_EOI); > > > #endif > > > } > > > > > > But I find it less readable. > > > > > > > > Why not define KVM_FEATURE_PV_EOI unconditionally? > > It comes from the KVM kernel headers, that are included only if > CONFIG_KVM is set, and probably won't even compile in non-Linux systems. > > I have a dejavu feeling. I believe we had this exact problem before, > maybe about some other #defines that come from the Linux KVM headers and > won't be available in non-Linux systems. > It is better to hide all KVM related differences somewhere in the headers where no one sees them instead of sprinkle them all over the code. We can put those defines in include/sysemu/kvm.h in !CONFIG_KVM part. Or have one ifdef CONFIG_KVM at the beginning of the file and define enable_kvm_pv_eoi() there and provide empty stub otherwise.

On Mon, Jan 07, 2013 at 10:30:40AM -0200, Eduardo Habkost wrote: > On Mon, Jan 07, 2013 at 02:15:59PM +0200, Gleb Natapov wrote: > > On Mon, Jan 07, 2013 at 10:09:24AM -0200, Eduardo Habkost wrote: > > > On Mon, Jan 07, 2013 at 01:42:53PM +0200, Gleb Natapov wrote: > > > > On Mon, Jan 07, 2013 at 09:42:36AM -0200, Eduardo Habkost wrote: > > > > > On Sun, Jan 06, 2013 at 01:32:34PM +0200, Gleb Natapov wrote: > > > > > > On Fri, Jan 04, 2013 at 08:01:02PM -0200, Eduardo Habkost wrote: > > > > > > > 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 > > > > > > > CONFIG_KVM and 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 > > > > > > > --- > > > > > > > target-i386/cpu.c | 8 +++++--- > > > > > > > 1 file changed, 5 insertions(+), 3 deletions(-) > > > > > > > > > > > > > > diff --git a/target-i386/cpu.c b/target-i386/cpu.c > > > > > > > index 82685dc..e6435da 100644 > > > > > > > --- a/target-i386/cpu.c > > > > > > > +++ b/target-i386/cpu.c > > > > > > > @@ -145,15 +145,17 @@ 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; > > > > > > > +#ifdef CONFIG_KVM > > > > > > You do not need ifdef here. > > > > > > > > > > We need it because KVM_FEATURE_PV_EOI is available only if CONFIG_KVM is > > > > > set. > > > > > > > > > > I could also write it as: > > > > > > > > > > if (kvm_enabled()) { > > > > > #ifdef CONFIG_KVM > > > > > kvm_default_features |= (1UL << KVM_FEATURE_PV_EOI); > > > > > #endif > > > > > } > > > > > > > > > > But I find it less readable. > > > > > > > > > > > > > > Why not define KVM_FEATURE_PV_EOI unconditionally? > > > > > > It comes from the KVM kernel headers, that are included only if > > > CONFIG_KVM is set, and probably won't even compile in non-Linux systems. > > > > > > I have a dejavu feeling. I believe we had this exact problem before, > > > maybe about some other #defines that come from the Linux KVM headers and > > > won't be available in non-Linux systems. > > > > > It is better to hide all KVM related differences somewhere in the > > headers where no one sees them instead of sprinkle them all over the > > code. We can put those defines in include/sysemu/kvm.h in !CONFIG_KVM > > part. Or have one ifdef CONFIG_KVM at the beginning of the file and > > define enable_kvm_pv_eoi() there and provide empty stub otherwise. > > If we had an empty enable_kvm_pv_eoi() stub, we would need an #ifdef > around the real implementation. I mean, I don't think this: > > #ifdef CONFIG_KVM > int enable_kvm_pv_eoi() { > [...] > } > #endif > You already have #ifdef CONFIG_KVM just above enable_kvm_pv_eoi(). Put everything KVM related there instead of adding #ifdef CONFIG_KVM all over the file.

> is any better than this: > > int enable_kvm_pv_eoi() { > #ifdef CONFIG_KVM > [...] > #endif > } > > So this is probably a good reason to duplicate the KVM_FEATURE_* > #defines in the QEMU code, instead? > Not even duplicate, they can be fake just to keep compiler happy.

The kvm_mmu_op feature was removed from the kernel since v3.3 (released in March 2012), it was marked for removal since January 2011 and it's slower than shadow or hardware assisted paging (see kernel commit fb92045843). It doesn't make sense to keep it enabled by default.

Also, keeping it enabled by default would cause unnecessary hassle when libvirt start using the "enforce" option. 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; I was planning to reverse the logic of the compat init functions and make pc_init_pci_1_3() enable kvm_mmu_op and then call pc_init_pci_1_4() instead. But that would require changing pc_init_pci_no_kvmclock() and pc_init_isa() as well. So to keep the changes simple, I am keeping the pattern used when pc_init_pci_1_3() was introduced, making pc_init_pci_1_4() disable kvm_mmu_op and then call pc_init_pci_1_3(). Changes v2: - Coding style fix - Removed redundant comments above machine init functions --- hw/pc_piix.c | 9 ++++++++- target-i386/cpu.c | 9 +++++++++ target-i386/cpu.h | 1 + 3 files changed, 18 insertions(+), 1 deletion(-) diff --git a/hw/pc_piix.c b/hw/pc_piix.c index 99747a7..a32af6a 100644 --- a/hw/pc_piix.c +++ b/hw/pc_piix.c @@ -217,6 +217,7 @@ static void pc_init1(MemoryRegion *system_memory, } } +/* machine init function for pc-0.14 - pc-1.2 */ static void pc_init_pci(QEMUMachineInitArgs *args) { ram_addr_t ram_size = args->ram_size; @@ -238,6 +239,12 @@ static void pc_init_pci_1_3(QEMUMachineInitArgs *args) pc_init_pci(args); } +static void pc_init_pci_1_4(QEMUMachineInitArgs *args) +{ + disable_kvm_mmu_op(); + pc_init_pci_1_3(args); +} + static void pc_init_pci_no_kvmclock(QEMUMachineInitArgs *args) { ram_addr_t ram_size = args->ram_size; @@ -285,7 +292,7 @@ static QEMUMachine pc_machine_v1_4 = { .name = "pc-1.4", .alias = "pc", .desc = "Standard PC", - .init = pc_init_pci_1_3, + .init = pc_init_pci_1_4, .max_cpus = 255, .is_default = 1, }; diff --git a/target-i386/cpu.c b/target-i386/cpu.c index e6435da..c83a566 100644 --- a/target-i386/cpu.c +++ b/target-i386/cpu.c @@ -158,6 +158,15 @@ void enable_kvm_pv_eoi(void) #endif } +void disable_kvm_mmu_op(void) +{ +#ifdef CONFIG_KVM

+ if (kvm_enabled()) { + kvm_default_features &= ~(1UL << KVM_FEATURE_MMU_OP);

+ } +#endif +} + void host_cpuid(uint32_t function, uint32_t count, uint32_t *eax, uint32_t *ebx, uint32_t *ecx, uint32_t *edx) { diff --git a/target-i386/cpu.h b/target-i386/cpu.h index 1283537..27c8d0c 100644 --- a/target-i386/cpu.h +++ b/target-i386/cpu.h @@ -1219,5 +1219,6 @@ void do_smm_enter(CPUX86State *env1); void cpu_report_tpr_access(CPUX86State *env, TPRAccess access); void enable_kvm_pv_eoi(void); +void disable_kvm_mmu_op(void); #endif /* CPU_I386_H */ -- 1.7.11.7

On Fri, Jan 04, 2013 at 08:01:06PM -0200, Eduardo Habkost 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(a)redhat.com&gt; Reviewed-by: Gleb Natapov <gleb(a)redhat.com&gt; But see the question below.

> --- > Cc: Gleb Natapov <gleb(a)redhat.com&gt; > Cc: Marcelo Tosatti <mtosatti(a)redhat.com&gt; > Cc: kvm(a)vger.kernel.org > > Changes v2: > - Coding style fixes > - Add assert() for invalid register numbers on > unavailable_host_feature() > --- > target-i386/cpu.c | 42 +++++++++++++++++++++++++++++++++--------- > target-i386/cpu.h | 3 +++ > 2 files changed, 36 insertions(+), 9 deletions(-) > > diff --git a/target-i386/cpu.c b/target-i386/cpu.c > index e916ae0..c3e5db8 100644 > --- a/target-i386/cpu.c > +++ b/target-i386/cpu.c > @@ -124,6 +124,25 @@ 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 +151,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; > @@ -923,10 +943,13 @@ 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); > + const char *reg = get_register_name_32(f->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); > break; > } > return 0; > @@ -945,13 +968,14 @@ 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} Why do we exclude PPRO_FEATURES/CPUID_EXT3_SVM from been checked? > + }; > > 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 -- Gleb.

On Fri, Jan 04, 2013 at 08:01:11PM -0200, Eduardo Habkost wrote: > 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(a)redhat.com&gt; > --- > target-i386/cpu.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/target-i386/cpu.c b/target-i386/cpu.c > index 1c3c7e1..876b0f6 100644 > --- a/target-i386/cpu.c > +++ b/target-i386/cpu.c > @@ -936,6 +936,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; > @@ -987,6 +988,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) > @@ -1410,10 +1412,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 Provide kvm_check_features_against_host() stub if !CONFIG_KVM and drop ifdef here.

On Mon, 7 Jan 2013 10:00:09 -0200 Eduardo Habkost <ehabkost(a)redhat.com&gt; wrote: > On Sun, Jan 06, 2013 at 04:27:19PM +0200, Gleb Natapov wrote: > > On Fri, Jan 04, 2013 at 08:01:11PM -0200, Eduardo Habkost wrote: > > > 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(a)redhat.com&gt; > > > --- > > > target-i386/cpu.c | 4 ++++ > > > 1 file changed, 4 insertions(+) > > > > > > diff --git a/target-i386/cpu.c b/target-i386/cpu.c > > > index 1c3c7e1..876b0f6 100644 > > > --- a/target-i386/cpu.c > > > +++ b/target-i386/cpu.c > > > @@ -936,6 +936,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; > > > @@ -987,6 +988,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) > > > @@ -1410,10 +1412,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 > > Provide kvm_check_features_against_host() stub if !CONFIG_KVM and drop > > ifdef here. > > I will do. Igor probably will have to change his "target-i386: move > kvm_check_features_against_host() check to realize time" patch to use > the same approach, too. Gleb, Why do stub here? As result we will be adding more ifdef-s just in other places. Currently kvm_cpu_fill_host(), unavailable_host_feature() and

kvm_check_features_against_host() are bundled together in cpu.c so we could instead ifdef whole block. Like here: http://www.mail-archive.com/qemu-devel@nongnu.org/msg146536.html

For me code looks more readable with ifdef here, if we have stub, a reader would have to look at kvm_check_features_against_host() body to see if it does anything.

On Mon, 7 Jan 2013 10:00:09 -0200 Eduardo Habkost <ehabkost(a)redhat.com&gt; wrote: > On Sun, Jan 06, 2013 at 04:27:19PM +0200, Gleb Natapov wrote: > > On Fri, Jan 04, 2013 at 08:01:11PM -0200, Eduardo Habkost wrote: > > > 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(a)redhat.com&gt; > > > --- > > > target-i386/cpu.c | 4 ++++ > > > 1 file changed, 4 insertions(+) > > > > > > diff --git a/target-i386/cpu.c b/target-i386/cpu.c > > > index 1c3c7e1..876b0f6 100644 > > > --- a/target-i386/cpu.c > > > +++ b/target-i386/cpu.c > > > @@ -936,6 +936,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; > > > @@ -987,6 +988,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) > > > @@ -1410,10 +1412,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 > > Provide kvm_check_features_against_host() stub if !CONFIG_KVM and drop > > ifdef here. > > I will do. Igor probably will have to change his "target-i386: move > kvm_check_features_against_host() check to realize time" patch to use > the same approach, too. Gleb, Why do stub here? As result we will be adding more ifdef-s just in other places. Currently kvm_cpu_fill_host(), unavailable_host_feature() and kvm_check_features_against_host() are bundled together in cpu.c so we could instead ifdef whole block. Like here: http://www.mail-archive.com/qemu-devel@nongnu.org/msg146536.html For me code looks more readable with ifdef here, if we have stub, a reader would have to look at kvm_check_features_against_host() body to see if it does anything.

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(a)redhat.com&gt; --- Cc: Gleb Natapov <gleb(a)redhat.com&gt; Cc: Marcelo Tosatti <mtosatti(a)redhat.com&gt; Cc: kvm(a)vger.kernel.org Cc: libvir-list(a)redhat.com Cc: Jiri Denemark <jdenemar(a)redhat.com&gt; 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. Changes v2: - Coding style fix --- target-i386/cpu.c | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/target-i386/cpu.c b/target-i386/cpu.c index 876b0f6..52727ad 100644 --- a/target-i386/cpu.c +++ b/target-i386/cpu.c @@ -955,8 +955,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. */ @@ -973,7 +974,15 @@ 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

On Sun, Jan 06, 2013 at 04:35:51PM +0200, Gleb Natapov wrote: > On Fri, Jan 04, 2013 at 08:01:12PM -0200, Eduardo Habkost wrote: > > 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(a)redhat.com&gt; > > --- > > Cc: Gleb Natapov <gleb(a)redhat.com&gt; > > Cc: Marcelo Tosatti <mtosatti(a)redhat.com&gt; > > Cc: kvm(a)vger.kernel.org > > Cc: libvir-list(a)redhat.com > > Cc: Jiri Denemark <jdenemar(a)redhat.com&gt; > > > > 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. > > > > Changes v2: > > - Coding style fix > > --- > > target-i386/cpu.c | 15 ++++++++++++--- > > 1 file changed, 12 insertions(+), 3 deletions(-) > > > > diff --git a/target-i386/cpu.c b/target-i386/cpu.c > > index 876b0f6..52727ad 100644 > > --- a/target-i386/cpu.c > > +++ b/target-i386/cpu.c > > @@ -955,8 +955,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. > > */ > > @@ -973,7 +974,15 @@ 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}, > Since there is not name array for ext4_features they cannot be added or > removed on the command line hence no need to check them, no? In theory, yes. But it won't hurt to check it, and it will be useful to unify the list of feature words in a single place, so we can be sure the checking/filtering/setting code at kvm_check_features_against_host()/kvm_filter_features_for_host()/kvm_cpu_fill_host(), will all check/filter/set exactly the same feature words.

On Mon, Jan 07, 2013 at 02:06:38PM +0200, Gleb Natapov wrote: > On Mon, Jan 07, 2013 at 10:06:21AM -0200, Eduardo Habkost wrote: > > On Sun, Jan 06, 2013 at 04:35:51PM +0200, Gleb Natapov wrote: > > > On Fri, Jan 04, 2013 at 08:01:12PM -0200, Eduardo Habkost wrote: > > > > 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(a)redhat.com&gt; > > > > --- > > > > Cc: Gleb Natapov <gleb(a)redhat.com&gt; > > > > Cc: Marcelo Tosatti <mtosatti(a)redhat.com&gt; > > > > Cc: kvm(a)vger.kernel.org > > > > Cc: libvir-list(a)redhat.com > > > > Cc: Jiri Denemark <jdenemar(a)redhat.com&gt; > > > > > > > > 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. > > > > > > > > Changes v2: > > > > - Coding style fix > > > > --- > > > > target-i386/cpu.c | 15 ++++++++++++--- > > > > 1 file changed, 12 insertions(+), 3 deletions(-) > > > > > > > > diff --git a/target-i386/cpu.c b/target-i386/cpu.c > > > > index 876b0f6..52727ad 100644 > > > > --- a/target-i386/cpu.c > > > > +++ b/target-i386/cpu.c > > > > @@ -955,8 +955,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. > > > > */ > > > > @@ -973,7 +974,15 @@ 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}, > > > Since there is not name array for ext4_features they cannot be added or > > > removed on the command line hence no need to check them, no? > > > > In theory, yes. But it won't hurt to check it, and it will be useful to > > unify the list of feature words in a single place, so we can be sure the > > checking/filtering/setting code at > > kvm_check_features_against_host()/kvm_filter_features_for_host()/kvm_cpu_fill_host(), > > will all check/filter/set exactly the same feature words. > > > May be add a name array for the leaf? :) If anybody find reliable documentation about the 0xC0000001 CPUID bits, I would happily do it. :-)

While we don't have the docs and feature names, I still believe that having the complete list of feature words in the kvm_check_features_against_host() code will save us trouble later, for the same reason we already filter the 0xC0000001 leaf in filter_features_for_kvm() today. -- Eduardo