Re: [Qemu-arm] [Qemu-devel] [PATCH v2 03/14] target/arm/monitor: Introduce qmp_query_cpu_model_expansion

[Andrew Jones]

On Wed, Jul 24, 2019 at 04:25:32PM +0200, Auger Eric wrote:
> >>>>> diff --git a/target/arm/monitor.c b/target/arm/monitor.c
> >>>>> index 41b32b94b258..19e3120eef95 100644
> >>>>> --- a/target/arm/monitor.c
> >>>>> +++ b/target/arm/monitor.c
> >>>>> @@ -23,7 +23,13 @@
> >>>>>  #include "qemu/osdep.h"
> >>>>>  #include "hw/boards.h"
> >>>>>  #include "kvm_arm.h"
> >>>>> +#include "qapi/error.h"
> >>>>> +#include "qapi/visitor.h"
> >>>>> +#include "qapi/qobject-input-visitor.h"
> >>>>>  #include "qapi/qapi-commands-target.h"
> >>>>> +#include "qapi/qmp/qerror.h"
> >>>>> +#include "qapi/qmp/qdict.h"
> >>>>> +#include "qom/qom-qobject.h"
> >>>>>  
> >>>>>  static GICCapability *gic_cap_new(int version)
> >>>>>  {
> >>>>> @@ -82,3 +88,129 @@ GICCapabilityList *qmp_query_gic_capabilities(Error 
> >>>>> **errp)
> >>>>>  
> >>>>>      return head;
> >>>>>  }
> >>>>> +
> >>>>> +static const char *cpu_model_advertised_features[] = {
> >>>>> +    "aarch64", "pmu",
> >>>>> +    NULL
> >>>>> +};
> >>>>> +
> >>>>> +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 definition '%s' requires KVM", 
> >>>>> model->name);
> >>>>> +        return NULL;
> >>>>> +    }
> >>>>> +
> >>>>> +    oc = cpu_class_by_name(TYPE_ARM_CPU, model->name);
> >>>>> +    if (!oc) {
> >>>>> +        error_setg(errp, "The CPU definition '%s' is unknown.", 
> >>>>> 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, "The CPU definition '%s' cannot "
> >>>> use model name instead of CPU definition?
> >>>
> >>> I took that wording from s390x, but maybe I prefer "The CPU type..."
> >>> better. I'll change it for v3.>> This CPU type is not recognized as an 
> >>> ARM CPU type?
> > 
> > That's not what this error message is stating. The CPU type may well be an
> > ARM CPU type, but it's not one you can expect to use with KVM enabled. I
> > currently have
> > 
> >   "The CPU type '%s' cannot "
> >   "be used with KVM on this host", model->name)
> > 
> > queued up for v3.
> 
> decidedly, I meant the error message associated to:
> 
> +    oc = cpu_class_by_name(TYPE_ARM_CPU, model->name);
> +    if (!oc) {
> +        error_setg(errp, "The CPU definition '%s' is unknown.",
> model->name);
> +        return NULL;
> +    }

Ah, OK. Yeah I can change that one too. Of course if we deviate from
s390x's generic error messages for common errors, then we're assuming
the messages aren't being parsed by upper layers using code that we'd
like to easily adopt to ARM. But, I think that assumption is reasonable.

> 
> Why am I always looking at your series when we suffer heat wave?
>

Climate change generates too many heat waves. Or I generate too much
code that requires comments. Or both.

Thanks,
drew