Re: [Qemu-devel] [PATCH v2] i386: Allow cpuid bit override

[Eduardo Habkost]

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