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

[Gustavo Romero]

Hi Cornelia and Ganapatrao,

On 9/17/24 11:13, Cornelia Huck wrote:
> On Thu, Sep 12 2024, 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>
> > ---
> > 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     |  7 +++--
> >   target/arm/cpu.h     |  2 ++
> >   target/arm/kvm.c     | 59 ++++++++++++++++++++++++++++++++++++
> >   target/arm/kvm_arm.h | 19 ++++++++++++
> >   5 files changed, 126 insertions(+), 33 deletions(-)
> (...)
>
> > diff --git a/target/arm/kvm.c b/target/arm/kvm.c
> > index 849e2e21b3..29865609fb 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,21 @@ bool kvm_arm_create_scratch_host_vcpu(const uint32_t 
> > *cpus_to_try,
> >       if (vmfd < 0) {
> >           goto err;
> >       }
> > +
> > +    /*
> > +     * MTE bits of register id_aa64pfr1 are masked if MTE is
> > +     * not enabled and required to enable before VCPU
> > +     * is created. Hence enable MTE(if supported) before VCPU
> > +     * is created to read unmasked MTE bits.
> > +     */
> Maybe
>
> "KVM will mask the MTE bits in id_aa64pfr1 unless the VMM has enabled
> the MTE KVM capability, so do it here for probing."
>
> ?

KVM_CAP_ARM_MTE must be set before creating the VCPUs, as

stated in the KVM API docs, so enabling this cap. here for later

probing the MTE bits correctly feels more like a consequence. So

I prefer the the original comment, which mentions that

requirement. But I think something shorter like the following

would work too:

"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."


Cheers,

Gustavo


> > +    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;