| Message ID | 1490653177-131484-1-git-send-email-agraf@suse.de |
|---|---|
| State | New |
| Headers | show |
On Tue, Mar 28, 2017 at 12:19:37AM +0200, Alexander Graf wrote: > KVM has a feature bitmap of CPUID bits that it knows works for guests. > QEMU removes bits that are not part of that bitmap automatically on VM > start. > > However, some times we just don't list features in that list because > they don't make sense for normal scenarios, but may be useful in specific, > targeted workloads. > > For that purpose, add a new =force option to all CPUID feature flags in > the CPU property. With that we can override the accel filtering and give > users full control over the CPUID feature bits exposed into guests. > > Signed-off-by: Alexander Graf <agraf@suse.de> > --- > target/i386/cpu.c | 30 ++++++++++++++++++++++++++---- > target/i386/cpu.h | 3 +++ > 2 files changed, 29 insertions(+), 4 deletions(-) > > diff --git a/target/i386/cpu.c b/target/i386/cpu.c > index 7aa7622..5a22f9a 100644 > --- a/target/i386/cpu.c > +++ b/target/i386/cpu.c > @@ -2229,7 +2229,7 @@ void x86_cpu_list(FILE *f, fprintf_function cpu_fprintf) > g_slist_foreach(list, x86_cpu_list_entry, &s); > g_slist_free(list); > > - (*cpu_fprintf)(f, "\nRecognized CPUID flags:\n"); > + (*cpu_fprintf)(f, "\nRecognized CPUID flags (=on|=off|=force):\n"); > for (i = 0; i < ARRAY_SIZE(feature_word_info); i++) { > FeatureWordInfo *fw = &feature_word_info[i]; > > @@ -3460,6 +3460,7 @@ static int x86_cpu_filter_features(X86CPU *cpu) > x86_cpu_get_supported_feature_word(w, false); > uint32_t requested_features = env->features[w]; > env->features[w] &= host_feat; > + env->features[w] |= cpu->forced_features[w]; > cpu->filtered_features[w] = requested_features & ~env->features[w]; > if (cpu->filtered_features[w]) { > rv = 1; > @@ -3693,6 +3694,7 @@ static void x86_cpu_unrealizefn(DeviceState *dev, Error **errp) > > typedef struct BitProperty { > uint32_t *ptr; > + uint32_t *force_ptr; > uint32_t mask; > } BitProperty; Please take a look at: Subject: [PATCH for-2.9 v2 1/2] i386: Replace uint32_t* with FeatureWord on feature getter/setter I plan to include that series in 2.9, and it would make the force_ptr field unnecessary. > > @@ -3701,7 +3703,15 @@ static void x86_cpu_get_bit_prop(Object *obj, Visitor *v, const char *name, > { > BitProperty *fp = opaque; > bool value = (*fp->ptr & fp->mask) == fp->mask; > - visit_type_bool(v, name, &value, errp); > + bool forced = (*fp->force_ptr & fp->mask) == fp->mask; > + char str[] = "force"; > + char *strval = str; > + > + if (!forced) { > + strcpy(str, value ? "on" : "off"); > + } > + > + visit_type_str(v, name, &strval, errp); You can define an enum type in qapi-schema.json, and use visit_type_<YourEnumType>(). You can grep for visit_type_OnOffAuto to find examples. (But I suggest naming the enum something like "X86CPUFeatureSetting" instead of "OnOffForce", because we will probably add other enum values in the future). However: we need to find a way to do this and not break compatibility with "feat=yes|true|no|false", that's supported by StringInputVisitor (which is used by object_property_parse()). Maybe fallback to visit_type_bool() in case visit_type_<YourEnumType>() fails? Except for that, the proposed logic looks good to me. > } > > static void x86_cpu_set_bit_prop(Object *obj, Visitor *v, const char *name, > @@ -3710,6 +3720,7 @@ static void x86_cpu_set_bit_prop(Object *obj, Visitor *v, const char *name, > DeviceState *dev = DEVICE(obj); > BitProperty *fp = opaque; > Error *local_err = NULL; > + char *strval = NULL; > bool value; > > if (dev->realized) { > @@ -3717,7 +3728,15 @@ static void x86_cpu_set_bit_prop(Object *obj, Visitor *v, const char *name, > return; > } > > - visit_type_bool(v, name, &value, &local_err); > + visit_type_str(v, name, &strval, &local_err); > + if (!local_err && !strcmp(strval, "force")) { > + value = true; > + *fp->force_ptr |= fp->mask; > + } else { > + local_err = NULL; > + visit_type_bool(v, name, &value, &local_err); > + } > + > if (local_err) { > error_propagate(errp, local_err); > return; > @@ -3746,6 +3765,7 @@ static void x86_cpu_release_bit_prop(Object *obj, const char *name, > static void x86_cpu_register_bit_prop(X86CPU *cpu, > const char *prop_name, > uint32_t *field, > + uint32_t *force_field, > int bitnr) > { > BitProperty *fp; > @@ -3760,6 +3780,7 @@ static void x86_cpu_register_bit_prop(X86CPU *cpu, > } else { > fp = g_new0(BitProperty, 1); > fp->ptr = field; > + fp->force_ptr = force_field; > fp->mask = mask; > object_property_add(OBJECT(cpu), prop_name, "bool", > x86_cpu_get_bit_prop, > @@ -3787,7 +3808,8 @@ static void x86_cpu_register_feature_bit_props(X86CPU *cpu, > /* aliases don't use "|" delimiters anymore, they are registered > * manually using object_property_add_alias() */ > assert(!strchr(name, '|')); > - x86_cpu_register_bit_prop(cpu, name, &cpu->env.features[w], bitnr); > + x86_cpu_register_bit_prop(cpu, name, &cpu->env.features[w], > + &cpu->forced_features[w], bitnr); > } > > static GuestPanicInformation *x86_cpu_get_crash_info(CPUState *cs) > diff --git a/target/i386/cpu.h b/target/i386/cpu.h > index 07401ad..128530e 100644 > --- a/target/i386/cpu.h > +++ b/target/i386/cpu.h > @@ -1228,6 +1228,9 @@ struct X86CPU { > /* Features that were filtered out because of missing host capabilities */ > uint32_t filtered_features[FEATURE_WORDS]; > > + /* Features that are force enabled despite incompatible accel */ > + uint32_t forced_features[FEATURE_WORDS]; > + > /* Enable PMU CPUID bits. This can't be enabled by default yet because > * it doesn't have ABI stability guarantees, as it passes all PMU CPUID > * bits returned by GET_SUPPORTED_CPUID (that depend on host CPU and kernel > -- > 1.8.5.6 >
On 03/28/2017 02:41 AM, Eduardo Habkost wrote: > On Tue, Mar 28, 2017 at 12:19:37AM +0200, Alexander Graf wrote: >> KVM has a feature bitmap of CPUID bits that it knows works for guests. >> QEMU removes bits that are not part of that bitmap automatically on VM >> start. >> >> However, some times we just don't list features in that list because >> they don't make sense for normal scenarios, but may be useful in specific, >> targeted workloads. >> >> For that purpose, add a new =force option to all CPUID feature flags in >> the CPU property. With that we can override the accel filtering and give >> users full control over the CPUID feature bits exposed into guests. >> >> Signed-off-by: Alexander Graf <agraf@suse.de> >> --- >> target/i386/cpu.c | 30 ++++++++++++++++++++++++++---- >> target/i386/cpu.h | 3 +++ >> 2 files changed, 29 insertions(+), 4 deletions(-) >> >> diff --git a/target/i386/cpu.c b/target/i386/cpu.c >> index 7aa7622..5a22f9a 100644 >> --- a/target/i386/cpu.c >> +++ b/target/i386/cpu.c >> @@ -2229,7 +2229,7 @@ void x86_cpu_list(FILE *f, fprintf_function cpu_fprintf) >> g_slist_foreach(list, x86_cpu_list_entry, &s); >> g_slist_free(list); >> >> - (*cpu_fprintf)(f, "\nRecognized CPUID flags:\n"); >> + (*cpu_fprintf)(f, "\nRecognized CPUID flags (=on|=off|=force):\n"); >> for (i = 0; i < ARRAY_SIZE(feature_word_info); i++) { >> FeatureWordInfo *fw = &feature_word_info[i]; >> >> @@ -3460,6 +3460,7 @@ static int x86_cpu_filter_features(X86CPU *cpu) >> x86_cpu_get_supported_feature_word(w, false); >> uint32_t requested_features = env->features[w]; >> env->features[w] &= host_feat; >> + env->features[w] |= cpu->forced_features[w]; >> cpu->filtered_features[w] = requested_features & ~env->features[w]; >> if (cpu->filtered_features[w]) { >> rv = 1; >> @@ -3693,6 +3694,7 @@ static void x86_cpu_unrealizefn(DeviceState *dev, Error **errp) >> >> typedef struct BitProperty { >> uint32_t *ptr; >> + uint32_t *force_ptr; >> uint32_t mask; >> } BitProperty; > Please take a look at: > Subject: [PATCH for-2.9 v2 1/2] i386: Replace uint32_t* with FeatureWord on feature getter/setter > > I plan to include that series in 2.9, and it would make the > force_ptr field unnecessary. > >> >> @@ -3701,7 +3703,15 @@ static void x86_cpu_get_bit_prop(Object *obj, Visitor *v, const char *name, >> { >> BitProperty *fp = opaque; >> bool value = (*fp->ptr & fp->mask) == fp->mask; >> - visit_type_bool(v, name, &value, errp); >> + bool forced = (*fp->force_ptr & fp->mask) == fp->mask; >> + char str[] = "force"; >> + char *strval = str; >> + >> + if (!forced) { >> + strcpy(str, value ? "on" : "off"); >> + } >> + >> + visit_type_str(v, name, &strval, errp); > You can define an enum type in qapi-schema.json, and use > visit_type_<YourEnumType>(). You can grep for > visit_type_OnOffAuto to find examples. > > (But I suggest naming the enum something like > "X86CPUFeatureSetting" instead of "OnOffForce", because we will > probably add other enum values in the future). > > However: we need to find a way to do this and not break > compatibility with "feat=yes|true|no|false", that's supported by > StringInputVisitor (which is used by object_property_parse()). > Maybe fallback to visit_type_bool() in case > visit_type_<YourEnumType>() fails? Putting it into a special enum sounds much more fragile than the current solution to me. We need to bool fallback either way, so I fail to see any benefit from having the enum. Alex
On Tue, Mar 28, 2017 at 01:26:04PM +0200, Alexander Graf wrote: > On 03/28/2017 02:41 AM, Eduardo Habkost wrote: > > On Tue, Mar 28, 2017 at 12:19:37AM +0200, Alexander Graf wrote: > > > KVM has a feature bitmap of CPUID bits that it knows works for guests. > > > QEMU removes bits that are not part of that bitmap automatically on VM > > > start. > > > > > > However, some times we just don't list features in that list because > > > they don't make sense for normal scenarios, but may be useful in specific, > > > targeted workloads. > > > > > > For that purpose, add a new =force option to all CPUID feature flags in > > > the CPU property. With that we can override the accel filtering and give > > > users full control over the CPUID feature bits exposed into guests. > > > > > > Signed-off-by: Alexander Graf <agraf@suse.de> > > > --- > > > target/i386/cpu.c | 30 ++++++++++++++++++++++++++---- > > > target/i386/cpu.h | 3 +++ > > > 2 files changed, 29 insertions(+), 4 deletions(-) > > > > > > diff --git a/target/i386/cpu.c b/target/i386/cpu.c > > > index 7aa7622..5a22f9a 100644 > > > --- a/target/i386/cpu.c > > > +++ b/target/i386/cpu.c > > > @@ -2229,7 +2229,7 @@ void x86_cpu_list(FILE *f, fprintf_function cpu_fprintf) > > > g_slist_foreach(list, x86_cpu_list_entry, &s); > > > g_slist_free(list); > > > - (*cpu_fprintf)(f, "\nRecognized CPUID flags:\n"); > > > + (*cpu_fprintf)(f, "\nRecognized CPUID flags (=on|=off|=force):\n"); > > > for (i = 0; i < ARRAY_SIZE(feature_word_info); i++) { > > > FeatureWordInfo *fw = &feature_word_info[i]; > > > @@ -3460,6 +3460,7 @@ static int x86_cpu_filter_features(X86CPU *cpu) > > > x86_cpu_get_supported_feature_word(w, false); > > > uint32_t requested_features = env->features[w]; > > > env->features[w] &= host_feat; > > > + env->features[w] |= cpu->forced_features[w]; > > > cpu->filtered_features[w] = requested_features & ~env->features[w]; > > > if (cpu->filtered_features[w]) { > > > rv = 1; > > > @@ -3693,6 +3694,7 @@ static void x86_cpu_unrealizefn(DeviceState *dev, Error **errp) > > > typedef struct BitProperty { > > > uint32_t *ptr; > > > + uint32_t *force_ptr; > > > uint32_t mask; > > > } BitProperty; > > Please take a look at: > > Subject: [PATCH for-2.9 v2 1/2] i386: Replace uint32_t* with FeatureWord on feature getter/setter > > > > I plan to include that series in 2.9, and it would make the > > force_ptr field unnecessary. > > > > > @@ -3701,7 +3703,15 @@ static void x86_cpu_get_bit_prop(Object *obj, Visitor *v, const char *name, > > > { > > > BitProperty *fp = opaque; > > > bool value = (*fp->ptr & fp->mask) == fp->mask; > > > - visit_type_bool(v, name, &value, errp); > > > + bool forced = (*fp->force_ptr & fp->mask) == fp->mask; > > > + char str[] = "force"; > > > + char *strval = str; > > > + > > > + if (!forced) { > > > + strcpy(str, value ? "on" : "off"); > > > + } > > > + > > > + visit_type_str(v, name, &strval, errp); > > You can define an enum type in qapi-schema.json, and use > > visit_type_<YourEnumType>(). You can grep for > > visit_type_OnOffAuto to find examples. > > > > (But I suggest naming the enum something like > > "X86CPUFeatureSetting" instead of "OnOffForce", because we will > > probably add other enum values in the future). > > > > However: we need to find a way to do this and not break > > compatibility with "feat=yes|true|no|false", that's supported by > > StringInputVisitor (which is used by object_property_parse()). > > Maybe fallback to visit_type_bool() in case > > visit_type_<YourEnumType>() fails? > > Putting it into a special enum sounds much more fragile than the current > solution to me. We need to bool fallback either way, so I fail to see any > benefit from having the enum. I don't see why the enum would be more fragile. With the QAPI enum, we: * Have a meaningful value for the QOM property 'type' field, and have some hope to make type information for QOM properties really useful one day; * Have the possible values for the property well-documented in the QAPI schema; * Have the string<->enum translation code automatically generated for us; * Can easily add other values later (I have been planning to support "feat=host" so "-cpu host/max" aren't special cases in the code.
On 03/28/2017 02:31 PM, Eduardo Habkost wrote: > On Tue, Mar 28, 2017 at 01:26:04PM +0200, Alexander Graf wrote: >> On 03/28/2017 02:41 AM, Eduardo Habkost wrote: >>> On Tue, Mar 28, 2017 at 12:19:37AM +0200, Alexander Graf wrote: >>>> KVM has a feature bitmap of CPUID bits that it knows works for guests. >>>> QEMU removes bits that are not part of that bitmap automatically on VM >>>> start. >>>> >>>> However, some times we just don't list features in that list because >>>> they don't make sense for normal scenarios, but may be useful in specific, >>>> targeted workloads. >>>> >>>> For that purpose, add a new =force option to all CPUID feature flags in >>>> the CPU property. With that we can override the accel filtering and give >>>> users full control over the CPUID feature bits exposed into guests. >>>> >>>> Signed-off-by: Alexander Graf <agraf@suse.de> >>>> --- >>>> target/i386/cpu.c | 30 ++++++++++++++++++++++++++---- >>>> target/i386/cpu.h | 3 +++ >>>> 2 files changed, 29 insertions(+), 4 deletions(-) >>>> >>>> diff --git a/target/i386/cpu.c b/target/i386/cpu.c >>>> index 7aa7622..5a22f9a 100644 >>>> --- a/target/i386/cpu.c >>>> +++ b/target/i386/cpu.c >>>> @@ -2229,7 +2229,7 @@ void x86_cpu_list(FILE *f, fprintf_function cpu_fprintf) >>>> g_slist_foreach(list, x86_cpu_list_entry, &s); >>>> g_slist_free(list); >>>> - (*cpu_fprintf)(f, "\nRecognized CPUID flags:\n"); >>>> + (*cpu_fprintf)(f, "\nRecognized CPUID flags (=on|=off|=force):\n"); >>>> for (i = 0; i < ARRAY_SIZE(feature_word_info); i++) { >>>> FeatureWordInfo *fw = &feature_word_info[i]; >>>> @@ -3460,6 +3460,7 @@ static int x86_cpu_filter_features(X86CPU *cpu) >>>> x86_cpu_get_supported_feature_word(w, false); >>>> uint32_t requested_features = env->features[w]; >>>> env->features[w] &= host_feat; >>>> + env->features[w] |= cpu->forced_features[w]; >>>> cpu->filtered_features[w] = requested_features & ~env->features[w]; >>>> if (cpu->filtered_features[w]) { >>>> rv = 1; >>>> @@ -3693,6 +3694,7 @@ static void x86_cpu_unrealizefn(DeviceState *dev, Error **errp) >>>> typedef struct BitProperty { >>>> uint32_t *ptr; >>>> + uint32_t *force_ptr; >>>> uint32_t mask; >>>> } BitProperty; >>> Please take a look at: >>> Subject: [PATCH for-2.9 v2 1/2] i386: Replace uint32_t* with FeatureWord on feature getter/setter >>> >>> I plan to include that series in 2.9, and it would make the >>> force_ptr field unnecessary. >>> >>>> @@ -3701,7 +3703,15 @@ static void x86_cpu_get_bit_prop(Object *obj, Visitor *v, const char *name, >>>> { >>>> BitProperty *fp = opaque; >>>> bool value = (*fp->ptr & fp->mask) == fp->mask; >>>> - visit_type_bool(v, name, &value, errp); >>>> + bool forced = (*fp->force_ptr & fp->mask) == fp->mask; >>>> + char str[] = "force"; >>>> + char *strval = str; >>>> + >>>> + if (!forced) { >>>> + strcpy(str, value ? "on" : "off"); >>>> + } >>>> + >>>> + visit_type_str(v, name, &strval, errp); >>> You can define an enum type in qapi-schema.json, and use >>> visit_type_<YourEnumType>(). You can grep for >>> visit_type_OnOffAuto to find examples. >>> >>> (But I suggest naming the enum something like >>> "X86CPUFeatureSetting" instead of "OnOffForce", because we will >>> probably add other enum values in the future). >>> >>> However: we need to find a way to do this and not break >>> compatibility with "feat=yes|true|no|false", that's supported by >>> StringInputVisitor (which is used by object_property_parse()). >>> Maybe fallback to visit_type_bool() in case >>> visit_type_<YourEnumType>() fails? >> Putting it into a special enum sounds much more fragile than the current >> solution to me. We need to bool fallback either way, so I fail to see any >> benefit from having the enum. > I don't see why the enum would be more fragile. With the QAPI > enum, we: > * Have a meaningful value for the QOM property 'type' field, > and have some hope to make type information for QOM properties > really useful one day; > * Have the possible values for the property well-documented in > the QAPI schema; > * Have the string<->enum translation code automatically generated > for us; > * Can easily add other values later (I have been planning to > support "feat=host" so "-cpu host/max" aren't special cases in > the code. > Ok, can you create the boilerplate for an OnOff enum type for me and I'll plug =force into that? All that visitor stuff scares me :). Alex
On 28/03/2017 13:26, Alexander Graf wrote: >> You can define an enum type in qapi-schema.json, and use >> visit_type_<YourEnumType>(). You can grep for >> visit_type_OnOffAuto to find examples. >> >> (But I suggest naming the enum something like >> "X86CPUFeatureSetting" instead of "OnOffForce", because we will >> probably add other enum values in the future). >> >> However: we need to find a way to do this and not break >> compatibility with "feat=yes|true|no|false", that's supported by >> StringInputVisitor (which is used by object_property_parse()). >> Maybe fallback to visit_type_bool() in case >> visit_type_<YourEnumType>() fails? > > Putting it into a special enum sounds much more fragile than the current > solution to me. We need to bool fallback either way, so I fail to see > any benefit from having the enum. Using an on/off/force enum sounds like the right thing to do. However, I would open code the getters and setters completely (using visit_type_str) instead of using visit_type_FooEnum+visit_type_bool. Then you can easily map yes/true to on and no/false to off. Paolo
On Tue, Mar 28, 2017 at 02:41:55PM +0200, Paolo Bonzini wrote: > > > On 28/03/2017 13:26, Alexander Graf wrote: > >> You can define an enum type in qapi-schema.json, and use > >> visit_type_<YourEnumType>(). You can grep for > >> visit_type_OnOffAuto to find examples. > >> > >> (But I suggest naming the enum something like > >> "X86CPUFeatureSetting" instead of "OnOffForce", because we will > >> probably add other enum values in the future). > >> > >> However: we need to find a way to do this and not break > >> compatibility with "feat=yes|true|no|false", that's supported by > >> StringInputVisitor (which is used by object_property_parse()). > >> Maybe fallback to visit_type_bool() in case > >> visit_type_<YourEnumType>() fails? > > > > Putting it into a special enum sounds much more fragile than the current > > solution to me. We need to bool fallback either way, so I fail to see > > any benefit from having the enum. > > Using an on/off/force enum sounds like the right thing to do. > > However, I would open code the getters and setters completely (using > visit_type_str) instead of using visit_type_FooEnum+visit_type_bool. > Then you can easily map yes/true to on and no/false to off. I am wondering if it isn't simpler to define the enum to be (on, off, force, yes, true, no, false), and document (yes, true, no, false) as deprecated.
On 28/03/2017 18:01, Eduardo Habkost wrote: >> Using an on/off/force enum sounds like the right thing to do. >> >> However, I would open code the getters and setters completely (using >> visit_type_str) instead of using visit_type_FooEnum+visit_type_bool. >> Then you can easily map yes/true to on and no/false to off. > I am wondering if it isn't simpler to define the enum to be > (on, off, force, yes, true, no, false), and document > (yes, true, no, false) as deprecated. This would require checks everywhere (plus I doubt that the deprecation would actually lead to anything). Doing the conversion in the getters and setters provides the right level of abstraction; the question is only whether to open code it or to use multiple calls to visit_type_*. Paolo
On Tue, Mar 28, 2017 at 02:35:57PM +0200, Alexander Graf wrote: > On 03/28/2017 02:31 PM, Eduardo Habkost wrote: > > On Tue, Mar 28, 2017 at 01:26:04PM +0200, Alexander Graf wrote: [...] > > > Putting it into a special enum sounds much more fragile than the current > > > solution to me. We need to bool fallback either way, so I fail to see any > > > benefit from having the enum. > > I don't see why the enum would be more fragile. With the QAPI > > enum, we: > > * Have a meaningful value for the QOM property 'type' field, > > and have some hope to make type information for QOM properties > > really useful one day; > > * Have the possible values for the property well-documented in > > the QAPI schema; > > * Have the string<->enum translation code automatically generated > > for us; > > * Can easily add other values later (I have been planning to > > support "feat=host" so "-cpu host/max" aren't special cases in > > the code. > > > > Ok, can you create the boilerplate for an OnOff enum type for me and I'll > plug =force into that? All that visitor stuff scares me :). I can do it, if you don't mind waiting for a few days. :)
On 03/30/2017 04:22 PM, Eduardo Habkost wrote: > On Tue, Mar 28, 2017 at 02:35:57PM +0200, Alexander Graf wrote: >> On 03/28/2017 02:31 PM, Eduardo Habkost wrote: >>> On Tue, Mar 28, 2017 at 01:26:04PM +0200, Alexander Graf wrote: > [...] >>>> Putting it into a special enum sounds much more fragile than the current >>>> solution to me. We need to bool fallback either way, so I fail to see any >>>> benefit from having the enum. >>> I don't see why the enum would be more fragile. With the QAPI >>> enum, we: >>> * Have a meaningful value for the QOM property 'type' field, >>> and have some hope to make type information for QOM properties >>> really useful one day; >>> * Have the possible values for the property well-documented in >>> the QAPI schema; >>> * Have the string<->enum translation code automatically generated >>> for us; >>> * Can easily add other values later (I have been planning to >>> support "feat=host" so "-cpu host/max" aren't special cases in >>> the code. >>> >> Ok, can you create the boilerplate for an OnOff enum type for me and I'll >> plug =force into that? All that visitor stuff scares me :). > I can do it, if you don't mind waiting for a few days. :) As long as we still manage to hit 2.9 with this I'm all happy :). Alex
On Thu, Mar 30, 2017 at 04:23:34PM +0200, Alexander Graf wrote: > On 03/30/2017 04:22 PM, Eduardo Habkost wrote: > > On Tue, Mar 28, 2017 at 02:35:57PM +0200, Alexander Graf wrote: > > > On 03/28/2017 02:31 PM, Eduardo Habkost wrote: > > > > On Tue, Mar 28, 2017 at 01:26:04PM +0200, Alexander Graf wrote: > > [...] > > > > > Putting it into a special enum sounds much more fragile than the current > > > > > solution to me. We need to bool fallback either way, so I fail to see any > > > > > benefit from having the enum. > > > > I don't see why the enum would be more fragile. With the QAPI > > > > enum, we: > > > > * Have a meaningful value for the QOM property 'type' field, > > > > and have some hope to make type information for QOM properties > > > > really useful one day; > > > > * Have the possible values for the property well-documented in > > > > the QAPI schema; > > > > * Have the string<->enum translation code automatically generated > > > > for us; > > > > * Can easily add other values later (I have been planning to > > > > support "feat=host" so "-cpu host/max" aren't special cases in > > > > the code. > > > > > > > Ok, can you create the boilerplate for an OnOff enum type for me and I'll > > > plug =force into that? All that visitor stuff scares me :). > > I can do it, if you don't mind waiting for a few days. :) > > > As long as we still manage to hit 2.9 with this I'm all happy :). We won't, as this is not a bug fix and we are past hard freeze. :(
On 03/30/2017 04:25 PM, Eduardo Habkost wrote: > On Thu, Mar 30, 2017 at 04:23:34PM +0200, Alexander Graf wrote: >> On 03/30/2017 04:22 PM, Eduardo Habkost wrote: >>> On Tue, Mar 28, 2017 at 02:35:57PM +0200, Alexander Graf wrote: >>>> On 03/28/2017 02:31 PM, Eduardo Habkost wrote: >>>>> On Tue, Mar 28, 2017 at 01:26:04PM +0200, Alexander Graf wrote: >>> [...] >>>>>> Putting it into a special enum sounds much more fragile than the current >>>>>> solution to me. We need to bool fallback either way, so I fail to see any >>>>>> benefit from having the enum. >>>>> I don't see why the enum would be more fragile. With the QAPI >>>>> enum, we: >>>>> * Have a meaningful value for the QOM property 'type' field, >>>>> and have some hope to make type information for QOM properties >>>>> really useful one day; >>>>> * Have the possible values for the property well-documented in >>>>> the QAPI schema; >>>>> * Have the string<->enum translation code automatically generated >>>>> for us; >>>>> * Can easily add other values later (I have been planning to >>>>> support "feat=host" so "-cpu host/max" aren't special cases in >>>>> the code. >>>>> >>>> Ok, can you create the boilerplate for an OnOff enum type for me and I'll >>>> plug =force into that? All that visitor stuff scares me :). >>> I can do it, if you don't mind waiting for a few days. :) >> >> As long as we still manage to hit 2.9 with this I'm all happy :). > We won't, as this is not a bug fix and we are past hard freeze. :( Fair point. Then timing isn't critical either way :) Alex
On Tue, Mar 28, 2017 at 09:31:05AM -0300, Eduardo Habkost wrote: > On Tue, Mar 28, 2017 at 01:26:04PM +0200, Alexander Graf wrote: > > On 03/28/2017 02:41 AM, Eduardo Habkost wrote: > > > On Tue, Mar 28, 2017 at 12:19:37AM +0200, Alexander Graf wrote: > > > > KVM has a feature bitmap of CPUID bits that it knows works for guests. > > > > QEMU removes bits that are not part of that bitmap automatically on VM > > > > start. > > > > > > > > However, some times we just don't list features in that list because > > > > they don't make sense for normal scenarios, but may be useful in specific, > > > > targeted workloads. > > > > > > > > For that purpose, add a new =force option to all CPUID feature flags in > > > > the CPU property. With that we can override the accel filtering and give > > > > users full control over the CPUID feature bits exposed into guests. > > > > > > > > Signed-off-by: Alexander Graf <agraf@suse.de> > > > > --- > > > > target/i386/cpu.c | 30 ++++++++++++++++++++++++++---- > > > > target/i386/cpu.h | 3 +++ > > > > 2 files changed, 29 insertions(+), 4 deletions(-) > > > > > > > > diff --git a/target/i386/cpu.c b/target/i386/cpu.c > > > > index 7aa7622..5a22f9a 100644 > > > > --- a/target/i386/cpu.c > > > > +++ b/target/i386/cpu.c > > > > @@ -2229,7 +2229,7 @@ void x86_cpu_list(FILE *f, fprintf_function cpu_fprintf) > > > > g_slist_foreach(list, x86_cpu_list_entry, &s); > > > > g_slist_free(list); > > > > - (*cpu_fprintf)(f, "\nRecognized CPUID flags:\n"); > > > > + (*cpu_fprintf)(f, "\nRecognized CPUID flags (=on|=off|=force):\n"); > > > > for (i = 0; i < ARRAY_SIZE(feature_word_info); i++) { > > > > FeatureWordInfo *fw = &feature_word_info[i]; > > > > @@ -3460,6 +3460,7 @@ static int x86_cpu_filter_features(X86CPU *cpu) > > > > x86_cpu_get_supported_feature_word(w, false); > > > > uint32_t requested_features = env->features[w]; > > > > env->features[w] &= host_feat; > > > > + env->features[w] |= cpu->forced_features[w]; > > > > cpu->filtered_features[w] = requested_features & ~env->features[w]; > > > > if (cpu->filtered_features[w]) { > > > > rv = 1; > > > > @@ -3693,6 +3694,7 @@ static void x86_cpu_unrealizefn(DeviceState *dev, Error **errp) > > > > typedef struct BitProperty { > > > > uint32_t *ptr; > > > > + uint32_t *force_ptr; > > > > uint32_t mask; > > > > } BitProperty; > > > Please take a look at: > > > Subject: [PATCH for-2.9 v2 1/2] i386: Replace uint32_t* with FeatureWord on feature getter/setter > > > > > > I plan to include that series in 2.9, and it would make the > > > force_ptr field unnecessary. > > > > > > > @@ -3701,7 +3703,15 @@ static void x86_cpu_get_bit_prop(Object *obj, Visitor *v, const char *name, > > > > { > > > > BitProperty *fp = opaque; > > > > bool value = (*fp->ptr & fp->mask) == fp->mask; > > > > - visit_type_bool(v, name, &value, errp); > > > > + bool forced = (*fp->force_ptr & fp->mask) == fp->mask; > > > > + char str[] = "force"; > > > > + char *strval = str; > > > > + > > > > + if (!forced) { > > > > + strcpy(str, value ? "on" : "off"); > > > > + } > > > > + > > > > + visit_type_str(v, name, &strval, errp); > > > You can define an enum type in qapi-schema.json, and use > > > visit_type_<YourEnumType>(). You can grep for > > > visit_type_OnOffAuto to find examples. > > > > > > (But I suggest naming the enum something like > > > "X86CPUFeatureSetting" instead of "OnOffForce", because we will > > > probably add other enum values in the future). > > > > > > However: we need to find a way to do this and not break > > > compatibility with "feat=yes|true|no|false", that's supported by > > > StringInputVisitor (which is used by object_property_parse()). > > > Maybe fallback to visit_type_bool() in case > > > visit_type_<YourEnumType>() fails? > > > > Putting it into a special enum sounds much more fragile than the current > > solution to me. We need to bool fallback either way, so I fail to see any > > benefit from having the enum. > > I don't see why the enum would be more fragile. With the QAPI > enum, we: > * Have a meaningful value for the QOM property 'type' field, > and have some hope to make type information for QOM properties > really useful one day; > * Have the possible values for the property well-documented in > the QAPI schema; > * Have the string<->enum translation code automatically generated > for us; > * Can easily add other values later (I have been planning to > support "feat=host" so "-cpu host/max" aren't special cases in > the code. Well, we have one problem: * Breaking compatibility with management code that expects the feature getter to return boolean values, not enums/strings. This affects users of query-cpu-model-expansion, in particular. We could still make the setter accept an OnOffForce enum, but we would have to keep returning a boolean from the property getter. We could create a set of "force-FEAT=on|off" boolean properties, and make the existing property setter accept "FEAT=force" as input just to make the command-line easier to use. But I'm not sure if making the list of X86CPU properties even larger is worth it. This means Alex' patch might be a reasonable solution, except for the property getter that was changed to return a string. I will make specific comments about the code as reply to the patch itself.
diff --git a/target/i386/cpu.c b/target/i386/cpu.c index 7aa7622..5a22f9a 100644 --- a/target/i386/cpu.c +++ b/target/i386/cpu.c @@ -2229,7 +2229,7 @@ void x86_cpu_list(FILE *f, fprintf_function cpu_fprintf) g_slist_foreach(list, x86_cpu_list_entry, &s); g_slist_free(list); - (*cpu_fprintf)(f, "\nRecognized CPUID flags:\n"); + (*cpu_fprintf)(f, "\nRecognized CPUID flags (=on|=off|=force):\n"); for (i = 0; i < ARRAY_SIZE(feature_word_info); i++) { FeatureWordInfo *fw = &feature_word_info[i]; @@ -3460,6 +3460,7 @@ static int x86_cpu_filter_features(X86CPU *cpu) x86_cpu_get_supported_feature_word(w, false); uint32_t requested_features = env->features[w]; env->features[w] &= host_feat; + env->features[w] |= cpu->forced_features[w]; cpu->filtered_features[w] = requested_features & ~env->features[w]; if (cpu->filtered_features[w]) { rv = 1; @@ -3693,6 +3694,7 @@ static void x86_cpu_unrealizefn(DeviceState *dev, Error **errp) typedef struct BitProperty { uint32_t *ptr; + uint32_t *force_ptr; uint32_t mask; } BitProperty; @@ -3701,7 +3703,15 @@ static void x86_cpu_get_bit_prop(Object *obj, Visitor *v, const char *name, { BitProperty *fp = opaque; bool value = (*fp->ptr & fp->mask) == fp->mask; - visit_type_bool(v, name, &value, errp); + bool forced = (*fp->force_ptr & fp->mask) == fp->mask; + char str[] = "force"; + char *strval = str; + + if (!forced) { + strcpy(str, value ? "on" : "off"); + } + + visit_type_str(v, name, &strval, errp); } static void x86_cpu_set_bit_prop(Object *obj, Visitor *v, const char *name, @@ -3710,6 +3720,7 @@ static void x86_cpu_set_bit_prop(Object *obj, Visitor *v, const char *name, DeviceState *dev = DEVICE(obj); BitProperty *fp = opaque; Error *local_err = NULL; + char *strval = NULL; bool value; if (dev->realized) { @@ -3717,7 +3728,15 @@ static void x86_cpu_set_bit_prop(Object *obj, Visitor *v, const char *name, return; } - visit_type_bool(v, name, &value, &local_err); + visit_type_str(v, name, &strval, &local_err); + if (!local_err && !strcmp(strval, "force")) { + value = true; + *fp->force_ptr |= fp->mask; + } else { + local_err = NULL; + visit_type_bool(v, name, &value, &local_err); + } + if (local_err) { error_propagate(errp, local_err); return; @@ -3746,6 +3765,7 @@ static void x86_cpu_release_bit_prop(Object *obj, const char *name, static void x86_cpu_register_bit_prop(X86CPU *cpu, const char *prop_name, uint32_t *field, + uint32_t *force_field, int bitnr) { BitProperty *fp; @@ -3760,6 +3780,7 @@ static void x86_cpu_register_bit_prop(X86CPU *cpu, } else { fp = g_new0(BitProperty, 1); fp->ptr = field; + fp->force_ptr = force_field; fp->mask = mask; object_property_add(OBJECT(cpu), prop_name, "bool", x86_cpu_get_bit_prop, @@ -3787,7 +3808,8 @@ static void x86_cpu_register_feature_bit_props(X86CPU *cpu, /* aliases don't use "|" delimiters anymore, they are registered * manually using object_property_add_alias() */ assert(!strchr(name, '|')); - x86_cpu_register_bit_prop(cpu, name, &cpu->env.features[w], bitnr); + x86_cpu_register_bit_prop(cpu, name, &cpu->env.features[w], + &cpu->forced_features[w], bitnr); } static GuestPanicInformation *x86_cpu_get_crash_info(CPUState *cs) diff --git a/target/i386/cpu.h b/target/i386/cpu.h index 07401ad..128530e 100644 --- a/target/i386/cpu.h +++ b/target/i386/cpu.h @@ -1228,6 +1228,9 @@ struct X86CPU { /* Features that were filtered out because of missing host capabilities */ uint32_t filtered_features[FEATURE_WORDS]; + /* Features that are force enabled despite incompatible accel */ + uint32_t forced_features[FEATURE_WORDS]; + /* Enable PMU CPUID bits. This can't be enabled by default yet because * it doesn't have ABI stability guarantees, as it passes all PMU CPUID * bits returned by GET_SUPPORTED_CPUID (that depend on host CPU and kernel
KVM has a feature bitmap of CPUID bits that it knows works for guests. QEMU removes bits that are not part of that bitmap automatically on VM start. However, some times we just don't list features in that list because they don't make sense for normal scenarios, but may be useful in specific, targeted workloads. For that purpose, add a new =force option to all CPUID feature flags in the CPU property. With that we can override the accel filtering and give users full control over the CPUID feature bits exposed into guests. Signed-off-by: Alexander Graf <agraf@suse.de> --- target/i386/cpu.c | 30 ++++++++++++++++++++++++++---- target/i386/cpu.h | 3 +++ 2 files changed, 29 insertions(+), 4 deletions(-)