Patchwork [1/2] target-i386: kvm: -cpu host: use GET_SUPPORTED_CPUID for SVM features

login
register
mail settings
Submitter Eduardo Habkost
Date Dec. 28, 2012, 6:37 p.m.
Message ID <1356719854-16401-2-git-send-email-ehabkost@redhat.com>
Download mbox | patch
Permalink /patch/208515/
State New
Headers show

Comments

Eduardo Habkost - Dec. 28, 2012, 6:37 p.m.
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>
---
 target-i386/cpu.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)
Igor Mammedov - Jan. 2, 2013, 2:33 p.m.
On Fri, 28 Dec 2012 16:37:33 -0200
Eduardo Habkost <ehabkost@redhat.com> 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: Igor Mammedov <imammedo@redhat.com>

> ---
>  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 3cd1cee..6e2d32d 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -897,13 +897,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 */
>  }
>
Andreas Färber - Jan. 2, 2013, 2:39 p.m.
Am 28.12.2012 19:37, schrieb Eduardo Habkost:
> 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>
> ---
>  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 3cd1cee..6e2d32d 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -897,13 +897,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);

Is there no #define for this, similar to KVM_CPUID_FEATURES in 2/2?
FWIW indentation looks odd.

Andreas

> +
>  #endif /* CONFIG_KVM */
>  }
>  
>
Eduardo Habkost - Jan. 2, 2013, 3:19 p.m.
On Wed, Jan 02, 2013 at 03:39:03PM +0100, Andreas Färber wrote:
> Am 28.12.2012 19:37, schrieb Eduardo Habkost:
> > 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>
> > ---
> >  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 3cd1cee..6e2d32d 100644
> > --- a/target-i386/cpu.c
> > +++ b/target-i386/cpu.c
> > @@ -897,13 +897,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);
> 
> Is there no #define for this, similar to KVM_CPUID_FEATURES in 2/2?

I believve KVM_CPUID_FEATURES is the exception, all other leaves have
their own numbers hardcoded in the code.

(The way I plan to fix this is to introduce the feature-array and a
CPUID leaf/register table for the feature-array, so kvm_cpu_fill_host(),
filter_features_for_kvm(), kvm_check_features_against_host() & similar
functions would always handle exactly the same set of CPUID leaves, by
simply looking at the table).


> FWIW indentation looks odd.

Oops, I intended to follow the existing style used for ext2_features and
ext3_features and use 8 spaces instead of 12. I will resubmit.

Patch

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 3cd1cee..6e2d32d 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -897,13 +897,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 */
 }