[PATCH RFC 0/2] arm: enable MTE for QEMU + kvm

[PATCH RFC 0/2] arm: enable MTE for QEMU + kvm

[Cornelia Huck]

This series enables MTE for kvm guests, if the kernel supports it.
Lightly tested while running under the simulator (the arm64/mte/
kselftests pass... if you wait patiently :)

A new cpu property "mte" (defaulting to on if possible) is introduced;
for tcg, you still need to enable mte at the machine as well.

I've hacked up some very basic qtests; not entirely sure if I'm going
about it the right way.

Some things to look out for:
- Migration is not (yet) supported. I added a migration blocker if we
  enable mte in the kvm case. AFAIK, there isn't any hardware available
  yet that allows mte + kvm to be used (I think the latest Gravitons
  implement mte, but no bare metal instances seem to be available), so
  that should not have any impact on real world usage.
- I'm not at all sure about the interaction between the virt machine 'mte'
  prop and the cpu 'mte' prop. To keep things working with tcg as before,
  a not-specified mte for the cpu should simply give us a guest without
  mte if it wasn't specified for the machine. However, mte on the cpu
  without mte on the machine should probably generate an error, but I'm not
  sure how to detect that without breaking the silent downgrade to preserve
  existing behaviour.
- As I'm still new to arm, please don't assume that I know what I'm doing :)


Cornelia Huck (2):
  arm/kvm: enable MTE if available
  qtests/arm: add some mte tests

 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 +
 tests/qtest/arm-cpu-features.c | 31 ++++++++++++++
 7 files changed, 137 insertions(+), 12 deletions(-)

-- 
2.34.3

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

[Cornelia Huck]

We need to disable migration, as we do not yet have a way to migrate
the tags as well.

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");
             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.
+     * 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
+             * 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_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
 };
 
-- 
2.34.3

[PATCH RFC 2/2] qtests/arm: add some mte tests

[Cornelia Huck]

Signed-off-by: Cornelia Huck <cohuck@redhat.com>
---
 tests/qtest/arm-cpu-features.c | 31 +++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

diff --git a/tests/qtest/arm-cpu-features.c b/tests/qtest/arm-cpu-features.c
index 5a145273860c..c0be645b1fb0 100644
--- a/tests/qtest/arm-cpu-features.c
+++ b/tests/qtest/arm-cpu-features.c
@@ -22,6 +22,7 @@
 
 #define MACHINE     "-machine virt,gic-version=max -accel tcg "
 #define MACHINE_KVM "-machine virt,gic-version=max -accel kvm -accel tcg "
+#define MACHINE_MTE "-machine virt,gic-version=max,mte=on -accel tcg "
 #define QUERY_HEAD  "{ 'execute': 'query-cpu-model-expansion', " \
                     "  'arguments': { 'type': 'full', "
 #define QUERY_TAIL  "}}"
@@ -412,6 +413,17 @@ static void sve_tests_sve_off_kvm(const void *data)
     qtest_quit(qts);
 }
 
+static void mte_tests_tag_memory_on(const void *data)
+{
+    QTestState *qts;
+
+    qts = qtest_init(MACHINE_MTE "-cpu max");
+
+    assert_has_feature(qts, "max", "mte");
+
+    qtest_quit(qts);
+}
+
 static void pauth_tests_default(QTestState *qts, const char *cpu_type)
 {
     assert_has_feature_enabled(qts, cpu_type, "pauth");
@@ -424,6 +436,14 @@ static void pauth_tests_default(QTestState *qts, const char *cpu_type)
                  "{ 'pauth': false, 'pauth-impdef': true }");
 }
 
