* [Qemu-devel] [PATCH 0/2] Fixes for -cpu host KVM/SVM feature initialization @ 2012-12-28 18:37 Eduardo Habkost 2012-12-28 18:37 ` [Qemu-devel] [PATCH 1/2] target-i386: kvm: -cpu host: use GET_SUPPORTED_CPUID for SVM features Eduardo Habkost ` (2 more replies) 0 siblings, 3 replies; 11+ messages in thread From: Eduardo Habkost @ 2012-12-28 18:37 UTC (permalink / raw) To: qemu-devel Cc: Igor Mammedov, Gleb Natapov, Marcelo Tosatti, Andreas Färber, kvm This series has two very similar fixes for feature initizliation for "-cpu host". This should allow us to make the check/enforce code check for host support of KVM and SVM features, later. Eduardo Habkost (2): target-i386: kvm: -cpu host: use GET_SUPPORTED_CPUID for SVM features target-i386: kvm: enable all supported KVM features for -cpu host target-i386/cpu.c | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) -- 1.7.11.7 ^ permalink raw reply [flat|nested] 11+ messages in thread
* [Qemu-devel] [PATCH 1/2] target-i386: kvm: -cpu host: use GET_SUPPORTED_CPUID for SVM features 2012-12-28 18:37 [Qemu-devel] [PATCH 0/2] Fixes for -cpu host KVM/SVM feature initialization Eduardo Habkost @ 2012-12-28 18:37 ` Eduardo Habkost 2013-01-02 14:33 ` Igor Mammedov 2013-01-02 14:39 ` Andreas Färber 2012-12-28 18:37 ` [Qemu-devel] [PATCH 2/2] target-i386: kvm: enable all supported KVM features for -cpu host Eduardo Habkost 2013-01-02 14:44 ` [Qemu-devel] [PATCH 0/2] Fixes for -cpu host KVM/SVM feature initialization Andreas Färber 2 siblings, 2 replies; 11+ messages in thread From: Eduardo Habkost @ 2012-12-28 18:37 UTC (permalink / raw) To: qemu-devel Cc: kvm, Gleb Natapov, Joerg Roedel, Marcelo Tosatti, Igor Mammedov, Andreas Färber 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); + #endif /* CONFIG_KVM */ } -- 1.7.11.7 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH 1/2] target-i386: kvm: -cpu host: use GET_SUPPORTED_CPUID for SVM features 2012-12-28 18:37 ` [Qemu-devel] [PATCH 1/2] target-i386: kvm: -cpu host: use GET_SUPPORTED_CPUID for SVM features Eduardo Habkost @ 2013-01-02 14:33 ` Igor Mammedov 2013-01-02 14:39 ` Andreas Färber 1 sibling, 0 replies; 11+ messages in thread From: Igor Mammedov @ 2013-01-02 14:33 UTC (permalink / raw) To: Eduardo Habkost Cc: kvm, Gleb Natapov, Joerg Roedel, Marcelo Tosatti, qemu-devel, Andreas Färber 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 */ > } > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH 1/2] target-i386: kvm: -cpu host: use GET_SUPPORTED_CPUID for SVM features 2012-12-28 18:37 ` [Qemu-devel] [PATCH 1/2] target-i386: kvm: -cpu host: use GET_SUPPORTED_CPUID for SVM features Eduardo Habkost 2013-01-02 14:33 ` Igor Mammedov @ 2013-01-02 14:39 ` Andreas Färber 2013-01-02 15:19 ` Eduardo Habkost 1 sibling, 1 reply; 11+ messages in thread From: Andreas Färber @ 2013-01-02 14:39 UTC (permalink / raw) To: Eduardo Habkost Cc: kvm, Gleb Natapov, Joerg Roedel, Marcelo Tosatti, qemu-devel, Igor Mammedov 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 */ > } > > -- SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH 1/2] target-i386: kvm: -cpu host: use GET_SUPPORTED_CPUID for SVM features 2013-01-02 14:39 ` Andreas Färber @ 2013-01-02 15:19 ` Eduardo Habkost 0 siblings, 0 replies; 11+ messages in thread From: Eduardo Habkost @ 2013-01-02 15:19 UTC (permalink / raw) To: Andreas Färber Cc: kvm, Gleb Natapov, Joerg Roedel, Marcelo Tosatti, qemu-devel, Igor Mammedov 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. -- Eduardo ^ permalink raw reply [flat|nested] 11+ messages in thread
* [Qemu-devel] [PATCH 2/2] target-i386: kvm: enable all supported KVM features for -cpu host 2012-12-28 18:37 [Qemu-devel] [PATCH 0/2] Fixes for -cpu host KVM/SVM feature initialization Eduardo Habkost 2012-12-28 18:37 ` [Qemu-devel] [PATCH 1/2] target-i386: kvm: -cpu host: use GET_SUPPORTED_CPUID for SVM features Eduardo Habkost @ 2012-12-28 18:37 ` Eduardo Habkost 2013-01-02 14:52 ` Igor Mammedov 2013-01-02 14:44 ` [Qemu-devel] [PATCH 0/2] Fixes for -cpu host KVM/SVM feature initialization Andreas Färber 2 siblings, 1 reply; 11+ messages in thread From: Eduardo Habkost @ 2012-12-28 18:37 UTC (permalink / raw) To: qemu-devel Cc: Gleb Natapov, kvm, Michael S. Tsirkin, Marcelo Tosatti, Igor Mammedov, Andreas Färber 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> --- target-i386/cpu.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/target-i386/cpu.c b/target-i386/cpu.c index 6e2d32d..76f19f0 100644 --- a/target-i386/cpu.c +++ b/target-i386/cpu.c @@ -900,6 +900,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 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2] target-i386: kvm: enable all supported KVM features for -cpu host 2012-12-28 18:37 ` [Qemu-devel] [PATCH 2/2] target-i386: kvm: enable all supported KVM features for -cpu host Eduardo Habkost @ 2013-01-02 14:52 ` Igor Mammedov 2013-01-02 15:29 ` Eduardo Habkost 0 siblings, 1 reply; 11+ messages in thread From: Igor Mammedov @ 2013-01-02 14:52 UTC (permalink / raw) To: Eduardo Habkost Cc: kvm, Gleb Natapov, Michael S. Tsirkin, Marcelo Tosatti, qemu-devel, Andreas Färber On Fri, 28 Dec 2012 16:37:34 -0200 Eduardo Habkost <ehabkost@redhat.com> 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. 1_2 and 1_3 compat machines diff on pv_eoi flag, with this patch 1_2 might have it set. Is it ok from compat machines pov? > > 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). It would be nice to have kvm_check_features_against_host() patch in this series to verify that this patch and previous patch works as expected. > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> > --- > target-i386/cpu.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/target-i386/cpu.c b/target-i386/cpu.c > index 6e2d32d..76f19f0 100644 > --- a/target-i386/cpu.c > +++ b/target-i386/cpu.c > @@ -900,6 +900,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 */ > } ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2] target-i386: kvm: enable all supported KVM features for -cpu host 2013-01-02 14:52 ` Igor Mammedov @ 2013-01-02 15:29 ` Eduardo Habkost 2013-01-02 20:30 ` Igor Mammedov 0 siblings, 1 reply; 11+ messages in thread From: Eduardo Habkost @ 2013-01-02 15:29 UTC (permalink / raw) To: Igor Mammedov Cc: kvm, Gleb Natapov, Michael S. Tsirkin, Marcelo Tosatti, qemu-devel, Andreas Färber On Wed, Jan 02, 2013 at 03:52:45PM +0100, Igor Mammedov wrote: > On Fri, 28 Dec 2012 16:37:34 -0200 > Eduardo Habkost <ehabkost@redhat.com> 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. > > 1_2 and 1_3 compat machines diff on pv_eoi flag, with this patch 1_2 might > have it set. > Is it ok from compat machines pov? -cpu host is completely dependent on host hardware and kernel version, there are no compatibility expectations. > > > > > 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). > It would be nice to have kvm_check_features_against_host() patch in this > series to verify that this patch and previous patch works as expected. The kvm_check_features_against_host() change would be a user-visible behavior change, and I wanted to keep the changes minimal by now. (the main reason I submitted this earlier is to make it easier to clean up the init code for CPU subclasses) I was planning to introduce those behavior changes only after introducing the feature-word array, so the kvm_check_features_against_host() code can become simpler and easier to review (instead of adding 4 additional items to the messy struct model_features_t array). But if you think we can introduce those changes now, I will be happy to send a series that changes that code as well. > > > > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> > > --- > > target-i386/cpu.c | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/target-i386/cpu.c b/target-i386/cpu.c > > index 6e2d32d..76f19f0 100644 > > --- a/target-i386/cpu.c > > +++ b/target-i386/cpu.c > > @@ -900,6 +900,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 */ > > } > -- Eduardo ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2] target-i386: kvm: enable all supported KVM features for -cpu host 2013-01-02 15:29 ` Eduardo Habkost @ 2013-01-02 20:30 ` Igor Mammedov 2013-01-02 20:52 ` Eduardo Habkost 0 siblings, 1 reply; 11+ messages in thread From: Igor Mammedov @ 2013-01-02 20:30 UTC (permalink / raw) To: Eduardo Habkost Cc: kvm, Gleb Natapov, Michael S. Tsirkin, Marcelo Tosatti, qemu-devel, Andreas Färber On Wed, 2 Jan 2013 13:29:10 -0200 Eduardo Habkost <ehabkost@redhat.com> wrote: > On Wed, Jan 02, 2013 at 03:52:45PM +0100, Igor Mammedov wrote: > > On Fri, 28 Dec 2012 16:37:34 -0200 > > Eduardo Habkost <ehabkost@redhat.com> 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. > > > > 1_2 and 1_3 compat machines diff on pv_eoi flag, with this patch 1_2 might > > have it set. > > Is it ok from compat machines pov? > > -cpu host is completely dependent on host hardware and kernel version, > there are no compatibility expectations. It's still kind of unpleasant surprise if on the same host "qemu-1.3 -cpu host -machine pc-1.2" and "qemu-1.3+ -cpu host -machine pc-1.2" would give different pv_eoi feature default, where pv-eoi should be available after -machine pc-1.2 by default. > > > > > > > > > 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). > > It would be nice to have kvm_check_features_against_host() patch in this > > series to verify that this patch and previous patch works as expected. > > The kvm_check_features_against_host() change would be a user-visible > behavior change, and I wanted to keep the changes minimal by now. (the > main reason I submitted this earlier is to make it easier to clean up > the init code for CPU subclasses) > > I was planning to introduce those behavior changes only after > introducing the feature-word array, so the kvm_check_features_against_host() > code can become simpler and easier to review (instead of adding 4 > additional items to the messy struct model_features_t array). But if you > think we can introduce those changes now, I will be happy to send a > series that changes that code as well. It would be better if it and simplifying kvm_check_features_against_host() were in here together. > > > > > > > > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> > > > --- > > > target-i386/cpu.c | 2 ++ > > > 1 file changed, 2 insertions(+) > > > > > > diff --git a/target-i386/cpu.c b/target-i386/cpu.c > > > index 6e2d32d..76f19f0 100644 > > > --- a/target-i386/cpu.c > > > +++ b/target-i386/cpu.c > > > @@ -900,6 +900,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 */ > > > } > > > > -- > Eduardo > -- Regards, Igor ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2] target-i386: kvm: enable all supported KVM features for -cpu host 2013-01-02 20:30 ` Igor Mammedov @ 2013-01-02 20:52 ` Eduardo Habkost 0 siblings, 0 replies; 11+ messages in thread From: Eduardo Habkost @ 2013-01-02 20:52 UTC (permalink / raw) To: Igor Mammedov Cc: kvm, Gleb Natapov, Michael S. Tsirkin, Marcelo Tosatti, qemu-devel, Andreas Färber On Wed, Jan 02, 2013 at 09:30:20PM +0100, Igor Mammedov wrote: > On Wed, 2 Jan 2013 13:29:10 -0200 > Eduardo Habkost <ehabkost@redhat.com> wrote: > > > On Wed, Jan 02, 2013 at 03:52:45PM +0100, Igor Mammedov wrote: > > > On Fri, 28 Dec 2012 16:37:34 -0200 > > > Eduardo Habkost <ehabkost@redhat.com> 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. > > > > > > 1_2 and 1_3 compat machines diff on pv_eoi flag, with this patch 1_2 might > > > have it set. > > > Is it ok from compat machines pov? > > > > -cpu host is completely dependent on host hardware and kernel version, > > there are no compatibility expectations. > > It's still kind of unpleasant surprise if on the same host > "qemu-1.3 -cpu host -machine pc-1.2" and "qemu-1.3+ -cpu host -machine pc-1.2" > would give different pv_eoi feature default, where pv-eoi should be > available after -machine pc-1.2 by default. Just like you may end up getting new features enabled by -cpu host after upgrading the kernel, you may end up getting new features enabled by -cpu host after upgrading qemu. If you don't like surprises, don't use -cpu host. ;-) I don't think machine-types exist to avoid user surprise, they exist to keep compatibility (which is not expected to happen when using -cpu host). Keeping compatibility is hard enough in the cases where we really need it, I don't think it is worth the extra work and complexity for an use case where compatibility is not expected. > > > > > > > > > > > > > > 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). > > > It would be nice to have kvm_check_features_against_host() patch in this > > > series to verify that this patch and previous patch works as expected. > > > > The kvm_check_features_against_host() change would be a user-visible > > behavior change, and I wanted to keep the changes minimal by now. (the > > main reason I submitted this earlier is to make it easier to clean up > > the init code for CPU subclasses) > > > > I was planning to introduce those behavior changes only after > > introducing the feature-word array, so the kvm_check_features_against_host() > > code can become simpler and easier to review (instead of adding 4 > > additional items to the messy struct model_features_t array). But if you > > think we can introduce those changes now, I will be happy to send a > > series that changes that code as well. > It would be better if it and simplifying kvm_check_features_against_host() > were in here together. The best way I see to simplify kvm_check_features_against_host() requires the feature words array patch, that touches _lots_ of code. I wanted to avoid adding such an intrusive patch as a dependency. > > > > > > > > > > > > > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> > > > > --- > > > > target-i386/cpu.c | 2 ++ > > > > 1 file changed, 2 insertions(+) > > > > > > > > diff --git a/target-i386/cpu.c b/target-i386/cpu.c > > > > index 6e2d32d..76f19f0 100644 > > > > --- a/target-i386/cpu.c > > > > +++ b/target-i386/cpu.c > > > > @@ -900,6 +900,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 */ > > > > } > > > > > > > -- > > Eduardo > > > > > -- > Regards, > Igor -- Eduardo ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH 0/2] Fixes for -cpu host KVM/SVM feature initialization 2012-12-28 18:37 [Qemu-devel] [PATCH 0/2] Fixes for -cpu host KVM/SVM feature initialization Eduardo Habkost 2012-12-28 18:37 ` [Qemu-devel] [PATCH 1/2] target-i386: kvm: -cpu host: use GET_SUPPORTED_CPUID for SVM features Eduardo Habkost 2012-12-28 18:37 ` [Qemu-devel] [PATCH 2/2] target-i386: kvm: enable all supported KVM features for -cpu host Eduardo Habkost @ 2013-01-02 14:44 ` Andreas Färber 2 siblings, 0 replies; 11+ messages in thread From: Andreas Färber @ 2013-01-02 14:44 UTC (permalink / raw) To: Eduardo Habkost, Gleb Natapov, Marcelo Tosatti Cc: Igor Mammedov, qemu-devel, kvm Am 28.12.2012 19:37, schrieb Eduardo Habkost: > This series has two very similar fixes for feature initizliation for "-cpu > host". This should allow us to make the check/enforce code check for host > support of KVM and SVM features, later. I am out of my field here to verify whether this is semantically correct and whether any fallback code may be needed. However, this will conflict with X86CPU subclasses, so I'd be interested in taking this through my qom-cpu queue if there are acks from the KVM folks. Regards, Andreas > Eduardo Habkost (2): > target-i386: kvm: -cpu host: use GET_SUPPORTED_CPUID for SVM features > target-i386: kvm: enable all supported KVM features for -cpu host > > target-i386/cpu.c | 13 ++++++------- > 1 file changed, 6 insertions(+), 7 deletions(-) -- SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2013-01-02 20:50 UTC | newest] Thread overview: 11+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2012-12-28 18:37 [Qemu-devel] [PATCH 0/2] Fixes for -cpu host KVM/SVM feature initialization Eduardo Habkost 2012-12-28 18:37 ` [Qemu-devel] [PATCH 1/2] target-i386: kvm: -cpu host: use GET_SUPPORTED_CPUID for SVM features Eduardo Habkost 2013-01-02 14:33 ` Igor Mammedov 2013-01-02 14:39 ` Andreas Färber 2013-01-02 15:19 ` Eduardo Habkost 2012-12-28 18:37 ` [Qemu-devel] [PATCH 2/2] target-i386: kvm: enable all supported KVM features for -cpu host Eduardo Habkost 2013-01-02 14:52 ` Igor Mammedov 2013-01-02 15:29 ` Eduardo Habkost 2013-01-02 20:30 ` Igor Mammedov 2013-01-02 20:52 ` Eduardo Habkost 2013-01-02 14:44 ` [Qemu-devel] [PATCH 0/2] Fixes for -cpu host KVM/SVM feature initialization Andreas Färber
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).