Re: [PATCH v4 1/2] arm/kvm: add support for MTE

[Richard Henderson]

On 1/11/23 06:13, Cornelia Huck wrote:
> @@ -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",
>                        kvm_enabled() ? "KVM" : "HVF");

Not your bug, but noticing this should use current_accel_name().

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

True for CONFIG_USER_ONLY, via page_get_target_data().
You should have seen check-tcg test failures...

> +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()) {

tcg_enabled(), here and everywhere else you test for !kvm.

> +#ifdef CONFIG_KVM
> +        if (kvm_enabled() && !kvm_arm_mte_supported()) {

kvm_arm.h should get a stub inline returning false, so that the ifdef is 
removed.
See e.g. kvm_arm_sve_supported().

> +    default: /* AUTO */
> +        if (!kvm_enabled()) {

tcg_enabled.

> +    /* 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");

Ideally this ifdef could go away as well.

> +        } else {
> +            /* TODO: add proper migration support with MTE enabled */
> +            if (!mte_migration_blocker) {

Move the global variable here, as a local static?

I guess this check is to avoid adding one blocker per cpu?
I would guess the cap doesn't need enabling more than once either?


> +                error_setg(&mte_migration_blocker,
> +                           "Live migration disabled due to MTE enabled");
> +                if (migrate_add_blocker(mte_migration_blocker, NULL)) {

You pass NULL to the migrate_add_blocker errp argument...

> +                    error_setg(errp, "Failed to add MTE migration blocker");

... then make up your own generic reason for why it failed.
In this case it seems only related to another command-line option: 
--only-migratable.


Anyway, I wonder about hiding all of this in target/arm/kvm.c:

bool kvm_arm_enable_mte(Error *errp)
{
    static bool once = false;
    Error *blocker;

    if (once) {
        return;
    }
    once = true;

    if (kvm_vm_enable_cap(kvm_state, KVM_CAP_ARM_MTE, 0)) {
        error_setg_errno(errp, "Failed to enable KVM_CAP_ARM_MTE");
        return false;
    }

    blocker = g_new0(Error);
    error_setg(blocker, "Live migration disabled....");
    return !migrate_add_blocker(blocker, errp);
}

with

static inline bool kvm_arm_enable_mte(Error *errp)
{
    g_assert_not_reached();
}

in the !CONFIG_KVM block in kvm_arm.h.