Re: [PATCH v5 2/3] arm/kvm: add support for MTE

[Cornelia Huck <cohuck@redhat.com>]

On Fri, Feb 03 2023, Richard Henderson <richard.henderson@linaro.org> wrote:

> On 2/3/23 03:44, Cornelia Huck wrote:
>> +static inline bool arm_machine_has_tag_memory(void)
>> +{
>> +#ifndef CONFIG_USER_ONLY
>> +    Object *obj = object_dynamic_cast(qdev_get_machine(), TYPE_VIRT_MACHINE);
>> +
>> +    /* so far, only the virt machine has support for tag memory */
>> +    if (obj) {
>> +        VirtMachineState *vms = VIRT_MACHINE(obj);
>
> VIRT_MACHINE() does object_dynamic_cast_assert, and we've just done that.
>
> As this is startup, it's not the speed that matters.  But it does look unfortunate.  Not 
> for this patch set, but perhaps we ought to add TRY_OBJ_NAME to DECLARE_INSTANCE_CHECKER?

Instead of the pattern above, we could also do

VirtMachineState *vms = (VirtMachineState *) object_dynamic_cast(...);
if (vms) {
(...)


>
>> +void arm_cpu_mte_finalize(ARMCPU *cpu, Error **errp)
>> +{
>> +    bool enable_mte;
>> +
>> +    switch (cpu->prop_mte) {
>> +    case ON_OFF_AUTO_OFF:
>> +        enable_mte = false;
>> +        break;
>> +    case ON_OFF_AUTO_ON:
>> +        if (tcg_enabled()) {
>> +            if (cpu_isar_feature(aa64_mte, cpu)) {
>> +                if (!arm_machine_has_tag_memory()) {
>> +                    error_setg(errp, "mte=on requires tag memory");
>> +                    return;
>> +                }
>> +            } else {
>> +                error_setg(errp, "mte not supported by this CPU type");
>> +                return;
>> +            }
>> +        }
>> +        if (kvm_enabled() && !kvm_arm_mte_supported()) {
>> +            error_setg(errp, "mte not supported by kvm");
>> +            return;
>> +        }
>> +        enable_mte = true;
>> +        break;
>
> What's here is not wrong, but maybe better structured as
>
> 	enable_mte = true;
>          if (qtest_enabled()) {
>              break;
>          }
>          if (tcg_enabled()) {
>              if (arm_machine_tag_mem) {
>                  break;
>              }
>              error;
>              return;
>          }
>          if (kvm_enabled() && kvm_arm_mte_supported) {
>              break;
>          }
>          error("mte not supported by %s", current_accel_type());
>          return;

That's indeed better, as we also see what's going on for the different
accelarators.

> We only add the property for tcg via -cpu max, so the isar check is redundant.