[v2] i386: Allow cpuid bit override
diff mbox

Message ID 1490700426-222115-1-git-send-email-agraf@suse.de
State New
Headers show

Commit Message

Alexander Graf March 28, 2017, 11:27 a.m. UTC
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(-)

Comments

Eduardo Habkost April 13, 2017, 5:36 p.m. UTC | #1
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
> 
>
Michael Roth April 18, 2017, 9:01 p.m. UTC | #2
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
>
Michael Roth April 18, 2017, 9:03 p.m. UTC | #3
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
> >
Eric Blake April 18, 2017, 9:19 p.m. UTC | #4
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.
Michael Roth April 18, 2017, 9:41 p.m. UTC | #5
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
>
Eduardo Habkost April 19, 2017, 5:49 p.m. UTC | #6
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!

> [...]
Eduardo Habkost April 28, 2017, 7:15 p.m. UTC | #7
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.

Patch
diff mbox

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