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

[Cornelia Huck <cohuck@xxxxxxxxxx>]

On Wed, Feb 15 2023, Eric Auger <eauger@xxxxxxxxxx> wrote:

> Hi Richard,
> On 2/6/23 19:27, Richard Henderson wrote:
>> On 2/6/23 03:32, Eric Auger wrote:
>>>> +void kvm_arm_enable_mte(Error **errp)
>>>> +{
>>>> +    static bool tried_to_enable = false;
>>>> +    Error *mte_migration_blocker = NULL;
>>> can't you make the mte_migration_blocker static instead?
>>>
>>>> +    int ret;
>>>> +
>>>> +    if (tried_to_enable) {
>>>> +        /*
>>>> +         * MTE on KVM is enabled on a per-VM basis (and retrying
>>>> doesn't make
>>>> +         * sense), and we only want a single migration blocker as well.
>>>> +         */
>>>> +        return;
>>>> +    }
>>>> +    tried_to_enable = true;
>>>> +
>>>> +    if ((ret = kvm_vm_enable_cap(kvm_state, KVM_CAP_ARM_MTE, 0))) {
>>>> +        error_setg_errno(errp, -ret, "Failed to enable
>>>> KVM_CAP_ARM_MTE");
>>>> +        return;
>>>> +    }
>>>> +
>>>> +    /* TODO: add proper migration support with MTE enabled */
>>>> +    error_setg(&mte_migration_blocker,
>>>> +               "Live migration disabled due to MTE enabled");
>> 
>> Making the blocker static wouldn't stop multiple errors from
>> kvm_vm_enable_cap.
> Sorry I don't get what you mean. instead of checking tried_to_enable why
> can't we check !mte_migration_blocker?

[missed this one]

Do you mean

if (mte_migration_blocker) {
    return;
}

error_setg(&mte_migration_blocker, ...);

if ((ret = kvm_vm_enable_cap(...))) {
    return;
}

if (migrate_add_blocker(...)) {
    error_free(mte_migration_blocker);
    // is mte_migration_blocker guaranteed != NULL here?
}