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

[Eric Auger <eauger@redhat.com>]

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.
> 
> 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.
>              return;
>          }
>      }
> @@ -1882,17 +1887,6 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
>                                         ID_PFR1, VIRTUALIZATION, 0);
>      }
>  
> -#ifndef CONFIG_USER_ONLY
> -    if (cpu->tag_memory == NULL && cpu_isar_feature(aa64_mte, cpu)) {
> -        /*
> -         * Disable the MTE feature bits if we do not have tag-memory
> -         * provided by the machine.
> -         */
> -        cpu->isar.id_aa64pfr1 =
> -            FIELD_DP64(cpu->isar.id_aa64pfr1, ID_AA64PFR1, MTE, 0);
> -    }
> -#endif
> -
>      /* MPU can be configured out of a PMSA CPU either by setting has-mpu
>       * to false or by setting pmsav7-dregion to 0.
>       */
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index 18ca61e8e25b..183506713e96 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -208,11 +208,13 @@ typedef struct {
>  void arm_cpu_sve_finalize(ARMCPU *cpu, Error **errp);
>  void arm_cpu_pauth_finalize(ARMCPU *cpu, Error **errp);
>  void arm_cpu_lpa2_finalize(ARMCPU *cpu, Error **errp);
> +void arm_cpu_mte_finalize(ARMCPU *cpu, Error **errp);
>  #else
>  # define ARM_MAX_VQ    1
>  static inline void arm_cpu_sve_finalize(ARMCPU *cpu, Error **errp) { }
>  static inline void arm_cpu_pauth_finalize(ARMCPU *cpu, Error **errp) { }
>  static inline void arm_cpu_lpa2_finalize(ARMCPU *cpu, Error **errp) { }
> +static inline void arm_cpu_mte_finalize(ARMCPU *cpu, Error **errp) { }
>  #endif
>  
>  typedef struct ARMVectorReg {
> @@ -993,6 +995,7 @@ struct ArchCPU {
>      bool prop_pauth;
>      bool prop_pauth_impdef;
>      bool prop_lpa2;
> +    bool prop_mte;
>  
>      /* DCZ blocksize, in log_2(words), ie low 4 bits of DCZID_EL0 */
>      uint32_t dcz_blocksize;
> @@ -1091,6 +1094,7 @@ void aarch64_sve_change_el(CPUARMState *env, int old_el,
>                             int new_el, bool el0_a64);
>  void aarch64_add_sve_properties(Object *obj);
>  void aarch64_add_pauth_properties(Object *obj);
> +void aarch64_add_mte_properties(Object *obj);
>  
>  /*
>   * SVE registers are encoded in KVM's memory in an endianness-invariant format.
> diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
> index 04427e073f17..eea9ad195470 100644
> --- a/target/arm/cpu64.c
> +++ b/target/arm/cpu64.c
> @@ -35,7 +35,11 @@
>  #include "qapi/visitor.h"
>  #include "hw/qdev-properties.h"
>  #include "internals.h"
> +#include "migration/blocker.h"
>  
> +#ifdef CONFIG_KVM
> +static Error *mte_migration_blocker;
> +#endif
>  
>  static void aarch64_a57_initfn(Object *obj)
>  {
> @@ -785,6 +789,78 @@ void arm_cpu_lpa2_finalize(ARMCPU *cpu, Error **errp)
>      cpu->isar.id_aa64mmfr0 = t;
>  }
>  
> +static Property arm_cpu_mte_property =
> +    DEFINE_PROP_BOOL("mte", ARMCPU, prop_mte, true);
> +
> +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?
> +     * 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 (?)

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;
> +                    error_setg(errp, "Failed to add MTE migration blocker");
> +                }
> +            }
> +        }
> +#endif
> +    }
> +    /* When HVF provides support for MTE, add it here */
> +#endif
> +}
> +
>  static void aarch64_host_initfn(Object *obj)
>  {
>  #if defined(CONFIG_KVM)
> @@ -793,6 +869,7 @@ static void aarch64_host_initfn(Object *obj)
>      if (arm_feature(&cpu->env, ARM_FEATURE_AARCH64)) {
>          aarch64_add_sve_properties(obj);
>          aarch64_add_pauth_properties(obj);
> +        aarch64_add_mte_properties(obj);
>      }
>  #elif defined(CONFIG_HVF)
>      ARMCPU *cpu = ARM_CPU(obj);
> @@ -958,6 +1035,7 @@ static void aarch64_max_initfn(Object *obj)
>      object_property_add(obj, "sve-max-vq", "uint32", cpu_max_get_sve_max_vq,
>                          cpu_max_set_sve_max_vq, NULL, NULL);
>      qdev_property_add_static(DEVICE(obj), &arm_cpu_lpa2_property);
> +    aarch64_add_mte_properties(obj);
>  }
>  
>  static void aarch64_a64fx_initfn(Object *obj)
> diff --git a/target/arm/kvm64.c b/target/arm/kvm64.c
> index b8cfaf5782ac..d129a264a3f6 100644
> --- a/target/arm/kvm64.c
> +++ b/target/arm/kvm64.c
> @@ -746,6 +746,11 @@ bool kvm_arm_steal_time_supported(void)
>      return kvm_check_extension(kvm_state, KVM_CAP_STEAL_TIME);
>  }
>  
> +bool kvm_arm_mte_supported(void)
> +{
> +    return kvm_check_extension(kvm_state, KVM_CAP_ARM_MTE);
> +}
> +
>  QEMU_BUILD_BUG_ON(KVM_ARM64_SVE_VQ_MIN != 1);
>  
>  void kvm_arm_sve_get_vls(CPUState *cs, unsigned long *map)
> diff --git a/target/arm/kvm_arm.h b/target/arm/kvm_arm.h
> index b7f78b521545..13f06ed5e0ea 100644
> --- a/target/arm/kvm_arm.h
> +++ b/target/arm/kvm_arm.h
> @@ -306,6 +306,13 @@ bool kvm_arm_pmu_supported(void);
>   */
>  bool kvm_arm_sve_supported(void);
>  
> +/**
> + * kvm_arm_mte_supported:
> + *
> + * Returns: true if KVM can enable MTE, and false otherwise.
> + */
> +bool kvm_arm_mte_supported(void);
> +
>  /**
>   * kvm_arm_get_max_vm_ipa_size:
>   * @ms: Machine state handle
> @@ -396,6 +403,11 @@ static inline bool kvm_arm_steal_time_supported(void)
>      return false;
>  }
>  
> +static inline bool kvm_arm_mte_supported(void)
> +{
> +    return false;
> +}
> +
>  /*
>   * These functions should never actually be called without KVM support.
>   */
> diff --git a/target/arm/monitor.c b/target/arm/monitor.c
> index 80c64fa3556d..f13ff2664b67 100644
> --- a/target/arm/monitor.c
> +++ b/target/arm/monitor.c
> @@ -96,6 +96,7 @@ static const char *cpu_model_advertised_features[] = {
>      "sve1408", "sve1536", "sve1664", "sve1792", "sve1920", "sve2048",
>      "kvm-no-adjvtime", "kvm-steal-time",
>      "pauth", "pauth-impdef",
> +    "mte",
>      NULL
>  };
>  
Eric