+static void mte_tests_default(QTestState *qts, const char *cpu_type)
+{
+    assert_has_feature(qts, cpu_type, "mte");
+
+    /* without tag memory, mte will be off under tcg */
+    assert_has_feature_disabled(qts, cpu_type, "mte");
+}
+
 static void test_query_cpu_model_expansion(const void *data)
 {
     QTestState *qts;
@@ -473,6 +493,7 @@ static void test_query_cpu_model_expansion(const void *data)
 
         sve_tests_default(qts, "max");
         pauth_tests_default(qts, "max");
+        mte_tests_default(qts, "max");
 
         /* Test that features that depend on KVM generate errors without. */
         assert_error(qts, "max",
@@ -499,6 +520,7 @@ static void test_query_cpu_model_expansion_kvm(const void *data)
     if (g_str_equal(qtest_get_arch(), "aarch64")) {
         bool kvm_supports_steal_time;
         bool kvm_supports_sve;
+        bool kvm_supports_mte;
         char max_name[8], name[8];
         uint32_t max_vq, vq;
         uint64_t vls;
@@ -523,10 +545,12 @@ static void test_query_cpu_model_expansion_kvm(const void *data)
          */
         assert_has_feature(qts, "host", "kvm-steal-time");
         assert_has_feature(qts, "host", "sve");
+        assert_has_feature(qts, "host", "mte");
 
         resp = do_query_no_props(qts, "host");
         kvm_supports_steal_time = resp_get_feature(resp, "kvm-steal-time");
         kvm_supports_sve = resp_get_feature(resp, "sve");
+        kvm_supports_mte = resp_get_feature(resp, "mte");
         vls = resp_get_sve_vls(resp);
         qobject_unref(resp);
 
@@ -592,6 +616,11 @@ static void test_query_cpu_model_expansion_kvm(const void *data)
         } else {
             g_assert(vls == 0);
         }
+        if (kvm_supports_mte) {
+            /* If we have mte then we should be able to toggle it. */
+            assert_set_feature(qts, "host", "mte", false);
+            assert_set_feature(qts, "host", "mte", true);
+        }
     } else {
         assert_has_not_feature(qts, "host", "aarch64");
         assert_has_not_feature(qts, "host", "pmu");
@@ -630,6 +659,8 @@ int main(int argc, char **argv)
                             NULL, sve_tests_sve_off);
         qtest_add_data_func("/arm/kvm/query-cpu-model-expansion/sve-off",
                             NULL, sve_tests_sve_off_kvm);
+        qtest_add_data_func("/arm/max/query-cpu-model-expansion/tag-memory",
+                            NULL, mte_tests_tag_memory_on);
     }
 
     return g_test_run();
-- 
2.34.3

Re: [PATCH RFC 0/2] arm: enable MTE for QEMU + kvm

[Cornelia Huck]

Friendly ping :)

On Thu, May 12 2022, Cornelia Huck <cohuck@redhat.com> wrote:

> This series enables MTE for kvm guests, if the kernel supports it.
> Lightly tested while running under the simulator (the arm64/mte/
> kselftests pass... if you wait patiently :)
>
> A new cpu property "mte" (defaulting to on if possible) is introduced;
> for tcg, you still need to enable mte at the machine as well.
>
> I've hacked up some very basic qtests; not entirely sure if I'm going
> about it the right way.
>
> Some things to look out for:
> - Migration is not (yet) supported. I added a migration blocker if we
>   enable mte in the kvm case. AFAIK, there isn't any hardware available
>   yet that allows mte + kvm to be used (I think the latest Gravitons
>   implement mte, but no bare metal instances seem to be available), so
>   that should not have any impact on real world usage.
> - I'm not at all sure about the interaction between the virt machine 'mte'
>   prop and the cpu 'mte' prop. To keep things working with tcg as before,
>   a not-specified mte for the cpu should simply give us a guest without
>   mte if it wasn't specified for the machine. However, mte on the cpu
>   without mte on the machine should probably generate an error, but I'm not
>   sure how to detect that without breaking the silent downgrade to preserve
>   existing behaviour.
> - As I'm still new to arm, please don't assume that I know what I'm doing :)
>
>
> Cornelia Huck (2):
>   arm/kvm: enable MTE if available
>   qtests/arm: add some mte tests
>
>  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 +
>  tests/qtest/arm-cpu-features.c | 31 ++++++++++++++
>  7 files changed, 137 insertions(+), 12 deletions(-)
>
> -- 
> 2.34.3

