[PATCH v2 01/11] KVM: arm64: vgic: Make kvm_vgic_inject_irq() take a vcpu pointer

[Zenghui Yu]

On 2023/9/21 2:17, Marc Zyngier wrote:
> Passing a vcpu_id to kvm_vgic_inject_irq() is silly for two reasons:
> 
> - we often confuse vcpu_id and vcpu_idx
> - we eventually have to convert it back to a vcpu
> - we can't count
> 
> Instead, pass a vcpu pointer, which is unambiguous. A NULL vcpu
> is also allowed for interrupts that are not private to a vcpu
> (such as SPIs).
> 
> Signed-off-by: Marc Zyngier <maz at kernel.org>
> ---
>  arch/arm64/kvm/arch_timer.c      |  2 +-
>  arch/arm64/kvm/arm.c             | 20 ++++++++++----------
>  arch/arm64/kvm/pmu-emul.c        |  2 +-
>  arch/arm64/kvm/vgic/vgic-irqfd.c |  2 +-
>  arch/arm64/kvm/vgic/vgic.c       | 12 +++++-------
>  include/kvm/arm_vgic.h           |  4 ++--
>  6 files changed, 20 insertions(+), 22 deletions(-)
> 
> diff --git a/arch/arm64/kvm/arch_timer.c b/arch/arm64/kvm/arch_timer.c
> index 6dcdae4d38cb..1f828f3b854c 100644
> --- a/arch/arm64/kvm/arch_timer.c
> +++ b/arch/arm64/kvm/arch_timer.c
> @@ -458,7 +458,7 @@ static void kvm_timer_update_irq(struct kvm_vcpu *vcpu, bool new_level,
>  				   timer_ctx->irq.level);
>  
>  	if (!userspace_irqchip(vcpu->kvm)) {
> -		ret = kvm_vgic_inject_irq(vcpu->kvm, vcpu->vcpu_id,
> +		ret = kvm_vgic_inject_irq(vcpu->kvm, vcpu,
>  					  timer_irq(timer_ctx),
>  					  timer_ctx->irq.level,
>  					  timer_ctx);
> diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
> index 4866b3f7b4ea..872679a0cbd7 100644
> --- a/arch/arm64/kvm/arm.c
> +++ b/arch/arm64/kvm/arm.c
> @@ -1134,27 +1134,27 @@ int kvm_vm_ioctl_irq_line(struct kvm *kvm, struct kvm_irq_level *irq_level,
>  			  bool line_status)
>  {
>  	u32 irq = irq_level->irq;
> -	unsigned int irq_type, vcpu_idx, irq_num;
> +	unsigned int irq_type, vcpu_id, irq_num;
>  	int nrcpus = atomic_read(&kvm->online_vcpus);
>  	struct kvm_vcpu *vcpu = NULL;
>  	bool level = irq_level->level;
>  
>  	irq_type = (irq >> KVM_ARM_IRQ_TYPE_SHIFT) & KVM_ARM_IRQ_TYPE_MASK;
> -	vcpu_idx = (irq >> KVM_ARM_IRQ_VCPU_SHIFT) & KVM_ARM_IRQ_VCPU_MASK;
> -	vcpu_idx += ((irq >> KVM_ARM_IRQ_VCPU2_SHIFT) & KVM_ARM_IRQ_VCPU2_MASK) * (KVM_ARM_IRQ_VCPU_MASK + 1);
> +	vcpu_id = (irq >> KVM_ARM_IRQ_VCPU_SHIFT) & KVM_ARM_IRQ_VCPU_MASK;
> +	vcpu_id += ((irq >> KVM_ARM_IRQ_VCPU2_SHIFT) & KVM_ARM_IRQ_VCPU2_MASK) * (KVM_ARM_IRQ_VCPU_MASK + 1);
>  	irq_num = (irq >> KVM_ARM_IRQ_NUM_SHIFT) & KVM_ARM_IRQ_NUM_MASK;
>  
> -	trace_kvm_irq_line(irq_type, vcpu_idx, irq_num, irq_level->level);
> +	trace_kvm_irq_line(irq_type, vcpu_id, irq_num, irq_level->level);
>  
>  	switch (irq_type) {
>  	case KVM_ARM_IRQ_TYPE_CPU:
>  		if (irqchip_in_kernel(kvm))
>  			return -ENXIO;
>  
> -		if (vcpu_idx >= nrcpus)
> +		if (vcpu_id >= nrcpus)
>  			return -EINVAL;

What we actually need to check is 'vcpu->vcpu_idx >= nrcpus' and this is
covered by the 'if (!vcpu)' statement below. Let's just drop this
_incorrect_ checking?

>  
> -		vcpu = kvm_get_vcpu(kvm, vcpu_idx);
> +		vcpu = kvm_get_vcpu_by_id(kvm, vcpu_id);
>  		if (!vcpu)
>  			return -EINVAL;
>  
> @@ -1166,17 +1166,17 @@ int kvm_vm_ioctl_irq_line(struct kvm *kvm, struct kvm_irq_level *irq_level,
>  		if (!irqchip_in_kernel(kvm))
>  			return -ENXIO;
>  
> -		if (vcpu_idx >= nrcpus)
> +		if (vcpu_id >= nrcpus)
>  			return -EINVAL;

Same here.

Thanks,
Zenghui