[v2] KVM: arm: VGIC: properly initialise private IRQ affinity
diff mbox series

Message ID 20190822170510.167076-1-andre.przywara@arm.com
State New
Headers show
Series
  • [v2] KVM: arm: VGIC: properly initialise private IRQ affinity
Related show

Commit Message

Andre Przywara Aug. 22, 2019, 5:05 p.m. UTC
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.

Because there is no requirement to create the VGIC device before the
VCPUs (and QEMU actually does it the other way round), we can't safely
initialise mpidr or targets in kvm_vgic_vcpu_init(). But since we touch
every private IRQ for each VCPU anyway later (in vgic_init()), we can
just move the initialisation of those fields into there, where we
definitely know the VGIC type.

On the way make sure we really have either a VGICv2 or a VGICv3 device,
since the former checks was just checking for "VGICv3 or not", silently
ignoring the uninitialised case.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
Reported-by: Dave Martin <dave.martin@arm.com>
---
Hi,

tested with 4, 8 and 33 VCPUs with kvmtool and QEMU, on a GICv2 and a
GICv3 machine.
Also briefly tested localhost migration on the GICv3 machine w/ 33
VCPUs, although I think all IRQs are group 1.

Cheers,
Andre

 virt/kvm/arm/vgic/vgic-init.c | 28 ++++++++++++++++++----------
 1 file changed, 18 insertions(+), 10 deletions(-)

Comments

Marc Zyngier Aug. 22, 2019, 7:29 p.m. UTC | #1
On Thu, 22 Aug 2019 18:05:10 +0100
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.
> 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.
> 
> Because there is no requirement to create the VGIC device before the
> VCPUs (and QEMU actually does it the other way round), we can't safely
> initialise mpidr or targets in kvm_vgic_vcpu_init(). But since we touch
> every private IRQ for each VCPU anyway later (in vgic_init()), we can
> just move the initialisation of those fields into there, where we
> definitely know the VGIC type.
> 
> On the way make sure we really have either a VGICv2 or a VGICv3 device,
> since the former checks was just checking for "VGICv3 or not", silently
> ignoring the uninitialised case.
> 
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> Reported-by: Dave Martin <dave.martin@arm.com>
> ---
> Hi,
> 
> tested with 4, 8 and 33 VCPUs with kvmtool and QEMU, on a GICv2 and a
> GICv3 machine.
> Also briefly tested localhost migration on the GICv3 machine w/ 33
> VCPUs, although I think all IRQs are group 1.
> 
> Cheers,
> Andre
> 
>  virt/kvm/arm/vgic/vgic-init.c | 28 ++++++++++++++++++----------
>  1 file changed, 18 insertions(+), 10 deletions(-)
> 
> diff --git a/virt/kvm/arm/vgic/vgic-init.c b/virt/kvm/arm/vgic/vgic-init.c
> index 80127ca9269f..413fb6a5525c 100644
> --- a/virt/kvm/arm/vgic/vgic-init.c
> +++ b/virt/kvm/arm/vgic/vgic-init.c
> @@ -8,6 +8,7 @@
>  #include <linux/cpu.h>
>  #include <linux/kvm_host.h>
>  #include <kvm/arm_vgic.h>
> +#include <asm/kvm_emulate.h>
>  #include <asm/kvm_mmu.h>
>  #include "vgic.h"
>  
> @@ -165,12 +166,17 @@ static int kvm_vgic_dist_init(struct kvm *kvm, unsigned int nr_spis)
>  		irq->vcpu = NULL;
>  		irq->target_vcpu = vcpu0;
>  		kref_init(&irq->refcount);
> -		if (dist->vgic_model == KVM_DEV_TYPE_ARM_VGIC_V2) {
> +		switch (dist->vgic_model) {
> +		case KVM_DEV_TYPE_ARM_VGIC_V2:
>  			irq->targets = 0;
>  			irq->group = 0;
> -		} else {
> +			break;
> +		case KVM_DEV_TYPE_ARM_VGIC_V3:
>  			irq->mpidr = 0;
>  			irq->group = 1;
> +			break;
> +		default:
> +			BUG_ON(1);
>  		}
>  	}
>  	return 0;
> @@ -210,7 +216,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 */
> @@ -220,11 +225,6 @@ int kvm_vgic_vcpu_init(struct kvm_vcpu *vcpu)
>  			/* PPIs */
>  			irq->config = VGIC_CONFIG_LEVEL;
>  		}
> -
> -		if (dist->vgic_model == KVM_DEV_TYPE_ARM_VGIC_V3)
> -			irq->group = 1;
> -		else
> -			irq->group = 0;
>  	}
>  
>  	if (!irqchip_in_kernel(vcpu->kvm))
> @@ -287,10 +287,18 @@ int vgic_init(struct kvm *kvm)
>  
>  		for (i = 0; i < VGIC_NR_PRIVATE_IRQS; i++) {
>  			struct vgic_irq *irq = &vgic_cpu->private_irqs[i];
> -			if (dist->vgic_model == KVM_DEV_TYPE_ARM_VGIC_V3)
> +			switch (dist->vgic_model) {
> +			case KVM_DEV_TYPE_ARM_VGIC_V3:
>  				irq->group = 1;
> -			else
> +				irq->mpidr = kvm_vcpu_get_mpidr_aff(vcpu);
> +				break;
> +			case KVM_DEV_TYPE_ARM_VGIC_V2:
>  				irq->group = 0;
> +				irq->targets = 1U << idx;
> +				break;
> +			default:
> +				BUG_ON(1);
> +			}
>  		}
>  	}
>  

