Re: [RFC PATCH v6 19/35] KVM: arm64: Trap PMBIDR_EL1 and PMSIDR_EL1

[Alexandru Elisei <alexandru.elisei@arm.com>]

Hi James,

On Fri, Jan 09, 2026 at 04:29:37PM +0000, James Clark wrote:
> 
> 
> On 14/11/2025 4:07 pm, Alexandru Elisei wrote:
> > PMBIDR_EL1 and PMSIDR_EL1 are read-only registers that describe the SPE
> > implementation. Trap reads to allow KVM to control how SPE is described to
> > a virtual machine. In particular, this is needed to:
> > 
> > - Advertise the maximum buffer size set by userspace in
> >    PMBIDR_EL1.MaxBuffSize.
> > - Hide in PMSIDR_EL1, if necessary, the presence of FEAT_SPE_FDS and
> >    FEAT_SPE_FnE, both of which add new registers which are already
> >    trapped via the FGU mechanism.
> > 
> > Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
> > ---
> >   arch/arm64/include/asm/kvm_spe.h |  4 +-
> >   arch/arm64/kvm/config.c          | 13 ++++-
> >   arch/arm64/kvm/spe.c             | 82 +++++++++++++++++++++++++++++++-
> >   arch/arm64/kvm/sys_regs.c        | 13 +++--
> >   4 files changed, 103 insertions(+), 9 deletions(-)
> > 
> > diff --git a/arch/arm64/include/asm/kvm_spe.h b/arch/arm64/include/asm/kvm_spe.h
> > index 3506d8c4c661..47b94794cc5f 100644
> > --- a/arch/arm64/include/asm/kvm_spe.h
> > +++ b/arch/arm64/include/asm/kvm_spe.h
> > @@ -40,7 +40,7 @@ int kvm_spe_get_attr(struct kvm_vcpu *vcpu, struct kvm_device_attr *attr);
> >   int kvm_spe_has_attr(struct kvm_vcpu *vcpu, struct kvm_device_attr *attr);
> >   bool kvm_spe_write_sysreg(struct kvm_vcpu *vcpu, int reg, u64 val);
> > -u64 kvm_spe_read_sysreg(struct kvm_vcpu *vcpu, int reg);
> > +u64 kvm_spe_read_sysreg(struct kvm_vcpu *vcpu, int reg, u32 encoding);
> >   #else
> >   struct kvm_spe {
> >   };
> > @@ -78,7 +78,7 @@ static inline bool kvm_spe_write_sysreg(struct kvm_vcpu *vcpu, int reg, u64 val)
> >   {
> >   	return true;
> >   }
> > -static inline u64 kvm_spe_read_sysreg(struct kvm_vcpu *vcpu, int reg)
> > +static inline u64 kvm_spe_read_sysreg(struct kvm_vcpu *vcpu, int reg, u32 encoding)
> >   {
> >   	return 0;
> >   }
> > diff --git a/arch/arm64/kvm/config.c b/arch/arm64/kvm/config.c
> > index 24bb3f36e9d5..ed6b167b7aa8 100644
> > --- a/arch/arm64/kvm/config.c
> > +++ b/arch/arm64/kvm/config.c
> > @@ -6,6 +6,7 @@
> >   #include <linux/kvm_host.h>
> >   #include <asm/kvm_emulate.h>
> > +#include <asm/kvm_spe.h>
> >   #include <asm/kvm_nested.h>
> >   #include <asm/sysreg.h>
> > @@ -1489,6 +1490,16 @@ static void __compute_hfgwtr(struct kvm_vcpu *vcpu)
> >   		*vcpu_fgt(vcpu, HFGWTR_EL2) |= HFGWTR_EL2_TCR_EL1;
> >   }
> > +static void __compute_hdfgrtr(struct kvm_vcpu *vcpu)
> > +{
> > +	__compute_fgt(vcpu, HDFGRTR_EL2);
> > +
> > +	if (vcpu_has_spe(vcpu)) {
> > +		*vcpu_fgt(vcpu, HDFGRTR_EL2) |= HDFGRTR_EL2_PMBIDR_EL1;
> > +		*vcpu_fgt(vcpu, HDFGRTR_EL2) |= HDFGRTR_EL2_PMSIDR_EL1;
> > +	}
> > +}
> > +
> >   static void __compute_hdfgwtr(struct kvm_vcpu *vcpu)
> >   {
> >   	__compute_fgt(vcpu, HDFGWTR_EL2);
> > @@ -1505,7 +1516,7 @@ void kvm_vcpu_load_fgt(struct kvm_vcpu *vcpu)
> >   	__compute_fgt(vcpu, HFGRTR_EL2);
> >   	__compute_hfgwtr(vcpu);
> >   	__compute_fgt(vcpu, HFGITR_EL2);
> > -	__compute_fgt(vcpu, HDFGRTR_EL2);
> > +	__compute_hdfgrtr(vcpu);
> >   	__compute_hdfgwtr(vcpu);
> >   	__compute_fgt(vcpu, HAFGRTR_EL2);
> > diff --git a/arch/arm64/kvm/spe.c b/arch/arm64/kvm/spe.c
> > index 5b3dc622cf82..92eb46276c71 100644
> > --- a/arch/arm64/kvm/spe.c
> > +++ b/arch/arm64/kvm/spe.c
> > @@ -22,10 +22,16 @@ struct arm_spu_entry {
> >   	struct arm_spe_pmu *arm_spu;
> >   };
> > +static u64 max_buffer_size_to_pmbidr_el1(u64 size);
> > +
> >   void kvm_host_spe_init(struct arm_spe_pmu *arm_spu)
> >   {
> >   	struct arm_spu_entry *entry;
> > +	/* PMBIDR_EL1 cannot be trapped without FEAT_FGT. */
> > +	if (!cpus_have_final_cap(ARM64_HAS_FGT))
> > +		return;
> > +
> 
> Isn't this only required if you want to report a different PMBIDR value? If
> the value in hardware is already what you want to give to guests then it's
> not strictly required. Maybe it's something we can do as an addition later
> to make it usable in more places.

The maximum buffer size is advertised in PMBIDR_EL1.

Thanks,
Alex

> 
> >   	/* TODO: pKVM and nVHE support */
> >   	if (is_protected_kvm_enabled() || !has_vhe())
> >   		return;
> > @@ -86,9 +92,81 @@ bool kvm_spe_write_sysreg(struct kvm_vcpu *vcpu, int reg, u64 val)
> >   	return true;
> >   }
> > -u64 kvm_spe_read_sysreg(struct kvm_vcpu *vcpu, int reg)
> > +static bool kvm_spe_has_feat_spe_fds(struct kvm *kvm)
> > +{
> > +	struct arm_spe_pmu *spe_pmu = kvm->arch.kvm_spe.arm_spu;
> > +
> > +	return kvm_has_feat(kvm, ID_AA64DFR0_EL1, PMSVer, V1P4) &&
> > +	       FIELD_GET(PMSIDR_EL1_FDS, spe_pmu->pmsidr_el1);
> > +}
> > +
> > +static bool kvm_spe_has_feat_spe_fne(struct kvm *kvm)
> > +{
> > +	struct arm_spe_pmu *spe_pmu = kvm->arch.kvm_spe.arm_spu;
> > +
> > +	return kvm_has_feat(kvm, ID_AA64DFR0_EL1, PMSVer, V1P2) &&
> > +	       FIELD_GET(PMSIDR_EL1_FnE, spe_pmu->pmsidr_el1);
> > +}
> > +
> > +static u64 kvm_spe_get_pmsidr_el1(struct kvm_vcpu *vcpu)
> >   {
> > -	return __vcpu_sys_reg(vcpu, reg);
> > +	struct kvm *kvm = vcpu->kvm;
> > +	u64 pmsidr = kvm->arch.kvm_spe.arm_spu->pmsidr_el1;
> > +
> > +	/* Filter out known RES0 bits. */
> > +	pmsidr &= ~GENMASK_ULL(63, 33);
> > +
> > +	if (!kvm_spe_has_feat_spe_fne(kvm))
> > +		pmsidr &= ~PMSIDR_EL1_FnE;
> > +
> > +	if (!kvm_spe_has_feat_spe_fds(kvm))
> > +		pmsidr &= ~PMSIDR_EL1_FDS;
> > +
> > +	return pmsidr;
> > +}
> > +
> > +static u64 kvm_spe_get_pmbidr_el1(struct kvm_vcpu *vcpu)
> > +{
> > +	struct kvm *kvm = vcpu->kvm;
> > +	struct kvm_spe *kvm_spe = &kvm->arch.kvm_spe;
> > +	u64 max_buffer_size = kvm_spe->max_buffer_size;
> > +	u64 pmbidr_el1 = kvm_spe->arm_spu->pmbidr_el1;
> > +
> > +	/* Filter out known RES0 bits. */
> > +	pmbidr_el1 &= ~(GENMASK_ULL(63, 48) | GENMASK_ULL(31, 12));
> > +
> > +	if (!kvm_has_feat(kvm, ID_AA64DFR2_EL1, SPE_nVM, IMP)) {
> > +		pmbidr_el1 &= ~PMBIDR_EL1_AddrMode;
> > +		pmbidr_el1 |= PMBIDR_EL1_AddrMode_VM_ONLY;
> > +	}
> > +
> > +	pmbidr_el1 &= ~PMBIDR_EL1_MaxBuffSize;
> > +	pmbidr_el1 |= max_buffer_size_to_pmbidr_el1(max_buffer_size);
> > +
> > +	return pmbidr_el1;
> > +}
> > +
> > +u64 kvm_spe_read_sysreg(struct kvm_vcpu *vcpu, int reg, u32 encoding)
> > +{
> > +	u64 val = 0;
> > +
> > +	switch (encoding) {
> > +	case SYS_PMBIDR_EL1:
> > +		val = kvm_spe_get_pmbidr_el1(vcpu);
> > +		break;
> > +	case SYS_PMSIDR_EL1:
> > +		val = kvm_spe_get_pmsidr_el1(vcpu);
> > +		break;
> > +	case SYS_PMBLIMITR_EL1:
> > +	case SYS_PMBPTR_EL1:
> > +	case SYS_PMBSR_EL1:
> > +		val = __vcpu_sys_reg(vcpu, reg);
> > +		break;
> > +	default:
> > +		WARN_ON_ONCE(1);
> > +	}
> > +
> > +	return val;
> >   }
> >   static u64 max_buffer_size_to_pmbidr_el1(u64 size)
> > diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> > index 5eeea229b46e..db86d1dcd148 100644
> > --- a/arch/arm64/kvm/sys_regs.c
> > +++ b/arch/arm64/kvm/sys_regs.c
> > @@ -1386,13 +1386,18 @@ static unsigned int spe_visibility(const struct kvm_vcpu *vcpu,
> >   static bool access_spe_reg(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
> >   			   const struct sys_reg_desc *r)
> >   {
> > +	u32 encoding = reg_to_encoding(r);
> >   	u64 val = p->regval;
> >   	int reg = r->reg;
> > -	if (p->is_write)
> > +	if (p->is_write) {
> > +		if (WARN_ON_ONCE(encoding == SYS_PMBIDR_EL1 ||
> > +				 encoding == SYS_PMSIDR_EL1))
> > +			return write_to_read_only(vcpu, p, r);
> >   		return kvm_spe_write_sysreg(vcpu, reg, val);
> > +	}
> > -	p->regval = kvm_spe_read_sysreg(vcpu, reg);
> > +	p->regval = kvm_spe_read_sysreg(vcpu, reg, encoding);
> >   	return true;
> >   }
> > @@ -3360,12 +3365,12 @@ static const struct sys_reg_desc sys_reg_descs[] = {
> >   	{ SPE_UNTRAPPED_REG(PMSFCR_EL1) },
> >   	{ SPE_UNTRAPPED_REG(PMSEVFR_EL1) },
> >   	{ SPE_UNTRAPPED_REG(PMSLATFR_EL1) },
> > -	{ SYS_DESC(SYS_PMSIDR_EL1), .access = undef_access },
> > +	{ SYS_DESC(SYS_PMSIDR_EL1), .access = access_spe_reg, .visibility = spe_visibility },
> >   	{ SPE_TRAPPED_REG(PMBLIMITR_EL1) },
> >   	{ SPE_TRAPPED_REG(PMBPTR_EL1) },
> >   	{ SPE_TRAPPED_REG(PMBSR_EL1) },
> >   	{ SPE_UNTRAPPED_REG(PMSDSFR_EL1) },
> > -	{ SYS_DESC(SYS_PMBIDR_EL1), .access = undef_access },
> > +	{ SYS_DESC(SYS_PMBIDR_EL1), .access = access_spe_reg, .visibility = spe_visibility },
> >   	{ PMU_SYS_REG(PMINTENSET_EL1),
> >   	  .access = access_pminten, .reg = PMINTENSET_EL1,
>