Re: [RFC PATCH v7 16/28] KVM: arm64: Add SPE system registers to VCPU context

["Lorenzo Stoakes (ARM)" <ljs@kernel.org>]

On Thu, Sep 03, 2026 at 05:06:11PM +0100, Alexandru Elisei wrote:
> Add the SPE registers to the VCPU context. Guest writes to the service bit,
> which change the state of the SPE maintenance interrupt, will be handled in
> a subsequent patch.
>
> Only the buffer registers are trapped, because KVM will need to keep track

Maybe worth saying which registers for ease?

Is this currently true as of this commit message? As this it is this commit
that's introducing these registers and I don't see anything that enforces
this?

If not, maybe worth saying something like 'ultimately, only the buffer registers
will be trapped. A warning is raised if a trap occurs for

> of the buffer enabled state for dirty page logging, errata handling and to
> sanitise writes to PMBLIMITR_EL1 based on the features exposed to the VM.

I think the commit message needs detail on what each of these means.

I think a key aspect of commit messages is to orient the reader in what the
series is trying to achieve - the code already tells you WHAT is going on but
the WHY and what foundations this patch establishes, etc. are not things that
people can automatically derive from the code.

In this case for instance, coming fresh to it - I wonder 'how does this interact
with dirty page logging, errata handling and santising writes to PMBLIMITR_EL1?
And why does SPE need to touch these things?' etc.

So I think thinking in those terms - giving context and orientating people
really helps drive a good commit message :)

I guess this is a long-winded way of suggesting that you should describe what
teach of these is needed for and how it influenced what you do here :P

>
> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>

Logic looks good here, but various nits and thoughts, and obviously the bot
raised a couple of things also.

> ---
>  arch/arm64/include/asm/kvm_host.h | 13 +++++++
>  arch/arm64/include/asm/kvm_spe.h  | 11 ++++++
>  arch/arm64/kvm/spe.c              | 29 ++++++++++++++++
>  arch/arm64/kvm/sys_regs.c         | 56 ++++++++++++++++++++++++-------
>  4 files changed, 96 insertions(+), 13 deletions(-)
>
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index 10fa223bdbda..b92b19229c36 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -483,6 +483,19 @@ enum vcpu_sysreg {
>  	PMOVSSET_EL0,	/* Overflow Flag Status Set Register */
>  	PMUSERENR_EL0,	/* User Enable Register */
>
> +	/* SPE registers */
> +	PMSCR_EL1,
> +	PMSNEVFR_EL1,
> +	PMSICR_EL1,
> +	PMSIRR_EL1,
> +	PMSFCR_EL1,
> +	PMSEVFR_EL1,
> +	PMSLATFR_EL1,
> +	PMBLIMITR_EL1,
> +	PMBPTR_EL1,
> +	PMBSR_EL1,
> +	PMSDSFR_EL1,

OK so this is establishing the registers that SPE uses.

> +
>  	/* Pointer Authentication Registers in a strict increasing order. */
>  	APIAKEYLO_EL1,
>  	APIAKEYHI_EL1,
> diff --git a/arch/arm64/include/asm/kvm_spe.h b/arch/arm64/include/asm/kvm_spe.h
> index 4bae3e6b05c8..56dc9e660bef 100644
> --- a/arch/arm64/include/asm/kvm_spe.h
> +++ b/arch/arm64/include/asm/kvm_spe.h
> @@ -37,6 +37,9 @@ int kvm_spe_has_attr(struct kvm_vcpu *vcpu, struct kvm_device_attr *attr);
>
>  bool kvm_spe_pmu_set(struct kvm *kvm);
>  u8 kvm_spe_get_pmsver(struct kvm *kvm);
> +
> +bool kvm_spe_write_sysreg(struct kvm_vcpu *vcpu, int reg, u64 val);
> +u64 kvm_spe_read_sysreg(struct kvm_vcpu *vcpu, int reg);
>  #else
>  struct kvm_spe {
>  };
> @@ -81,6 +84,14 @@ static inline u8 kvm_spe_get_pmsver(struct kvm *kvm)
>  {
>  	return 0;
>  }
> +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)
> +{
> +	return 0;
> +}
>  #endif /* CONFIG_KVM_ARM_SPE */
>
>  #endif /* __ARM64_KVM_SPE_H__ */
> diff --git a/arch/arm64/kvm/spe.c b/arch/arm64/kvm/spe.c
> index 4426d614a9e0..6a00a44c73ed 100644
> --- a/arch/arm64/kvm/spe.c
> +++ b/arch/arm64/kvm/spe.c
> @@ -7,6 +7,7 @@
>  #include <linux/kvm_host.h>
>  #include <linux/perf/arm_spe_pmu.h>
>
> +#include <asm/kvm_emulate.h>
>  #include <asm/kvm_spe.h>
>  #include <asm/sysreg.h>
>
> @@ -82,6 +83,34 @@ void kvm_spe_destroy_vm(struct kvm *kvm)
>  	module_put(spe_pmu->pmu.module);
>  }
>
> +bool kvm_spe_write_sysreg(struct kvm_vcpu *vcpu, int reg, u64 val)
> +{
> +	switch (reg) {
> +	case PMBLIMITR_EL1:
> +	case PMBSR_EL1:
> +	case PMBPTR_EL1:
> +		__vcpu_assign_sys_reg(vcpu, reg, val);
> +		break;
> +	default:
> +		WARN_ON_ONCE("unexpected trap");

I see the bot already raised this :)

> +	}
> +
> +	return true;