Re: [PATCH RFC 0/2] arm: enable MTE for QEMU + kvm

[Cornelia Huck]

On Thu, May 12 2022, Cornelia Huck <cohuck@redhat.com> wrote:

> This series enables MTE for kvm guests, if the kernel supports it.
> Lightly tested while running under the simulator (the arm64/mte/
> kselftests pass... if you wait patiently :)
>
> A new cpu property "mte" (defaulting to on if possible) is introduced;
> for tcg, you still need to enable mte at the machine as well.
>
> I've hacked up some very basic qtests; not entirely sure if I'm going
> about it the right way.
>
> Some things to look out for:
> - Migration is not (yet) supported. I added a migration blocker if we
>   enable mte in the kvm case. AFAIK, there isn't any hardware available
>   yet that allows mte + kvm to be used (I think the latest Gravitons
>   implement mte, but no bare metal instances seem to be available), so
>   that should not have any impact on real world usage.

...so it seems there was a series some time ago related to MTE +
migration, which my initial search somehow managed to avoid.

https://lore.kernel.org/all/CAJc+Z1FZxSYB_zJit4+0uTR-88VqQL+-01XNMSEfua-dXDy6Wg@mail.gmail.com/
talks about some kernel changes needed in order to support postcopy; has
there been any update, as my search fu might be failing me again?

> - I'm not at all sure about the interaction between the virt machine 'mte'
>   prop and the cpu 'mte' prop. To keep things working with tcg as before,
>   a not-specified mte for the cpu should simply give us a guest without
>   mte if it wasn't specified for the machine. However, mte on the cpu
>   without mte on the machine should probably generate an error, but I'm not
>   sure how to detect that without breaking the silent downgrade to preserve
>   existing behaviour.
> - As I'm still new to arm, please don't assume that I know what I'm doing :)
>
>
> Cornelia Huck (2):
>   arm/kvm: enable MTE if available
>   qtests/arm: add some mte tests
>
>  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 +
>  tests/qtest/arm-cpu-features.c | 31 ++++++++++++++
>  7 files changed, 137 insertions(+), 12 deletions(-)

Re: [PATCH RFC 0/2] arm: enable MTE for QEMU + kvm

[Eric Auger]

Hi Connie,

On 5/12/22 15:11, Cornelia Huck wrote:
> This series enables MTE for kvm guests, if the kernel supports it.
> Lightly tested while running under the simulator (the arm64/mte/
> kselftests pass... if you wait patiently :)
> 
> A new cpu property "mte" (defaulting to on if possible) is introduced;
> for tcg, you still need to enable mte at the machine as well.
isn't the property set to off by default when kvm is enabled (because of
the migration blocker).

Eric
> 
> I've hacked up some very basic qtests; not entirely sure if I'm going
> about it the right way.
> 
> Some things to look out for:
> - Migration is not (yet) supported. I added a migration blocker if we
>   enable mte in the kvm case. AFAIK, there isn't any hardware available
>   yet that allows mte + kvm to be used (I think the latest Gravitons
>   implement mte, but no bare metal instances seem to be available), so
>   that should not have any impact on real world usage.
> - I'm not at all sure about the interaction between the virt machine 'mte'
>   prop and the cpu 'mte' prop. To keep things working with tcg as before,
>   a not-specified mte for the cpu should simply give us a guest without
>   mte if it wasn't specified for the machine. However, mte on the cpu
>   without mte on the machine should probably generate an error, but I'm not
>   sure how to detect that without breaking the silent downgrade to preserve
>   existing behaviour.
> - As I'm still new to arm, please don't assume that I know what I'm doing :)
> 
> 
> Cornelia Huck (2):
>   arm/kvm: enable MTE if available
>   qtests/arm: add some mte tests
> 
>  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 +
>  tests/qtest/arm-cpu-features.c | 31 ++++++++++++++
>  7 files changed, 137 insertions(+), 12 deletions(-)
> 

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

