Hi Connie,
On 1/11/23 17:13, Cornelia Huck wrote:
> Introduce a new cpu feature flag to control MTE support. To preserve
> backwards compatibility for tcg, MTE will continue to be enabled as
> long as tag memory has been provided.
>
> If MTE has been enabled, we need to disable migration, as we do not
this only applies to KVM acceleration
> yet have a way to migrate the tags as well. Therefore, MTE will stay
> off with KVM unless requested explicitly.
>
> Signed-off-by: Cornelia Huck <cohuck@xxxxxxxxxx>
> ---
> docs/system/arm/cpu-features.rst | 21 +++++
> hw/arm/virt.c | 2 +-
> target/arm/cpu.c | 18 ++---
> target/arm/cpu.h | 1 +
> target/arm/cpu64.c | 133 +++++++++++++++++++++++++++++++
> target/arm/internals.h | 1 +
> target/arm/kvm64.c | 5 ++
> target/arm/kvm_arm.h | 12 +++
> target/arm/monitor.c | 1 +
> 9 files changed, 181 insertions(+), 13 deletions(-)
>
> diff --git a/docs/system/arm/cpu-features.rst b/docs/system/arm/cpu-features.rst
> index 00c444042ff5..e278650c837e 100644
> --- a/docs/system/arm/cpu-features.rst
> +++ b/docs/system/arm/cpu-features.rst
> @@ -443,3 +443,24 @@ As with ``sve-default-vector-length``, if the default length is larger
> than the maximum vector length enabled, the actual vector length will
> be reduced. If this property is set to ``-1`` then the default vector
> length is set to the maximum possible length.
> +
> +MTE CPU Property
> +================
> +
> +The ``mte`` property controls the Memory Tagging Extension. For TCG, it requires
> +presence of tag memory (which can be turned on for the ``virt`` machine via
> +``mte=on``). For KVM, it requires the ``KVM_CAP_ARM_MTE`` capability; until
> +proper migration support is implemented, enabling MTE will install a migration
> +blocker.
maybe re-emphasize: when KVM is enabled
> +
> +If not specified explicitly via ``on`` or ``off``, MTE will be available
> +according to the following rules:
> +
> +* When TCG is used, MTE will be available iff tag memory is available; i.e. it
suggestion: is available at machine level
> + preserves the behaviour prior to introduction of the feature.
s/prior to/prior to the ?
> +
> +* When KVM is used, MTE will default to off, so that migration will not
> + unintentionally be blocked.
> +
> +* Other accelerators currently don't support MTE.
> +
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index ea2413a0bad7..42359e256ad0 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -2136,7 +2136,7 @@ static void machvirt_init(MachineState *machine)
>
> if (vms->mte && (kvm_enabled() || hvf_enabled())) {
> error_report("mach-virt: %s does not support providing "
> - "MTE to the guest CPU",
> + "emulated MTE to the guest CPU",
each time I read this message I feel difficult to understand it. Why not
replacing by
"mach-virt does not support tag memory with %s acceleration" or
something alike?
> kvm_enabled() ? "KVM" : "HVF");
> exit(1);
> }
> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
> index 5f63316dbf22..decab743d0d5 100644
> --- a/target/arm/cpu.c
> +++ b/target/arm/cpu.c
> @@ -1529,6 +1529,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;
> + }
> }
> #endif
>
> @@ -1605,7 +1610,7 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
> }
> if (cpu->tag_memory) {
> error_setg(errp,
> - "Cannot enable %s when guest CPUs has MTE enabled",
> + "Cannot enable %s when guest CPUs has tag memory enabled",
> current_accel_name());
> return;
> }
> @@ -1984,17 +1989,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
> -
> if (tcg_enabled()) {
> /*
> * Don't report the Statistical Profiling Extension in the ID
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index bf2bce046d56..f1a9015a7ed7 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -1038,6 +1038,7 @@ struct ArchCPU {
> bool prop_pauth;
> bool prop_pauth_impdef;
> bool prop_lpa2;
> + OnOffAuto prop_mte;
>
> /* DCZ blocksize, in log_2(words), ie low 4 bits of DCZID_EL0 */
> uint32_t dcz_blocksize;
> diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
> index 0e021960fb5b..3cf42ee05ca3 100644
> --- a/target/arm/cpu64.c
> +++ b/target/arm/cpu64.c
> @@ -29,6 +29,13 @@
> #include "qapi/visitor.h"
> #include "hw/qdev-properties.h"
> #include "internals.h"
> +#include "migration/blocker.h"
> +#include "qapi/qapi-visit-common.h"
> +#include "hw/arm/virt.h"
> +
> +#ifdef CONFIG_KVM
> +static Error *mte_migration_blocker;
> +#endif
>
> static void aarch64_a35_initfn(Object *obj)
> {
> @@ -1096,6 +1103,130 @@ static void aarch64_neoverse_n1_initfn(Object *obj)
> cpu->isar.reset_pmcr_el0 = 0x410c3000;
> }
>
> +static void aarch64_cpu_get_mte(Object *obj, Visitor *v, const char *name,
> + void *opaque, Error **errp)
> +{
> + ARMCPU *cpu = ARM_CPU(obj);
> + OnOffAuto mte = cpu->prop_mte;
> +
> + visit_type_OnOffAuto(v, name, &mte, errp);
> +}
> +
> +static void aarch64_cpu_set_mte(Object *obj, Visitor *v, const char *name,
> + void *opaque, Error **errp)
> +{
> + ARMCPU *cpu = ARM_CPU(obj);
> +
> + visit_type_OnOffAuto(v, name, &cpu->prop_mte, errp);
> +
nit: spare void line
> +}
> +
> +static void aarch64_add_mte_properties(Object *obj)
> +{
> + /*
> + * For tcg, "AUTO" means turn on mte if tag memory has been provided, and
> + * turn it off (without error) if not.
> + * For kvm, "AUTO" currently means mte off, as migration is not supported
> + * yet.
> + * For all others, "AUTO" means mte off.
> + */
> + object_property_add(obj, "mte", "OnOffAuto", aarch64_cpu_get_mte,
> + aarch64_cpu_set_mte, NULL, NULL);
> +}
> +
> +static inline bool arm_machine_has_tag_memory(void)
> +{
> +#ifndef CONFIG_USER_ONLY
> + Object *obj = object_dynamic_cast(qdev_get_machine(), TYPE_VIRT_MACHINE);
> +
> + /* so far, only the virt machine has support for tag memory */
> + if (obj) {
> + VirtMachineState *vms = VIRT_MACHINE(obj);
> +
> + return vms->mte;
> + }
> +#endif
> + return false;
> +}
> +
> +void arm_cpu_mte_finalize(ARMCPU *cpu, Error **errp)
> +{
> + bool enable_mte;
> +
> + switch (cpu->prop_mte) {
> + case ON_OFF_AUTO_OFF:
> + enable_mte = false;
> + break;
> + case ON_OFF_AUTO_ON:
> + if (!kvm_enabled()) {
> + if (cpu_isar_feature(aa64_mte, cpu)) {
> + if (!arm_machine_has_tag_memory()) {
> + error_setg(errp, "mte=on requires tag memory");
> + return;
> + }
> + } else {
> + error_setg(errp, "mte not provided");
mte not supported by this CPU type?
> + return;
> + }
> + }
> +#ifdef CONFIG_KVM
> + if (kvm_enabled() && !kvm_arm_mte_supported()) {
as you have stubs for both, is the #ifdef needed?
> + error_setg(errp, "mte not supported by kvm");
> + return;
> + }
> +#endif
> + enable_mte = true;
> + break;
> + default: /* AUTO */
> + if (!kvm_enabled()) {
> + if (cpu_isar_feature(aa64_mte, cpu)) {
> + /*
> + * Tie mte enablement to presence of tag memory, in order to
> + * preserve pre-existing behaviour.
> + */
> + enable_mte = arm_machine_has_tag_memory();
> + } else {
> + enable_mte = false;
> + }
> + break;
> + } else {
> + /*
> + * This cannot yet be
> + * enable_mte = kvm_arm_mte_supported();
> + * as we don't support migration yet.
> + */
> + enable_mte = false;
> + }
> + }
> +
> + if (!enable_mte) {
> + /* Disable MTE feature bits. */
> + cpu->isar.id_aa64pfr1 =
> + FIELD_DP64(cpu->isar.id_aa64pfr1, ID_AA64PFR1, MTE, 0);
> + return;
> + }
> +
> + /* accelerator-specific enablement */
> + if (kvm_enabled()) {
> +#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");
nit: return and remove the else?
> + } 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)) {
Can't you pass the erro directly to migrate_add_blocker. Also in
arm_gicv3_its_kvm.c or virtio-gpu-pci, < 0 is checked. Maybe worth to
double check the rationale.
> + error_setg(errp, "Failed to add MTE migration blocker");
> + error_free(mte_migration_blocker);
> + mte_migration_blocker = NULL;
> + }
> + }
> + }
> +#endif
> + }
> +}
> +
> static void aarch64_host_initfn(Object *obj)
> {
> #if defined(CONFIG_KVM)
> @@ -1104,6 +1235,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);
> @@ -1300,6 +1432,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 const ARMCPUInfo aarch64_cpus[] = {
> diff --git a/target/arm/internals.h b/target/arm/internals.h
> index d9555309df0f..4dc6d19be42b 100644
> --- a/target/arm/internals.h
> +++ b/target/arm/internals.h
> @@ -1348,6 +1348,7 @@ void arm_cpu_sve_finalize(ARMCPU *cpu, Error **errp);
> void arm_cpu_sme_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);
> #endif
>
> #ifdef CONFIG_USER_ONLY
> diff --git a/target/arm/kvm64.c b/target/arm/kvm64.c
> index 1197253d12f7..b777bd0a11d2 100644
> --- a/target/arm/kvm64.c
> +++ b/target/arm/kvm64.c
> @@ -764,6 +764,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);
>
> uint32_t kvm_arm_sve_get_vls(CPUState *cs)
> diff --git a/target/arm/kvm_arm.h b/target/arm/kvm_arm.h
> index 99017b635ce4..762443f8a7c0 100644
> --- a/target/arm/kvm_arm.h
> +++ b/target/arm/kvm_arm.h
> @@ -305,6 +305,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
> @@ -395,6 +402,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 ecdd5ee81742..c419c81612ed 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
> };
>
Thanks
Eric
