On Wed, 6 Mar 2019 12:42:21 +0100
Christophe de Dinechin <christophe.de.dinechin@xxxxxxxxx> wrote:
> > On 6 Mar 2019, at 11:40, Andre Przywara <andre.przywara@xxxxxxx> 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.
>
> Just for my education, “targets” seems defined as an u8,
> so it looks like you were silently running out of bits before, no?
Yes, but UBSAN only complained about the shift >=32.
If you look at /sys/kernel/debug/kvm/<pid-vcpus>/vgic-state, you will see
that the targets mask for a VCPU ID > 7 is represented as 0 due to the u8
type:
VCPU 0: 1
VCPU 1: 2
VCPU 2: 4
VCPU 3: 8
VCPU 4: 10
VCPU 5: 20
VCPU 6: 40
VCPU 7: 80
VCPU 8: 0
VCPU 9: 0
VCPU 10: 0
...
VCPU 32: 0
VCPU 33: 1
VCPU 34: 2
So this is quite bogus at the moment. This patch should fix the UBSAN
splat and at least avoids the weird representation, by making target "0"
for all private IRQs on a vGICv3. Not sure if there is an easy way to put
in the actual virtual MPIDR there.
Cheers,
Andre.
> > 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@xxxxxxx>
> > Reported-by: Dave Martin <dave.martin@xxxxxxx>
> > ---
> > 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 3bdb31eaed64..3172b2c916f1 100644
> > --- a/virt/kvm/arm/vgic/vgic-init.c
> > +++ b/virt/kvm/arm/vgic/vgic-init.c
> > @@ -220,7 +220,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 */
> > @@ -231,10 +230,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) {
> > 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))
> > --
> > 2.17.1
> >
>
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