[Eric Auger]

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

Re: [PATCH RFC 0/2] arm: enable MTE for QEMU + kvm

[Cornelia Huck]

On Fri, Jun 10 2022, Eric Auger <eauger@redhat.com> wrote:

> Hi Connie,
>
> On 5/12/22 15:11, Cornelia Huck wrote:
>> This series enables MTE for kvm guests, if the kernel supports it.
>> Lightly tested while running under the simulator (the arm64/mte/
>> kselftests pass... if you wait patiently :)
>> 
>> A new cpu property "mte" (defaulting to on if possible) is introduced;
>> for tcg, you still need to enable mte at the machine as well.
> isn't the property set to off by default when kvm is enabled (because of
> the migration blocker).

Oh, I had changed that around several times, and it seems I ended up
being confused when I wrote this cover letter... I wonder what the best
state would be (assuming that I don't manage to implement it soonish,
but it seems we still would need kernel changes as by the discussion in
that other patch series.)

>
> Eric
>> 
>> I've hacked up some very basic qtests; not entirely sure if I'm going
>> about it the right way.
>> 
>> Some things to look out for:
>> - Migration is not (yet) supported. I added a migration blocker if we
>>   enable mte in the kvm case. AFAIK, there isn't any hardware available
>>   yet that allows mte + kvm to be used (I think the latest Gravitons
>>   implement mte, but no bare metal instances seem to be available), so
>>   that should not have any impact on real world usage.
>> - I'm not at all sure about the interaction between the virt machine 'mte'
>>   prop and the cpu 'mte' prop. To keep things working with tcg as before,
>>   a not-specified mte for the cpu should simply give us a guest without
>>   mte if it wasn't specified for the machine. However, mte on the cpu
>>   without mte on the machine should probably generate an error, but I'm not
>>   sure how to detect that without breaking the silent downgrade to preserve
>>   existing behaviour.
>> - As I'm still new to arm, please don't assume that I know what I'm doing :)
>> 
>> 
>> Cornelia Huck (2):
>>   arm/kvm: enable MTE if available
>>   qtests/arm: add some mte tests
>> 
>>  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 +
>>  tests/qtest/arm-cpu-features.c | 31 ++++++++++++++
>>  7 files changed, 137 insertions(+), 12 deletions(-)
>> 

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

[Cornelia Huck]

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.

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

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.

>> +     * 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 :)

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.

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

Re: [PATCH RFC 0/2] arm: enable MTE for QEMU + kvm

[Eric Auger]

Hi Connie,

On 6/13/22 18:02, 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:
>>> This series enables MTE for kvm guests, if the kernel supports it.
>>> Lightly tested while running under the simulator (the arm64/mte/
>>> kselftests pass... if you wait patiently :)
>>>
>>> A new cpu property "mte" (defaulting to on if possible) is introduced;
>>> for tcg, you still need to enable mte at the machine as well.
>> isn't the property set to off by default when kvm is enabled (because of
>> the migration blocker).
> 
> Oh, I had changed that around several times, and it seems I ended up
> being confused when I wrote this cover letter... I wonder what the best
> state would be (assuming that I don't manage to implement it soonish,
> but it seems we still would need kernel changes as by the discussion in
> that other patch series.)
Having mte=off by default along with KVM, until the migration gets
supported, looks OK to me. Does it prevent you from having it set to
another value by default with TCG (depending on the virt machine
tag_memory option)?

		tag_memory=on	tag_memory=off
KVM CPU mte=off	invalid		mte=off
KVM CPU mte=on	invalid		mte=on
TCG CPU mte=off	invalid		mte=off
TCG CPU mte=on	mte=on		invalid

default value:
KVM mte = off until migration gets supported
TCG mte = machine.tag_memory

Thanks

Eric

