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

[Eric Blake]

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.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

signature.asc
Description: OpenPGP digital signature