[RFC PATCH 0/5] arm/arm64: KVM: dynamic VGIC sizing

[RFC PATCH 0/5] arm/arm64: KVM: dynamic VGIC sizing

[Marc Zyngier]

So far, the VGIC data structures have been statically sized, meaning
that we always have to support more interrupts than we actually want,
and more CPU interfaces than we should.

This is a waste of resource, and is the kind of things that should be
tuneable. This series partially addresses that issue by changing the
data structures to be dynamically allocated.

The various sizes are still static, but the VGIC code itself doesn't
have any reference to these anymore. Once plugged into the new device
API, we should be able to size it entierely dynamically.

I'm hopping for this series to be merged with Christoffer's VGIC
device control API.

This has been (lightly) tested on both ARM (TC2) and arm64 (model).

     M.

Marc Zyngier (5):
  arm/arm64: KVM: vgic: switch to dynamic allocation
  arm/arm64: KVM: vgic: kill VGIC_NR_SHARED_IRQS
  arm/arm64: KVM: vgic: kill VGIC_MAX_CPUS
  arm/arm64: KVM: vgic: handle out-of-range MMIO accesses
  arm/arm64: KVM: vgic: kill VGIC_NR_IRQS

 arch/arm/kvm/arm.c     |   5 +-
 include/kvm/arm_vgic.h |  49 ++++-----
 virt/kvm/arm/vgic.c    | 263 ++++++++++++++++++++++++++++++++++++++++---------
 3 files changed, 249 insertions(+), 68 deletions(-)

-- 
1.8.2.3

[RFC PATCH 1/5] arm/arm64: KVM: vgic: switch to dynamic allocation

[Marc Zyngier]

So far, all the VGIC data structures are statically defined by the
*maximum* number of vcpus and interrupts it supports. It means that
we always have to oversize it to cater for the worse case.

Start by changing the data structures to be dynamically sizeable,
and allocate them at runtime.

The sizes are still very static though.

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 arch/arm/kvm/arm.c     |   5 +-
 include/kvm/arm_vgic.h |  38 ++++++-----
 virt/kvm/arm/vgic.c    | 180 +++++++++++++++++++++++++++++++++++++++++++------
 3 files changed, 184 insertions(+), 39 deletions(-)

diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
index 9c697db..f0f7a8a 100644
--- a/arch/arm/kvm/arm.c
+++ b/arch/arm/kvm/arm.c
@@ -178,6 +178,8 @@ void kvm_arch_destroy_vm(struct kvm *kvm)
 			kvm->vcpus[i] = NULL;
 		}
 	}
+
+	kvm_vgic_destroy(kvm);
 }
 
 int kvm_dev_ioctl_check_extension(long ext)
@@ -284,6 +286,7 @@ void kvm_arch_vcpu_free(struct kvm_vcpu *vcpu)
 {
 	kvm_mmu_free_memory_caches(vcpu);
 	kvm_timer_vcpu_terminate(vcpu);
+	kvm_vgic_vcpu_destroy(vcpu);
 	kmem_cache_free(kvm_vcpu_cache, vcpu);
 }
 
