On Mon, Jan 23 2023, Eric Auger <eauger@xxxxxxxxxx> wrote:
> 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
"If MTE has been enabled with KVM," ...
>> 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
I think it's explicit enough, since it is in the "For KVM" phrase?
>> +
>> +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
It's only configured at machine level, not sure if that clarifies
anything?
>> + preserves the behaviour prior to introduction of the feature.
> s/prior to/prior to the ?
ok
>> +
>> +* 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?
Hmm... well, it does not support tag memory with kvm/hvf, and the
consequence of this is that kvm/hvf cannot provide support for emulated
mte... what about
"mach-virt: tag memory not supported with %s, emulated MTE cannot be
provided to the guest CPU"
Might be a bit long, though.
>> kvm_enabled() ? "KVM" : "HVF");
>> exit(1);
>> }
(...)
>> +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
will drop
>> +}
>> +
>> +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?
yes, probably better
>> + return;
>> + }
>> + }
>> +#ifdef CONFIG_KVM
>> + if (kvm_enabled() && !kvm_arm_mte_supported()) {
> as you have stubs for both, is the #ifdef needed?
see prior discussion :) Doesn't look like it.
>> + 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?
I've reworked that anyway (no need to enable a vm cap for every cpu.)
>> + } 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.
I've rewritten that in the meanwhile as well :)
>> + error_setg(errp, "Failed to add MTE migration blocker");
>> + error_free(mte_migration_blocker);
>> + mte_migration_blocker = NULL;
>> + }
>> + }
>> + }
>> +#endif
>> + }
>> +}
