Re: [RFC PATCH v7 15/28] KVM: arm64: Use PMSVer from the assigned SPE instance

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

On Thu, Sep 03, 2026 at 05:06:10PM +0100, Alexandru Elisei wrote:
> KVM allows SPE to be virtualized on heterogeneous system, where hardware
> can implement multiple SPE instances for different CPUs, or where SPE
> might even be missing on some of the physical CPUs.
>
> KVM resets the VCPU ID registers following a KVM_ARM_VCPU_INIT ioctl.
> Using the value for ID_AA64DFR0_EL1.PMSVer on the physical CPU where the
> ioctl is executing is unreliable, since userspace might want to later
> assign a different SPE instance, with a different PMSVer value. Worse yet,
> it is be possible that the VCPU reset is being performed on a physical CPU

'it is be' -> 'it is'.

> without SPE. Be consistent and use PMSVer for the SPE instance assigned to
> the VM to initialize ID_AA64DFR0_EL1.PMSVer.
>
> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>

Note that this has to be rebased as 2ce80f624032 changed how all this works :)

> ---
>  arch/arm64/include/asm/kvm_spe.h | 10 ++++++++++
>  arch/arm64/kvm/spe.c             | 24 ++++++++++++++++++++++++
>  arch/arm64/kvm/sys_regs.c        |  8 +++++++-
>  3 files changed, 41 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm64/include/asm/kvm_spe.h b/arch/arm64/include/asm/kvm_spe.h
> index a6999709b298..4bae3e6b05c8 100644
> --- a/arch/arm64/include/asm/kvm_spe.h
> +++ b/arch/arm64/include/asm/kvm_spe.h
> @@ -29,11 +29,14 @@ bool kvm_supports_spe(void);
>  int kvm_spe_vcpu_first_run_init(struct kvm_vcpu *vcpu);
>  void kvm_spe_destroy_vm(struct kvm *kvm);
>
> +void kvm_spe_finalize_idregs(struct kvm *kvm);
> +
>  int kvm_spe_set_attr(struct kvm_vcpu *vcpu, struct kvm_device_attr *attr);
>  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_pmu_set(struct kvm *kvm);
> +u8 kvm_spe_get_pmsver(struct kvm *kvm);

Hmm, the field it returns is u16?

	u16					pmsver;