@@ -786,7 +789,7 @@ long kvm_arch_vm_ioctl(struct file *filp,
 	switch (ioctl) {
 	case KVM_CREATE_IRQCHIP: {
 		if (vgic_present)
-			return kvm_vgic_create(kvm);
+			return kvm_vgic_create(kvm, VGIC_MAX_CPUS, VGIC_NR_IRQS);
 		else
 			return -ENXIO;
 	}
diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
index 7e2d158..3cc3a9f 100644
--- a/include/kvm/arm_vgic.h
+++ b/include/kvm/arm_vgic.h
@@ -53,19 +53,18 @@
  * - a bunch of shared interrupts (SPI)
  */
 struct vgic_bitmap {
-	union {
-		u32 reg[VGIC_NR_PRIVATE_IRQS / 32];
-		DECLARE_BITMAP(reg_ul, VGIC_NR_PRIVATE_IRQS);
-	} percpu[VGIC_MAX_CPUS];
-	union {
-		u32 reg[VGIC_NR_SHARED_IRQS / 32];
-		DECLARE_BITMAP(reg_ul, VGIC_NR_SHARED_IRQS);
-	} shared;
+	/*
+	 * - One UL per VCPU for private interrupts (assumes UL is at
+	 * least 32 bits)
+	 * - As many UL as necessary for shared interrupts.
+	 */
+	int nr_cpus;
+	unsigned long *bits;
 };
 
 struct vgic_bytemap {
-	u32 percpu[VGIC_MAX_CPUS][VGIC_NR_PRIVATE_IRQS / 4];
-	u32 shared[VGIC_NR_SHARED_IRQS  / 4];
+	int nr_cpus;
+	u32 *regs;
 };
 
 struct vgic_dist {
@@ -73,6 +72,9 @@ struct vgic_dist {
 	spinlock_t		lock;
 	bool			ready;
 
+	int			nr_cpus;
+	int			nr_irqs;
+
 	/* Virtual control interface mapping */
 	void __iomem		*vctrl_base;
 
@@ -98,12 +100,12 @@ struct vgic_dist {
 	/* Level/edge triggered */
 	struct vgic_bitmap	irq_cfg;
 
-	/* Source CPU per SGI and target CPU */
-	u8			irq_sgi_sources[VGIC_MAX_CPUS][VGIC_NR_SGIS];
+	/* Source CPU per SGI and target CPU : 16 bytes per CPU */
+	u8			*irq_sgi_sources;
 
 	/* Target CPU for each IRQ */
-	u8			irq_spi_cpu[VGIC_NR_SHARED_IRQS];
-	struct vgic_bitmap	irq_spi_target[VGIC_MAX_CPUS];
+	u8			*irq_spi_cpu;
+	struct vgic_bitmap	*irq_spi_target;
 
 	/* Bitmap indicating which CPU has something pending */
 	unsigned long		irq_pending_on_cpu;
@@ -113,11 +115,11 @@ struct vgic_dist {
 struct vgic_cpu {
 #ifdef CONFIG_KVM_ARM_VGIC
 	/* per IRQ to LR mapping */
-	u8		vgic_irq_lr_map[VGIC_NR_IRQS];
+	u8		*vgic_irq_lr_map;
 
 	/* Pending interrupts on this VCPU */
 	DECLARE_BITMAP(	pending_percpu, VGIC_NR_PRIVATE_IRQS);
-	DECLARE_BITMAP(	pending_shared, VGIC_NR_SHARED_IRQS);
+	unsigned long	*pending_shared;
 
 	/* Bitmap of used/free list registers */
 	DECLARE_BITMAP(	lr_used, VGIC_MAX_LRS);
@@ -147,8 +149,10 @@ struct kvm_exit_mmio;
 int kvm_vgic_set_addr(struct kvm *kvm, unsigned long type, u64 addr);
 int kvm_vgic_hyp_init(void);
 int kvm_vgic_init(struct kvm *kvm);
-int kvm_vgic_create(struct kvm *kvm);
+int kvm_vgic_create(struct kvm *kvm, int nr_cpus, int nr_irqs);
+void kvm_vgic_destroy(struct kvm *kvm);
 int kvm_vgic_vcpu_init(struct kvm_vcpu *vcpu);
+void kvm_vgic_vcpu_destroy(struct kvm_vcpu *vcpu);
 void kvm_vgic_flush_hwstate(struct kvm_vcpu *vcpu);
 void kvm_vgic_sync_hwstate(struct kvm_vcpu *vcpu);
 int kvm_vgic_inject_irq(struct kvm *kvm, int cpuid, unsigned int irq_num,
diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
index 685fc72..f16c848 100644
--- a/virt/kvm/arm/vgic.c
+++ b/virt/kvm/arm/vgic.c
@@ -96,34 +96,54 @@ static u32 vgic_nr_lr;
 
 static unsigned int vgic_maint_irq;
 
+static int vgic_init_bitmap(struct vgic_bitmap *b, int nr_cpus, int nr_irqs)
+{
+	int nr_longs;
+
+	nr_longs = nr_cpus + BITS_TO_LONGS(nr_irqs - VGIC_NR_PRIVATE_IRQS);
+
+	b->bits = kzalloc(sizeof(unsigned long) * nr_longs, GFP_KERNEL);
+	if (!b->bits)
+		return -ENOMEM;
+
+	b->nr_cpus = nr_cpus;
+	return 0;
+}
+
+static void vgic_free_bitmap(struct vgic_bitmap *b)
+{
+	kfree(b->bits);
+}
+
 static u32 *vgic_bitmap_get_reg(struct vgic_bitmap *x,
 				int cpuid, u32 offset)
 {
 	offset >>= 2;
 	if (!offset)
-		return x->percpu[cpuid].reg;
+		return (u32 *)(x->bits + cpuid);
 	else
-		return x->shared.reg + offset - 1;
+		return (u32 *)(x->bits + x->nr_cpus) + offset - 1;
 }
 
 static int vgic_bitmap_get_irq_val(struct vgic_bitmap *x,
 				   int cpuid, int irq)
 {
 	if (irq < VGIC_NR_PRIVATE_IRQS)
-		return test_bit(irq, x->percpu[cpuid].reg_ul);
+		return test_bit(irq, x->bits + cpuid);
 
-	return test_bit(irq - VGIC_NR_PRIVATE_IRQS, x->shared.reg_ul);
+	return test_bit(irq - VGIC_NR_PRIVATE_IRQS, x->bits + x->nr_cpus);
 }
 
 static void vgic_bitmap_set_irq_val(struct vgic_bitmap *x, int cpuid,
 				    int irq, int val)
 {
-	unsigned long *reg;
+	unsigned long *reg = x->bits;
 
+	BUG_ON(!reg);
 	if (irq < VGIC_NR_PRIVATE_IRQS) {
-		reg = x->percpu[cpuid].reg_ul;
+		reg += cpuid;
 	} else {
-		reg =  x->shared.reg_ul;
+		reg += x->nr_cpus;
 		irq -= VGIC_NR_PRIVATE_IRQS;
 	}
 
@@ -135,24 +155,42 @@ static void vgic_bitmap_set_irq_val(struct vgic_bitmap *x, int cpuid,
 
 static unsigned long *vgic_bitmap_get_cpu_map(struct vgic_bitmap *x, int cpuid)
 {
-	if (unlikely(cpuid >= VGIC_MAX_CPUS))
-		return NULL;
-	return x->percpu[cpuid].reg_ul;
+	return x->bits + cpuid;
 }
 
 static unsigned long *vgic_bitmap_get_shared_map(struct vgic_bitmap *x)
 {
-	return x->shared.reg_ul;
+	return x->bits + x->nr_cpus;
+}
+
+static int vgic_init_bytemap(struct vgic_bytemap *x, int nr_cpus, int nr_irqs)
+{
+	int size;
+
+	size  = nr_cpus * VGIC_NR_PRIVATE_IRQS;
+	size += nr_irqs - VGIC_NR_PRIVATE_IRQS;
+
+	x->regs = kzalloc(size, GFP_KERNEL);
+	if (!x->regs)
+		return -ENOMEM;
+
+	x->nr_cpus = nr_cpus;
+	return 0;
+}
+
+static void vgic_free_bytemap(struct vgic_bytemap *b)
+{
+	kfree(b->regs);
 }
 
 static u32 *vgic_bytemap_get_reg(struct vgic_bytemap *x, int cpuid, u32 offset)
 {
-	offset >>= 2;
-	BUG_ON(offset > (VGIC_NR_IRQS / 4));
-	if (offset < 8)
-		return x->percpu[cpuid] + offset;
+	if (offset < 32)
+		offset += cpuid * VGIC_NR_PRIVATE_IRQS;
 	else
-		return x->shared + offset - 8;
+		offset += (x->nr_cpus - 1) * VGIC_NR_PRIVATE_IRQS;
+
+	return x->regs + (offset / sizeof(u32));
 }
 
 #define VGIC_CFG_LEVEL	0
@@ -733,6 +771,11 @@ bool vgic_handle_mmio(struct kvm_vcpu *vcpu, struct kvm_run *run,
 	return true;
 }
 
+static u8 *vgic_get_sgi_sources(struct vgic_dist *dist, int vcpu_id, int sgi)
+{
+	return dist->irq_sgi_sources + vcpu_id * VGIC_NR_SGIS + sgi;
+}
+
 static void vgic_dispatch_sgi(struct kvm_vcpu *vcpu, u32 reg)
 {
 	struct kvm *kvm = vcpu->kvm;
@@ -765,7 +808,7 @@ static void vgic_dispatch_sgi(struct kvm_vcpu *vcpu, u32 reg)
 		if (target_cpus & 1) {
 			/* Flag the SGI as pending */
 			vgic_dist_irq_set(vcpu, sgi);
-			dist->irq_sgi_sources[c][sgi] |= 1 << vcpu_id;
+			*vgic_get_sgi_sources(dist, c, sgi) |= 1 << vcpu_id;
 			kvm_debug("SGI%d from CPU%d to CPU%d\n", sgi, vcpu_id, c);
 		}
 
@@ -908,14 +951,14 @@ static bool vgic_queue_sgi(struct kvm_vcpu *vcpu, int irq)
 	int vcpu_id = vcpu->vcpu_id;
 	int c;
 
-	sources = dist->irq_sgi_sources[vcpu_id][irq];
+	sources = *vgic_get_sgi_sources(dist, vcpu_id, irq);
 
 	for_each_set_bit(c, &sources, VGIC_MAX_CPUS) {
 		if (vgic_queue_irq(vcpu, c, irq))
 			clear_bit(c, &sources);
 	}
 
-	dist->irq_sgi_sources[vcpu_id][irq] = sources;
+	*vgic_get_sgi_sources(dist, vcpu_id, irq) = sources;
 
 	/*
 	 * If the sources bitmap has been cleared it means that we
@@ -1243,15 +1286,44 @@ static irqreturn_t vgic_maintenance_handler(int irq, void *data)
 	return IRQ_HANDLED;
 }
 
+static void vgic_vcpu_free_maps(struct vgic_cpu *vgic_cpu)
+{
+	kfree(vgic_cpu->pending_shared);
+	kfree(vgic_cpu->vgic_irq_lr_map);
+}
+
+static int vgic_vcpu_init_maps(struct vgic_cpu *vgic_cpu, int nr_irqs)
+{
+	vgic_cpu->pending_shared = kzalloc(nr_irqs - VGIC_NR_PRIVATE_IRQS,
+					   GFP_KERNEL);
+	vgic_cpu->vgic_irq_lr_map = kzalloc(nr_irqs, GFP_KERNEL);
+
+	if (!vgic_cpu->pending_shared || !vgic_cpu->vgic_irq_lr_map) {
+		vgic_vcpu_free_maps(vgic_cpu);
+		return -ENOMEM;
+	}
+
+	return 0;
+}
+
+void kvm_vgic_vcpu_destroy(struct kvm_vcpu *vcpu)
+{
+	vgic_vcpu_free_maps(&vcpu->arch.vgic_cpu);
+}
+
 int kvm_vgic_vcpu_init(struct kvm_vcpu *vcpu)
 {
 	struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu;
 	struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
-	int i;
+	int i, ret;
 
 	if (!irqchip_in_kernel(vcpu->kvm))
 		return 0;
 
+	ret = vgic_vcpu_init_maps(vgic_cpu, dist->nr_irqs);
+	if (ret)
+		return ret;
+
 	if (vcpu->vcpu_id >= VGIC_MAX_CPUS)
 		return -EBUSY;
 
@@ -1383,6 +1455,68 @@ out:
 	return ret;
 }
 
+static void vgic_free_maps(struct vgic_dist *dist)
+{
+	int i;
+
+	vgic_free_bitmap(&dist->irq_enabled);
+	vgic_free_bitmap(&dist->irq_state);
+	vgic_free_bitmap(&dist->irq_active);
+	vgic_free_bitmap(&dist->irq_cfg);
+	vgic_free_bytemap(&dist->irq_priority);
+	if (dist->irq_spi_target)
+		for (i = 0; i < dist->nr_cpus; i++)
+			vgic_free_bitmap(&dist->irq_spi_target[i]);
+	kfree(dist->irq_sgi_sources);
+	kfree(dist->irq_spi_cpu);
+	kfree(dist->irq_spi_target);
+}
+
+static int vgic_init_maps(struct vgic_dist *dist, int nr_cpus, int nr_irqs)
+{
+	int ret = 0, i;
+
+	dist->nr_cpus = nr_cpus;
+	dist->nr_irqs = nr_irqs;
+
+	ret  = vgic_init_bitmap(&dist->irq_enabled, nr_cpus, nr_irqs);
+	ret |= vgic_init_bitmap(&dist->irq_state, nr_cpus, nr_irqs);
+	ret |= vgic_init_bitmap(&dist->irq_active, nr_cpus, nr_irqs);
+	ret |= vgic_init_bitmap(&dist->irq_cfg, nr_cpus, nr_irqs);
+	ret |= vgic_init_bytemap(&dist->irq_priority, nr_cpus, nr_irqs);
+
+	if (!ret) {
+		dist->irq_sgi_sources = kzalloc(nr_cpus * VGIC_NR_SGIS,
+						GFP_KERNEL);
+		dist->irq_spi_cpu = kzalloc(nr_irqs - VGIC_NR_PRIVATE_IRQS,
+					    GFP_KERNEL);
+		dist->irq_spi_target = kzalloc(sizeof(*dist->irq_spi_target) * nr_cpus,
+					       GFP_KERNEL);
+		if (!dist->irq_sgi_sources ||
+		    !dist->irq_spi_cpu ||
+		    !dist->irq_spi_target) {
+			ret = -ENOMEM;
+			goto out;
+		}
+
+		for (i = 0; i < nr_cpus; i++)
+			ret |= vgic_init_bitmap(&dist->irq_spi_target[i],
+						nr_cpus, nr_irqs);
+
+	}
+
+out:
+	if (ret)
+		vgic_free_maps(dist);
+
+	return ret;
+}
+
+void kvm_vgic_destroy(struct kvm *kvm)
+{
+	vgic_free_maps(&kvm->arch.vgic);
+}
+
 int kvm_vgic_init(struct kvm *kvm)
 {
 	int ret = 0, i;
@@ -1416,7 +1550,7 @@ out:
 	return ret;
 }
 
-int kvm_vgic_create(struct kvm *kvm)
+int kvm_vgic_create(struct kvm *kvm, int nr_cpus, int nr_irqs)
 {
 	int ret = 0;
 
@@ -1432,6 +1566,10 @@ int kvm_vgic_create(struct kvm *kvm)
 	kvm->arch.vgic.vgic_dist_base = VGIC_ADDR_UNDEF;
 	kvm->arch.vgic.vgic_cpu_base = VGIC_ADDR_UNDEF;
 
+	ret = vgic_init_maps(&kvm->arch.vgic, nr_cpus, nr_irqs);
+	if (ret)
+		kvm_err("Unable to allocate maps\n");
+
 out:
 	mutex_unlock(&kvm->lock);
 	return ret;
-- 
1.8.2.3

[RFC PATCH 1/5] arm/arm64: KVM: vgic: switch to dynamic allocation

[Christoffer Dall]

On Fri, Oct 04, 2013 at 01:16:14PM +0100, Marc Zyngier wrote:
> So far, all the VGIC data structures are statically defined by the
> *maximum* number of vcpus and interrupts it supports. It means that
> we always have to oversize it to cater for the worse case.
> 
> Start by changing the data structures to be dynamically sizeable,
> and allocate them at runtime.
> 
> The sizes are still very static though.
> 
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> ---
>  arch/arm/kvm/arm.c     |   5 +-
>  include/kvm/arm_vgic.h |  38 ++++++-----
>  virt/kvm/arm/vgic.c    | 180 +++++++++++++++++++++++++++++++++++++++++++------
>  3 files changed, 184 insertions(+), 39 deletions(-)
> 
> diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
> index 9c697db..f0f7a8a 100644
> --- a/arch/arm/kvm/arm.c
> +++ b/arch/arm/kvm/arm.c
> @@ -178,6 +178,8 @@ void kvm_arch_destroy_vm(struct kvm *kvm)
>  			kvm->vcpus[i] = NULL;
>  		}
>  	}
> +
> +	kvm_vgic_destroy(kvm);
>  }
>  
>  int kvm_dev_ioctl_check_extension(long ext)
> @@ -284,6 +286,7 @@ void kvm_arch_vcpu_free(struct kvm_vcpu *vcpu)
>  {
>  	kvm_mmu_free_memory_caches(vcpu);
>  	kvm_timer_vcpu_terminate(vcpu);
> +	kvm_vgic_vcpu_destroy(vcpu);
>  	kmem_cache_free(kvm_vcpu_cache, vcpu);
>  }
>  
> @@ -786,7 +789,7 @@ long kvm_arch_vm_ioctl(struct file *filp,
>  	switch (ioctl) {
>  	case KVM_CREATE_IRQCHIP: {
>  		if (vgic_present)
> -			return kvm_vgic_create(kvm);
> +			return kvm_vgic_create(kvm, VGIC_MAX_CPUS, VGIC_NR_IRQS);
>  		else
>  			return -ENXIO;
>  	}
> diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
> index 7e2d158..3cc3a9f 100644
> --- a/include/kvm/arm_vgic.h
> +++ b/include/kvm/arm_vgic.h
> @@ -53,19 +53,18 @@
>   * - a bunch of shared interrupts (SPI)
>   */
>  struct vgic_bitmap {
> -	union {
> -		u32 reg[VGIC_NR_PRIVATE_IRQS / 32];
> -		DECLARE_BITMAP(reg_ul, VGIC_NR_PRIVATE_IRQS);
> -	} percpu[VGIC_MAX_CPUS];
> -	union {
> -		u32 reg[VGIC_NR_SHARED_IRQS / 32];
> -		DECLARE_BITMAP(reg_ul, VGIC_NR_SHARED_IRQS);
> -	} shared;
> +	/*
> +	 * - One UL per VCPU for private interrupts (assumes UL is at
> +	 * least 32 bits)
> +	 * - As many UL as necessary for shared interrupts.
> +	 */
> +	int nr_cpus;
> +	unsigned long *bits;

Is this much slower or make for less elegant code if we had a separate
pointer for percpu and shared?

Also, I assume the first UL will be for vcpu 0, etc.

I liked the struct layout we had before
because it was self-documenting.

>  };
>  
>  struct vgic_bytemap {
> -	u32 percpu[VGIC_MAX_CPUS][VGIC_NR_PRIVATE_IRQS / 4];
> -	u32 shared[VGIC_NR_SHARED_IRQS  / 4];
> +	int nr_cpus;
> +	u32 *regs;

We're duplicating this state around so we can always just pass the
b[yte/it]map pointer to accessor functions right?

Can we document the layout of the *regs in this struct as well?

>  };
>  
>  struct vgic_dist {
> @@ -73,6 +72,9 @@ struct vgic_dist {
>  	spinlock_t		lock;
>  	bool			ready;
>  
> +	int			nr_cpus;
> +	int			nr_irqs;
> +
>  	/* Virtual control interface mapping */
>  	void __iomem		*vctrl_base;
>  
> @@ -98,12 +100,12 @@ struct vgic_dist {
>  	/* Level/edge triggered */
>  	struct vgic_bitmap	irq_cfg;
>  
> -	/* Source CPU per SGI and target CPU */
> -	u8			irq_sgi_sources[VGIC_MAX_CPUS][VGIC_NR_SGIS];
> +	/* Source CPU per SGI and target CPU : 16 bytes per CPU */
> +	u8			*irq_sgi_sources;

>From the accessor below it looks like the ordering is CPU0, SGI0->SGI16,
CPU1, SGI... and so on, so 

if I want to know the sources for SGI n for CPU x I get it by

*(irq_sgi_sources + (x * VGIC_NR_SGIS) + n) ?

I sort of like to have the data structure definiton clear without having
to go look at code.

>  
>  	/* Target CPU for each IRQ */
> -	u8			irq_spi_cpu[VGIC_NR_SHARED_IRQS];
> -	struct vgic_bitmap	irq_spi_target[VGIC_MAX_CPUS];
> +	u8			*irq_spi_cpu;
> +	struct vgic_bitmap	*irq_spi_target;

without the array numbers it actually becomes hard to guess what these
do?  I assume this is still an array of the structs, just that we are
pointing to n consecutive elements of the structure instead?

Could we add something like the above before the irq_spi_target:

/* Reverse lookup of irq_spu_cpu for faster compute pending */ ?



>  
>  	/* Bitmap indicating which CPU has something pending */
>  	unsigned long		irq_pending_on_cpu;
> @@ -113,11 +115,11 @@ struct vgic_dist {
>  struct vgic_cpu {
>  #ifdef CONFIG_KVM_ARM_VGIC
>  	/* per IRQ to LR mapping */
> -	u8		vgic_irq_lr_map[VGIC_NR_IRQS];
> +	u8		*vgic_irq_lr_map;
>  
>  	/* Pending interrupts on this VCPU */

We could add "Pending interrupt bitmaps on this VCPU" to be crystal
clear.

>  	DECLARE_BITMAP(	pending_percpu, VGIC_NR_PRIVATE_IRQS);
> -	DECLARE_BITMAP(	pending_shared, VGIC_NR_SHARED_IRQS);
> +	unsigned long	*pending_shared;
>  
>  	/* Bitmap of used/free list registers */
>  	DECLARE_BITMAP(	lr_used, VGIC_MAX_LRS);
> @@ -147,8 +149,10 @@ struct kvm_exit_mmio;
>  int kvm_vgic_set_addr(struct kvm *kvm, unsigned long type, u64 addr);
>  int kvm_vgic_hyp_init(void);
>  int kvm_vgic_init(struct kvm *kvm);
> -int kvm_vgic_create(struct kvm *kvm);
> +int kvm_vgic_create(struct kvm *kvm, int nr_cpus, int nr_irqs);
> +void kvm_vgic_destroy(struct kvm *kvm);
>  int kvm_vgic_vcpu_init(struct kvm_vcpu *vcpu);
> +void kvm_vgic_vcpu_destroy(struct kvm_vcpu *vcpu);
>  void kvm_vgic_flush_hwstate(struct kvm_vcpu *vcpu);
>  void kvm_vgic_sync_hwstate(struct kvm_vcpu *vcpu);
>  int kvm_vgic_inject_irq(struct kvm *kvm, int cpuid, unsigned int irq_num,
> diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
> index 685fc72..f16c848 100644
> --- a/virt/kvm/arm/vgic.c
> +++ b/virt/kvm/arm/vgic.c
> @@ -96,34 +96,54 @@ static u32 vgic_nr_lr;
>  
>  static unsigned int vgic_maint_irq;
>  
> +static int vgic_init_bitmap(struct vgic_bitmap *b, int nr_cpus, int nr_irqs)
> +{
> +	int nr_longs;
> +
> +	nr_longs = nr_cpus + BITS_TO_LONGS(nr_irqs - VGIC_NR_PRIVATE_IRQS);
> +
> +	b->bits = kzalloc(sizeof(unsigned long) * nr_longs, GFP_KERNEL);
> +	if (!b->bits)
> +		return -ENOMEM;
> +
> +	b->nr_cpus = nr_cpus;
> +	return 0;
> +}
> +
> +static void vgic_free_bitmap(struct vgic_bitmap *b)
> +{
> +	kfree(b->bits);
> +}
> +
>  static u32 *vgic_bitmap_get_reg(struct vgic_bitmap *x,
>  				int cpuid, u32 offset)
>  {
>  	offset >>= 2;
>  	if (!offset)
> -		return x->percpu[cpuid].reg;
> +		return (u32 *)(x->bits + cpuid);
>  	else
> -		return x->shared.reg + offset - 1;
> +		return (u32 *)(x->bits + x->nr_cpus) + offset - 1;

this is now completely void of any hints towards what's going on.  Could
we add just /* percpu */  and /* SPIs */ at the end of the lines or
something?

>  }
>  
>  static int vgic_bitmap_get_irq_val(struct vgic_bitmap *x,
>  				   int cpuid, int irq)
>  {
>  	if (irq < VGIC_NR_PRIVATE_IRQS)
> -		return test_bit(irq, x->percpu[cpuid].reg_ul);
> +		return test_bit(irq, x->bits + cpuid);
>  
> -	return test_bit(irq - VGIC_NR_PRIVATE_IRQS, x->shared.reg_ul);
> +	return test_bit(irq - VGIC_NR_PRIVATE_IRQS, x->bits + x->nr_cpus);
>  }
>  
>  static void vgic_bitmap_set_irq_val(struct vgic_bitmap *x, int cpuid,
>  				    int irq, int val)
>  {
> -	unsigned long *reg;
> +	unsigned long *reg = x->bits;
>  
> +	BUG_ON(!reg);

huh?  that would be one terrible bug...

>  	if (irq < VGIC_NR_PRIVATE_IRQS) {
> -		reg = x->percpu[cpuid].reg_ul;
> +		reg += cpuid;
>  	} else {
> -		reg =  x->shared.reg_ul;
> +		reg += x->nr_cpus;
>  		irq -= VGIC_NR_PRIVATE_IRQS;
>  	}
>  
> @@ -135,24 +155,42 @@ static void vgic_bitmap_set_irq_val(struct vgic_bitmap *x, int cpuid,
>  
>  static unsigned long *vgic_bitmap_get_cpu_map(struct vgic_bitmap *x, int cpuid)
>  {
> -	if (unlikely(cpuid >= VGIC_MAX_CPUS))
> -		return NULL;

why did we have this before?

> -	return x->percpu[cpuid].reg_ul;
> +	return x->bits + cpuid;
>  }
>  
>  static unsigned long *vgic_bitmap_get_shared_map(struct vgic_bitmap *x)
>  {
> -	return x->shared.reg_ul;
> +	return x->bits + x->nr_cpus;
> +}
> +
> +static int vgic_init_bytemap(struct vgic_bytemap *x, int nr_cpus, int nr_irqs)
> +{
> +	int size;
> +
> +	size  = nr_cpus * VGIC_NR_PRIVATE_IRQS;
> +	size += nr_irqs - VGIC_NR_PRIVATE_IRQS;
> +
> +	x->regs = kzalloc(size, GFP_KERNEL);
> +	if (!x->regs)
> +		return -ENOMEM;
> +
> +	x->nr_cpus = nr_cpus;
> +	return 0;
> +}
> +
> +static void vgic_free_bytemap(struct vgic_bytemap *b)
> +{
> +	kfree(b->regs);
>  }
>  
>  static u32 *vgic_bytemap_get_reg(struct vgic_bytemap *x, int cpuid, u32 offset)
>  {
> -	offset >>= 2;
> -	BUG_ON(offset > (VGIC_NR_IRQS / 4));
> -	if (offset < 8)
> -		return x->percpu[cpuid] + offset;
> +	if (offset < 32)
> +		offset += cpuid * VGIC_NR_PRIVATE_IRQS;
>  	else
> -		return x->shared + offset - 8;
> +		offset += (x->nr_cpus - 1) * VGIC_NR_PRIVATE_IRQS;
> +
> +	return x->regs + (offset / sizeof(u32));
>  }
>  
>  #define VGIC_CFG_LEVEL	0
> @@ -733,6 +771,11 @@ bool vgic_handle_mmio(struct kvm_vcpu *vcpu, struct kvm_run *run,
>  	return true;
>  }
>  
> +static u8 *vgic_get_sgi_sources(struct vgic_dist *dist, int vcpu_id, int sgi)
> +{
> +	return dist->irq_sgi_sources + vcpu_id * VGIC_NR_SGIS + sgi;
> +}
> +
>  static void vgic_dispatch_sgi(struct kvm_vcpu *vcpu, u32 reg)
>  {
>  	struct kvm *kvm = vcpu->kvm;
> @@ -765,7 +808,7 @@ static void vgic_dispatch_sgi(struct kvm_vcpu *vcpu, u32 reg)
>  		if (target_cpus & 1) {
>  			/* Flag the SGI as pending */
>  			vgic_dist_irq_set(vcpu, sgi);
> -			dist->irq_sgi_sources[c][sgi] |= 1 << vcpu_id;
> +			*vgic_get_sgi_sources(dist, c, sgi) |= 1 << vcpu_id;
>  			kvm_debug("SGI%d from CPU%d to CPU%d\n", sgi, vcpu_id, c);
>  		}
>  
> @@ -908,14 +951,14 @@ static bool vgic_queue_sgi(struct kvm_vcpu *vcpu, int irq)
>  	int vcpu_id = vcpu->vcpu_id;
>  	int c;
>  
> -	sources = dist->irq_sgi_sources[vcpu_id][irq];
> +	sources = *vgic_get_sgi_sources(dist, vcpu_id, irq);
>  
>  	for_each_set_bit(c, &sources, VGIC_MAX_CPUS) {
>  		if (vgic_queue_irq(vcpu, c, irq))
>  			clear_bit(c, &sources);
>  	}
>  
> -	dist->irq_sgi_sources[vcpu_id][irq] = sources;
> +	*vgic_get_sgi_sources(dist, vcpu_id, irq) = sources;
>  
>  	/*
>  	 * If the sources bitmap has been cleared it means that we
> @@ -1243,15 +1286,44 @@ static irqreturn_t vgic_maintenance_handler(int irq, void *data)
>  	return IRQ_HANDLED;
>  }
>  
> +static void vgic_vcpu_free_maps(struct vgic_cpu *vgic_cpu)
> +{
> +	kfree(vgic_cpu->pending_shared);
> +	kfree(vgic_cpu->vgic_irq_lr_map);
> +}
> +
> +static int vgic_vcpu_init_maps(struct vgic_cpu *vgic_cpu, int nr_irqs)
> +{
> +	vgic_cpu->pending_shared = kzalloc(nr_irqs - VGIC_NR_PRIVATE_IRQS,
> +					   GFP_KERNEL);

aren't you allocate x8 too many bits here?

> +	vgic_cpu->vgic_irq_lr_map = kzalloc(nr_irqs, GFP_KERNEL);
> +
> +	if (!vgic_cpu->pending_shared || !vgic_cpu->vgic_irq_lr_map) {
> +		vgic_vcpu_free_maps(vgic_cpu);
> +		return -ENOMEM;
> +	}
> +
> +	return 0;
> +}
> +
> +void kvm_vgic_vcpu_destroy(struct kvm_vcpu *vcpu)
> +{
> +	vgic_vcpu_free_maps(&vcpu->arch.vgic_cpu);
> +}
> +
>  int kvm_vgic_vcpu_init(struct kvm_vcpu *vcpu)
>  {
>  	struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu;
>  	struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
> -	int i;
> +	int i, ret;
>  
>  	if (!irqchip_in_kernel(vcpu->kvm))
>  		return 0;
>  
> +	ret = vgic_vcpu_init_maps(vgic_cpu, dist->nr_irqs);
> +	if (ret)
> +		return ret;
> +
>  	if (vcpu->vcpu_id >= VGIC_MAX_CPUS)
>  		return -EBUSY;
>  
> @@ -1383,6 +1455,68 @@ out:
>  	return ret;
>  }
>  
> +static void vgic_free_maps(struct vgic_dist *dist)
> +{
> +	int i;
> +
> +	vgic_free_bitmap(&dist->irq_enabled);
> +	vgic_free_bitmap(&dist->irq_state);
> +	vgic_free_bitmap(&dist->irq_active);
> +	vgic_free_bitmap(&dist->irq_cfg);
> +	vgic_free_bytemap(&dist->irq_priority);
> +	if (dist->irq_spi_target)
> +		for (i = 0; i < dist->nr_cpus; i++)
> +			vgic_free_bitmap(&dist->irq_spi_target[i]);
> +	kfree(dist->irq_sgi_sources);
> +	kfree(dist->irq_spi_cpu);
> +	kfree(dist->irq_spi_target);

I would love for this to be ordered like the fields in the structure,
but this is obviously not a functional requirement.

> +}
> +
> +static int vgic_init_maps(struct vgic_dist *dist, int nr_cpus, int nr_irqs)
> +{
> +	int ret = 0, i;

no need to do ret = 0 here.

> +
> +	dist->nr_cpus = nr_cpus;
> +	dist->nr_irqs = nr_irqs;
> +
> +	ret  = vgic_init_bitmap(&dist->irq_enabled, nr_cpus, nr_irqs);
> +	ret |= vgic_init_bitmap(&dist->irq_state, nr_cpus, nr_irqs);
> +	ret |= vgic_init_bitmap(&dist->irq_active, nr_cpus, nr_irqs);
> +	ret |= vgic_init_bitmap(&dist->irq_cfg, nr_cpus, nr_irqs);
> +	ret |= vgic_init_bytemap(&dist->irq_priority, nr_cpus, nr_irqs);
> +
> +	if (!ret) {
> +		dist->irq_sgi_sources = kzalloc(nr_cpus * VGIC_NR_SGIS,
> +						GFP_KERNEL);
> +		dist->irq_spi_cpu = kzalloc(nr_irqs - VGIC_NR_PRIVATE_IRQS,
> +					    GFP_KERNEL);

We could redefine VGIC_NR_SHARED_IRQS() to take nr_irqs as an
argument...

> +		dist->irq_spi_target = kzalloc(sizeof(*dist->irq_spi_target) * nr_cpus,
> +					       GFP_KERNEL);
> +		if (!dist->irq_sgi_sources ||
> +		    !dist->irq_spi_cpu ||
> +		    !dist->irq_spi_target) {
> +			ret = -ENOMEM;
> +			goto out;
> +		}
> +
> +		for (i = 0; i < nr_cpus; i++)
> +			ret |= vgic_init_bitmap(&dist->irq_spi_target[i],
> +						nr_cpus, nr_irqs);
> +
> +	}
> +
> +out:
> +	if (ret)
> +		vgic_free_maps(dist);
> +
> +	return ret;
> +}
> +
> +void kvm_vgic_destroy(struct kvm *kvm)
> +{
> +	vgic_free_maps(&kvm->arch.vgic);
> +}
> +
>  int kvm_vgic_init(struct kvm *kvm)
>  {
>  	int ret = 0, i;
> @@ -1416,7 +1550,7 @@ out:
>  	return ret;
>  }
>  
> -int kvm_vgic_create(struct kvm *kvm)
> +int kvm_vgic_create(struct kvm *kvm, int nr_cpus, int nr_irqs)
>  {
>  	int ret = 0;
>  
> @@ -1432,6 +1566,10 @@ int kvm_vgic_create(struct kvm *kvm)
>  	kvm->arch.vgic.vgic_dist_base = VGIC_ADDR_UNDEF;
>  	kvm->arch.vgic.vgic_cpu_base = VGIC_ADDR_UNDEF;
>  
> +	ret = vgic_init_maps(&kvm->arch.vgic, nr_cpus, nr_irqs);
> +	if (ret)
> +		kvm_err("Unable to allocate maps\n");
> +
>  out:
>  	mutex_unlock(&kvm->lock);
>  	return ret;
> -- 
> 1.8.2.3
> 
> 

-Christoffer

[RFC PATCH 2/5] arm/arm64: KVM: vgic: kill VGIC_NR_SHARED_IRQS

[Marc Zyngier]

having a dynamic number of supported interrupts means that we
cannot relly on VGIC_NR_SHARED_IRQS anymore, as the upper bound
is not known anymore.

Just nuke it.

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 include/kvm/arm_vgic.h |  1 -
 virt/kvm/arm/vgic.c    | 12 +++++++-----
 2 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
index 3cc3a9f..8ce9c08 100644
--- a/include/kvm/arm_vgic.h
+++ b/include/kvm/arm_vgic.h
@@ -30,7 +30,6 @@
 #define VGIC_NR_SGIS		16
 #define VGIC_NR_PPIS		16
 #define VGIC_NR_PRIVATE_IRQS	(VGIC_NR_SGIS + VGIC_NR_PPIS)
-#define VGIC_NR_SHARED_IRQS	(VGIC_NR_IRQS - VGIC_NR_PRIVATE_IRQS)
 #define VGIC_MAX_CPUS		KVM_MAX_VCPUS
 #define VGIC_MAX_LRS		(1 << 6)
 
diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
index f16c848..951b560 100644
--- a/virt/kvm/arm/vgic.c
+++ b/virt/kvm/arm/vgic.c
@@ -821,6 +821,7 @@ static int compute_pending_for_cpu(struct kvm_vcpu *vcpu)
 	struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
 	unsigned long *pending, *enabled, *pend_percpu, *pend_shared;
 	unsigned long pending_private, pending_shared;
+	int nr_shared_irqs = dist->nr_irqs - VGIC_NR_PRIVATE_IRQS;
 	int vcpu_id;
 
 	vcpu_id = vcpu->vcpu_id;
@@ -833,15 +834,15 @@ static int compute_pending_for_cpu(struct kvm_vcpu *vcpu)
 
 	pending = vgic_bitmap_get_shared_map(&dist->irq_state);
 	enabled = vgic_bitmap_get_shared_map(&dist->irq_enabled);
-	bitmap_and(pend_shared, pending, enabled, VGIC_NR_SHARED_IRQS);
+	bitmap_and(pend_shared, pending, enabled, nr_shared_irqs);
 	bitmap_and(pend_shared, pend_shared,
 		   vgic_bitmap_get_shared_map(&dist->irq_spi_target[vcpu_id]),
-		   VGIC_NR_SHARED_IRQS);
+		   nr_shared_irqs);
 
 	pending_private = find_first_bit(pend_percpu, VGIC_NR_PRIVATE_IRQS);
-	pending_shared = find_first_bit(pend_shared, VGIC_NR_SHARED_IRQS);
+	pending_shared = find_first_bit(pend_shared, nr_shared_irqs);
 	return (pending_private < VGIC_NR_PRIVATE_IRQS ||
-		pending_shared < VGIC_NR_SHARED_IRQS);
+		pending_shared < nr_shared_irqs);
 }
 
 /*
@@ -1002,6 +1003,7 @@ static void __kvm_vgic_flush_hwstate(struct kvm_vcpu *vcpu)
 {
 	struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu;
 	struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
+	int nr_shared_irqs = dist->nr_irqs - VGIC_NR_PRIVATE_IRQS;
 	int i, vcpu_id;
 	int overflow = 0;
 
@@ -1030,7 +1032,7 @@ static void __kvm_vgic_flush_hwstate(struct kvm_vcpu *vcpu)
 	}
 
 	/* SPIs */
-	for_each_set_bit(i, vgic_cpu->pending_shared, VGIC_NR_SHARED_IRQS) {
+	for_each_set_bit(i, vgic_cpu->pending_shared, nr_shared_irqs) {
 		if (!vgic_queue_hwirq(vcpu, i + VGIC_NR_PRIVATE_IRQS))
 			overflow = 1;
 	}
-- 
1.8.2.3

[RFC PATCH 2/5] arm/arm64: KVM: vgic: kill VGIC_NR_SHARED_IRQS

[Christoffer Dall]

On Fri, Oct 04, 2013 at 01:16:15PM +0100, Marc Zyngier wrote:
> having a dynamic number of supported interrupts means that we
> cannot relly on VGIC_NR_SHARED_IRQS anymore, as the upper bound

rely

> is not known anymore.
> 
> Just nuke it.
> 
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> ---
>  include/kvm/arm_vgic.h |  1 -
>  virt/kvm/arm/vgic.c    | 12 +++++++-----
>  2 files changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
> index 3cc3a9f..8ce9c08 100644
> --- a/include/kvm/arm_vgic.h
> +++ b/include/kvm/arm_vgic.h
> @@ -30,7 +30,6 @@
>  #define VGIC_NR_SGIS		16
>  #define VGIC_NR_PPIS		16
>  #define VGIC_NR_PRIVATE_IRQS	(VGIC_NR_SGIS + VGIC_NR_PPIS)
> -#define VGIC_NR_SHARED_IRQS	(VGIC_NR_IRQS - VGIC_NR_PRIVATE_IRQS)
>  #define VGIC_MAX_CPUS		KVM_MAX_VCPUS
>  #define VGIC_MAX_LRS		(1 << 6)
>  
> diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
> index f16c848..951b560 100644
> --- a/virt/kvm/arm/vgic.c
> +++ b/virt/kvm/arm/vgic.c
> @@ -821,6 +821,7 @@ static int compute_pending_for_cpu(struct kvm_vcpu *vcpu)
>  	struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
>  	unsigned long *pending, *enabled, *pend_percpu, *pend_shared;
>  	unsigned long pending_private, pending_shared;
> +	int nr_shared_irqs = dist->nr_irqs - VGIC_NR_PRIVATE_IRQS;
>  	int vcpu_id;
>  
>  	vcpu_id = vcpu->vcpu_id;
> @@ -833,15 +834,15 @@ static int compute_pending_for_cpu(struct kvm_vcpu *vcpu)
>  
>  	pending = vgic_bitmap_get_shared_map(&dist->irq_state);
>  	enabled = vgic_bitmap_get_shared_map(&dist->irq_enabled);
> -	bitmap_and(pend_shared, pending, enabled, VGIC_NR_SHARED_IRQS);
> +	bitmap_and(pend_shared, pending, enabled, nr_shared_irqs);
>  	bitmap_and(pend_shared, pend_shared,
>  		   vgic_bitmap_get_shared_map(&dist->irq_spi_target[vcpu_id]),
> -		   VGIC_NR_SHARED_IRQS);
> +		   nr_shared_irqs);
>  
>  	pending_private = find_first_bit(pend_percpu, VGIC_NR_PRIVATE_IRQS);
> -	pending_shared = find_first_bit(pend_shared, VGIC_NR_SHARED_IRQS);
> +	pending_shared = find_first_bit(pend_shared, nr_shared_irqs);
>  	return (pending_private < VGIC_NR_PRIVATE_IRQS ||
> -		pending_shared < VGIC_NR_SHARED_IRQS);
> +		pending_shared < nr_shared_irqs);
>  }
>  
>  /*
> @@ -1002,6 +1003,7 @@ static void __kvm_vgic_flush_hwstate(struct kvm_vcpu *vcpu)
>  {
>  	struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu;
>  	struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
> +	int nr_shared_irqs = dist->nr_irqs - VGIC_NR_PRIVATE_IRQS;
>  	int i, vcpu_id;
>  	int overflow = 0;
>  
> @@ -1030,7 +1032,7 @@ static void __kvm_vgic_flush_hwstate(struct kvm_vcpu *vcpu)
>  	}
>  
>  	/* SPIs */
> -	for_each_set_bit(i, vgic_cpu->pending_shared, VGIC_NR_SHARED_IRQS) {
> +	for_each_set_bit(i, vgic_cpu->pending_shared, nr_shared_irqs) {
>  		if (!vgic_queue_hwirq(vcpu, i + VGIC_NR_PRIVATE_IRQS))
>  			overflow = 1;
>  	}
> -- 
> 1.8.2.3
> 
> 

nr_shared_irqs could be precomputed on the vgic_dist struct or just be
made a define that takes an argument everywhere - I think the latter
will make some of the code in the previous patch easier to read...

-- 
Christoffer

[RFC PATCH 3/5] arm/arm64: KVM: vgic: kill VGIC_MAX_CPUS

[Marc Zyngier]

We now have the information about the number of CPU interfaces in
the distributor itself. Let's get rid of VGIC_MAX_CPUS, and just
rely on KVM_MAX_VCPUS where we don't have the choice. Yet.

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 arch/arm/kvm/arm.c     | 2 +-
 include/kvm/arm_vgic.h | 3 +--
 virt/kvm/arm/vgic.c    | 4 ++--
 3 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
index f0f7a8a..2f465fe 100644
--- a/arch/arm/kvm/arm.c
+++ b/arch/arm/kvm/arm.c
@@ -789,7 +789,7 @@ long kvm_arch_vm_ioctl(struct file *filp,
 	switch (ioctl) {
 	case KVM_CREATE_IRQCHIP: {
 		if (vgic_present)
-			return kvm_vgic_create(kvm, VGIC_MAX_CPUS, VGIC_NR_IRQS);
+			return kvm_vgic_create(kvm, KVM_MAX_VCPUS, VGIC_NR_IRQS);
 		else
 			return -ENXIO;
 	}
diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
index 8ce9c08..4d4ab2e 100644
--- a/include/kvm/arm_vgic.h
+++ b/include/kvm/arm_vgic.h
@@ -30,11 +30,10 @@
 #define VGIC_NR_SGIS		16
 #define VGIC_NR_PPIS		16
 #define VGIC_NR_PRIVATE_IRQS	(VGIC_NR_SGIS + VGIC_NR_PPIS)
-#define VGIC_MAX_CPUS		KVM_MAX_VCPUS
 #define VGIC_MAX_LRS		(1 << 6)
 
 /* Sanity checks... */
-#if (VGIC_MAX_CPUS > 8)
+#if (KVM_MAX_VCPUS > 8)
 #error	Invalid number of CPU interfaces
 #endif
 
diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
index 951b560..259b9dd 100644
--- a/virt/kvm/arm/vgic.c
+++ b/virt/kvm/arm/vgic.c
@@ -954,7 +954,7 @@ static bool vgic_queue_sgi(struct kvm_vcpu *vcpu, int irq)
 
 	sources = *vgic_get_sgi_sources(dist, vcpu_id, irq);
 
-	for_each_set_bit(c, &sources, VGIC_MAX_CPUS) {
+	for_each_set_bit(c, &sources, dist->nr_cpus) {
 		if (vgic_queue_irq(vcpu, c, irq))
 			clear_bit(c, &sources);
 	}
@@ -1326,7 +1326,7 @@ int kvm_vgic_vcpu_init(struct kvm_vcpu *vcpu)
 	if (ret)
 		return ret;
 
-	if (vcpu->vcpu_id >= VGIC_MAX_CPUS)
+	if (vcpu->vcpu_id >= dist->nr_cpus)
 		return -EBUSY;
 
 	for (i = 0; i < VGIC_NR_IRQS; i++) {
-- 
1.8.2.3

[RFC PATCH 3/5] arm/arm64: KVM: vgic: kill VGIC_MAX_CPUS

[Christoffer Dall]

On Fri, Oct 04, 2013 at 01:16:16PM +0100, Marc Zyngier wrote:
> We now have the information about the number of CPU interfaces in
> the distributor itself. Let's get rid of VGIC_MAX_CPUS, and just
> rely on KVM_MAX_VCPUS where we don't have the choice. Yet.
> 
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> ---
>  arch/arm/kvm/arm.c     | 2 +-
>  include/kvm/arm_vgic.h | 3 +--
>  virt/kvm/arm/vgic.c    | 4 ++--
>  3 files changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
> index f0f7a8a..2f465fe 100644
> --- a/arch/arm/kvm/arm.c
> +++ b/arch/arm/kvm/arm.c
> @@ -789,7 +789,7 @@ long kvm_arch_vm_ioctl(struct file *filp,
>  	switch (ioctl) {
>  	case KVM_CREATE_IRQCHIP: {
>  		if (vgic_present)
> -			return kvm_vgic_create(kvm, VGIC_MAX_CPUS, VGIC_NR_IRQS);
> +			return kvm_vgic_create(kvm, KVM_MAX_VCPUS, VGIC_NR_IRQS);
>  		else
>  			return -ENXIO;
>  	}
> diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
> index 8ce9c08..4d4ab2e 100644
> --- a/include/kvm/arm_vgic.h
> +++ b/include/kvm/arm_vgic.h
> @@ -30,11 +30,10 @@
>  #define VGIC_NR_SGIS		16
>  #define VGIC_NR_PPIS		16
>  #define VGIC_NR_PRIVATE_IRQS	(VGIC_NR_SGIS + VGIC_NR_PPIS)
> -#define VGIC_MAX_CPUS		KVM_MAX_VCPUS
>  #define VGIC_MAX_LRS		(1 << 6)
>  
>  /* Sanity checks... */
> -#if (VGIC_MAX_CPUS > 8)
> +#if (KVM_MAX_VCPUS > 8)
>  #error	Invalid number of CPU interfaces
>  #endif
>  
> diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
> index 951b560..259b9dd 100644
> --- a/virt/kvm/arm/vgic.c
> +++ b/virt/kvm/arm/vgic.c
> @@ -954,7 +954,7 @@ static bool vgic_queue_sgi(struct kvm_vcpu *vcpu, int irq)
>  
>  	sources = *vgic_get_sgi_sources(dist, vcpu_id, irq);
>  
> -	for_each_set_bit(c, &sources, VGIC_MAX_CPUS) {
> +	for_each_set_bit(c, &sources, dist->nr_cpus) {
>  		if (vgic_queue_irq(vcpu, c, irq))
>  			clear_bit(c, &sources);
>  	}
> @@ -1326,7 +1326,7 @@ int kvm_vgic_vcpu_init(struct kvm_vcpu *vcpu)
>  	if (ret)
>  		return ret;
>  
> -	if (vcpu->vcpu_id >= VGIC_MAX_CPUS)
> +	if (vcpu->vcpu_id >= dist->nr_cpus)
>  		return -EBUSY;
>  
>  	for (i = 0; i < VGIC_NR_IRQS; i++) {
> -- 
> 1.8.2.3
> 
> 
Looks fine,

-- 
Christoffer

[RFC PATCH 4/5] arm/arm64: KVM: vgic: handle out-of-range MMIO accesses

[Marc Zyngier]

Now that we can (almost) dynamically size the number of interrupts,
we're facing an interesting issue:

We have to evaluate at runtime whether or not an access hits a valid
register, based on the sizing of this particular instance of the
distributor. Furthermore, the GIC spec says that accessing a reserved
register is RAZ/WI.

For this, add a new field to our range structure, indicating the number
of bits a single interrupts uses. That allows us to find out whether or
not the access is in range.

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 include/kvm/arm_vgic.h |  3 ++-
 virt/kvm/arm/vgic.c    | 56 ++++++++++++++++++++++++++++++++++++++++----------
 2 files changed, 47 insertions(+), 12 deletions(-)

diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
index 4d4ab2e..a57757d 100644
--- a/include/kvm/arm_vgic.h
+++ b/include/kvm/arm_vgic.h
@@ -31,6 +31,7 @@
 #define VGIC_NR_PPIS		16
 #define VGIC_NR_PRIVATE_IRQS	(VGIC_NR_SGIS + VGIC_NR_PPIS)
 #define VGIC_MAX_LRS		(1 << 6)
+#define VGIC_MAX_IRQS		1024
 
 /* Sanity checks... */
 #if (KVM_MAX_VCPUS > 8)
@@ -41,7 +42,7 @@
 #error "VGIC_NR_IRQS must be a multiple of 32"
 #endif
 
-#if (VGIC_NR_IRQS > 1024)
+#if (VGIC_NR_IRQS > VGIC_MAX_IRQS)
 #error "VGIC_NR_IRQS must be <= 1024"
 #endif
 
diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
index 259b9dd..c9d706b 100644
--- a/virt/kvm/arm/vgic.c
+++ b/virt/kvm/arm/vgic.c
@@ -636,6 +636,7 @@ static bool handle_mmio_sgi_reg(struct kvm_vcpu *vcpu,
 struct mmio_range {
 	phys_addr_t base;
 	unsigned long len;
+	int bits_per_irq;
 	bool (*handle_mmio)(struct kvm_vcpu *vcpu, struct kvm_exit_mmio *mmio,
 			    phys_addr_t offset);
 };
@@ -644,56 +645,67 @@ static const struct mmio_range vgic_ranges[] = {
 	{
 		.base		= GIC_DIST_CTRL,
 		.len		= 12,
+		.bits_per_irq	= 0,
 		.handle_mmio	= handle_mmio_misc,
 	},
 	{
 		.base		= GIC_DIST_IGROUP,
-		.len		= VGIC_NR_IRQS / 8,
+		.len		= VGIC_MAX_IRQS / 8,
+		.bits_per_irq	= 1,
 		.handle_mmio	= handle_mmio_raz_wi,
 	},
 	{
 		.base		= GIC_DIST_ENABLE_SET,
-		.len		= VGIC_NR_IRQS / 8,
+		.len		= VGIC_MAX_IRQS / 8,
+		.bits_per_irq	= 1,
 		.handle_mmio	= handle_mmio_set_enable_reg,
 	},
 	{
 		.base		= GIC_DIST_ENABLE_CLEAR,
-		.len		= VGIC_NR_IRQS / 8,
+		.len		= VGIC_MAX_IRQS / 8,
+		.bits_per_irq	= 1,
 		.handle_mmio	= handle_mmio_clear_enable_reg,
 	},
 	{
 		.base		= GIC_DIST_PENDING_SET,
-		.len		= VGIC_NR_IRQS / 8,
+		.len		= VGIC_MAX_IRQS / 8,
+		.bits_per_irq	= 1,
 		.handle_mmio	= handle_mmio_set_pending_reg,
 	},
 	{
 		.base		= GIC_DIST_PENDING_CLEAR,
-		.len		= VGIC_NR_IRQS / 8,
+		.len		= VGIC_MAX_IRQS / 8,
+		.bits_per_irq	= 1,
 		.handle_mmio	= handle_mmio_clear_pending_reg,
 	},
 	{
 		.base		= GIC_DIST_ACTIVE_SET,
-		.len		= VGIC_NR_IRQS / 8,
+		.len		= VGIC_MAX_IRQS / 8,
+		.bits_per_irq	= 1,
 		.handle_mmio	= handle_mmio_raz_wi,
 	},
 	{
 		.base		= GIC_DIST_ACTIVE_CLEAR,
-		.len		= VGIC_NR_IRQS / 8,
+		.len		= VGIC_MAX_IRQS / 8,
+		.bits_per_irq	= 1,
 		.handle_mmio	= handle_mmio_raz_wi,
 	},
 	{
 		.base		= GIC_DIST_PRI,
-		.len		= VGIC_NR_IRQS,
+		.len		= VGIC_MAX_IRQS,
+		.bits_per_irq	= 8,
 		.handle_mmio	= handle_mmio_priority_reg,
 	},
 	{
 		.base		= GIC_DIST_TARGET,
-		.len		= VGIC_NR_IRQS,
+		.len		= VGIC_MAX_IRQS,
+		.bits_per_irq	= 8,
 		.handle_mmio	= handle_mmio_target_reg,
 	},
 	{
 		.base		= GIC_DIST_CONFIG,
-		.len		= VGIC_NR_IRQS / 4,
+		.len		= VGIC_MAX_IRQS / 4,
+		.bits_per_irq	= 2,
 		.handle_mmio	= handle_mmio_cfg_reg,
 	},
 	{
@@ -722,6 +734,22 @@ struct mmio_range *find_matching_range(const struct mmio_range *ranges,
 	return NULL;
 }
 
+static bool vgic_validate_access(const struct vgic_dist *dist,
+				 const struct mmio_range *range,
+				 unsigned long offset)
+{
+	int irq;
+
+	if (!range->bits_per_irq)
+		return true;	/* Not an irq-based access */
+
+	irq = offset * 8 / range->bits_per_irq;
+	if (irq >= dist->nr_irqs)
+		return false;
+
+	return true;
+}
+
 /**
  * vgic_handle_mmio - handle an in-kernel MMIO access
  * @vcpu:	pointer to the vcpu performing the access
@@ -760,7 +788,13 @@ bool vgic_handle_mmio(struct kvm_vcpu *vcpu, struct kvm_run *run,
 
 	spin_lock(&vcpu->kvm->arch.vgic.lock);
 	offset = mmio->phys_addr - range->base - base;
-	updated_state = range->handle_mmio(vcpu, mmio, offset);
+	if (vgic_validate_access(dist, range, offset)) {
+		updated_state = range->handle_mmio(vcpu, mmio, offset);
+	} else {
+		vgic_reg_access(mmio, NULL, offset,
+				ACCESS_READ_RAZ | ACCESS_WRITE_IGNORED);
+		updated_state = false;
+	}
 	spin_unlock(&vcpu->kvm->arch.vgic.lock);
 	kvm_prepare_mmio(run, mmio);
 	kvm_handle_mmio_return(vcpu, run);
-- 
1.8.2.3

[RFC PATCH 4/5] arm/arm64: KVM: vgic: handle out-of-range MMIO accesses

[Christoffer Dall]

On Fri, Oct 04, 2013 at 01:16:17PM +0100, Marc Zyngier wrote:
> Now that we can (almost) dynamically size the number of interrupts,
> we're facing an interesting issue:
> 
> We have to evaluate at runtime whether or not an access hits a valid
> register, based on the sizing of this particular instance of the
> distributor. Furthermore, the GIC spec says that accessing a reserved
> register is RAZ/WI.

So this was a problem before anything relating to dynamic sizing of the
gic, right?

> 
> For this, add a new field to our range structure, indicating the number
> of bits a single interrupts uses. That allows us to find out whether or
> not the access is in range.

Hmmm, I don't think this is really what this is doing: this is
addressing the registers for unimplemented interrupts, which is
described per register in the GIC specs, and should therefore reasonably
be handled within the accessor functions individually instead of a
global validate function.  I feel the current approach makes the code
slightly harder to read to avoid replicating some very trivial logic,
but I don't feel strongly about this.

However, we still need to add the reserved register regions (e.g.
offsets 0x00C-0x01C and friends).

> 
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> ---
>  include/kvm/arm_vgic.h |  3 ++-
>  virt/kvm/arm/vgic.c    | 56 ++++++++++++++++++++++++++++++++++++++++----------
>  2 files changed, 47 insertions(+), 12 deletions(-)
> 
> diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
> index 4d4ab2e..a57757d 100644
> --- a/include/kvm/arm_vgic.h
> +++ b/include/kvm/arm_vgic.h
> @@ -31,6 +31,7 @@
>  #define VGIC_NR_PPIS		16
>  #define VGIC_NR_PRIVATE_IRQS	(VGIC_NR_SGIS + VGIC_NR_PPIS)
>  #define VGIC_MAX_LRS		(1 << 6)
> +#define VGIC_MAX_IRQS		1024
>  
>  /* Sanity checks... */
>  #if (KVM_MAX_VCPUS > 8)
> @@ -41,7 +42,7 @@
>  #error "VGIC_NR_IRQS must be a multiple of 32"
>  #endif
>  
> -#if (VGIC_NR_IRQS > 1024)
> +#if (VGIC_NR_IRQS > VGIC_MAX_IRQS)
>  #error "VGIC_NR_IRQS must be <= 1024"
>  #endif
>  
> diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
> index 259b9dd..c9d706b 100644
> --- a/virt/kvm/arm/vgic.c
> +++ b/virt/kvm/arm/vgic.c
> @@ -636,6 +636,7 @@ static bool handle_mmio_sgi_reg(struct kvm_vcpu *vcpu,
>  struct mmio_range {
>  	phys_addr_t base;
>  	unsigned long len;
> +	int bits_per_irq;
>  	bool (*handle_mmio)(struct kvm_vcpu *vcpu, struct kvm_exit_mmio *mmio,
>  			    phys_addr_t offset);
>  };
> @@ -644,56 +645,67 @@ static const struct mmio_range vgic_ranges[] = {
>  	{
>  		.base		= GIC_DIST_CTRL,
>  		.len		= 12,
> +		.bits_per_irq	= 0,
>  		.handle_mmio	= handle_mmio_misc,
>  	},
>  	{
>  		.base		= GIC_DIST_IGROUP,
> -		.len		= VGIC_NR_IRQS / 8,
> +		.len		= VGIC_MAX_IRQS / 8,
> +		.bits_per_irq	= 1,
>  		.handle_mmio	= handle_mmio_raz_wi,
>  	},
>  	{
>  		.base		= GIC_DIST_ENABLE_SET,
> -		.len		= VGIC_NR_IRQS / 8,
> +		.len		= VGIC_MAX_IRQS / 8,
> +		.bits_per_irq	= 1,
>  		.handle_mmio	= handle_mmio_set_enable_reg,
>  	},
>  	{
>  		.base		= GIC_DIST_ENABLE_CLEAR,
> -		.len		= VGIC_NR_IRQS / 8,
> +		.len		= VGIC_MAX_IRQS / 8,
> +		.bits_per_irq	= 1,
>  		.handle_mmio	= handle_mmio_clear_enable_reg,
>  	},
>  	{
>  		.base		= GIC_DIST_PENDING_SET,
> -		.len		= VGIC_NR_IRQS / 8,
> +		.len		= VGIC_MAX_IRQS / 8,
> +		.bits_per_irq	= 1,
>  		.handle_mmio	= handle_mmio_set_pending_reg,
>  	},
>  	{
>  		.base		= GIC_DIST_PENDING_CLEAR,
> -		.len		= VGIC_NR_IRQS / 8,
> +		.len		= VGIC_MAX_IRQS / 8,
> +		.bits_per_irq	= 1,
>  		.handle_mmio	= handle_mmio_clear_pending_reg,
>  	},
>  	{
>  		.base		= GIC_DIST_ACTIVE_SET,
> -		.len		= VGIC_NR_IRQS / 8,
> +		.len		= VGIC_MAX_IRQS / 8,
> +		.bits_per_irq	= 1,
>  		.handle_mmio	= handle_mmio_raz_wi,
>  	},
>  	{
>  		.base		= GIC_DIST_ACTIVE_CLEAR,
> -		.len		= VGIC_NR_IRQS / 8,
> +		.len		= VGIC_MAX_IRQS / 8,
> +		.bits_per_irq	= 1,
>  		.handle_mmio	= handle_mmio_raz_wi,
>  	},
>  	{
>  		.base		= GIC_DIST_PRI,
> -		.len		= VGIC_NR_IRQS,
> +		.len		= VGIC_MAX_IRQS,
> +		.bits_per_irq	= 8,
>  		.handle_mmio	= handle_mmio_priority_reg,
>  	},
>  	{
>  		.base		= GIC_DIST_TARGET,
> -		.len		= VGIC_NR_IRQS,
> +		.len		= VGIC_MAX_IRQS,
> +		.bits_per_irq	= 8,
>  		.handle_mmio	= handle_mmio_target_reg,
>  	},
>  	{
>  		.base		= GIC_DIST_CONFIG,
> -		.len		= VGIC_NR_IRQS / 4,
> +		.len		= VGIC_MAX_IRQS / 4,
> +		.bits_per_irq	= 2,
>  		.handle_mmio	= handle_mmio_cfg_reg,
>  	},
>  	{
> @@ -722,6 +734,22 @@ struct mmio_range *find_matching_range(const struct mmio_range *ranges,
>  	return NULL;
>  }
>  
> +static bool vgic_validate_access(const struct vgic_dist *dist,
> +				 const struct mmio_range *range,
> +				 unsigned long offset)
> +{
> +	int irq;
> +
> +	if (!range->bits_per_irq)
> +		return true;	/* Not an irq-based access */
> +
> +	irq = offset * 8 / range->bits_per_irq;
> +	if (irq >= dist->nr_irqs)
> +		return false;
> +
> +	return true;
> +}
> +
>  /**
>   * vgic_handle_mmio - handle an in-kernel MMIO access
>   * @vcpu:	pointer to the vcpu performing the access
> @@ -760,7 +788,13 @@ bool vgic_handle_mmio(struct kvm_vcpu *vcpu, struct kvm_run *run,
>  
>  	spin_lock(&vcpu->kvm->arch.vgic.lock);
>  	offset = mmio->phys_addr - range->base - base;
> -	updated_state = range->handle_mmio(vcpu, mmio, offset);
> +	if (vgic_validate_access(dist, range, offset)) {
> +		updated_state = range->handle_mmio(vcpu, mmio, offset);
> +	} else {
> +		vgic_reg_access(mmio, NULL, offset,
> +				ACCESS_READ_RAZ | ACCESS_WRITE_IGNORED);
> +		updated_state = false;
> +	}
>  	spin_unlock(&vcpu->kvm->arch.vgic.lock);
>  	kvm_prepare_mmio(run, mmio);
>  	kvm_handle_mmio_return(vcpu, run);
> -- 
> 1.8.2.3
> 
> 

-- 
Christoffer

[RFC PATCH 5/5] arm/arm64: KVM: vgic: kill VGIC_NR_IRQS

[Marc Zyngier]

Nuke VGIC_NR_IRQS entierly, now that the distributor instance
contains the number of IRQ allocated to this GIC.

Also add VGIC_NR_IRQS_LEGACY to preserve the current API.

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 arch/arm/kvm/arm.c     |  2 +-
 include/kvm/arm_vgic.h |  6 +++---
 virt/kvm/arm/vgic.c    | 11 ++++++-----
 3 files changed, 10 insertions(+), 9 deletions(-)

diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
index 2f465fe..2255719 100644
--- a/arch/arm/kvm/arm.c
+++ b/arch/arm/kvm/arm.c
@@ -789,7 +789,7 @@ long kvm_arch_vm_ioctl(struct file *filp,
 	switch (ioctl) {
 	case KVM_CREATE_IRQCHIP: {
 		if (vgic_present)
-			return kvm_vgic_create(kvm, KVM_MAX_VCPUS, VGIC_NR_IRQS);
+			return kvm_vgic_create(kvm, KVM_MAX_VCPUS, VGIC_NR_IRQS_LEGACY);
 		else
 			return -ENXIO;
 	}
diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
index a57757d..03f497d 100644
--- a/include/kvm/arm_vgic.h
+++ b/include/kvm/arm_vgic.h
@@ -26,7 +26,7 @@
 #include <linux/types.h>
 #include <linux/irqchip/arm-gic.h>
 
-#define VGIC_NR_IRQS		256
+#define VGIC_NR_IRQS_LEGACY	256
 #define VGIC_NR_SGIS		16
 #define VGIC_NR_PPIS		16
 #define VGIC_NR_PRIVATE_IRQS	(VGIC_NR_SGIS + VGIC_NR_PPIS)
@@ -38,11 +38,11 @@
 #error	Invalid number of CPU interfaces
 #endif
 
-#if (VGIC_NR_IRQS & 31)
+#if (VGIC_NR_IRQS_LEGACY & 31)
 #error "VGIC_NR_IRQS must be a multiple of 32"
 #endif
 
-#if (VGIC_NR_IRQS > VGIC_MAX_IRQS)
+#if (VGIC_NR_IRQS_LEGACY > VGIC_MAX_IRQS)
 #error "VGIC_NR_IRQS must be <= 1024"
 #endif
 
diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
index c9d706b..357fd23 100644
--- a/virt/kvm/arm/vgic.c
+++ b/virt/kvm/arm/vgic.c
@@ -363,7 +363,7 @@ static bool handle_mmio_misc(struct kvm_vcpu *vcpu,
 
 	case 4:			/* TYPER */
 		reg  = (atomic_read(&vcpu->kvm->online_vcpus) - 1) << 5;
-		reg |= (VGIC_NR_IRQS >> 5) - 1;
+		reg |= (vcpu->kvm->arch.vgic.nr_irqs >> 5) - 1;
 		vgic_reg_access(mmio, &reg, word_offset,
 				ACCESS_READ_VALUE | ACCESS_WRITE_IGNORED);
 		break;
@@ -941,12 +941,13 @@ static void vgic_retire_disabled_irqs(struct kvm_vcpu *vcpu)
 static bool vgic_queue_irq(struct kvm_vcpu *vcpu, u8 sgi_source_id, int irq)
 {
 	struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu;
+	struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
 	int lr;
 
 	/* Sanitize the input... */
 	BUG_ON(sgi_source_id & ~7);
 	BUG_ON(sgi_source_id && irq >= VGIC_NR_SGIS);
-	BUG_ON(irq >= VGIC_NR_IRQS);
+	BUG_ON(irq >= dist->nr_irqs);
 
 	kvm_debug("Queue IRQ%d\n", irq);
 
@@ -1153,7 +1154,7 @@ static void __kvm_vgic_sync_hwstate(struct kvm_vcpu *vcpu)
 
 		irq = vgic_cpu->vgic_lr[lr] & GICH_LR_VIRTUALID;
 
-		BUG_ON(irq >= VGIC_NR_IRQS);
+		BUG_ON(irq >= dist->nr_irqs);
 		vgic_cpu->vgic_irq_lr_map[irq] = LR_EMPTY;
 	}
 
@@ -1363,7 +1364,7 @@ int kvm_vgic_vcpu_init(struct kvm_vcpu *vcpu)
 	if (vcpu->vcpu_id >= dist->nr_cpus)
 		return -EBUSY;
 
-	for (i = 0; i < VGIC_NR_IRQS; i++) {
+	for (i = 0; i < dist->nr_irqs; i++) {
 		if (i < VGIC_NR_PPIS)
 			vgic_bitmap_set_irq_val(&dist->irq_enabled,
 						vcpu->vcpu_id, i, 1);
@@ -1576,7 +1577,7 @@ int kvm_vgic_init(struct kvm *kvm)
 		goto out;
 	}
 
-	for (i = VGIC_NR_PRIVATE_IRQS; i < VGIC_NR_IRQS; i += 4)
+	for (i = VGIC_NR_PRIVATE_IRQS; i < kvm->arch.vgic.nr_irqs; i += 4)
 		vgic_set_target_reg(kvm, 0, i);
 
 	kvm_timer_init(kvm);
-- 
1.8.2.3

[RFC PATCH 5/5] arm/arm64: KVM: vgic: kill VGIC_NR_IRQS

[Christoffer Dall]

On Fri, Oct 04, 2013 at 01:16:18PM +0100, Marc Zyngier wrote:
> Nuke VGIC_NR_IRQS entierly, now that the distributor instance
> contains the number of IRQ allocated to this GIC.
> 
> Also add VGIC_NR_IRQS_LEGACY to preserve the current API.
> 
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> ---
>  arch/arm/kvm/arm.c     |  2 +-
>  include/kvm/arm_vgic.h |  6 +++---
>  virt/kvm/arm/vgic.c    | 11 ++++++-----
>  3 files changed, 10 insertions(+), 9 deletions(-)
> 
> diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
> index 2f465fe..2255719 100644
> --- a/arch/arm/kvm/arm.c
> +++ b/arch/arm/kvm/arm.c
> @@ -789,7 +789,7 @@ long kvm_arch_vm_ioctl(struct file *filp,
>  	switch (ioctl) {
>  	case KVM_CREATE_IRQCHIP: {
>  		if (vgic_present)
> -			return kvm_vgic_create(kvm, KVM_MAX_VCPUS, VGIC_NR_IRQS);
> +			return kvm_vgic_create(kvm, KVM_MAX_VCPUS, VGIC_NR_IRQS_LEGACY);
>  		else
>  			return -ENXIO;
>  	}
> diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
> index a57757d..03f497d 100644
> --- a/include/kvm/arm_vgic.h
> +++ b/include/kvm/arm_vgic.h
> @@ -26,7 +26,7 @@
>  #include <linux/types.h>
>  #include <linux/irqchip/arm-gic.h>
>  
> -#define VGIC_NR_IRQS		256
> +#define VGIC_NR_IRQS_LEGACY	256
>  #define VGIC_NR_SGIS		16
>  #define VGIC_NR_PPIS		16
>  #define VGIC_NR_PRIVATE_IRQS	(VGIC_NR_SGIS + VGIC_NR_PPIS)
> @@ -38,11 +38,11 @@
>  #error	Invalid number of CPU interfaces
>  #endif
>  
> -#if (VGIC_NR_IRQS & 31)
> +#if (VGIC_NR_IRQS_LEGACY & 31)
>  #error "VGIC_NR_IRQS must be a multiple of 32"
>  #endif
>  
> -#if (VGIC_NR_IRQS > VGIC_MAX_IRQS)
> +#if (VGIC_NR_IRQS_LEGACY > VGIC_MAX_IRQS)
>  #error "VGIC_NR_IRQS must be <= 1024"
>  #endif
>  
> diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
> index c9d706b..357fd23 100644
> --- a/virt/kvm/arm/vgic.c
> +++ b/virt/kvm/arm/vgic.c
> @@ -363,7 +363,7 @@ static bool handle_mmio_misc(struct kvm_vcpu *vcpu,
>  
>  	case 4:			/* TYPER */
>  		reg  = (atomic_read(&vcpu->kvm->online_vcpus) - 1) << 5;
> -		reg |= (VGIC_NR_IRQS >> 5) - 1;
> +		reg |= (vcpu->kvm->arch.vgic.nr_irqs >> 5) - 1;
>  		vgic_reg_access(mmio, &reg, word_offset,
>  				ACCESS_READ_VALUE | ACCESS_WRITE_IGNORED);
>  		break;
> @@ -941,12 +941,13 @@ static void vgic_retire_disabled_irqs(struct kvm_vcpu *vcpu)
>  static bool vgic_queue_irq(struct kvm_vcpu *vcpu, u8 sgi_source_id, int irq)
>  {
>  	struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu;
> +	struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
>  	int lr;
>  
>  	/* Sanitize the input... */
>  	BUG_ON(sgi_source_id & ~7);
>  	BUG_ON(sgi_source_id && irq >= VGIC_NR_SGIS);
> -	BUG_ON(irq >= VGIC_NR_IRQS);
> +	BUG_ON(irq >= dist->nr_irqs);
>  
>  	kvm_debug("Queue IRQ%d\n", irq);
>  
> @@ -1153,7 +1154,7 @@ static void __kvm_vgic_sync_hwstate(struct kvm_vcpu *vcpu)
>  
>  		irq = vgic_cpu->vgic_lr[lr] & GICH_LR_VIRTUALID;
>  
> -		BUG_ON(irq >= VGIC_NR_IRQS);
> +		BUG_ON(irq >= dist->nr_irqs);
>  		vgic_cpu->vgic_irq_lr_map[irq] = LR_EMPTY;
>  	}
>  
> @@ -1363,7 +1364,7 @@ int kvm_vgic_vcpu_init(struct kvm_vcpu *vcpu)
>  	if (vcpu->vcpu_id >= dist->nr_cpus)
>  		return -EBUSY;
>  
> -	for (i = 0; i < VGIC_NR_IRQS; i++) {
> +	for (i = 0; i < dist->nr_irqs; i++) {
>  		if (i < VGIC_NR_PPIS)
>  			vgic_bitmap_set_irq_val(&dist->irq_enabled,
>  						vcpu->vcpu_id, i, 1);
> @@ -1576,7 +1577,7 @@ int kvm_vgic_init(struct kvm *kvm)
>  		goto out;
>  	}
>  
> -	for (i = VGIC_NR_PRIVATE_IRQS; i < VGIC_NR_IRQS; i += 4)
> +	for (i = VGIC_NR_PRIVATE_IRQS; i < kvm->arch.vgic.nr_irqs; i += 4)
>  		vgic_set_target_reg(kvm, 0, i);
>  
>  	kvm_timer_init(kvm);
> -- 
> 1.8.2.3
> 
> 
Looks good,

-- 
Christoffer

[RFC PATCH 0/5] arm/arm64: KVM: dynamic VGIC sizing

[Christoffer Dall]

On Fri, Oct 04, 2013 at 01:16:13PM +0100, Marc Zyngier wrote:
> So far, the VGIC data structures have been statically sized, meaning
> that we always have to support more interrupts than we actually want,
> and more CPU interfaces than we should.
> 
> This is a waste of resource, and is the kind of things that should be
> tuneable. This series partially addresses that issue by changing the
> data structures to be dynamically allocated.
> 
> The various sizes are still static, but the VGIC code itself doesn't
> have any reference to these anymore. Once plugged into the new device
> API, we should be able to size it entierely dynamically.
> 
> I'm hopping for this series to be merged with Christoffer's VGIC
> device control API.

Review it and you can stop hopping (must be hard on your legs).
I'll be happy to help with the merge ;)

> 
> This has been (lightly) tested on both ARM (TC2) and arm64 (model).
> 
>      M.
> 
> Marc Zyngier (5):
>   arm/arm64: KVM: vgic: switch to dynamic allocation
>   arm/arm64: KVM: vgic: kill VGIC_NR_SHARED_IRQS
>   arm/arm64: KVM: vgic: kill VGIC_MAX_CPUS
>   arm/arm64: KVM: vgic: handle out-of-range MMIO accesses
>   arm/arm64: KVM: vgic: kill VGIC_NR_IRQS
> 
>  arch/arm/kvm/arm.c     |   5 +-
>  include/kvm/arm_vgic.h |  49 ++++-----
>  virt/kvm/arm/vgic.c    | 263 ++++++++++++++++++++++++++++++++++++++++---------
>  3 files changed, 249 insertions(+), 68 deletions(-)
> 
> -- 
> 1.8.2.3
> 
> 

-- 
Christoffer