Re: [PATCH v6 17/28] KVM: arm64: Support SME identification registers for guests

[Marc Zyngier <maz@kernel.org>]

On Wed, 25 Jun 2025 11:48:08 +0100,
Mark Brown <broonie@kernel.org> wrote:
> 
> The primary register for identifying SME is ID_AA64PFR1_EL1.SME.  This
> is hidden from guests unless SME is enabled by the VMM.
> When it is visible it is writable and can be used to control the
> availability of SME2.
> 
> There is also a new register ID_AA64SMFR0_EL1 which we make writable,
> forcing it to all bits 0 if SME is disabled.  This includes the field
> SMEver giving the SME version, userspace is responsible for ensuring
> the value is consistent with ID_AA64PFR1_EL1.SME.  It also includes
> FA64, a separately enableable extension which provides the full FPSIMD
> and SVE instruction set including FFR in streaming mode.  Userspace can
> control the availability of FA64 by writing to this field.  The other
> features enumerated there only add new instructions, there are no
> architectural controls for these.
> 
> There is a further identification register SMIDR_EL1 which provides a
> basic description of the SME microarchitecture, in a manner similar to
> MIDR_EL1 for the PE.  It also describes support for priority management
> and a basic affinity description for shared SME units, plus some RES0
> space.  We do not support priority management and affinity is not
> meaningful for guests so we mask out everything except for the
> microarchitecture description.

Both are extremely useful and meaningful to guests. You just have made
the choice not to expose this.

> 
> As for MIDR_EL1 and REVIDR_EL1 we expose the implementer and revision
> information to guests with the raw value from the CPU we are running on,
> this may present issues for asymmetric systems or for migration as it
> does for the existing registers.

MIDR/REVIDR are writable from userspace in order to alleviate this
problem. So should be SMIDR.

> 
> Signed-off-by: Mark Brown <broonie@kernel.org>
> ---
>  arch/arm64/include/asm/kvm_host.h |  1 +
>  arch/arm64/kvm/sys_regs.c         | 46 +++++++++++++++++++++++++++++++++++----
>  2 files changed, 43 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index c26099f74648..29b8697c8144 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -494,6 +494,7 @@ enum vcpu_sysreg {
>  	/* FP/SIMD/SVE */
>  	SVCR,
>  	FPMR,
> +	SMIDR_EL1,	/* Streaming Mode Identification Register */

No. We have long made ID registers to be per-VM, not per-vcpu.
SMIDR_EL1 must have the same behaviour.

>  
>  	/* 32bit specific registers. */
>  	DACR32_EL2,	/* Domain Access Control Register */
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index caa90dae8184..b11bb95e9e35 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -774,6 +774,38 @@ static u64 reset_mpidr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
>  	return mpidr;
>  }
>  
> +static u64 reset_smidr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
> +{
> +	u64 smidr = 0;
> +
> +	if (!system_supports_sme())
> +		return smidr;
> +
> +	smidr = read_sysreg_s(SYS_SMIDR_EL1);
> +
> +	/*
> +	 * Mask out everything except for the implementer and revison,
> +	 * in particular priority management is not implemented.
> +	 */
> +	smidr &= SMIDR_EL1_IMPLEMENTER_MASK | SMIDR_EL1_REVISION_MASK;
> +
> +	vcpu_write_sys_reg(vcpu, smidr, SMIDR_EL1);
> +
> +	return smidr;
> +}
> +
> +static bool access_smidr(struct kvm_vcpu *vcpu,
> +			 struct sys_reg_params *p,
> +			 const struct sys_reg_desc *r)
> +{
> +	if (p->is_write)
> +		return write_to_read_only(vcpu, p, r);
> +
> +	p->regval = vcpu_read_sys_reg(vcpu, r->reg);
> +
> +	return true;
> +}

We already have 2 similar copies of this function. We're not adding a
third one.

