| Message ID | 20190306104008.69414-1-andre.przywara@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: arm: VGIC: properly initialise private IRQ affinity | expand |
> On 6 Mar 2019, at 11:40, Andre Przywara <andre.przywara@arm.com> 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? > 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> > --- > 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 >
On 06/03/2019 11:42, Christophe de Dinechin wrote: > > >> On 6 Mar 2019, at 11:40, Andre Przywara <andre.przywara@arm.com> 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 and no. For a GICv3 guest, this field is never evaluated as a u8, but as the u32 it is associated with (see arm_vgic.h). Past the 8th vcpu, we'd indeed end-up setting this field to zero. But PPIs are per CPU anyway, and we never evaluate the mpidr field for those. So yes, stupid bug, but a fairly harmless one. Thanks, M.
On Wed, 6 Mar 2019 12:42:21 +0100 Christophe de Dinechin <christophe.de.dinechin@gmail.com> wrote: > > On 6 Mar 2019, at 11:40, Andre Przywara <andre.przywara@arm.com> 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@arm.com> > > Reported-by: Dave Martin <dave.martin@arm.com> > > --- > > 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 > > >
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))
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> --- virt/kvm/arm/vgic/vgic-init.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)