Re: [RESEND PATCH] KVM: arm: VGIC: properly initialise private IRQ affinity

[Andre Przywara <andre.przywara@arm.com>]

On Thu, 22 Aug 2019 01:13:32 +0800
Zenghui Yu <yuzenghui@huawei.com> wrote:

Hi,

> On 2019/8/22 1:00, Andre Przywara wrote:
> > At the moment we initialise the target *mask* of a virtual IRQ to the
> > VCPU it belongs to, even though this mask is only defined for GICv2 and
> > quickly runs out of bits for many GICv3 guests.
> > This behaviour triggers an UBSAN complaint for more than 32 VCPUs:
> > ------
> > [ 5659.462377] UBSAN: Undefined behaviour in virt/kvm/arm/vgic/vgic-init.c:223:21
> > [ 5659.471689] shift exponent 32 is too large for 32-bit type 'unsigned int'
> > ------
> > Also for GICv3 guests the reporting of TARGET in the "vgic-state" debugfs
> > dump is wrong, due to this very same problem.
> > 
> > Fix both issues by only initialising vgic_irq->targets for a vGICv2 guest,
> > and by initialising vgic_irq->mpdir for vGICv3 guests instead. We can't
> > use the actual MPIDR for that, as the VCPU's system register is not
> > initialised at this point yet. This is not really an issue, as ->mpidr
> > is just used for the debugfs output and the IROUTER MMIO register, which
> > does not exist in redistributors (dealing with SGIs and PPIs).
> > 
> > Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> > Reported-by: Dave Martin <dave.martin@arm.com>
> > ---
> > Hi,
> > 
> > this came up here again, I think it fell through the cracks back in
> > March:
> > http://lists.infradead.org/pipermail/linux-arm-kernel/2019-March/637209.html
> > 
> > Cheers,
> > Andre.
> > 
> >   virt/kvm/arm/vgic/vgic-init.c | 9 ++++++---
> >   1 file changed, 6 insertions(+), 3 deletions(-)
> > 
> > diff --git a/virt/kvm/arm/vgic/vgic-init.c b/virt/kvm/arm/vgic/vgic-init.c
> > index 80127ca9269f..8bce2f75e0c1 100644
> > --- a/virt/kvm/arm/vgic/vgic-init.c
> > +++ b/virt/kvm/arm/vgic/vgic-init.c
> > @@ -210,7 +210,6 @@ int kvm_vgic_vcpu_init(struct kvm_vcpu *vcpu)
> >   		irq->intid = i;
> >   		irq->vcpu = NULL;
> >   		irq->target_vcpu = vcpu;
> > -		irq->targets = 1U << vcpu->vcpu_id;
> >   		kref_init(&irq->refcount);
> >   		if (vgic_irq_is_sgi(i)) {
> >   			/* SGIs */
> > @@ -221,10 +220,14 @@ int kvm_vgic_vcpu_init(struct kvm_vcpu *vcpu)
> >   			irq->config = VGIC_CONFIG_LEVEL;
> >   		}
> >   
> > -		if (dist->vgic_model == KVM_DEV_TYPE_ARM_VGIC_V3)
> > +		if (dist->vgic_model == KVM_DEV_TYPE_ARM_VGIC_V3) {  
> 
> I still think that if user-space create VCPUs before vGIC (like what
> Qemu does), the actual vGIC model will be unknown here. The UBSAN
> warning will still show up when booting a vGIC-v3 guest (with Qemu).

Yes, you are right. I vaguely remembered this issue, but couldn't find anything on the list about it. So thanks for the heads up!

So I think I have a solution, where we drop the initialisation part here altogether, instead initialise mpdir/targets together with the group in vgic_init(). Unless there is some code which needs irq->group before that point?

Will send a patch shortly.

Cheers,
Andre.

> >   			irq->group = 1;
> > -		else
> > +			/* The actual MPIDR is not initialised at this point. */
> > +			irq->mpidr = 0;
> > +		} else {
> >   			irq->group = 0;
> > +			irq->targets = 1U << vcpu->vcpu_id;
> > +		}
> >   	}
> >   
> >   	if (!irqchip_in_kernel(vcpu->kvm))
> >   
> 

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm