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@redhat.com> --- Cc: kvm@vger.kernel.org Cc: Michael S. Tsirkin <mst@redhat.com> Cc: Gleb Natapov <gleb@redhat.com> Cc: Marcelo Tosatti <mtosatti@redhat.com>

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@redhat.com> --- Cc: kvm@vger.kernel.org Cc: Michael S. Tsirkin <mst@redhat.com> Cc: Gleb Natapov <gleb@redhat.com> Cc: Marcelo Tosatti <mtosatti@redhat.com>

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@redhat.com> --- Cc: kvm@vger.kernel.org Cc: Michael S. Tsirkin <mst@redhat.com> Cc: Gleb Natapov <gleb@redhat.com> Cc: Marcelo Tosatti <mtosatti@redhat.com>

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. -- Gleb.

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@redhat.com> > --- > Cc: kvm@vger.kernel.org > Cc: Michael S. Tsirkin <mst@redhat.com> > Cc: Gleb Natapov <gleb@redhat.com> > Cc: Marcelo Tosatti <mtosatti@redhat.com> > > 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. -- Gleb.

On Sun, Jan 06, 2013 at 03:38:28PM +0200, Gleb Natapov wrote:

On Fri, Jan 04, 2013 at 08:01:03PM -0200, Eduardo Habkost wrote:

...

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.

Actually it was effectively removed Oct 1 2009 by a68a6a7282373. After 3 and a half years of not having it I think we can safely drop it without trying to preserve it in older machine types.

Agreed. Especially considering that the check/enforce code for KVM flags is currently broken. So probably people using pc-1.0, pc-1.1, pc-1.2 are probably _not_ getting the kvm_mmu feature exposed to the guest.

...

Also, keeping it enabled by default would cause unnecessary hassle when libvirt start using the "enforce" option.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> --- Cc: kvm@vger.kernel.org Cc: Michael S. Tsirkin <mst@redhat.com> Cc: Gleb Natapov <gleb@redhat.com> Cc: Marcelo Tosatti <mtosatti@redhat.com> Cc: libvir-list@redhat.com Cc: Jiri Denemark <jdenemar@redhat.com>

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 No need for ifdef here too.

Same case of the previous patch: KVM_FEATURE_MMU_OP is available only if CONFIG_KVM is set. -- Eduardo

On Fri, Jan 04, 2013 at 08:01:04PM -0200, Eduardo Habkost wrote:

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> Reviewed-by: Gleb Natapov <gleb@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 c83a566..c49a97c 100644 --- a/target-i386/cpu.c +++ b/target-i386/cpu.c @@ -908,13 +908,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

-- Gleb.

On Fri, Jan 04, 2013 at 08:01:05PM -0200, Eduardo Habkost wrote:

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> Reviewed-by: Gleb Natapov <gleb@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 c49a97c..e916ae0 100644 --- a/target-i386/cpu.c +++ b/target-i386/cpu.c @@ -911,6 +911,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

-- Gleb.

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@redhat.com> Reviewed-by: Gleb Natapov <gleb@redhat.com> But see the question below.

--- Cc: Gleb Natapov <gleb@redhat.com> Cc: Marcelo Tosatti <mtosatti@redhat.com> Cc: kvm@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 Sun, Jan 06, 2013 at 04:12:54PM +0200, Gleb Natapov wrote:

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@redhat.com> Reviewed-by: Gleb Natapov <gleb@redhat.com> But see the question below.

...

--- Cc: Gleb Natapov <gleb@redhat.com> Cc: Marcelo Tosatti <mtosatti@redhat.com> Cc: kvm@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?

I simply have no idea. I just have a few guesses: - PPRO_FEATURES was supposed to be CPUID_EXT2_AMD_ALIASES, as the user could want to expose vendor=AMD on a non-AMD host; - Maybe CPUID_EXT3_SVM was being excluded because the old code checked the host CPUID directly (instead of using GET_SUPPORTED_CPUID), so the SVM bit in the host CPU wouldn't tell us anything about nested SVM support. They are removed on in separate patches that follow, and then the check_feat field is removed completely.

...

+ };

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.

-- Eduardo

On Fri, Jan 04, 2013 at 08:01:07PM -0200, Eduardo Habkost wrote:

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> Reviewed-by: Gleb Natapov <gleb@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 c3e5db8..42c4c99 100644 --- a/target-i386/cpu.c +++ b/target-i386/cpu.c @@ -970,7 +970,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

-- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html

-- Gleb.

On Fri, Jan 04, 2013 at 08:01:08PM -0200, Eduardo Habkost wrote:

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> Reviewed-by: Gleb Natapov <gleb@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 42c4c99..ce64b98 100644 --- a/target-i386/cpu.c +++ b/target-i386/cpu.c @@ -972,7 +972,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

-- Gleb.

On Fri, Jan 04, 2013 at 08:01:09PM -0200, Eduardo Habkost wrote:

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> Reviewed-by: Gleb Natapov <gleb@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.

Changes v2: - Coding style fix --- 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 ce64b98..a9dd959 100644 --- a/target-i386/cpu.c +++ b/target-i386/cpu.c @@ -974,7 +974,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

-- Gleb.

On Fri, Jan 04, 2013 at 08:01:10PM -0200, Eduardo Habkost wrote:

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> Reviewed-by: Gleb Natapov <gleb@redhat.com>

--- Changes v2: - Coding style fix --- 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 a9dd959..1c3c7e1 100644 --- a/target-i386/cpu.c +++ b/target-i386/cpu.c @@ -148,7 +148,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; @@ -956,8 +955,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. */ @@ -968,13 +966,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, 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()); @@ -982,7 +980,7 @@ static int kvm_check_features_against_host(x86_def_t *guest_def) 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

-- 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@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 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.

return 0;

error: -- 1.7.11.7

-- Gleb.

On Mon, 7 Jan 2013 10:00:09 -0200 Eduardo Habkost <ehabkost@redhat.com> 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@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 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.

-- Eduardo

-- Regards, Igor

On Mon, 7 Jan 2013 15:30:26 +0200 Gleb Natapov <gleb@redhat.com> wrote:

On Mon, Jan 07, 2013 at 02:15:14PM +0100, Igor Mammedov wrote:

...

On Mon, 7 Jan 2013 10:00:09 -0200 Eduardo Habkost <ehabkost@redhat.com> 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@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 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 Why will we be adding more ifdef-s in other places? unavailable_host_feature() is being ifdef-ed above

...

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

That's fine, but you can avoid things like:

if (kvm_enabled() && name && strcmp(name, "host") == 0) { +#ifdef CONFIG_KVM kvm_cpu_fill_host(x86_cpu_def); +#endif

in your patch by providing stub for kvm_cpu_fill_host() for !CONFIG_KVM case. This is common practice really. Avoid ifdefs in the code.

This ifdef could be eliminated later when cpus are converted into sub-classes. Then we would put host subclass close to kvm_cpu_fill_host inside of the same ifdef. that would leave ifdef around kvm_check_features_against_host() in cpu_x86_parse_featurestr().

...

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.

If reader cares about kvm it has to anyway. If he does not, there is friendly kvm_enabled() (which is stub in case of !CONFIG_KVM BTW) to tell him that he does not care. No need additional ifdef there.

both ways would work, but if stubs are preferred style then there is no point arguing.

-- Gleb.

-- Regards, Igor

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@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.

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.

...

+ {&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

-- Gleb.

-- Eduardo

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@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.

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