Hi, On 2/3/23 21:40, Richard Henderson wrote: > On 2/3/23 03:44, Cornelia Huck wrote: >> +static void aarch64_cpu_get_mte(Object *obj, Visitor *v, const char >> *name, >> + void *opaque, Error **errp) >> +{ >> + ARMCPU *cpu = ARM_CPU(obj); >> + OnOffAuto mte = cpu->prop_mte; >> + >> + visit_type_OnOffAuto(v, name, &mte, errp); >> +} > > You don't need to copy to a local variable here. > >> + >> +static void aarch64_cpu_set_mte(Object *obj, Visitor *v, const char >> *name, >> + void *opaque, Error **errp) >> +{ >> + ARMCPU *cpu = ARM_CPU(obj); >> + >> + visit_type_OnOffAuto(v, name, &cpu->prop_mte, errp); >> +} > > ... which makes get and set functions identical. > No need for both. This looks like a common pattern though. virt_get_acpi/set_acpi in virt.c or pc_machine_get_vmport/set_vmport in i386/pc.c and many other places (microvm ...). Do those other callers also need some simplifications?
Eric > >> +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? > >> +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; > > We only add the property for tcg via -cpu max, so the isar check is > redundant. > > > r~ >