RE: [PATCH v3 4/6] KVM: arm64: Disable MPAM visibility by default and ignore VMM writes

[Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>]

Hi James,

> -----Original Message-----
> From: linux-arm-kernel <linux-arm-kernel-bounces@lists.infradead.org> On
> Behalf Of James Morse
> Sent: Thursday, March 21, 2024 4:57 PM
> To: linux-arm-kernel@lists.infradead.org; kvmarm@lists.linux.dev
> Cc: Marc Zyngier <maz@kernel.org>; Oliver Upton <oliver.upton@linux.dev>;
> Suzuki K Poulose <suzuki.poulose@arm.com>; yuzenghui
> <yuzenghui@huawei.com>; Catalin Marinas <catalin.marinas@arm.com>; Will
> Deacon <will@kernel.org>; Jing Zhang <jingzhangos@google.com>; James
> Morse <james.morse@arm.com>
> Subject: [PATCH v3 4/6] KVM: arm64: Disable MPAM visibility by default and
> ignore VMM writes
> 
> commit 011e5f5bf529f ("arm64/cpufeature: Add remaining feature bits in
> ID_AA64PFR0 register") exposed the MPAM field of AA64PFR0_EL1 to guests,
> but didn't add trap handling. A previous patch supplied the missing trap
> handling.
> 
> Existing VMs that have the MPAM field of ID_AA64PFR0_EL1 set need to
> be migratable, but there is little point enabling the MPAM CPU
> interface on new VMs until there is something a guest can do with it.
> 
> Clear the MPAM field from the guest's ID_AA64PFR0_EL1 and on hardware
> that supports MPAM, politely ignore the VMMs attempts to set this bit.
> 
> Guests expossed to this bug have the sanitised value of the MPAM field,
> so only the correct value needs to be ignored. This means the field
> can continue to be used to block migration to incompatible hardware
> (between MPAM=1 and MPAM=5), and the VMM can't rely on the field
> being ignored.
> 
> Signed-off-by: James Morse <james.morse@arm.com>
> ---
>  arch/arm64/kvm/sys_regs.c | 32 +++++++++++++++++++++++++++++++-
>  1 file changed, 31 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index d6afb21849de..56d70a90c965 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -1685,6 +1685,13 @@ static u64 read_sanitised_id_aa64pfr0_el1(struct
> kvm_vcpu *vcpu,
> 
>  	val &= ~ID_AA64PFR0_EL1_AMU_MASK;
> 
> +	/*
> +	 * MPAM is disabled by default as KVM also needs a set of PARTID to
> +	 * program the MPAMVPMx_EL2 PARTID remapping registers with. But
> some
> +	 * older kernels let the guest see the ID bit.
> +	 */
> +	val &= ~ID_AA64PFR0_EL1_MPAM_MASK;
> +
>  	return val;
>  }
> 
> @@ -1795,6 +1802,29 @@ static int set_id_dfr0_el1(struct kvm_vcpu *vcpu,
>  	return set_id_reg(vcpu, rd, val);
>  }
> 
> +static int set_id_aa64pfr0_el1(struct kvm_vcpu *vcpu,
> +			       const struct sys_reg_desc *rd, u64 user_val)
> +{
> +	u64 hw_val = read_sanitised_ftr_reg(SYS_ID_AA64PFR0_EL1);
> +	u64 mpam_mask = ID_AA64PFR0_EL1_MPAM_MASK;
> +
> +	/*
> +	 * Commit 011e5f5bf529f ("arm64/cpufeature: Add remaining feature
> bits
> +	 * in ID_AA64PFR0 register") exposed the MPAM field of AA64PFR0_EL1
> to
> +	 * guests, but didn't add trap handling. KVM doesn't support MPAM and
> +	 * always returns an UNDEF for these registers. The guest must see 0
> +	 * for this field.
> +	 *
> +	 * But KVM must also accept values from user-space that were provided
> +	 * by KVM. On CPUs that support MPAM, permit user-space to write
> +	 * the santisied value to ID_AA64PFR0_EL1.MPAM, but ignore this field.
> +	 */
> +	if ((hw_val & mpam_mask) == (user_val & mpam_mask))
> +		user_val &= ~ID_AA64PFR0_EL1_MPAM_MASK;
> +
> +	return set_id_reg(vcpu, rd, user_val);
> +}

Commit 14e270fa5c4c(arm64/cpufeature: Add remaining feature bits in ID_AA64PFR1 register")
exposes the MPAMFRAC filed in in AA64PFR1_EL1. Don't we need a similar handling for that 
as well to handle the FEAT_MPAMv0p1 case? 

Also any chance you plan to respin this series soon? We do have hardware that benefits from this
series.

Thanks,
Shameer