Please drop the BUG_ON()s. If something is unexpected, just fail to
init the guest, but don't kill the box.

Thanks,

	M.

Patch
diff mbox series

diff --git a/virt/kvm/arm/vgic/vgic-init.c b/virt/kvm/arm/vgic/vgic-init.c
index 80127ca9269f..413fb6a5525c 100644
--- a/virt/kvm/arm/vgic/vgic-init.c
+++ b/virt/kvm/arm/vgic/vgic-init.c
@@ -8,6 +8,7 @@ 
 #include <linux/cpu.h>
 #include <linux/kvm_host.h>
 #include <kvm/arm_vgic.h>
+#include <asm/kvm_emulate.h>
 #include <asm/kvm_mmu.h>
 #include "vgic.h"
 
@@ -165,12 +166,17 @@  static int kvm_vgic_dist_init(struct kvm *kvm, unsigned int nr_spis)
 		irq->vcpu = NULL;
 		irq->target_vcpu = vcpu0;
 		kref_init(&irq->refcount);
-		if (dist->vgic_model == KVM_DEV_TYPE_ARM_VGIC_V2) {
+		switch (dist->vgic_model) {
+		case KVM_DEV_TYPE_ARM_VGIC_V2:
 			irq->targets = 0;
 			irq->group = 0;
-		} else {
+			break;
+		case KVM_DEV_TYPE_ARM_VGIC_V3:
 			irq->mpidr = 0;
 			irq->group = 1;
+			break;
+		default:
+			BUG_ON(1);
 		}
 	}
 	return 0;
@@ -210,7 +216,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 */
@@ -220,11 +225,6 @@  int kvm_vgic_vcpu_init(struct kvm_vcpu *vcpu)
 			/* PPIs */
 			irq->config = VGIC_CONFIG_LEVEL;
 		}
-
-		if (dist->vgic_model == KVM_DEV_TYPE_ARM_VGIC_V3)
-			irq->group = 1;
-		else
-			irq->group = 0;
 	}
 
 	if (!irqchip_in_kernel(vcpu->kvm))
@@ -287,10 +287,18 @@  int vgic_init(struct kvm *kvm)
 
 		for (i = 0; i < VGIC_NR_PRIVATE_IRQS; i++) {
 			struct vgic_irq *irq = &vgic_cpu->private_irqs[i];
-			if (dist->vgic_model == KVM_DEV_TYPE_ARM_VGIC_V3)
+			switch (dist->vgic_model) {
+			case KVM_DEV_TYPE_ARM_VGIC_V3:
 				irq->group = 1;
-			else
+				irq->mpidr = kvm_vcpu_get_mpidr_aff(vcpu);
+				break;
+			case KVM_DEV_TYPE_ARM_VGIC_V2:
 				irq->group = 0;
+				irq->targets = 1U << idx;
+				break;
+			default:
+				BUG_ON(1);
+			}
 		}
 	}