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

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

[Andre Przywara]

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(-)

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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Christophe de Dinechin]

> 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
> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Marc Zyngier]

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.
-- 
Jazz is not dead. It just smells funny...

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Andre Przywara]

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
> >   
> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel