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