Re: [PATCH v5 1/9] target/arm/monitor: Introduce qmp_query_cpu_model_expansion

[Beata Michalska]

On Tue, 15 Oct 2019 at 12:56, Beata Michalska
<address@hidden> wrote:
>
> On Tue, 15 Oct 2019 at 11:56, Andrew Jones <address@hidden> wrote:
> >
> > On Tue, Oct 15, 2019 at 10:59:16AM +0100, Beata Michalska wrote:
> > > On Tue, 1 Oct 2019 at 14:04, Andrew Jones <address@hidden> wrote:
> > > > +
> > > > +    obj = object_new(object_class_get_name(oc));
> > > > +
> > > > +    if (qdict_in) {
> > > > +        Visitor *visitor;
> > > > +        Error *err = NULL;
> > > > +
> > > > +        visitor = qobject_input_visitor_new(model->props);
> > > > +        visit_start_struct(visitor, NULL, NULL, 0, &err);
> > > > +        if (err) {
> > > > +            object_unref(obj);
> > >
> > > Shouldn't we free the 'visitor' here as well ?
> >
> > Yes. Good catch. So we also need to fix
> > target/s390x/cpu_models.c:cpu_model_from_info(), which has the same
> > construction (the construction from which I derived this)
> >
> > >
> > > > +            error_propagate(errp, err);
> > > > +            return NULL;
> > > > +        }
> > > > +
> >
> > What about the rest of the patch? With that fixed for v6 can I
> > add your r-b?
> >
>
> I still got this feeling that we could optimize that a bit - which I'm
> currently on, so hopefully I'll be able to add more comments soon if
> that proves to be the case.
>
> BR
> Beata

I think there are few options that might be considered though the gain
is not huge .. but it's always smth:

> +CpuModelExpansionInfo *qmp_query_cpu_model_expansion(CpuModelExpansionType 
> type,
> +                                                     CpuModelInfo *model,
> +                                                     Error **errp)
> +{
> +    CpuModelExpansionInfo *expansion_info;
> +    const QDict *qdict_in = NULL;
> +    QDict *qdict_out;
> +    ObjectClass *oc;
> +    Object *obj;
> +    const char *name;
> +    int i;
> +
> +    if (type != CPU_MODEL_EXPANSION_TYPE_FULL) {
> +        error_setg(errp, "The requested expansion type is not supported");
> +        return NULL;
> +    }
> +
> +    if (!kvm_enabled() && !strcmp(model->name, "host")) {
> +        error_setg(errp, "The CPU type '%s' requires KVM", model->name);
> +        return NULL;
> +    }
> +
> +    oc = cpu_class_by_name(TYPE_ARM_CPU, model->name);
> +    if (!oc) {
> +        error_setg(errp, "The CPU type '%s' is not a recognized ARM CPU 
> type",
> +                   model->name);
> +        return NULL;
> +    }
> +
> +    if (kvm_enabled()) {
> +        const char *cpu_type = current_machine->cpu_type;
> +        int len = strlen(cpu_type) - strlen(ARM_CPU_TYPE_SUFFIX);
> +        bool supported = false;
> +
> +        if (!strcmp(model->name, "host") || !strcmp(model->name, "max")) {
> +            /* These are kvmarm's recommended cpu types */
> +            supported = true;
> +        } else if (strlen(model->name) == len &&
> +                   !strncmp(model->name, cpu_type, len)) {
> +            /* KVM is enabled and we're using this type, so it works. */
> +            supported = true;
> +        }
> +        if (!supported) {
> +            error_setg(errp, "We cannot guarantee the CPU type '%s' works "
> +                             "with KVM on this host", model->name);
> +            return NULL;
> +        }
> +    }
> +

The above section can be slightly reduced and rearranged - preferably
moved to a separate function
-> get_cpu_model (...) ?

* You can check the 'host' model first and then validate the accelerator ->
    if ( !strcmp(model->name, "host")
        if (!kvm_enabled())
            log_error & leave
       else
          goto cpu_class_by_name /*cpu_class_by_name moved after the
final model check @see below */

* the kvm_enabled section can be than slightly improved (dropping the
second compare against 'host')

      if (kvm_enabled() && strcmp(model->name, "max") {
           /*Validate the current_machine->cpu_type against the
model->name and report error case mismatch
          /* otherwise just fall through */
      }
 * cpu_class_by_name moved here ...
> +    if (model->props) {
MInor: the CPUModelInfo seems to have dedicated field for that
verification -> has_props

> +        qdict_in = qobject_to(QDict, model->props);
> +        if (!qdict_in) {
> +            error_setg(errp, QERR_INVALID_PARAMETER_TYPE, "props", "dict");
> +            return NULL;
> +        }
> +    }
> +
> +    obj = object_new(object_class_get_name(oc));
> +
> +    if (qdict_in) {
> +        Visitor *visitor;
> +        Error *err = NULL;
> +
> +        visitor = qobject_input_visitor_new(model->props);
> +        visit_start_struct(visitor, NULL, NULL, 0, &err);
> +        if (err) {
> +            object_unref(obj);
> +            error_propagate(errp, err);
> +            return NULL;
> +        }
> +
> +        i = 0;
> +        while ((name = cpu_model_advertised_features[i++]) != NULL) {
> +            if (qdict_get(qdict_in, name)) {
> +                object_property_set(obj, visitor, name, &err);
> +                if (err) {
> +                    break;
> +                }
> +            }
> +        }
> +
> +        if (!err) {
> +            visit_check_struct(visitor, &err);
> +        }
> +        visit_end_struct(visitor, NULL);
> +        visit_free(visitor);
> +        if (err) {
> +            object_unref(obj);
> +            error_propagate(errp, err);
> +            return NULL;
> +        }
> +    }

The both >> if (err) << blocks could be extracted and moved at the end
of the function
to mark a 'cleanup section'  and both here and few lines before
(with the visit_start_struct failure) could use goto.
Easier to maintain and to make sure we make the proper cleanup in any case.

> +
> +    expansion_info = g_new0(CpuModelExpansionInfo, 1);
> +    expansion_info->model = g_malloc0(sizeof(*expansion_info->model));
> +    expansion_info->model->name = g_strdup(model->name);
> +
> +    qdict_out = qdict_new();
> +
> +    i = 0;
> +    while ((name = cpu_model_advertised_features[i++]) != NULL) {
> +        ObjectProperty *prop = object_property_find(obj, name, NULL);
> +        if (prop) {
> +            Error *err = NULL;
> +            QObject *value;
> +
> +            assert(prop->get);
> +            value = object_property_get_qobject(obj, name, &err);
> +            assert(!err);
> +
> +            qdict_put_obj(qdict_out, name, value);
> +        }
> +    }
> +

This could be merged with the first iteration over the features,
smth like:

    while () {
        if ((value = qdict_get(qdict_in, name))) {
            object_property_set ...
           if (!err)
               qobject_ref(value) /* we have the weak reference */
            else
                break;
        } else {
             value = object_property_get_qobject()
        }
        if (value) qdict_put_object(qdict_out, name, value)
    }

This way you iterate over the table once and you do not query
for the same property twice by getting the value from the qdict_in.
If the value is not acceptable we will fail either way so should be all good.


> +    if (!qdict_size(qdict_out)) {
> +        qobject_unref(qdict_out);
> +    } else {
> +        expansion_info->model->props = QOBJECT(qdict_out);
> +        expansion_info->model->has_props = true;
> +    }
> +
> +    object_unref(obj);
> +
> +    return expansion_info;

Mentioned earlier cleanup section:
cleanup:
   object_unref(obj);
   qobject_unref(qdict_out) ; /* if single loop is used */
   error_propagate(errp,err);
   return NULL;

> +}
> --
> 2.20.1
>

Hope I haven't missed anything.
What do you think ?

BR
Beata
> > Thanks,
> > drew