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

[Cornelia Huck <cohuck@redhat.com>]

On Fri, Aug 02 2024, Ganapatrao Kulkarni <gankulkarni@os.amperecomputing.com> wrote:

> Hi Peter,
>
> On 16-07-2024 09:15 pm, Peter Maydell wrote:
>> On Tue, 9 Jul 2024 at 07:05, Ganapatrao Kulkarni
>> <gankulkarni@os.amperecomputing.com> wrote:
>>>
>>> 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.
>>>
>>> Signed-off-by: Cornelia Huck <cohuck@redhat.com>
>>> Signed-off-by: Ganapatrao Kulkarni <gankulkarni@os.amperecomputing.com>
>>> ---
>> 
>> In target/arm/cpu.c:arm_cpu_realizefn() there is this code:
>> 
>>      if (cpu_isar_feature(aa64_mte, cpu)) {
>>          /*
>>           * The architectural range of GM blocksize is 2-6, however qemu
>>           * doesn't support blocksize of 2 (see HELPER(ldgm)).
>>           */
>>          if (tcg_enabled()) {
>>              assert(cpu->gm_blocksize >= 3 && cpu->gm_blocksize <= 6);
>>          }
>> 
>> #ifndef CONFIG_USER_ONLY
>>          /*
>>           * If we do not have tag-memory provided by the machine,
>>           * reduce MTE support to instructions enabled at EL0.
>>           * This matches Cortex-A710 BROADCASTMTE input being LOW.
>>           */
>>          if (cpu->tag_memory == NULL) {
>>              cpu->isar.id_aa64pfr1 =
>>                  FIELD_DP64(cpu->isar.id_aa64pfr1, ID_AA64PFR1, MTE, 1);
>>          }
>> #endif
>>      }
>> 
>> With this patch, for KVM we will end up going through the
>> "squash ID_AA64PFR1_EL1.MTE to 1" codepath, because KVM doesn't
>> set cpu->tag_memory and this is still using that as its check.
>> 
>> More generally, how does the enabling of the MTE KVM cap
>> interact with the ID_AA64PFR1_EL1 value that we read from
>> the host in kvm_arm_get_host_cpu_features() ? We care that we
>> have the right ID register values because we use ID field
>> checks to determine whether the vcpu has a feature or not,
>> even in the KVM case.
>
> Using ID_AA64PFR1_EL1.MTE bits seems to have issues with the Linux 
> kernel implementation.
> This register is sensitized to user space and value read differs time to 
> time. ID_AA64PFR1_EL1 register read at the beginning of qemu code will 
> have the MTE bits cleared/masked. However ID_AA64PFR1_EL1.MTE bits are 
> unmasked and shows the real value of MTE after ioctl KVM_CAP_ARM_MTE is 
> executed to enable MTE.
> In QEMU,. isar.id_aa64pfr1 is read at the very beginning and cached, by 
> creating dummy(kvm_arm_create_scratch_host_vcpu) interfaces(fds) for 
> kvm, vm and vcpu. At later stages use of isar.id_aa64pfr1.mte at 
> function like arm_cpu_realizefn does not show the right value and code 
> never enters the "if (cpu_isar_feature(aa64_mte, cpu)" loop.
>
> Not sure about other feature bits, but for MTE, using isar.id_aa64pfr1 
> may not be appropriate but I do see it is getting used already many 
> places of the code. I am not sure how it is behaving on emulator/TCG mode?
>
> Having said that, I have tried to remove the sensitization of 
> ID_AA64PFR1_EL1 for MTE bits in the kernel, but that will have bigger 
> problem like the VM boot crashes with the default compiled qemu binary 
> with normal VM boot itself, since VM detects as MTE present and starts 
> init of it.

Not sure if you've seen
https://lore.kernel.org/all/20240723072004.1470688-1-shahuang@redhat.com/,
which aims to make aa64pfr1 writable by userspace -- but still needs
special handling for MTE. (That series also masks out MTEX and MTE_frac
because the Linux kernel does not yet handle them; might become relevant
if we want to support it from QEMU at some time in the future, but then
the command line would also need some thought.)

I fear that we'll be stuck with special handling for the MTE bits in
aa64pfr1 because the kernel will not want to break its userspace API.