i386: Allow cpuid bit override
diff mbox

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

Commit Message

Alexander Graf March 27, 2017, 10:19 p.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>
---
 target/i386/cpu.c | 30 ++++++++++++++++++++++++++----
 target/i386/cpu.h |  3 +++
 2 files changed, 29 insertions(+), 4 deletions(-)

Comments

Eduardo Habkost March 28, 2017, 12:41 a.m. UTC | #1
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
>
Alexander Graf March 28, 2017, 11:26 a.m. UTC | #2
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
Eduardo Habkost March 28, 2017, 12:31 p.m. UTC | #3
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.
Alexander Graf March 28, 2017, 12:35 p.m. UTC | #4
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
Paolo Bonzini March 28, 2017, 12:41 p.m. UTC | #5
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
Eduardo Habkost March 28, 2017, 4:01 p.m. UTC | #6
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.
Paolo Bonzini March 28, 2017, 4:35 p.m. UTC | #7
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
Eduardo Habkost March 30, 2017, 2:22 p.m. UTC | #8
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. :)
Alexander Graf March 30, 2017, 2:23 p.m. UTC | #9
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
Eduardo Habkost March 30, 2017, 2:25 p.m. UTC | #10
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. :(
Alexander Graf March 30, 2017, 2:27 p.m. UTC | #11
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
Eduardo Habkost April 13, 2017, 5:20 p.m. UTC | #12
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.

Patch
diff mbox

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