Re: [Qemu-devel] [PATCH] target-i386: Disable CPUID_EXT_MONITOR when KVM is enabled

[Bandan Das]

Forwarding message by Eduardo. I had misspelled nongnu.org in my first attempt!
The spaces/tab comment by Eduardo has been fixed. 

Eduardo Habkost <address@hidden> writes:
> 
> By default, CPUID_EXT_MONITOR is enabled for some cpu models 
> such as Opteron_G3. Disable it if kvm_enabled() is true since 
> monitor/mwait aren't supported by KVM yet. 
> 
> Signed-off-by: Bandan Das <address@hidden>

Interesting, I haven't noticed that TCG supports CPUID_EXT_MONITOR.

I believe that's yet another reason to make the KVM CPU models separate
classes from the TCG CPU models: because
"-machine ...,accel=kvm -cpu Foo" and "-machine ...,accel=tcg -cpu Foo"
_already_ have different meanings today and result in different CPUs.
Making them classes would just make the fact that they _are_ different
CPU models explicit.


> ---
> There is no user visible side-effect to this behavior, the aim 
> is to clean up the default flags that are not supported (yet).

There is one user-visible effect: "-cpu ...,enforce" will stop failing
because of missing KVM support for CPUID_EXT_MONITOR. But that's exactly
the point: there's no point in having CPU model definitions that would
never work as-is with neither TCG or KVM. This patch is changing the
meaning of (e.g.) "-machine ...,accel=kvm -cpu Opteron_G3" to match what
was already happening in practice.

> 
>  target-i386/cpu.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 1a501d9..c83ba1c 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -1749,6 +1749,7 @@ static void cpu_x86_register(X86CPU *cpu, const char 
> *name, Error **errp)
>  
>      if (kvm_enabled()) {
>          def->features[FEAT_KVM] |= kvm_default_features;
> +     def->features[FEAT_1_ECX] &= ~CPUID_EXT_MONITOR;

You are mixing spaces and tabs, here.

>      }
>      def->features[FEAT_1_ECX] |= CPUID_EXT_HYPERVISOR;
>  
> -- 
> 1.8.1.4
> 

-- 
Eduardo

> By default, CPUID_EXT_MONITOR is enabled for some cpu models 
> such as Opteron_G3. Disable it if kvm_enabled() is true since 
> monitor/mwait aren't supported by KVM yet. 
>
> Signed-off-by: Bandan Das <address@hidden>
> ---
> There is no user visible side-effect to this behavior, the aim 
> is to clean up the default flags that are not supported (yet).
>
>  target-i386/cpu.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 1a501d9..c83ba1c 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -1749,6 +1749,7 @@ static void cpu_x86_register(X86CPU *cpu, const char 
> *name, Error **errp)
>  
>      if (kvm_enabled()) {
>          def->features[FEAT_KVM] |= kvm_default_features;
> +        def->features[FEAT_1_ECX] &= ~CPUID_EXT_MONITOR;
>      }
>      def->features[FEAT_1_ECX] |= CPUID_EXT_HYPERVISOR;