Re: [PATCH V3] arm/kvm: add support for MTE

[Gustavo Romero <gustavo.romero@linaro.org>]

Hi Cornelia and Ganapatrao,

On 9/25/24 14:54, Cornelia Huck wrote:
> On Fri, Sep 20 2024, Ganapatrao Kulkarni <gankulkarni@os.amperecomputing.com> wrote:
>
> Mostly nit-picking below, otherwise LGTM.
>
>> Extend the 'mte' property for the virt machine to cover KVM as
>> well. For KVM, we don't allocate tag memory, but instead enable
>> the capability.
>>
>> If MTE has been enabled, we need to disable migration, as we do not
>> yet have a way to migrate the tags as well. Therefore, MTE will stay
>> off with KVM unless requested explicitly.
>>
>> This patch is rework of commit b320e21c48ce64853904bea6631c0158cc2ef227
>> which broke TCG since it made the TCG -cpu max
>> report the presence of MTE to the guest even if the board hadn't
>> enabled MTE by wiring up the tag RAM. This meant that if the guest
>> then tried to use MTE QEMU would segfault accessing the
>> non-existent tag RAM.
> I think the more canonical way to express this would be
>
> [$AUTHOR: reworked original patch by doing X to avoid problem Y]
>
>> Signed-off-by: Cornelia Huck <cohuck@redhat.com>
>> Signed-off-by: Ganapatrao Kulkarni <gankulkarni@os.amperecomputing.com>
> Also, the S-o-b chain is a bit confusing that way, because you are
> listed as author of the patch, but I'm in the chain in front of you -- I
> think I should still be listed as the author?
>
>> ---
>>
>> Changes since V2:
>> 	Updated with review comments.
>>
>> Changes since V1:
>> 	Added code to enable MTE before reading register
>> id_aa64pfr1 (unmasked MTE bits).
>>
>> This patch is boot tested on ARM64 with KVM and on X86 with TCG for mte=on
>> and default case(i.e, no mte).
>>
>>   hw/arm/virt.c        | 72 ++++++++++++++++++++++++++------------------
>>   target/arm/cpu.c     | 11 +++++--
>>   target/arm/cpu.h     |  2 ++
>>   target/arm/kvm.c     | 57 +++++++++++++++++++++++++++++++++++
>>   target/arm/kvm_arm.h | 19 ++++++++++++
>>   5 files changed, 129 insertions(+), 32 deletions(-)
>>
> (...)
>
>> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
>> index 19191c2391..8a2fc471ce 100644
>> --- a/target/arm/cpu.c
>> +++ b/target/arm/cpu.c
>> @@ -2390,14 +2390,21 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
>>   
>>   #ifndef CONFIG_USER_ONLY
>>           /*
>> -         * If we do not have tag-memory provided by the machine,
>> +         * If we do not have tag-memory provided by the TCG,
> Maybe
>
> "If we run with TCG and do not have tag-memory provided by the machine"
>
> ?

Yep, I agree, this is better.


>>            * reduce MTE support to instructions enabled at EL0.
>>            * This matches Cortex-A710 BROADCASTMTE input being LOW.
>>            */
>> -        if (cpu->tag_memory == NULL) {
>> +        if (tcg_enabled() && cpu->tag_memory == NULL) {
>>               cpu->isar.id_aa64pfr1 =
>>                   FIELD_DP64(cpu->isar.id_aa64pfr1, ID_AA64PFR1, MTE, 1);
>>           }
>> +
>> +        /*
>> +         * Clear MTE bits, if not enabled in KVM mode.
> Maybe add "This matches the MTE bits being masked by KVM in that case."?

Clearing the MTE bits is also necessary when MTE is supported by the
host (and so KVM can enable the MTE capability - so won't mask the MTE
bits, but the user didn't want MTE enabled in the guest (mte=on no given
or explicitly set to =off), so this comment is not always true?

How about something like:

"If MTE is supported by the host but could not be enabled on KVM mode or
MTE should not be enabled on the guest (e.i. mte=off), clear guest's MTE 
bits."

I do assume MTE is supported by the host (i.e. MTE bits >= 2 in the host)
because otherwise condition "if (cpu_isar_feature(aa64_mte, cpu)) { ... 
}" is not
taken; and at this point cpu->isar->id_aa64pfr1 is set from the host's 
bits via
kvm_arm_set_cpu_features_from_host() and kvm_arm_get_host_cpu_features ().


>> +         */
>> +        if (kvm_enabled() && !cpu->kvm_mte) {
>> +                FIELD_DP64(cpu->isar.id_aa64pfr1, ID_AA64PFR1, MTE, 0);
>> +        }
>>   #endif
>>       }
>>   
> (...)
>
>> diff --git a/target/arm/kvm.c b/target/arm/kvm.c
>> index 849e2e21b3..af7a98517d 100644
>> --- a/target/arm/kvm.c
>> +++ b/target/arm/kvm.c
>> @@ -39,6 +39,7 @@
>>   #include "hw/acpi/acpi.h"
>>   #include "hw/acpi/ghes.h"
>>   #include "target/arm/gtimer.h"
>> +#include "migration/blocker.h"
>>   
>>   const KVMCapabilityInfo kvm_arch_required_capabilities[] = {
>>       KVM_CAP_LAST_INFO
>> @@ -119,6 +120,20 @@ bool kvm_arm_create_scratch_host_vcpu(const uint32_t *cpus_to_try,
>>       if (vmfd < 0) {
>>           goto err;
>>       }
>> +
>> +    /*
>> +     * MTE capability must be enabled by the VMM before creating
>> +     * any VCPUs. The MTE bits of the ID_AA64PFR1 register are masked
>> +     * if MTE is not enabled, allowing them to be probed correctly.
> This reads a bit confusing. Maybe
>
> "The MTE capability must be enabled by the VMM before creating any VCPUs
> in order to allow the MTE bits of the ID_AA64PFR1 register to be probed
> correctly, as they are masked if MTE is not enabled."

I agree.


>
>> +     */
>> +    if (kvm_arm_mte_supported()) {
>> +        KVMState kvm_state;
>> +
>> +        kvm_state.fd = kvmfd;
>> +        kvm_state.vmfd = vmfd;
>> +        kvm_vm_enable_cap(&kvm_state, KVM_CAP_ARM_MTE, 0);
>> +    }
>> +
>>       cpufd = ioctl(vmfd, KVM_CREATE_VCPU, 0);
>>       if (cpufd < 0) {
>>           goto err;
> (...)
>

Comments aside:

Reviewed-by: Gustavo Romero <gustavo.romero@linaro.org>