> +
>  static unsigned int pmu_visibility(const struct kvm_vcpu *vcpu,
>  				   const struct sys_reg_desc *r)
>  {
> @@ -1607,7 +1639,9 @@ static u64 __kvm_read_sanitised_id_reg(const struct kvm_vcpu *vcpu,
>  			val &= ~ARM64_FEATURE_MASK(ID_AA64PFR1_EL1_MTE_frac);
>  		}
>  
> -		val &= ~ARM64_FEATURE_MASK(ID_AA64PFR1_EL1_SME);
> +		if (!vcpu_has_sme(vcpu))
> +			val &= ~ARM64_FEATURE_MASK(ID_AA64PFR1_EL1_SME);
> +
>  		val &= ~ARM64_FEATURE_MASK(ID_AA64PFR1_EL1_RNDR_trap);
>  		val &= ~ARM64_FEATURE_MASK(ID_AA64PFR1_EL1_NMI);
>  		val &= ~ARM64_FEATURE_MASK(ID_AA64PFR1_EL1_GCS);
> @@ -1723,6 +1757,10 @@ static unsigned int id_visibility(const struct kvm_vcpu *vcpu,
>  		if (!vcpu_has_sve(vcpu))
>  			return REG_RAZ;
>  		break;
> +	case SYS_ID_AA64SMFR0_EL1:
> +		if (!vcpu_has_sme(vcpu))
> +			return REG_RAZ;
> +		break;
>  	}
>  
>  	return 0;
> @@ -2905,7 +2943,6 @@ static const struct sys_reg_desc sys_reg_descs[] = {
>  				       ID_AA64PFR1_EL1_MTE_frac |
>  				       ID_AA64PFR1_EL1_NMI |
>  				       ID_AA64PFR1_EL1_RNDR_trap |
> -				       ID_AA64PFR1_EL1_SME |
>  				       ID_AA64PFR1_EL1_RES0 |
>  				       ID_AA64PFR1_EL1_MPAM_frac |
>  				       ID_AA64PFR1_EL1_RAS_frac |
> @@ -2913,7 +2950,7 @@ static const struct sys_reg_desc sys_reg_descs[] = {
>  	ID_WRITABLE(ID_AA64PFR2_EL1, ID_AA64PFR2_EL1_FPMR),
>  	ID_UNALLOCATED(4,3),
>  	ID_WRITABLE(ID_AA64ZFR0_EL1, ~ID_AA64ZFR0_EL1_RES0),
> -	ID_HIDDEN(ID_AA64SMFR0_EL1),
> +	ID_WRITABLE(ID_AA64SMFR0_EL1, ~ID_AA64SMFR0_EL1_RES0),
>  	ID_UNALLOCATED(4,6),
>  	ID_WRITABLE(ID_AA64FPFR0_EL1, ~ID_AA64FPFR0_EL1_RES0),
>  
> @@ -3112,7 +3149,8 @@ static const struct sys_reg_desc sys_reg_descs[] = {
>  	{ SYS_DESC(SYS_CLIDR_EL1), access_clidr, reset_clidr, CLIDR_EL1,
>  	  .set_user = set_clidr, .val = ~CLIDR_EL1_RES0 },
>  	{ SYS_DESC(SYS_CCSIDR2_EL1), undef_access },
> -	{ SYS_DESC(SYS_SMIDR_EL1), undef_access },
> +	{ SYS_DESC(SYS_SMIDR_EL1), .access = access_smidr, .reset = reset_smidr,
> +	  .reg = SMIDR_EL1, .visibility = sme_visibility },
>  	IMPLEMENTATION_ID(AIDR_EL1, GENMASK_ULL(63, 0)),
>  	{ SYS_DESC(SYS_CSSELR_EL1), access_csselr, reset_unknown, CSSELR_EL1 },
>  	ID_FILTERED(CTR_EL0, ctr_el0,
> 

Please also handle the comment about FEAT_SME_SMPS in config.c.

	M.

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