Re: [RFC PATCH v7 12/28] KVM: arm64: Add SPE VCPU device attribute to set the interrupt number

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

On Thu, Sep 03, 2026 at 05:06:07PM +0100, Alexandru Elisei wrote:
> From: Sudeep Holla <sudeep.holla@arm.com>
>
> Add KVM_ARM_VCPU_SPE_CTRL(KVM_ARM_VCPU_SPE_IRQ) to allow the user to set
> the interrupt number for the buffer management interrupt.

I know this is a bit of a theme of the review :P but feels a little short.

Be good to see why you want to do this, broadly what you are changing
(shouldn't duplicate stuff you can see from the diff but something like '

>
> [ Alexandru E: Split from "KVM: arm64: Add a new VCPU device control group
>                for SPE" ]
>
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>

I guess similar comment to before - is this a significant change? If so
then probably best to go Co-developed-by route, otherwise not a big deal.

> ---
>  Documentation/virt/kvm/devices/vcpu.rst | 22 +++++++
>  arch/arm64/include/asm/kvm_host.h       |  2 +
>  arch/arm64/include/asm/kvm_spe.h        | 11 ++++
>  arch/arm64/include/uapi/asm/kvm.h       |  1 +
>  arch/arm64/kvm/guest.c                  |  2 +
>  arch/arm64/kvm/spe.c                    | 82 +++++++++++++++++++++++++
>  6 files changed, 120 insertions(+)
>
> diff --git a/Documentation/virt/kvm/devices/vcpu.rst b/Documentation/virt/kvm/devices/vcpu.rst
> index e0a7fa35dfd2..d30b2f7fec9a 100644
> --- a/Documentation/virt/kvm/devices/vcpu.rst
> +++ b/Documentation/virt/kvm/devices/vcpu.rst
> @@ -307,3 +307,25 @@ From the destination VMM process:
>  ===============================
>
>  :Architectures: ARM64
> +
> +5.1 ATTRIBUTE: KVM_ARM_VCPU_SPE_IRQ
> +-----------------------------------
> +
> +:Parameters: in kvm_device_attr.addr the address for the Profiling Buffer
> +             management interrupt number as a pointer to an int
> +
> +Returns:

Was going to say maybe it should be ':Returns:' but actually the doc seems
super inconsistent on this... :P so it doesn't really matter.

> +
> +	 =======  ==========================================================
> +	 -EFAULT  Error accessing the buffer management interrupt number
> +	 -EINVAL  Invalid interrupt number or not using an in-kernel irqchip
> +	 -ENODEV  KVM_ARM_VCPU_HAS_SPE VCPU feature not set

Seems like a typo, should be KVM_ARM_VCPU_SPE_IRQ right?

> +	 -ENXIO   SPE not supported or not properly configured
> +	 =======  ==========================================================
> +
> +Required.
> +
> +Specifies the Profiling Buffer management interrupt number. The interrupt number
> +must be a PPI and the interrupt number must be the same for each VCPU. Arm
> +recommends 21 as the interrupt number. SPE virtualization requires an in-kernel
> +vGIC implementation.
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index 27fe0cd5b2d7..e7332bd82bf3 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -27,6 +27,7 @@
>  #include <asm/fpsimd.h>
>  #include <asm/kvm.h>
>  #include <asm/kvm_asm.h>
> +#include <asm/kvm_spe.h>
>  #include <asm/vncr_mapping.h>
>
>  #define __KVM_HAVE_ARCH_INTC_INITIALIZED
> @@ -918,6 +919,7 @@ struct kvm_vcpu_arch {
>  	struct vgic_cpu vgic_cpu;
>  	struct arch_timer_cpu timer_cpu;
>  	struct kvm_pmu pmu;
> +	struct kvm_vcpu_spe vcpu_spe;
>
>  	/* vcpu power state */
>  	struct kvm_mp_state mp_state;
> diff --git a/arch/arm64/include/asm/kvm_spe.h b/arch/arm64/include/asm/kvm_spe.h
> index 1327064d0984..8ded2d821bff 100644
> --- a/arch/arm64/include/asm/kvm_spe.h
> +++ b/arch/arm64/include/asm/kvm_spe.h
> @@ -6,7 +6,15 @@
>  #ifndef __ARM64_KVM_SPE_H__
>  #define __ARM64_KVM_SPE_H__
>
> +struct kvm;
> +struct kvm_vcpu;
> +struct kvm_device_attr;
> +
>  #ifdef CONFIG_KVM_ARM_SPE
> +struct kvm_vcpu_spe {
> +	int irq_num;		/* Buffer management interrupt number */
> +};

I guess this struct will be expanded over time?

> +
>  bool kvm_supports_spe(void);
>
>  #define vcpu_has_spe(vcpu)					\
> @@ -16,6 +24,9 @@ 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);
>  #else

NIT, and probably applicable to a previous patch, but useful to put /*
CONFIG_KVM_ARM_SPE */ there, certainly useful as things grow.

> +struct kvm_vcpu_spe {
> +};
> +

OK good this covers off the !CONFIG_KVM_ARM_SPE case.

>  static __always_inline bool kvm_supports_spe(void)
>  {
>  	return false;
> diff --git a/arch/arm64/include/uapi/asm/kvm.h b/arch/arm64/include/uapi/asm/kvm.h
> index e8a68e34be63..b9e5019181eb 100644
> --- a/arch/arm64/include/uapi/asm/kvm.h
> +++ b/arch/arm64/include/uapi/asm/kvm.h
> @@ -447,6 +447,7 @@ enum {
>  #define KVM_ARM_VCPU_PVTIME_CTRL	2
>  #define   KVM_ARM_VCPU_PVTIME_IPA	0
>  #define KVM_ARM_VCPU_SPE_CTRL		3
> +#define   KVM_ARM_VCPU_SPE_IRQ		0
>
>  /* KVM_IRQ_LINE irq field index values */
>  #define KVM_ARM_IRQ_VCPU2_SHIFT		28
> diff --git a/arch/arm64/kvm/guest.c b/arch/arm64/kvm/guest.c
> index 56c5f937fcdb..f953b235b458 100644
> --- a/arch/arm64/kvm/guest.c
> +++ b/arch/arm64/kvm/guest.c
> @@ -920,7 +920,9 @@ int kvm_arm_vcpu_arch_set_attr(struct kvm_vcpu *vcpu,
>  		ret = kvm_arm_pvtime_set_attr(vcpu, attr);
>  		break;
>  	case KVM_ARM_VCPU_SPE_CTRL:
> +		mutex_lock(&vcpu->kvm->arch.config_lock);
>  		ret = kvm_spe_set_attr(vcpu, attr);
> +		mutex_unlock(&vcpu->kvm->arch.config_lock);
>  		break;
>  	default:
>  		ret = -ENXIO;
> diff --git a/arch/arm64/kvm/spe.c b/arch/arm64/kvm/spe.c
> index fe45ad4c703a..dd5f80318ba4 100644
> --- a/arch/arm64/kvm/spe.c
> +++ b/arch/arm64/kvm/spe.c
> @@ -59,17 +59,99 @@ bool kvm_supports_spe(void)
>  	return !list_empty(&spe_pmus);
>  }
>
> +static bool kvm_spe_irq_is_valid(struct kvm *kvm, int irq)
> +{
> +	struct kvm_vcpu *vcpu;
> +	struct kvm_vcpu_spe *vcpu_spe;
> +	unsigned long i;
> +

Maybe worth a small comment:

	/* Every vCPU must have either the same or no IRQ set. */

> +	kvm_for_each_vcpu(i, vcpu, kvm) {
> +		vcpu_spe = &vcpu->arch.vcpu_spe;
> +
> +		if (!vcpu_spe->irq_num)
> +			continue;
> +
> +		if (vcpu_spe->irq_num != irq)
> +			return false;
> +	}
> +
> +	return true;
> +}
> +

>  int kvm_spe_set_attr(struct kvm_vcpu *vcpu, struct kvm_device_attr *attr)
>  {
> +	struct kvm_vcpu_spe *vcpu_spe = &vcpu->arch.vcpu_spe;
> +	struct kvm *kvm = vcpu->kvm;
> +
> +	lockdep_assert_held(&kvm->arch.config_lock);
> +
> +	if (!vcpu_has_spe(vcpu))
> +		return -ENODEV;
> +
> +	switch (attr->attr) {
> +	case KVM_ARM_VCPU_SPE_IRQ: {
> +		int __user *uaddr = (int __user *)(long)attr->addr;
> +		int irq;
> +
> +		if (!irqchip_in_kernel(kvm))
> +			return -EINVAL;
> +
> +		if (get_user(irq, uaddr))
> +			return -EFAULT;
> +
> +		if (!irq_is_ppi(kvm, irq))
> +			return -EINVAL;
> +
> +		if (!kvm_spe_irq_is_valid(kvm, irq))
> +			return -EINVAL;

What about changing the IRQ for vcpu? kvm_spe_irq_is_valid() will abort
even if only the current vCPU has an IRQ set.

Also what about the case of setting the current vCPU's IRQ to the same
value? Should that be -EBUSY?

PMU seems to do something like that, so maybe worth doing that for
consistency? Not exactly a huge thing, that though!

Also looking at PMU there's:

	/* On GICv5, the PMUIRQ is architecturally mandated to be PPI 23 */
	if (vgic_is_v5(kvm) && irq != KVM_ARMV8_PMU_GICV5_IRQ)
		return false;

Is that something you have to worry about for SPE?


> +
> +		vcpu_spe->irq_num = irq;
> +		return 0;
> +	}
> +	}
> +
>  	return -ENXIO;
>  }
>
>  int kvm_spe_get_attr(struct kvm_vcpu *vcpu, struct kvm_device_attr *attr)
>  {
> +	struct kvm_vcpu_spe *vcpu_spe = &vcpu->arch.vcpu_spe;
> +	struct kvm *kvm = vcpu->kvm;
> +
> +	if (!vcpu_has_spe(vcpu))
> +		return -ENODEV;
> +
> +	switch (attr->attr) {
> +	case KVM_ARM_VCPU_SPE_IRQ: {
> +		int __user *uaddr = (int __user *)(long)attr->addr;
> +		int irq;
> +
> +		if (!irqchip_in_kernel(kvm))
> +			return -EINVAL;
> +
> +		if (!vcpu_spe->irq_num)
> +			return -ENXIO;
> +
> +		irq = vcpu_spe->irq_num;
> +		if (put_user(irq, uaddr))
> +			return -EFAULT;
> +
> +		return 0;
> +	}
> +	}
> +
>  	return -ENXIO;
>  }
>
>  int kvm_spe_has_attr(struct kvm_vcpu *vcpu, struct kvm_device_attr *attr)
>  {
> +	if (!vcpu_has_spe(vcpu))
> +		return -ENODEV;
> +
> +	switch(attr->attr) {
> +	case KVM_ARM_VCPU_SPE_IRQ:
> +		return 0;
> +	}
> +
>  	return -ENXIO;
>  }
> --
> 2.43.0
>
>

--
Cheers, Lorenzo