Re: [PATCH v4 6/7] KVM: arm64: Disable MPAM visibility by default and ignore VMM writes

[Gavin Shan <gshan@redhat.com>]

On 10/4/24 9:07 PM, Joey Gouly wrote:
> From: James Morse <james.morse@arm.com>
> 
> 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 exposed 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>
> Signed-off-by: Joey Gouly <joey.gouly@arm.com>
> ---
>   arch/arm64/kvm/sys_regs.c | 53 ++++++++++++++++++++++++++++++++++++---
>   1 file changed, 50 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index 1fd08f12f2bb..f0c55d3907d9 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -1723,6 +1723,23 @@ 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;
> +}
> +
> +static u64 read_sanitised_id_aa64pfr1_el1(struct kvm_vcpu *vcpu,
> +					  const struct sys_reg_desc *rd)
> +{
> +	u64 val = read_sanitised_ftr_reg(SYS_ID_AA64PFR1_EL1);
> +
> +	val &= ~ID_AA64PFR1_EL1_MPAM_frac_MASK;
> +
>   	return val;
>   }
>   
> @@ -1834,9 +1851,38 @@ static int set_id_dfr0_el1(struct kvm_vcpu *vcpu,
>   }
>   
>   static int set_id_aa64pfr0_el1(struct kvm_vcpu *vcpu,
> -			       const struct sys_reg_desc *rd, u64 val)
> +			       const struct sys_reg_desc *rd, u64 user_val)
>   {
> -	return set_id_reg(vcpu, rd, 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 sanitizied 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);
> +}
> +
> +static int set_id_aa64pfr1_el1(struct kvm_vcpu *vcpu,
> +			       const struct sys_reg_desc *rd, u64 user_val)
> +{
> +	u64 hw_val = read_sanitised_ftr_reg(SYS_ID_AA64PFR1_EL1);
> +	u64 mpam_mask = ID_AA64PFR1_EL1_MPAM_frac_MASK;
> +
> +	if ((hw_val & mpam_mask) == (user_val & mpam_mask))
> +		user_val &= ~ID_AA64PFR1_EL1_MPAM_frac_MASK;
> +
> +	return set_id_reg(vcpu, rd, user_val);
>   }
>   
>   /*
> @@ -2390,7 +2436,8 @@ static const struct sys_reg_desc sys_reg_descs[] = {
>   		      ID_AA64PFR0_EL1_RAS |
>   		      ID_AA64PFR0_EL1_AdvSIMD |
>   		      ID_AA64PFR0_EL1_FP)),
> -	ID_SANITISED(ID_AA64PFR1_EL1),
> +	ID_FILTERED(ID_AA64PFR1_EL1, id_aa64pfr1_el1,
> +		    ~ID_AA64PFR1_EL1_MPAM_frac),

The 'reset' callback has been changed from kvm_read_sanitised_id_reg() to the newly
introduced read_sanitised_id_aa64pfr1_el1(). They're not interchangeable apart from
the MPAM_frac bits. For example, The SME bit has been hidden unconditionally and
the MTE bit is hidden conditionally in __kvm_read_sanitised_id_reg(), called by
kvm_read_sanitised_id_reg().

>   	ID_WRITABLE(ID_AA64PFR2_EL1, ID_AA64PFR2_EL1_FPMR),
>   	ID_UNALLOCATED(4,3),
>   	ID_WRITABLE(ID_AA64ZFR0_EL1, ~ID_AA64ZFR0_EL1_RES0),

Thanks,
Gavin