Re: [PATCH RFC 1/2] arm/kvm: enable MTE if available

[Eric Auger]

Hi Connie,

On 6/14/22 10:40, Cornelia Huck wrote:
> On Fri, Jun 10 2022, Eric Auger <eauger@redhat.com> wrote:
> 
>> Hi Connie,
>> On 5/12/22 15:11, Cornelia Huck wrote:
>>> We need to disable migration, as we do not yet have a way to migrate
>>> the tags as well.
>>
>> This patch does much more than adding a migration blocker ;-) you may
>> describe the new cpu option and how it works.
> 
> I admit this is a bit terse ;) The idea is to control mte at the cpu
> level directly (and not indirectly via tag memory at the machine
> level). I.e. the user gets whatever is available given the constraints
> (host support etc.) if they don't specify anything, and they can
> explicitly turn it off/on.

Could the OnOffAuto property value be helpful?
> 
>>>
>>> Signed-off-by: Cornelia Huck <cohuck@redhat.com>
>>> ---
>>>  target/arm/cpu.c     | 18 ++++------
>>>  target/arm/cpu.h     |  4 +++
>>>  target/arm/cpu64.c   | 78 ++++++++++++++++++++++++++++++++++++++++++++
>>>  target/arm/kvm64.c   |  5 +++
>>>  target/arm/kvm_arm.h | 12 +++++++
>>>  target/arm/monitor.c |  1 +
>>>  6 files changed, 106 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
>>> index 029f644768b1..f0505815b1e7 100644
>>> --- a/target/arm/cpu.c
>>> +++ b/target/arm/cpu.c
>>> @@ -1435,6 +1435,11 @@ void arm_cpu_finalize_features(ARMCPU *cpu, Error 
>>> **errp)
>>>              error_propagate(errp, local_err);
>>>              return;
>>>          }
>>> +        arm_cpu_mte_finalize(cpu, &local_err);
>>> +        if (local_err != NULL) {
>>> +            error_propagate(errp, local_err);
>>> +            return;
>>> +        }
>>>      }
>>>  
>>>      if (kvm_enabled()) {
>>> @@ -1504,7 +1509,7 @@ static void arm_cpu_realizefn(DeviceState *dev, Error 
>>> **errp)
>>>          }
>>>          if (cpu->tag_memory) {
>>>              error_setg(errp,
>>> -                       "Cannot enable KVM when guest CPUs has MTE 
>>> enabled");
>>> +                       "Cannot enable KVM when guest CPUs has tag memory 
>>> enabled");
>> before this series, tag_memory was used to detect MTE was enabled at
>> machine level. And this was not compatible with KVM.
>>
>> Hasn't it changed now with this series? Sorry I don't know much about
>> that tag_memory along with the KVM use case? Can you describe it as well
>> in the cover letter.
> 
> IIU the current code correctly, the purpose of tag_memory is twofold:
> - control whether mte should be available in the first place
> - provide a place where a memory region used by the tcg implemtation can
>   be linked

OK I now understand the tag memory is a pure TCG thingy. So its setting
along with KVM shall be invalid indeed.
> 
> The latter part (extra memory region) is not compatible with
> kvm. "Presence of extra memory for the implementation" as the knob to
> configure mte for tcg makes sense, but it didn't seem right to me to use
> it for kvm while controlling something which is basically a cpu property.


> 
>>>              return;
>>>          }
>>>      }
> 
> (...)
> 
>>> +void aarch64_add_mte_properties(Object *obj)
>>> +{
>>> +    ARMCPU *cpu = ARM_CPU(obj);
>>> +
>>> +    /*
>>> +     * For tcg, the machine type may provide tag memory for MTE emulation.
>> s/machine type/machine?
> 
> Either, I guess, as only the virt machine type provides tag memory in
> the first place.
yeah that's what just a nit about the terminology.
> 
>>> +     * We do not know whether that is the case at this point in time, so
>>> +     * default MTE to on and check later.
>>> +     * This preserves pre-existing behaviour, but is really a bit awkward.
>>> +     */
>>> +    qdev_property_add_static(DEVICE(obj), &arm_cpu_mte_property);
>>> +    if (kvm_enabled()) {
>>> +        /*
>>> +         * Default MTE to off, as long as migration support is not
>>> +         * yet implemented.
>>> +         * TODO: implement migration support for kvm
>>> +         */
>>> +        cpu->prop_mte = false;
>>> +    }
>>> +}
>>> +
>>> +void arm_cpu_mte_finalize(ARMCPU *cpu, Error **errp)
>>> +{
>>> +    if (!cpu->prop_mte) {
>>> +        /* Disable MTE feature bits. */
>>> +        cpu->isar.id_aa64pfr1 =
>>> +            FIELD_DP64(cpu->isar.id_aa64pfr1, ID_AA64PFR1, MTE, 0);
>>> +        return;
>>> +    }
>>> +#ifndef CONFIG_USER_ONLY
>>> +    if (!kvm_enabled()) {
>>> +        if (cpu_isar_feature(aa64_mte, cpu) && !cpu->tag_memory) {
>>> +            /*
>>> +             * Disable the MTE feature bits, unless we have tag-memory
>>> +             * provided by the machine.
>>> +             * This silent downgrade is not really nice if the user had
>>> +             * explicitly requested MTE to be enabled by the cpu, but it
>>> +             * preserves pre-existing behaviour. In an ideal world, we
>>
>>
>> Can't we "simply" prevent the end-user from using the prop_mte option
>> with a TCG CPU? and have something like
>>
>> For TCG, MTE depends on the CPU feature availability + machine tag memory
>> For KVM, MTE depends on the user opt-in + CPU feature avail (if
>> relevant) + host VM capability (?)
> 
> I don't like kvm and tcg cpus behaving too differently... but then, tcg
> is already different as it needs tag_memory.
> 
> Thinking about it, maybe we could repurpose tag_memory in the kvm case
> (e.g. for a temporary buffer for migration purposes) and require it in
> all cases (making kvm fail if the user specified tag memory, but the
> host doesn't support it). A cpu feature still looks more natural to me,
> but I'm not yet quite used to how things are done in arm :)
If the tag memory is an implementation "detail" for TCG then I agree
with you that a CPU property would be more adapted for KVM.
> 
> The big elefant in the room is how migration will end up
> working... after reading the disscussions in
> https://lore.kernel.org/all/CAJc+Z1FZxSYB_zJit4+0uTR-88VqQL+-01XNMSEfua-dXDy6Wg@mail.gmail.com/
> I don't think it will be as "easy" as I thought, and we probably require
> some further fiddling on the kernel side.
Yes maybe the MTE migration process shall be documented and discussed
separately on the ML? Is Haibu Xu's address bouncing?

Eric
> 
>>
>> But maybe I miss the point ...
>>> +             * would fail if MTE was requested, but no tag memory has
>>> +             * been provided.
>>> +             */
>>> +            cpu->isar.id_aa64pfr1 =
>>> +                FIELD_DP64(cpu->isar.id_aa64pfr1, ID_AA64PFR1, MTE, 0);
>>> +        }
>>> +        if (!cpu_isar_feature(aa64_mte, cpu)) {
>>> +            cpu->prop_mte = false;
>>> +        }
>>> +        return;
>>> +    }
>>> +    if (kvm_arm_mte_supported()) {
>>> +#ifdef CONFIG_KVM
>>> +        if (kvm_vm_enable_cap(kvm_state, KVM_CAP_ARM_MTE, 0)) {
>>> +            error_setg(errp, "Failed to enable KVM_CAP_ARM_MTE");
>>> +        } else {
>>> +            /* TODO: add proper migration support with MTE enabled */
>>> +            if (!mte_migration_blocker) {
>>> +                error_setg(&mte_migration_blocker,
>>> +                           "Live migration disabled due to MTE enabled");
>>> +                if (migrate_add_blocker(mte_migration_blocker, NULL)) {
>> error_free(mte_migration_blocker);
>> mte_migration_blocker = NULL;
> 
> Ah, I missed that, thanks.
> 
>>> +                    error_setg(errp, "Failed to add MTE migration 
>>> blocker");
>>> +                }
>>> +            }
>>> +        }
>>> +#endif
>>> +    }
>>> +    /* When HVF provides support for MTE, add it here */
>>> +#endif
>>> +}
>>> +
>