[Qemu-devel] [PATCH] target-i386: Enable "check" mode by default

[Qemu-devel] [PATCH] target-i386: Enable "check" mode by default

[Eduardo Habkost]

Current default behavior of QEMU is to silently disable features that
are not supported by the host when a CPU model is requested in the
command-line. This means that in addition to risking breaking guest ABI
by default, we are silent about it.

I would like to enable "enforce" by default, but this can easily break
existing production systems because of the way libvirt makes assumptions
about CPU models today (this will change in the future, once QEMU
provide a proper interface for checking if a CPU model is runnable).

But there's no reason we should be silent about it. So, change
target-i386 to enable "check" mode by default so at least we have some
warning printed to stderr (and hopefully logged somewhere) when QEMU
disables a feature that is not supported by the host system.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 target-i386/cpu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index cfb8aa7..32fee00 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -3121,7 +3121,7 @@ static Property x86_cpu_properties[] = {
     DEFINE_PROP_BOOL("hv-relaxed", X86CPU, hyperv_relaxed_timing, false),
     DEFINE_PROP_BOOL("hv-vapic", X86CPU, hyperv_vapic, false),
     DEFINE_PROP_BOOL("hv-time", X86CPU, hyperv_time, false),
-    DEFINE_PROP_BOOL("check", X86CPU, check_cpuid, false),
+    DEFINE_PROP_BOOL("check", X86CPU, check_cpuid, true),
     DEFINE_PROP_BOOL("enforce", X86CPU, enforce_cpuid, false),
     DEFINE_PROP_BOOL("kvm", X86CPU, expose_kvm, true),
     DEFINE_PROP_UINT32("level", X86CPU, env.cpuid_level, 0),
-- 
2.1.0

Re: [Qemu-devel] [PATCH] target-i386: Enable "check" mode by default

[Igor Mammedov]

On Wed, 26 Aug 2015 13:50:25 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> Current default behavior of QEMU is to silently disable features that
> are not supported by the host when a CPU model is requested in the
> command-line. This means that in addition to risking breaking guest ABI
> by default, we are silent about it.
> 
> I would like to enable "enforce" by default, but this can easily break
> existing production systems because of the way libvirt makes assumptions
> about CPU models today (this will change in the future, once QEMU
> provide a proper interface for checking if a CPU model is runnable).
> 
> But there's no reason we should be silent about it. So, change
> target-i386 to enable "check" mode by default so at least we have some
> warning printed to stderr (and hopefully logged somewhere) when QEMU
> disables a feature that is not supported by the host system.
> 
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
>  target-i386/cpu.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index cfb8aa7..32fee00 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -3121,7 +3121,7 @@ static Property x86_cpu_properties[] = {
>      DEFINE_PROP_BOOL("hv-relaxed", X86CPU, hyperv_relaxed_timing, false),
>      DEFINE_PROP_BOOL("hv-vapic", X86CPU, hyperv_vapic, false),
>      DEFINE_PROP_BOOL("hv-time", X86CPU, hyperv_time, false),
> -    DEFINE_PROP_BOOL("check", X86CPU, check_cpuid, false),
> +    DEFINE_PROP_BOOL("check", X86CPU, check_cpuid, true),
>      DEFINE_PROP_BOOL("enforce", X86CPU, enforce_cpuid, false),
>      DEFINE_PROP_BOOL("kvm", X86CPU, expose_kvm, true),
>      DEFINE_PROP_UINT32("level", X86CPU, env.cpuid_level, 0),

Reviewed-by: Igor Mammedov <imammedo@redhat.com>

Re: [Qemu-devel] [PATCH] target-i386: Enable "check" mode by default

[Eduardo Habkost]

On Mon, Aug 31, 2015 at 02:20:54PM +0200, Igor Mammedov wrote:
> On Wed, 26 Aug 2015 13:50:25 -0300
> Eduardo Habkost <ehabkost@redhat.com> wrote:
> > Current default behavior of QEMU is to silently disable features that
> > are not supported by the host when a CPU model is requested in the
> > command-line. This means that in addition to risking breaking guest ABI
> > by default, we are silent about it.
> > 
> > I would like to enable "enforce" by default, but this can easily break
> > existing production systems because of the way libvirt makes assumptions
> > about CPU models today (this will change in the future, once QEMU
> > provide a proper interface for checking if a CPU model is runnable).
> > 
> > But there's no reason we should be silent about it. So, change
> > target-i386 to enable "check" mode by default so at least we have some
> > warning printed to stderr (and hopefully logged somewhere) when QEMU
> > disables a feature that is not supported by the host system.
> > 
> > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
[...]
> Reviewed-by: Igor Mammedov <imammedo@redhat.com>

Thanks! Applied to x86 tree.

-- 
Eduardo

Re: [Qemu-devel] [PATCH] target-i386: Enable "check" mode by default

[Paolo Bonzini]

On 26/08/2015 18:50, Eduardo Habkost wrote:
> Current default behavior of QEMU is to silently disable features that
> are not supported by the host when a CPU model is requested in the
> command-line. This means that in addition to risking breaking guest ABI
> by default, we are silent about it.
> 
> I would like to enable "enforce" by default, but this can easily break
> existing production systems because of the way libvirt makes assumptions
> about CPU models today (this will change in the future, once QEMU
> provide a proper interface for checking if a CPU model is runnable).
> 
> But there's no reason we should be silent about it. So, change
> target-i386 to enable "check" mode by default so at least we have some
> warning printed to stderr (and hopefully logged somewhere) when QEMU
> disables a feature that is not supported by the host system.
> 
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
>  target-i386/cpu.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

This adds a warning to TCG with the default qemu32/qemu64 CPU models,
due to lack of DE implementation in TCG.  It can be fixed before
release, so this patch is okay.  But please remind me to do it, or
implement it yourself. :)

Paolo

Re: [Qemu-devel] [PATCH] target-i386: Enable "check" mode by default

[Eduardo Habkost]

On Mon, Sep 07, 2015 at 12:37:52PM +0200, Paolo Bonzini wrote:
> 
> 
> On 26/08/2015 18:50, Eduardo Habkost wrote:
> > Current default behavior of QEMU is to silently disable features that
> > are not supported by the host when a CPU model is requested in the
> > command-line. This means that in addition to risking breaking guest ABI
> > by default, we are silent about it.
> > 
> > I would like to enable "enforce" by default, but this can easily break
> > existing production systems because of the way libvirt makes assumptions
> > about CPU models today (this will change in the future, once QEMU
> > provide a proper interface for checking if a CPU model is runnable).
> > 
> > But there's no reason we should be silent about it. So, change
> > target-i386 to enable "check" mode by default so at least we have some
> > warning printed to stderr (and hopefully logged somewhere) when QEMU
> > disables a feature that is not supported by the host system.
> > 
> > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> > ---
> >  target-i386/cpu.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> This adds a warning to TCG with the default qemu32/qemu64 CPU models,
> due to lack of DE implementation in TCG.  It can be fixed before
> release, so this patch is okay.  But please remind me to do it, or
> implement it yourself. :)

I will do it and submit a patch soon. :)

-- 
Eduardo