Re: [RFC PATCH v7 18/18] arm-qmp-cmds: introspection for ID register props

[Markus Armbruster <armbru@redhat.com>]

Eric Auger <eric.auger@redhat.com> writes:

> From: Cornelia Huck <cohuck@redhat.com>
>
> Implement the capability to query available ID register values by
> adding SYSREG_* options and values to the cpu model expansion for the
> host model, if available.
>
> Excerpt:
> (QEMU) query-cpu-model-expansion type=full model={"name":"host"}

Is this qmp-shell?

> {"return": {"model": {"name": "host", "props": {"SYSREG_ID_AA64PFR0_EL1_EL3": 1,
> "SYSREG_ID_AA64ISAR2_EL1_CLRBHB": 0, "SYSREG_CTR_EL0_L1Ip": 3,
> "SYSREG_MIDR_EL1_PartNum": 3407, "SYSREG_CTR_EL0_DminLine": 4,
> "SYSREG_ID_AA64MMFR0_EL1_PARange": 5, "SYSREG_ID_AA64MMFR1_EL1_ECBHB": 0
> ../..
>
> This allows the upper stack to detect available writable ID regs and
> the "host passthrough model" values.
>
> It also allows to test some ID reg field values:
> (QEMU) query-cpu-model-expansion type=full model={"name":"host","props":{"SYSREG_ID_AA64ISAR0_EL1_DP":0x13}}
> {"error": {"class": "GenericError", "desc": "idreg SYSREG_ID_AA64ISAR0_EL1_DP set value (0x13) exceeds length of field (4)!"}}
>
> (QEMU) query-cpu-model-expansion type=full model={"name":"host","props":{"SYSREG_ID_AA64ISAR0_EL1_DP":0x2}}
> {"error": {"class": "GenericError", "desc": "idreg SYSREG_ID_AA64ISAR0_EL1_DP set value (0x2) does not match any arch valid enum value!"}}

The examples use CPU "host".  Inconvenient for me, because I run into
"The CPU type 'host' requires KVM" when I try to play with them myself.
If you don't actually need "host" here, consider using something that
works more widely, just to help reviewers.

>
> The following checks are performed:
> - the value does not exceed the field size
> - if the field is associated to enum values, the set value must be one
>   of those enum values
> - the value is applied against a scratch vcpu making sure the setting is
>   not rejected for this host
>
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> Signed-off-by: Cornelia Huck <cohuck@redhat.com>
>
> ---
>
> v5 -> v6:
> - add the write capability
> ---
>  target/arm/arm-qmp-cmds.c | 98 +++++++++++++++++++++++++++++++++++++++
>  1 file changed, 98 insertions(+)
>
> diff --git a/target/arm/arm-qmp-cmds.c b/target/arm/arm-qmp-cmds.c
> index 83ec95c290..41fefadcea 100644
> --- a/target/arm/arm-qmp-cmds.c
> +++ b/target/arm/arm-qmp-cmds.c
> @@ -21,6 +21,7 @@
>   */
>  
>  #include "qemu/osdep.h"
> +#include "qemu/error-report.h"
>  #include "qemu/target-info.h"
>  #include "hw/core/boards.h"
>  #include "kvm_arm.h"
> @@ -30,7 +31,10 @@
>  #include "qapi/qapi-commands-machine.h"
>  #include "qapi/qapi-commands-misc-arm.h"
>  #include "qobject/qdict.h"
> +#include "qobject/qnum.h"
>  #include "qom/qom-qobject.h"
> +#include <linux/kvm.h>
> +#include "system/kvm.h"
>  #include "cpu.h"
>  
>  static GICCapability *gic_cap_new(int version)
> @@ -84,11 +88,14 @@ CpuModelExpansionInfo *qmp_query_cpu_model_expansion(CpuModelExpansionType type,
>                                                       Error **errp)
>  {
>      CpuModelExpansionInfo *expansion_info;
> +    ObjectPropertyIterator iter;
>      const QDict *qdict_in;
> +    ObjectProperty *idregprop;
>      QDict *qdict_out;
>      ObjectClass *oc;
>      Object *obj;
>      const char *name;
> +    int fdarray[3];
>      int i;
>  
>      if (type != CPU_MODEL_EXPANSION_TYPE_FULL) {
> @@ -133,6 +140,38 @@ CpuModelExpansionInfo *qmp_query_cpu_model_expansion(CpuModelExpansionType type,
>  
>      obj = object_new(object_class_get_name(oc));
>  
> +    if (kvm_enabled()) {
> +        bool pmuv3_supported = kvm_check_extension(kvm_state, KVM_CAP_ARM_PMU_V3);
> +        bool sve_supported = kvm_check_extension(kvm_state, KVM_CAP_ARM_SVE);
> +        struct kvm_vcpu_init init = { .target = -1, };
> +        bool el2_supported = kvm_arm_el2_supported();
> +        bool pauth_supported;
> +        int ret;
> +
> +        pauth_supported = kvm_check_extension(kvm_state, KVM_CAP_ARM_PTRAUTH_ADDRESS) &&
> +                          kvm_check_extension(kvm_state, KVM_CAP_ARM_PTRAUTH_GENERIC);
> +
> +        if (sve_supported) {
> +            init.features[0] |= 1 << KVM_ARM_VCPU_SVE;
> +        }
> +        if (el2_supported) {
> +            init.features[0] |= 1 << KVM_ARM_VCPU_HAS_EL2;
> +        }
> +        if (pauth_supported) {
> +            init.features[0] |= (1 << KVM_ARM_VCPU_PTRAUTH_ADDRESS |
> +                             1 << KVM_ARM_VCPU_PTRAUTH_GENERIC);
> +        }
> +        if (pmuv3_supported) {
> +            init.features[0] |= 1 << KVM_ARM_VCPU_PMU_V3;
> +        }
> +
> +        ret = kvm_arm_create_scratch_host_vcpu(fdarray, &init);
> +        if (!ret) {
> +            error_setg(errp, "failing creating a scratch vcpu");

This error message feels off.  I'd use something like "can't create
FOO".  Still bad, because it provides no clue on why.  What are the
possible failure modes?

> +            return NULL;
> +        }
> +    }
> +
>      if (model->props) {
>          Visitor *visitor;
>          Error *err = NULL;
> @@ -145,6 +184,49 @@ CpuModelExpansionInfo *qmp_query_cpu_model_expansion(CpuModelExpansionType type,
>          }
>  
>          qdict_in = qobject_to(QDict, model->props);
> +
> +        for (const QDictEntry *entry = qdict_first(qdict_in);
> +                 entry != NULL; entry = qdict_next(qdict_in, entry)) {
> +            const char *key = qdict_entry_key(entry);
> +            QObject *val_obj = qdict_entry_value(entry);
> +            ObjectProperty *prop;
> +            Visitor *v;
> +            bool success;
> +            uint64_t val;
> +
> +            prop = object_property_find(obj, key);
> +            if (!prop) {
> +                error_setg(errp, "%s does not exist!", key);

Scratch the !

From error_setg()'s contract:

 * The resulting message should be a single phrase, with no newline or
 * trailing punctuation.

Is this error reachable?  How?

> +                return NULL;
> +            }
> +
> +            if (!g_str_has_prefix(key, "SYSREG_")) {
> +                continue;
> +            }
> +
> +            /* consume the prop to avoid unexpected parameter */
> +            if (!visit_type_uint64(visitor, key, &val, errp)) {
> +                return NULL;
> +            }
> +
> +            v = qobject_input_visitor_new(val_obj);
> +
> +            if (!object_property_set(obj, key, v, &err)) {
> +                error_propagate(errp, err);

Pass @errp to object_property_set() and drop the error_propagate()?

> +                visit_free(v);
> +                return NULL;
> +            }
> +
> +            success = kvm_idreg_write_scratch_vcpu(fdarray[2], v, key,
> +                                                   prop->opaque, &err);
> +            if (!success) {
> +                error_propagate(errp, err);

Likewise?

> +                visit_free(v);
> +                return NULL;
> +            }
> +            visit_free(v);
> +        }
> +
>          i = 0;
>          while ((name = cpu_model_advertised_features[i++]) != NULL) {
>              if (qdict_get(qdict_in, name)) {
> @@ -160,6 +242,10 @@ CpuModelExpansionInfo *qmp_query_cpu_model_expansion(CpuModelExpansionType type,
>          if (!err) {
>              arm_cpu_finalize_features(ARM_CPU(obj), &err);
>          }
> +
> +        if (kvm_enabled()) {
> +            kvm_arm_destroy_scratch_host_vcpu(fdarray);
> +        }
>          visit_end_struct(visitor, NULL);
>          visit_free(visitor);
>          if (err) {
> @@ -190,6 +276,18 @@ CpuModelExpansionInfo *qmp_query_cpu_model_expansion(CpuModelExpansionType type,
>          }
>      }
>  
> +    object_property_iter_init(&iter, obj);
> +
> +    while ((idregprop = object_property_iter_next(&iter))) {
> +        QObject *value;
> +
> +        if (!g_str_has_prefix(idregprop->name, "SYSREG_")) {
> +            continue;
> +        }
> +        value = object_property_get_qobject(obj, idregprop->name, &error_abort);
> +        qdict_put_obj(qdict_out, idregprop->name, value);
> +    }
> +
>      if (!qdict_size(qdict_out)) {
>          qobject_unref(qdict_out);
>      } else {