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

From: Marc Zyngier <maz@kernel.org>
To: Zenghui Yu <yuzenghui@huawei.com>
Cc: <kvmarm@lists.linux.dev>, <linux-arm-kernel@lists.infradead.org>,
	<kvm@vger.kernel.org>, James Morse <james.morse@arm.com>,
	Suzuki K Poulose <suzuki.poulose@arm.com>,
	Oliver Upton <oliver.upton@linux.dev>,
	Joey Gouly <joey.gouly@arm.com>,
	Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>,
	Xu Zhao <zhaoxu.35@bytedance.com>,
	Eric Auger <eric.auger@redhat.com>
Subject: Re: [PATCH v2 01/11] KVM: arm64: vgic: Make kvm_vgic_inject_irq() take a vcpu pointer
Date: Thu, 21 Sep 2023 12:20:02 +0100	[thread overview]
Message-ID: <8634z7pycd.wl-maz@kernel.org> (raw)
In-Reply-To: <c511ff67-fd8c-6edd-8239-2bacc3ad16f6@huawei.com>

On Thu, 21 Sep 2023 10:11:00 +0100,
Zenghui Yu <yuzenghui@huawei.com> wrote:
> 
> 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@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?

Good point. Let me fix this.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.