| Message ID | 1490700426-222115-1-git-send-email-agraf@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Mar 28, 2017 at 01:27:06PM +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> > > --- > > v1 -> v2: > > - Base on "i386: Replace uint32_t* with FeatureWord on feature getter/setter" > --- > target/i386/cpu.c | 25 ++++++++++++++++++++++--- > target/i386/cpu.h | 3 +++ > 2 files changed, 25 insertions(+), 3 deletions(-) > > diff --git a/target/i386/cpu.c b/target/i386/cpu.c > index 13c0985..6105fc5 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]; > > @@ -3464,6 +3464,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; > @@ -3706,8 +3707,17 @@ static void x86_cpu_get_bit_prop(Object *obj, Visitor *v, const char *name, > X86CPU *cpu = X86_CPU(obj); > BitProperty *fp = opaque; > uint32_t f = cpu->env.features[fp->w]; > + uint32_t ff = cpu->forced_features[fp->w]; > bool value = (f & fp->mask) == fp->mask; > - visit_type_bool(v, name, &value, errp); > + bool forced = (ff & fp->mask) == fp->mask; > + char str[] = "force"; > + char *strval = str; > + > + if (forced) { > + visit_type_str(v, name, &strval, errp); > + } else { > + visit_type_bool(v, name, &value, errp); Interesting approach. This means we keep returning a boolean value on the property getter (which is important to not break compatibility), but return a string in case the feature was set to "force". We probably should update the 'type' field of the property, as it won't be a real "bool" property anymore. I won't say I love that solution, but it seems to work. I'm CCing the QAPI maintainers to see what they think about it. (For reference: in the v1 thread I have suggested introducing a new enum type in the QAPI schema, but this would break compatibility with existing management code that expects the property to return a boolean value [like the users of query-cpu-model-expansion]). > + } > } > > static void x86_cpu_set_bit_prop(Object *obj, Visitor *v, const char *name, > @@ -3717,6 +3727,7 @@ static void x86_cpu_set_bit_prop(Object *obj, Visitor *v, const char *name, > X86CPU *cpu = X86_CPU(obj); > BitProperty *fp = opaque; > Error *local_err = NULL; > + char *strval = NULL; > bool value; > > if (dev->realized) { > @@ -3724,7 +3735,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; > + cpu->forced_features[fp->w] |= fp->mask; > + } else { > + local_err = NULL; > + visit_type_bool(v, name, &value, &local_err); > + } I'm not sure this is valid usage of the visitor API. If the visit_type_str() call succeeds, isn't the visitor allowed to consume the original value, and return something completely different when visit_type_bool() is called? > + > if (local_err) { > error_propagate(errp, local_err); > return; > diff --git a/target/i386/cpu.h b/target/i386/cpu.h > index c4602ca..69efe6c 100644 > --- a/target/i386/cpu.h > +++ b/target/i386/cpu.h > @@ -1230,6 +1230,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 > >
Quoting Eduardo Habkost (2017-04-13 12:36:25) > On Tue, Mar 28, 2017 at 01:27:06PM +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> > > > > --- > > > > v1 -> v2: > > > > - Base on "i386: Replace uint32_t* with FeatureWord on feature getter/setter" > > --- > > target/i386/cpu.c | 25 ++++++++++++++++++++++--- > > target/i386/cpu.h | 3 +++ > > 2 files changed, 25 insertions(+), 3 deletions(-) > > > > diff --git a/target/i386/cpu.c b/target/i386/cpu.c > > index 13c0985..6105fc5 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]; > > > > @@ -3464,6 +3464,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; > > @@ -3706,8 +3707,17 @@ static void x86_cpu_get_bit_prop(Object *obj, Visitor *v, const char *name, > > X86CPU *cpu = X86_CPU(obj); > > BitProperty *fp = opaque; > > uint32_t f = cpu->env.features[fp->w]; > > + uint32_t ff = cpu->forced_features[fp->w]; > > bool value = (f & fp->mask) == fp->mask; > > - visit_type_bool(v, name, &value, errp); > > + bool forced = (ff & fp->mask) == fp->mask; > > + char str[] = "force"; > > + char *strval = str; > > + > > + if (forced) { > > + visit_type_str(v, name, &strval, errp); > > + } else { > > + visit_type_bool(v, name, &value, errp); > > Interesting approach. This means we keep returning a boolean > value on the property getter (which is important to not break > compatibility), but return a string in case the feature was set > to "force". > > We probably should update the 'type' field of the property, as it > won't be a real "bool" property anymore. > > I won't say I love that solution, but it seems to work. I'm CCing > the QAPI maintainers to see what they think about it. > > (For reference: in the v1 thread I have suggested introducing a > new enum type in the QAPI schema, but this would break > compatibility with existing management code that expects the > property to return a boolean value [like the users of > query-cpu-model-expansion]). > > > + } > > } > > > > static void x86_cpu_set_bit_prop(Object *obj, Visitor *v, const char *name, > > @@ -3717,6 +3727,7 @@ static void x86_cpu_set_bit_prop(Object *obj, Visitor *v, const char *name, > > X86CPU *cpu = X86_CPU(obj); > > BitProperty *fp = opaque; > > Error *local_err = NULL; > > + char *strval = NULL; > > bool value; > > > > if (dev->realized) { > > @@ -3724,7 +3735,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; > > + cpu->forced_features[fp->w] |= fp->mask; > > + } else { > > + local_err = NULL; > > + visit_type_bool(v, name, &value, &local_err); > > + } > > I'm not sure this is valid usage of the visitor API. If the > visit_type_str() call succeeds, isn't the visitor allowed to > consume the original value, and return something completely > different when visit_type_bool() is called? That was indeed one of the design goals, to the extent that you could even take a stream of data types and actually build up a dictionary based on the ordering visited (e.g. the once-proposed BER visitor for migration and why QAPI uses OrderedDict when generating visitors). I'm not sure how important that is anymore, but even setting that aside that we still have the issue of not handling the data type as declared, and relying on error-handling to probe it, which doesn't necessarily leave the visitor in a recoverable state where we can continue visiting. I think it might be possible to rely on the "alternate" QAPI type to handle this case. Maybe something like this? GenericAlternate *alt = NULL; visit_start_alternate(v, name, &alt, sizeof(*alt), false, &local_err); if (local_err) { goto out; } if (!alt) { goto out_obj; } switch (alt->type) { case QTYPE_QBOOL: visit_type_bool(v, name, &value, &local_err); break; case QTYPE_QSTRING: visit_type_str(v, name, &strval, &local_err); if (!local_err && !strcmp(strval, "force")) { value = true; cpu->forced_features[fp->w] |= fp->mask; } break; default: error_setg(&local_err, "invalid type"); } out_obj: visit_end_alternate(v, (void **)obj); out: ... This would only work for setters/qobject-input-visitor ATM though since qobject-output-visitor doesn't support the visit_start_alternate() interface. I'm not sure why it wasn't enabled on the output side, maybe Eric knows if doing so would be feasible for this situation? > > > + > > if (local_err) { > > error_propagate(errp, local_err); > > return; > > diff --git a/target/i386/cpu.h b/target/i386/cpu.h > > index c4602ca..69efe6c 100644 > > --- a/target/i386/cpu.h > > +++ b/target/i386/cpu.h > > @@ -1230,6 +1230,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 > > > > > > -- > Eduardo >
Quoting Michael Roth (2017-04-18 16:01:20) > Quoting Eduardo Habkost (2017-04-13 12:36:25) > > On Tue, Mar 28, 2017 at 01:27:06PM +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> > > > > > > --- > > > > > > v1 -> v2: > > > > > > - Base on "i386: Replace uint32_t* with FeatureWord on feature getter/setter" > > > --- > > > target/i386/cpu.c | 25 ++++++++++++++++++++++--- > > > target/i386/cpu.h | 3 +++ > > > 2 files changed, 25 insertions(+), 3 deletions(-) > > > > > > diff --git a/target/i386/cpu.c b/target/i386/cpu.c > > > index 13c0985..6105fc5 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]; > > > > > > @@ -3464,6 +3464,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; > > > @@ -3706,8 +3707,17 @@ static void x86_cpu_get_bit_prop(Object *obj, Visitor *v, const char *name, > > > X86CPU *cpu = X86_CPU(obj); > > > BitProperty *fp = opaque; > > > uint32_t f = cpu->env.features[fp->w]; > > > + uint32_t ff = cpu->forced_features[fp->w]; > > > bool value = (f & fp->mask) == fp->mask; > > > - visit_type_bool(v, name, &value, errp); > > > + bool forced = (ff & fp->mask) == fp->mask; > > > + char str[] = "force"; > > > + char *strval = str; > > > + > > > + if (forced) { > > > + visit_type_str(v, name, &strval, errp); > > > + } else { > > > + visit_type_bool(v, name, &value, errp); > > > > Interesting approach. This means we keep returning a boolean > > value on the property getter (which is important to not break > > compatibility), but return a string in case the feature was set > > to "force". > > > > We probably should update the 'type' field of the property, as it > > won't be a real "bool" property anymore. > > > > I won't say I love that solution, but it seems to work. I'm CCing > > the QAPI maintainers to see what they think about it. > > > > (For reference: in the v1 thread I have suggested introducing a > > new enum type in the QAPI schema, but this would break > > compatibility with existing management code that expects the > > property to return a boolean value [like the users of > > query-cpu-model-expansion]). > > > > > + } > > > } > > > > > > static void x86_cpu_set_bit_prop(Object *obj, Visitor *v, const char *name, > > > @@ -3717,6 +3727,7 @@ static void x86_cpu_set_bit_prop(Object *obj, Visitor *v, const char *name, > > > X86CPU *cpu = X86_CPU(obj); > > > BitProperty *fp = opaque; > > > Error *local_err = NULL; > > > + char *strval = NULL; > > > bool value; > > > > > > if (dev->realized) { > > > @@ -3724,7 +3735,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; > > > + cpu->forced_features[fp->w] |= fp->mask; > > > + } else { > > > + local_err = NULL; > > > + visit_type_bool(v, name, &value, &local_err); > > > + } > > > > I'm not sure this is valid usage of the visitor API. If the > > visit_type_str() call succeeds, isn't the visitor allowed to > > consume the original value, and return something completely > > different when visit_type_bool() is called? > > That was indeed one of the design goals, to the extent that > you could even take a stream of data types and actually build > up a dictionary based on the ordering visited (e.g. the > once-proposed BER visitor for migration and why QAPI uses > OrderedDict when generating visitors). > > I'm not sure how important that is anymore, but even setting > that aside that we still have the issue of not handling the > data type as declared, and relying on error-handling to probe > it, which doesn't necessarily leave the visitor in a recoverable > state where we can continue visiting. I think it might be possible > to rely on the "alternate" QAPI type to handle this case. Maybe > something like this? > > GenericAlternate *alt = NULL; > visit_start_alternate(v, name, &alt, sizeof(*alt), false, &local_err); > if (local_err) { > goto out; > } > if (!alt) { > goto out_obj; > } > switch (alt->type) { > case QTYPE_QBOOL: > visit_type_bool(v, name, &value, &local_err); > break; > case QTYPE_QSTRING: > visit_type_str(v, name, &strval, &local_err); > if (!local_err && !strcmp(strval, "force")) { > value = true; > cpu->forced_features[fp->w] |= fp->mask; > } > break; > default: > error_setg(&local_err, "invalid type"); > } > > out_obj: > visit_end_alternate(v, (void **)obj); > out: > ... > > This would only work for setters/qobject-input-visitor ATM though since > qobject-output-visitor doesn't support the visit_start_alternate() > interface. I'm not sure why it wasn't enabled on the output side, maybe > Eric knows if doing so would be feasible for this situation? Forgot to Cc: Eric > > > > > > + > > > if (local_err) { > > > error_propagate(errp, local_err); > > > return; > > > diff --git a/target/i386/cpu.h b/target/i386/cpu.h > > > index c4602ca..69efe6c 100644 > > > --- a/target/i386/cpu.h > > > +++ b/target/i386/cpu.h > > > @@ -1230,6 +1230,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 > > > > > > > > > > -- > > Eduardo > >
On 04/18/2017 04:01 PM, Michael Roth wrote: >>> @@ -3706,8 +3707,17 @@ static void x86_cpu_get_bit_prop(Object *obj, Visitor *v, const char *name, >>> X86CPU *cpu = X86_CPU(obj); >>> BitProperty *fp = opaque; >>> uint32_t f = cpu->env.features[fp->w]; >>> + uint32_t ff = cpu->forced_features[fp->w]; >>> bool value = (f & fp->mask) == fp->mask; >>> - visit_type_bool(v, name, &value, errp); >>> + bool forced = (ff & fp->mask) == fp->mask; >>> + char str[] = "force"; >>> + char *strval = str; >>> + >>> + if (forced) { >>> + visit_type_str(v, name, &strval, errp); >>> + } else { >>> + visit_type_bool(v, name, &value, errp); >> >> Interesting approach. This means we keep returning a boolean >> value on the property getter (which is important to not break >> compatibility), but return a string in case the feature was set >> to "force". >> >> We probably should update the 'type' field of the property, as it >> won't be a real "bool" property anymore. >> >> I won't say I love that solution, but it seems to work. I'm CCing >> the QAPI maintainers to see what they think about it. Use of a formal QAPI alternate type seems reasonable here. >> >> (For reference: in the v1 thread I have suggested introducing a >> new enum type in the QAPI schema, but this would break >> compatibility with existing management code that expects the >> property to return a boolean value [like the users of >> query-cpu-model-expansion]). >>> @@ -3724,7 +3735,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; >>> + cpu->forced_features[fp->w] |= fp->mask; >>> + } else { >>> + local_err = NULL; >>> + visit_type_bool(v, name, &value, &local_err); >>> + } >> >> I'm not sure this is valid usage of the visitor API. If the >> visit_type_str() call succeeds, isn't the visitor allowed to >> consume the original value, and return something completely >> different when visit_type_bool() is called? > Better would be defining the QAPI alternate type: { 'enum': 'Force', 'data': ['force']} { 'alternate': 'BoolOrForce', 'data': { 'b': 'bool', 's': 'Force' } } then given a BoolOrForce *state; declaration, you do either: state->type = QTYPE_QBOOL; state->u.b = true; or state->type = QTYPE_QSTRING; state->u.s = FORCE_FORCE; and then call visit_type_BoolOrForce(..., &state ...) Qcow2OverlapChecks is an example of an alternate in use, if that helps. > That was indeed one of the design goals, to the extent that > you could even take a stream of data types and actually build > up a dictionary based on the ordering visited (e.g. the > once-proposed BER visitor for migration and why QAPI uses > OrderedDict when generating visitors). We document that QMP is order-insensitive, but you are right that QAPI-generated code still respects ordering. Some of our testsuite at least relies on the stability you get by always visiting things in declaration order, even though other dictionary orders should still be valid. > > I'm not sure how important that is anymore, but even setting > that aside that we still have the issue of not handling the > data type as declared, and relying on error-handling to probe > it, which doesn't necessarily leave the visitor in a recoverable > state where we can continue visiting. I think it might be possible > to rely on the "alternate" QAPI type to handle this case. Maybe > something like this? Yes, that was the proposal I outlined above. Note that your rough-idea code: > > GenericAlternate *alt = NULL; > visit_start_alternate(v, name, &alt, sizeof(*alt), false, &local_err); > if (local_err) { > goto out; > } > if (!alt) { > goto out_obj; > } > switch (alt->type) { > case QTYPE_QBOOL: > visit_type_bool(v, name, &value, &local_err); > break; > case QTYPE_QSTRING: > visit_type_str(v, name, &strval, &local_err); ... is a duplicate of what is already generated for visit_type_BoolOrForce(), so you don't need to open-code it. > > This would only work for setters/qobject-input-visitor ATM though since > qobject-output-visitor doesn't support the visit_start_alternate() > interface. I'm not sure why it wasn't enabled on the output side, maybe > Eric knows if doing so would be feasible for this situation? qobject-output-visitor DOES support alternates. It doesn't need a .start_alternate callback, because the generic visitor-core.c code handles things just fine for output visitors (only input visitors _have_ to have a callback). The comments in visitor-impl.h confirm that.
Quoting Eric Blake (2017-04-18 16:19:10) > On 04/18/2017 04:01 PM, Michael Roth wrote: > > >>> @@ -3706,8 +3707,17 @@ static void x86_cpu_get_bit_prop(Object *obj, Visitor *v, const char *name, > >>> X86CPU *cpu = X86_CPU(obj); > >>> BitProperty *fp = opaque; > >>> uint32_t f = cpu->env.features[fp->w]; > >>> + uint32_t ff = cpu->forced_features[fp->w]; > >>> bool value = (f & fp->mask) == fp->mask; > >>> - visit_type_bool(v, name, &value, errp); > >>> + bool forced = (ff & fp->mask) == fp->mask; > >>> + char str[] = "force"; > >>> + char *strval = str; > >>> + > >>> + if (forced) { > >>> + visit_type_str(v, name, &strval, errp); > >>> + } else { > >>> + visit_type_bool(v, name, &value, errp); > >> > >> Interesting approach. This means we keep returning a boolean > >> value on the property getter (which is important to not break > >> compatibility), but return a string in case the feature was set > >> to "force". > >> > >> We probably should update the 'type' field of the property, as it > >> won't be a real "bool" property anymore. > >> > >> I won't say I love that solution, but it seems to work. I'm CCing > >> the QAPI maintainers to see what they think about it. > > Use of a formal QAPI alternate type seems reasonable here. > > >> > >> (For reference: in the v1 thread I have suggested introducing a > >> new enum type in the QAPI schema, but this would break > >> compatibility with existing management code that expects the > >> property to return a boolean value [like the users of > >> query-cpu-model-expansion]). > > >>> @@ -3724,7 +3735,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; > >>> + cpu->forced_features[fp->w] |= fp->mask; > >>> + } else { > >>> + local_err = NULL; > >>> + visit_type_bool(v, name, &value, &local_err); > >>> + } > >> > >> I'm not sure this is valid usage of the visitor API. If the > >> visit_type_str() call succeeds, isn't the visitor allowed to > >> consume the original value, and return something completely > >> different when visit_type_bool() is called? > > > > Better would be defining the QAPI alternate type: > > { 'enum': 'Force', 'data': ['force']} > { 'alternate': 'BoolOrForce', > 'data': { 'b': 'bool', 's': 'Force' } } > > then given a BoolOrForce *state; declaration, you do either: > > state->type = QTYPE_QBOOL; > state->u.b = true; > > or > > state->type = QTYPE_QSTRING; > state->u.s = FORCE_FORCE; > > and then call visit_type_BoolOrForce(..., &state ...) > > Qcow2OverlapChecks is an example of an alternate in use, if that helps. > > > That was indeed one of the design goals, to the extent that > > you could even take a stream of data types and actually build > > up a dictionary based on the ordering visited (e.g. the > > once-proposed BER visitor for migration and why QAPI uses > > OrderedDict when generating visitors). > > We document that QMP is order-insensitive, but you are right that > QAPI-generated code still respects ordering. Some of our testsuite at > least relies on the stability you get by always visiting things in > declaration order, even though other dictionary orders should still be > valid. > > > > > I'm not sure how important that is anymore, but even setting > > that aside that we still have the issue of not handling the > > data type as declared, and relying on error-handling to probe > > it, which doesn't necessarily leave the visitor in a recoverable > > state where we can continue visiting. I think it might be possible > > to rely on the "alternate" QAPI type to handle this case. Maybe > > something like this? > > Yes, that was the proposal I outlined above. Note that your rough-idea > code: > > > > > GenericAlternate *alt = NULL; > > visit_start_alternate(v, name, &alt, sizeof(*alt), false, &local_err); > > if (local_err) { > > goto out; > > } > > if (!alt) { > > goto out_obj; > > } > > switch (alt->type) { > > case QTYPE_QBOOL: > > visit_type_bool(v, name, &value, &local_err); > > break; > > case QTYPE_QSTRING: > > visit_type_str(v, name, &strval, &local_err); > > ... > is a duplicate of what is already generated for > visit_type_BoolOrForce(), so you don't need to open-code it. Nice, that's much cleaner. > > > > > This would only work for setters/qobject-input-visitor ATM though since > > qobject-output-visitor doesn't support the visit_start_alternate() > > interface. I'm not sure why it wasn't enabled on the output side, maybe > > Eric knows if doing so would be feasible for this situation? > > qobject-output-visitor DOES support alternates. It doesn't need a > .start_alternate callback, because the generic visitor-core.c code > handles things just fine for output visitors (only input visitors _have_ > to have a callback). The comments in visitor-impl.h confirm that. Ok, that makes sense. I was struggling to think of what useful purpose a start_alternate would serve for qov, and didn't realize it was optional within visit_start_alternate. Thanks for clarifying. > > -- > Eric Blake, Principal Software Engineer > Red Hat, Inc. +1-919-301-3266 > Virtualization: qemu.org | libvirt.org >
On Tue, Apr 18, 2017 at 04:19:10PM -0500, Eric Blake wrote: > On 04/18/2017 04:01 PM, Michael Roth wrote: > >>> @@ -3706,8 +3707,17 @@ static void x86_cpu_get_bit_prop(Object *obj, Visitor *v, const char *name, > >>> X86CPU *cpu = X86_CPU(obj); > >>> BitProperty *fp = opaque; > >>> uint32_t f = cpu->env.features[fp->w]; > >>> + uint32_t ff = cpu->forced_features[fp->w]; > >>> bool value = (f & fp->mask) == fp->mask; > >>> - visit_type_bool(v, name, &value, errp); > >>> + bool forced = (ff & fp->mask) == fp->mask; > >>> + char str[] = "force"; > >>> + char *strval = str; > >>> + > >>> + if (forced) { > >>> + visit_type_str(v, name, &strval, errp); > >>> + } else { > >>> + visit_type_bool(v, name, &value, errp); > >> > >> Interesting approach. This means we keep returning a boolean > >> value on the property getter (which is important to not break > >> compatibility), but return a string in case the feature was set > >> to "force". > >> > >> We probably should update the 'type' field of the property, as it > >> won't be a real "bool" property anymore. > >> > >> I won't say I love that solution, but it seems to work. I'm CCing > >> the QAPI maintainers to see what they think about it. > > Use of a formal QAPI alternate type seems reasonable here. > > >> > >> (For reference: in the v1 thread I have suggested introducing a > >> new enum type in the QAPI schema, but this would break > >> compatibility with existing management code that expects the > >> property to return a boolean value [like the users of > >> query-cpu-model-expansion]). > > >>> @@ -3724,7 +3735,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; > >>> + cpu->forced_features[fp->w] |= fp->mask; > >>> + } else { > >>> + local_err = NULL; > >>> + visit_type_bool(v, name, &value, &local_err); > >>> + } > >> > >> I'm not sure this is valid usage of the visitor API. If the > >> visit_type_str() call succeeds, isn't the visitor allowed to > >> consume the original value, and return something completely > >> different when visit_type_bool() is called? > > > > Better would be defining the QAPI alternate type: > > { 'enum': 'Force', 'data': ['force']} > { 'alternate': 'BoolOrForce', > 'data': { 'b': 'bool', 's': 'Force' } } > > then given a BoolOrForce *state; declaration, you do either: > > state->type = QTYPE_QBOOL; > state->u.b = true; > > or > > state->type = QTYPE_QSTRING; > state->u.s = FORCE_FORCE; > > and then call visit_type_BoolOrForce(..., &state ...) > > Qcow2OverlapChecks is an example of an alternate in use, if that helps. Wow, this is wonderful. I will take a look. Thanks! > [...]
On Tue, Apr 18, 2017 at 04:19:10PM -0500, Eric Blake wrote: > On 04/18/2017 04:01 PM, Michael Roth wrote: > > >>> @@ -3706,8 +3707,17 @@ static void x86_cpu_get_bit_prop(Object *obj, Visitor *v, const char *name, > >>> X86CPU *cpu = X86_CPU(obj); > >>> BitProperty *fp = opaque; > >>> uint32_t f = cpu->env.features[fp->w]; > >>> + uint32_t ff = cpu->forced_features[fp->w]; > >>> bool value = (f & fp->mask) == fp->mask; > >>> - visit_type_bool(v, name, &value, errp); > >>> + bool forced = (ff & fp->mask) == fp->mask; > >>> + char str[] = "force"; > >>> + char *strval = str; > >>> + > >>> + if (forced) { > >>> + visit_type_str(v, name, &strval, errp); > >>> + } else { > >>> + visit_type_bool(v, name, &value, errp); > >> > >> Interesting approach. This means we keep returning a boolean > >> value on the property getter (which is important to not break > >> compatibility), but return a string in case the feature was set > >> to "force". > >> > >> We probably should update the 'type' field of the property, as it > >> won't be a real "bool" property anymore. > >> > >> I won't say I love that solution, but it seems to work. I'm CCing > >> the QAPI maintainers to see what they think about it. > > Use of a formal QAPI alternate type seems reasonable here. > > >> > >> (For reference: in the v1 thread I have suggested introducing a > >> new enum type in the QAPI schema, but this would break > >> compatibility with existing management code that expects the > >> property to return a boolean value [like the users of > >> query-cpu-model-expansion]). > > >>> @@ -3724,7 +3735,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; > >>> + cpu->forced_features[fp->w] |= fp->mask; > >>> + } else { > >>> + local_err = NULL; > >>> + visit_type_bool(v, name, &value, &local_err); > >>> + } > >> > >> I'm not sure this is valid usage of the visitor API. If the > >> visit_type_str() call succeeds, isn't the visitor allowed to > >> consume the original value, and return something completely > >> different when visit_type_bool() is called? > > > > Better would be defining the QAPI alternate type: > > { 'enum': 'Force', 'data': ['force']} > { 'alternate': 'BoolOrForce', > 'data': { 'b': 'bool', 's': 'Force' } } > > then given a BoolOrForce *state; declaration, you do either: > > state->type = QTYPE_QBOOL; > state->u.b = true; > > or > > state->type = QTYPE_QSTRING; > state->u.s = FORCE_FORCE; > > and then call visit_type_BoolOrForce(..., &state ...) > > Qcow2OverlapChecks is an example of an alternate in use, if that helps. > > > That was indeed one of the design goals, to the extent that > > you could even take a stream of data types and actually build > > up a dictionary based on the ordering visited (e.g. the > > once-proposed BER visitor for migration and why QAPI uses > > OrderedDict when generating visitors). > > We document that QMP is order-insensitive, but you are right that > QAPI-generated code still respects ordering. Some of our testsuite at > least relies on the stability you get by always visiting things in > declaration order, even though other dictionary orders should still be > valid. > > > > > I'm not sure how important that is anymore, but even setting > > that aside that we still have the issue of not handling the > > data type as declared, and relying on error-handling to probe > > it, which doesn't necessarily leave the visitor in a recoverable > > state where we can continue visiting. I think it might be possible > > to rely on the "alternate" QAPI type to handle this case. Maybe > > something like this? > > Yes, that was the proposal I outlined above. Note that your rough-idea > code: > > > > > GenericAlternate *alt = NULL; > > visit_start_alternate(v, name, &alt, sizeof(*alt), false, &local_err); > > if (local_err) { > > goto out; > > } > > if (!alt) { > > goto out_obj; > > } > > switch (alt->type) { > > case QTYPE_QBOOL: > > visit_type_bool(v, name, &value, &local_err); > > break; > > case QTYPE_QSTRING: > > visit_type_str(v, name, &strval, &local_err); > > ... > is a duplicate of what is already generated for > visit_type_BoolOrForce(), so you don't need to open-code it. > > > > > This would only work for setters/qobject-input-visitor ATM though since > > qobject-output-visitor doesn't support the visit_start_alternate() > > interface. I'm not sure why it wasn't enabled on the output side, maybe > > Eric knows if doing so would be feasible for this situation? > > qobject-output-visitor DOES support alternates. It doesn't need a > .start_alternate callback, because the generic visitor-core.c code > handles things just fine for output visitors (only input visitors _have_ > to have a callback). The comments in visitor-impl.h confirm that. We use string-input-visitor to parse -cpu, though, and it doesn't have .start_alternate. It should be possible implement it, but only for a subset of alternates. We can implement it for a bool/enum alternate if the enum doesn't have any members named on/off/true/false/yes/no, but .start_alternate() needs more information to decide if that's the case.
diff --git a/target/i386/cpu.c b/target/i386/cpu.c index 13c0985..6105fc5 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]; @@ -3464,6 +3464,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; @@ -3706,8 +3707,17 @@ static void x86_cpu_get_bit_prop(Object *obj, Visitor *v, const char *name, X86CPU *cpu = X86_CPU(obj); BitProperty *fp = opaque; uint32_t f = cpu->env.features[fp->w]; + uint32_t ff = cpu->forced_features[fp->w]; bool value = (f & fp->mask) == fp->mask; - visit_type_bool(v, name, &value, errp); + bool forced = (ff & fp->mask) == fp->mask; + char str[] = "force"; + char *strval = str; + + if (forced) { + visit_type_str(v, name, &strval, errp); + } else { + visit_type_bool(v, name, &value, errp); + } } static void x86_cpu_set_bit_prop(Object *obj, Visitor *v, const char *name, @@ -3717,6 +3727,7 @@ static void x86_cpu_set_bit_prop(Object *obj, Visitor *v, const char *name, X86CPU *cpu = X86_CPU(obj); BitProperty *fp = opaque; Error *local_err = NULL; + char *strval = NULL; bool value; if (dev->realized) { @@ -3724,7 +3735,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; + cpu->forced_features[fp->w] |= fp->mask; + } else { + local_err = NULL; + visit_type_bool(v, name, &value, &local_err); + } + if (local_err) { error_propagate(errp, local_err); return; diff --git a/target/i386/cpu.h b/target/i386/cpu.h index c4602ca..69efe6c 100644 --- a/target/i386/cpu.h +++ b/target/i386/cpu.h @@ -1230,6 +1230,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> --- v1 -> v2: - Base on "i386: Replace uint32_t* with FeatureWord on feature getter/setter" --- target/i386/cpu.c | 25 ++++++++++++++++++++++--- target/i386/cpu.h | 3 +++ 2 files changed, 25 insertions(+), 3 deletions(-)