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~
>