> 
>>
>> Eric
>>>
>>> I've hacked up some very basic qtests; not entirely sure if I'm going
>>> about it the right way.
>>>
>>> Some things to look out for:
>>> - Migration is not (yet) supported. I added a migration blocker if we
>>>   enable mte in the kvm case. AFAIK, there isn't any hardware available
>>>   yet that allows mte + kvm to be used (I think the latest Gravitons
>>>   implement mte, but no bare metal instances seem to be available), so
>>>   that should not have any impact on real world usage.
>>> - I'm not at all sure about the interaction between the virt machine 'mte'
>>>   prop and the cpu 'mte' prop. To keep things working with tcg as before,
>>>   a not-specified mte for the cpu should simply give us a guest without
>>>   mte if it wasn't specified for the machine. However, mte on the cpu
>>>   without mte on the machine should probably generate an error, but I'm not
>>>   sure how to detect that without breaking the silent downgrade to preserve
>>>   existing behaviour.
>>> - As I'm still new to arm, please don't assume that I know what I'm doing :)
>>>
>>>
>>> Cornelia Huck (2):
>>>   arm/kvm: enable MTE if available
>>>   qtests/arm: add some mte tests
>>>
>>>  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 +
>>>  tests/qtest/arm-cpu-features.c | 31 ++++++++++++++
>>>  7 files changed, 137 insertions(+), 12 deletions(-)
>>>
> 

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

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

[Cornelia Huck]

On Wed, Jun 29 2022, Eric Auger <eauger@redhat.com> wrote:

> 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?

I completely forgot that this exists; I hacked up something (still
untested), and it seems to be able to do what I want.

I'll post it after I've verified that it actually works :)

>> 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?

Yes, that address is bouncing...

I've piggybacked onto a recent kvm discussion in
https://lore.kernel.org/all/875ykmcd8q.fsf@redhat.com/ -- I guess there
had not been any change for migration in the meantime, we need to find a
way to tie page data + metadata together.

Re: [PATCH RFC 0/2] arm: enable MTE for QEMU + kvm

[Cornelia Huck]

On Wed, Jun 29 2022, Eric Auger <eauger@redhat.com> wrote:

> Hi Connie,
>
> On 6/13/22 18:02, 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:
>>>> This series enables MTE for kvm guests, if the kernel supports it.
>>>> Lightly tested while running under the simulator (the arm64/mte/
>>>> kselftests pass... if you wait patiently :)
>>>>
>>>> A new cpu property "mte" (defaulting to on if possible) is introduced;
>>>> for tcg, you still need to enable mte at the machine as well.
>>> isn't the property set to off by default when kvm is enabled (because of
>>> the migration blocker).
>> 
>> Oh, I had changed that around several times, and it seems I ended up
>> being confused when I wrote this cover letter... I wonder what the best
>> state would be (assuming that I don't manage to implement it soonish,
>> but it seems we still would need kernel changes as by the discussion in
>> that other patch series.)
> Having mte=off by default along with KVM, until the migration gets
> supported, looks OK to me. Does it prevent you from having it set to
> another value by default with TCG (depending on the virt machine
> tag_memory option)?
>
> 		tag_memory=on	tag_memory=off
> KVM CPU mte=off	invalid		mte=off
> KVM CPU mte=on	invalid		mte=on
> TCG CPU mte=off	invalid		mte=off
> TCG CPU mte=on	mte=on		invalid
>
> default value:
> KVM mte = off until migration gets supported
> TCG mte = machine.tag_memory

With OnOffAuto, I currently have:

valid for tcg: cpu.mte=on, tag_memory=on (result: mte on)
               cpu.mte=off, tag_memory either on or off (result: mte off)
               cpu.mte unspecified, tag_memory either on or off (result:
               mte==tag_memory)
valid for kvm: tag_memory always off
               cpu.mte=off (result: mte off)
               cpu.mte=on if mte supported in kvm (result: mte on)
               cpu.mte unspecified (result: mte on if kvm supports it;
               this I can flip)
all other combinations: error