Re: [RFC PATCH v7 12/18] target/arm/kvm: Introduce kvm_arm_expose_idreg_properties

[Markus Armbruster <armbru@redhat.com>]

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

> This helper decode the ID reg writable mask, matches it against
> ID reg fields defined in target/arm/cpu-idregs.h.inc and
> for each writable named field, generates a uint64 property.
>
> REVIDR_EL1 and AIDR_EL1 are writable but they do not expose any
> field. They will be handled separately.
>
> Signed-off-by: Eric Auger <eric.auger@redhat.com>

[...]

> diff --git a/target/arm/kvm.c b/target/arm/kvm.c
> index 4f9b0a559d..1a78893f87 100644
> --- a/target/arm/kvm.c
> +++ b/target/arm/kvm.c

[...]

> +static void set_sysreg_prop(Object *obj, Visitor *v,
> +                            const char *name, void *opaque,
> +                            Error **errp)
> +{
> +    ARM64SysRegField *field = (ARM64SysRegField *)opaque;
> +    ARMCPU *cpu = ARM_CPU(obj);
> +    uint64_t *idregs = cpu->isar.idregs;
> +    uint64_t old, value, mask;
> +    int lower = field->shift;
> +    int length = field->length;
> +    int index = field->index;
> +
> +    if (!visit_type_uint64(v, name, &value, errp)) {
> +        return;
> +    }
> +
> +    if (length < 64 && value > ((1 << length) - 1)) {
> +        error_setg(errp,
> +                   "idreg %s set value (0x%lx) exceeds length of field (%d)!",
> +                   name, value, length);
> +        return;
> +    }
> +
> +    if (field->arch_vals) {
> +        /* this field has some enum values */
> +        for (int i = 0; i < field->arch_vals_count; i++) {
> +            if (value == field->arch_vals[i].value) {
> +                goto valid;
> +            }
> +        }
> +        error_setg(errp,
> +                   "idreg %s set value (0x%lx) does not match any "
> +                   "arch valid enum value!", name, value);

Scratch the !

From error_setg()'s contract:

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

> +        return;
> +    }
> +
> +valid:
> +
> +    mask = MAKE_64BIT_MASK(lower, length);
> +    value = value << lower;
> +    old = idregs[index];
> +    idregs[index] = old & ~mask;
> +    idregs[index] |= value;
> +    trace_set_sysreg_prop(name, old, mask, value, idregs[index]);
> +}

[...]