Hmm, this returns true unconditionally, should it be a void?

> +}
> +
> +u64 kvm_spe_read_sysreg(struct kvm_vcpu *vcpu, int reg)
> +{
> +	switch (reg) {
> +	case PMBLIMITR_EL1:
> +	case PMBSR_EL1:
> +	case PMBPTR_EL1:
> +		return __vcpu_sys_reg(vcpu, reg);

OK so only the buffer registers can be read?

> +	default:
> +		WARN_ON_ONCE("unexpected trap");

Obviously also needs a fixup as per bot :P (I'm pretty sure I've made this
exactly same mistake myself quite a few times so don't worry!)

> +		return 0;
> +	}
> +}
> +
>  bool kvm_spe_pmu_set(struct kvm *kvm)
>  {
>  	return !!kvm->arch.kvm_spe.spe_pmu;
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index a6bb6884b965..89b01b6ac783 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -1554,6 +1554,28 @@ static int set_pmcr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r,
>  	return 0;
>  }
>
> +static unsigned int spe_visibility(const struct kvm_vcpu *vcpu,
> +				   const struct sys_reg_desc *r)
> +{
> +	if (vcpu_has_spe(vcpu))
> +		return 0;
> +
> +	return REG_HIDDEN;
> +}

OK so this, I gather, is simply saying 'if the VCPU supports SPE, allow access
to the registers, otherwise don't.

> +
> +static bool access_spe_reg(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
> +			   const struct sys_reg_desc *r)
> +{
> +	u64 val = p->regval;
> +	int reg = r->reg;
> +
> +	if (p->is_write)
> +		return kvm_spe_write_sysreg(vcpu, reg, val);
> +
> +	p->regval = kvm_spe_read_sysreg(vcpu, reg);
> +	return true;

NIT: I'm not sure the locals are really needed here, especially since you
reassign p->regval, so I think this would be better as:

static bool access_spe_reg(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
			   const struct sys_reg_desc *r)
{
	if (p->is_write)
		return kvm_spe_write_sysreg(vcpu, r->reg, p->regval);

	p->regval = kvm_spe_read_sysreg(vcpu, r->reg);
	return true;
}

> +}
> +
>  /* Silly macro to expand the DBG{BCR,BVR,WVR,WCR}n_EL1 registers in one go */
>  #define DBG_BCR_BVR_WCR_WVR_EL1(n)					\
>  	{ SYS_DESC(SYS_DBGBVRn_EL1(n)),					\
> @@ -1586,6 +1608,14 @@ static int set_pmcr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r,
>  	  .reset = reset_pmevtyper,					\
>  	  .access = access_pmu_evtyper, .reg = (PMEVTYPER0_EL0 + n), }
>
> +#define SPE_SYS_REG(name)						\
> +	SYS_DESC(SYS_##name), .reg = name, .access = access_spe_reg,	\
> +	.reset = reset_val, .val = 0, .visibility = spe_visibility
> +
> +#define SPE_UNTRAPPED_REG(name)						\
> +	SYS_DESC(SYS_##name), .reg = name, .access = undef_access,	\
> +	.reset = reset_val, .val = 0, .visibility = spe_visibility

Could the first just be SPE_REG()? 'SYS' seems a bit redundant here.

And these are identical other than access, could you just make this
SPE_REG() with a parameter for visibility?

Actually, it seems like this pattern already exists, e.g. TIMER_REG():

#define TIMER_REG(name, vis)					   \
	SYS_REG_USER_FILTER(name, access_arch_timer, reset_val, 0, \
			    arch_timer_get_user, arch_timer_set_user, vis)

So this could be something like:

#define SPE_REG(name, acc)						\
	SYS_REG_USER_FILTER(name, acc, reset_val, 0, NULL, NULL,	\
			    spe_visibility)

Perhaps?

> +
>  /* Macro to expand the AMU counter and type registers*/
>  #define AMU_AMEVCNTR0_EL0(n) { SYS_DESC(SYS_AMEVCNTR0_EL0(n)), undef_access }
>  #define AMU_AMEVTYPER0_EL0(n) { SYS_DESC(SYS_AMEVTYPER0_EL0(n)), undef_access }
> @@ -3525,19 +3555,19 @@ static const struct sys_reg_desc sys_reg_descs[] = {
>  	{ SYS_DESC(SYS_FAR_EL1), access_vm_reg, reset_unknown, FAR_EL1 },
>  	{ SYS_DESC(SYS_PAR_EL1), NULL, reset_unknown, PAR_EL1 },
>
> -	{ SYS_DESC(SYS_PMSCR_EL1), undef_access },
> -	{ SYS_DESC(SYS_PMSNEVFR_EL1), undef_access },
> -	{ SYS_DESC(SYS_PMSICR_EL1), undef_access },
> -	{ SYS_DESC(SYS_PMSIRR_EL1), undef_access },
> -	{ SYS_DESC(SYS_PMSFCR_EL1), undef_access },
> -	{ SYS_DESC(SYS_PMSEVFR_EL1), undef_access },
> -	{ SYS_DESC(SYS_PMSLATFR_EL1), undef_access },
> -	{ SYS_DESC(SYS_PMSIDR_EL1), undef_access },
> -	{ SYS_DESC(SYS_PMBLIMITR_EL1), undef_access },
> -	{ SYS_DESC(SYS_PMBPTR_EL1), undef_access },
> -	{ SYS_DESC(SYS_PMBSR_EL1), undef_access },
> -	{ SYS_DESC(SYS_PMSDSFR_EL1), undef_access },
> -	/* PMBIDR_EL1 is not trapped */
> +	{ SPE_UNTRAPPED_REG(PMSCR_EL1) },
> +	{ SPE_UNTRAPPED_REG(PMSNEVFR_EL1) },
> +	{ SPE_UNTRAPPED_REG(PMSICR_EL1) },
> +	{ SPE_UNTRAPPED_REG(PMSIRR_EL1) },
> +	{ SPE_UNTRAPPED_REG(PMSFCR_EL1) },
> +	{ SPE_UNTRAPPED_REG(PMSEVFR_EL1) },
> +	{ SPE_UNTRAPPED_REG(PMSLATFR_EL1) },
> +	{ SYS_DESC(SYS_PMSIDR_EL1), .access = undef_access },
> +	{ SPE_SYS_REG(PMBLIMITR_EL1) },
> +	{ SPE_SYS_REG(PMBPTR_EL1) },
> +	{ SPE_SYS_REG(PMBSR_EL1) },
> +	{ SPE_UNTRAPPED_REG(PMSDSFR_EL1) },
> +	/* PMBIDR_EL1 is not trapped and read-only */
>
>  	{ PMU_SYS_REG(PMINTENSET_EL1),
>  	  .access = access_pminten, .reg = PMINTENSET_EL1,
> --
> 2.43.0
>
>

--
Cheers, Lorenzo