Re: [PATCH 3/3] KVM: arm64: Use per guest ID register for ID_AA64PFR1_EL1.MTE

[Marc Zyngier <maz@kernel.org>]

On Fri, 02 Jun 2023 23:14:47 +0100,
Suraj Jitindar Singh <surajjs@amazon.com> wrote:
> 
> With per guest ID registers, MTE settings from userspace can be stored in
> its corresponding ID register.
> 
> No functional change intended.
> 
> Signed-off-by: Suraj Jitindar Singh <surajjs@amazon.com>
> ---
>  arch/arm64/include/asm/kvm_host.h | 21 ++++++++++-----------
>  arch/arm64/kvm/arm.c              | 11 ++++++++++-
>  arch/arm64/kvm/sys_regs.c         |  5 +++++
>  3 files changed, 25 insertions(+), 12 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index 7b0f43373dbe..861997a14ba1 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -226,9 +226,7 @@ struct kvm_arch {
>  	 */
>  #define KVM_ARCH_FLAG_RETURN_NISV_IO_ABORT_TO_USER	0
>  	/* Memory Tagging Extension enabled for the guest */
> -#define KVM_ARCH_FLAG_MTE_ENABLED			1
> -	/* At least one vCPU has ran in the VM */
> -#define KVM_ARCH_FLAG_HAS_RAN_ONCE			2
> +#define KVM_ARCH_FLAG_HAS_RAN_ONCE			1
>  	/*
>  	 * The following two bits are used to indicate the guest's EL1
>  	 * register width configuration. A value of KVM_ARCH_FLAG_EL1_32BIT
> @@ -236,22 +234,22 @@ struct kvm_arch {
>  	 * Otherwise, the guest's EL1 register width has not yet been
>  	 * determined yet.
>  	 */
> -#define KVM_ARCH_FLAG_REG_WIDTH_CONFIGURED		3
> -#define KVM_ARCH_FLAG_EL1_32BIT				4
> +#define KVM_ARCH_FLAG_REG_WIDTH_CONFIGURED		2
> +#define KVM_ARCH_FLAG_EL1_32BIT				3
>  	/* PSCI SYSTEM_SUSPEND enabled for the guest */
> -#define KVM_ARCH_FLAG_SYSTEM_SUSPEND_ENABLED		5
> +#define KVM_ARCH_FLAG_SYSTEM_SUSPEND_ENABLED		4
>  	/* VM counter offset */
> -#define KVM_ARCH_FLAG_VM_COUNTER_OFFSET			6
> +#define KVM_ARCH_FLAG_VM_COUNTER_OFFSET			5
>  	/* Timer PPIs made immutable */
> -#define KVM_ARCH_FLAG_TIMER_PPIS_IMMUTABLE		7
> +#define KVM_ARCH_FLAG_TIMER_PPIS_IMMUTABLE		6
>  	/* SMCCC filter initialized for the VM */
> -#define KVM_ARCH_FLAG_SMCCC_FILTER_CONFIGURED		8
> +#define KVM_ARCH_FLAG_SMCCC_FILTER_CONFIGURED		7
>  	/*
>  	 * AA64DFR0_EL1.PMUver was set as ID_AA64DFR0_EL1_PMUVer_IMP_DEF
>  	 * or DFR0_EL1.PerfMon was set as ID_DFR0_EL1_PerfMon_IMPDEF from
>  	 * userspace for VCPUs without PMU.
>  	 */
> -#define KVM_ARCH_FLAG_VCPU_HAS_IMP_DEF_PMU		9
> +#define KVM_ARCH_FLAG_VCPU_HAS_IMP_DEF_PMU		8
>  
>  	unsigned long flags;
>  
> @@ -1112,7 +1110,8 @@ bool kvm_arm_vcpu_is_finalized(struct kvm_vcpu *vcpu);
>  
>  #define kvm_has_mte(kvm)					\
>  	(system_supports_mte() &&				\
> -	 test_bit(KVM_ARCH_FLAG_MTE_ENABLED, &(kvm)->arch.flags))
> +	 FIELD_GET(ID_AA64PFR1_EL1_MTE_MASK,			\
> +		   IDREG(kvm, SYS_ID_AA64PFR1_EL1)))
>  
>  #define kvm_supports_32bit_el0()				\
>  	(system_supports_32bit_el0() &&				\
> diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
> index ca18c09ccf82..6fc4190559d1 100644
> --- a/arch/arm64/kvm/arm.c
> +++ b/arch/arm64/kvm/arm.c
> @@ -80,8 +80,17 @@ int kvm_vm_ioctl_enable_cap(struct kvm *kvm,
>  		if (!system_supports_mte() || kvm->created_vcpus) {
>  			r = -EINVAL;
>  		} else {
> +			u64 val;
> +
> +			/* Protects the idregs against modification */
> +			mutex_lock(&kvm->arch.config_lock);
> +
> +			val = IDREG(kvm, SYS_ID_AA64PFR1_EL1);
> +			val |= FIELD_PREP(ID_AA64PFR1_EL1_MTE_MASK, 1);

The architecture specifies 3 versions of MTE in the published ARM ARM,
with a 4th coming up as part of the 2022 extensions. Why are you
actively crippling the MTE version presented to the guest, and
potentially introduce unexpected behaviours?

> +			IDREG(kvm, SYS_ID_AA64PFR1_EL1) = val;
> +
> +			mutex_unlock(&kvm->arch.config_lock);
>  			r = 0;
> -			set_bit(KVM_ARCH_FLAG_MTE_ENABLED, &kvm->arch.flags);
>  		}
>  		mutex_unlock(&kvm->lock);
>  		break;
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index 59f8adda47fa..8cffb82dd10d 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -3672,6 +3672,11 @@ void kvm_arm_init_id_regs(struct kvm *kvm)
>  		idreg++;
>  		id = reg_to_encoding(idreg);
>  	}
> +
> +	/* MTE disabled by default even when supported */
> +	val = IDREG(kvm, SYS_ID_AA64PFR1_EL1);
> +	val &= ~ARM64_FEATURE_MASK(ID_AA64PFR1_EL1_MTE);
> +	IDREG(kvm, SYS_ID_AA64PFR1_EL1) = val;
>  }
>  
>  int __init kvm_sys_reg_table_init(void)

Overall, I don't really see the point of such a change. It creates
more problems than it solves.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.