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

[Michael Roth <mdroth@linux.vnet.ibm.com>]

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
>