>  #else
>  struct kvm_spe {
>  };
> @@ -55,6 +58,9 @@ static inline int kvm_spe_vcpu_first_run_init(struct kvm_vcpu *vcpu)
>  static inline void kvm_spe_destroy_vm(struct kvm *kvm)
>  {
>  }
> +static inline void kvm_spe_finalize_idregs(struct kvm *kvm)
> +{
> +}
>  static inline int kvm_spe_set_attr(struct kvm_vcpu *vcpu, struct kvm_device_attr *attr)
>  {
>  	return -ENXIO;
> @@ -71,6 +77,10 @@ static inline bool kvm_spe_pmu_set(struct kvm *kvm)
>  {
>  	return false;
>  }
> +static inline u8 kvm_spe_get_pmsver(struct kvm *kvm)
> +{
> +	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 fa87d224c8af..4426d614a9e0 100644
> --- a/arch/arm64/kvm/spe.c
> +++ b/arch/arm64/kvm/spe.c
> @@ -87,6 +87,30 @@ bool kvm_spe_pmu_set(struct kvm *kvm)
>  	return !!kvm->arch.kvm_spe.spe_pmu;
>  }
>
> +void kvm_spe_finalize_idregs(struct kvm *kvm)
> +{
> +	struct arm_spe_pmu *spe_pmu = kvm->arch.kvm_spe.spe_pmu;
> +	u64 reg;
> +
> +	if (!spe_pmu)
> +		return;
> +
> +	reg = kvm_read_vm_id_reg(kvm, SYS_ID_AA64DFR0_EL1);
> +	reg &= ~ID_AA64DFR0_EL1_PMSVer_MASK;
> +	reg |= SYS_FIELD_PREP(ID_AA64DFR0_EL1, PMSVer, spe_pmu->pmsver);
> +	kvm_set_vm_id_reg(kvm, SYS_ID_AA64DFR0_EL1, reg);
> +}

I know sashiko raised something about this, and mostly you're right to dismiss
those, but I think one issue remains here.

Consider:

1. KVM_ARCH_VCPU_INIT

__kvm_vcpu_set_target()
  -> kvm_reset_vcpu()
    -> ...
      -> __kvm_read_sanitised_id_reg()
        -> sanitise_id_aa64dfr0_el1()
          -> kvm_spe_get_pmsver()
	    No instance assigned yet

	    SET: PMSVer = 0

2. KVM_SET_DEVICE_ATTR(KVM_ARM_VCPU_SPE_ID)

kvm_arm_vcpu_arch_set_attr()
  -> kvm_spe_set_attr()
    -> kvm_spe_set_spe_id()
       Nothing updates the register.

       SET: kvm_spe->spe_pmu = spe_pmu

3. KVM_GET_ONE_REG(ID_AA64DFR0_EL1)

get_id_reg()
  -> read_id_reg()
    -> kvm_read_vm_id_reg()

       RETURNS: PMSVer == 0

4. KVM_SET_ONE_REG(ID_AA64DFR0_EL1) with val from 3 (=0)

set_id_aa64dfr0_el1()
  -> set_id_reg()
    -> arm64_check_features()
       u64 limit = rd->reset(vcpu, rd);
         -> ...
           -> [ same sanitise hook as 1 ]
              kvm_spe->spe_pmu now assigned.

       limit = N (!!)

       Fails, because:

            if ((val & ~mask) != (limit & ~mask))
                return -E2BIG;

       PMSVer isn't in the writable mask, val from step 3 is 0, limit
       is N.

So I think the right solution is to call kvm_spe_finalize_idregs() in
kvm_spe_set_spe_id(), after setting kvm_spe->spe_pmu = spe_pmu.

That way steps 3 and 4 above work, and step 2, which sets the SPE ID,
also sets PMSVer.

> +
> +u8 kvm_spe_get_pmsver(struct kvm *kvm)
> +{
> +	struct arm_spe_pmu *spe_pmu = kvm->arch.kvm_spe.spe_pmu;

NIT: could be const?

> +
> +	if (!spe_pmu)
> +		return 0;
> +
> +	return spe_pmu->pmsver;
> +}
> +
>  static int kvm_spe_update_supported_cpus(struct kvm *kvm, cpumask_t *spe_cpus)
>  {
>  	return kvm_update_supported_cpus(kvm, NULL, spe_cpus);
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index 44aae52c473d..a6bb6884b965 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -2220,8 +2220,11 @@ static u64 sanitise_id_aa64dfr0_el1(const struct kvm_vcpu *vcpu, u64 val)
>  		val |= SYS_FIELD_PREP(ID_AA64DFR0_EL1, PMUVer,
>  				      kvm_arm_pmu_get_pmuver_limit());
>
> -	/* Hide SPE from guests */
> +	/* Only initialize the SPE version if the vCPU has the feature. */
>  	val &= ~ID_AA64DFR0_EL1_PMSVer_MASK;
> +	if (vcpu_has_spe(vcpu))
> +		val |= SYS_FIELD_PREP(ID_AA64DFR0_EL1, PMSVer,
> +				      kvm_spe_get_pmsver(vcpu->kvm));
>
>  	/* Hide BRBE from guests */
>  	val &= ~ID_AA64DFR0_EL1_BRBE_MASK;
> @@ -5928,6 +5931,9 @@ int kvm_finalize_sys_regs(struct kvm_vcpu *vcpu)
>  		kvm_vgic_finalize_idregs(kvm);
>  	}
>
> +	if (vcpu_has_spe(vcpu))
> +		kvm_spe_finalize_idregs(kvm);

kvm_spe_finalize_idregs() already exits if !spe_pmu, so this could be
invoked unconditionally?

> +
>  	return 0;
>  }
>
> --
> 2.43.0
>
>

--
Cheers, Lorenzo