On Mon, Jan 14, 2013 at 10:39 AM, Will Deacon [off-list ref] wrote:

On Tue, Jan 08, 2013 at 06:42:04PM +0000, Christoffer Dall wrote:

quoted

From: Marc Zyngier <redacted>

Add the GIC distributor emulation code. A number of the GIC features
are simply ignored as they are not required to boot a Linux guest.

Signed-off-by: Marc Zyngier <redacted>
Signed-off-by: Christoffer Dall <redacted>
---
 arch/arm/include/asm/kvm_vgic.h |   82 +++++
 arch/arm/kvm/vgic.c             |  593 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 674 insertions(+), 1 deletion(-)

diff --git a/arch/arm/include/asm/kvm_vgic.h b/arch/arm/include/asm/kvm_vgic.h
index 270dcd2..9ff0d9c 100644
--- a/arch/arm/include/asm/kvm_vgic.h
+++ b/arch/arm/include/asm/kvm_vgic.h

@@ -19,12 +19,94 @@
 #ifndef __ASM_ARM_KVM_VGIC_H
 #define __ASM_ARM_KVM_VGIC_H

+#include <linux/kernel.h>
+#include <linux/kvm.h>
+#include <linux/kvm_host.h>
+#include <linux/irqreturn.h>
+#include <linux/spinlock.h>
+#include <linux/types.h>
 #include <asm/hardware/gic.h>

+#define VGIC_NR_IRQS           128
+#define VGIC_NR_SGIS           16

Now that you have this, you can use it in a few places (see also the BUG_ONs
in vgic_queue_irq).

quoted

+#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
+
+/* Sanity checks... */
+#if (VGIC_MAX_CPUS > 8)
+#error Invalid number of CPU interfaces
+#endif
+
+#if (VGIC_NR_IRQS & 31)
+#error "VGIC_NR_IRQS must be a multiple of 32"
+#endif
+
+#if (VGIC_NR_IRQS > 1024)
+#error "VGIC_NR_IRQS must be <= 1024"
+#endif
+
+/*
+ * The GIC distributor registers describing interrupts have two parts:
+ * - 32 per-CPU interrupts (SGI + PPI)
+ * - 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;
+};
+
+struct vgic_bytemap {
+       u32 percpu[VGIC_MAX_CPUS][VGIC_NR_PRIVATE_IRQS / 4];
+       u32 shared[VGIC_NR_SHARED_IRQS  / 4];
+};
+
 struct vgic_dist {
+#ifdef CONFIG_KVM_ARM_VGIC
+       spinlock_t              lock;
+
+       /* Virtual control interface mapping */
+       void __iomem            *vctrl_base;
+
        /* Distributor and vcpu interface mapping in the guest */
        phys_addr_t             vgic_dist_base;
        phys_addr_t             vgic_cpu_base;
+
+       /* Distributor enabled */
+       u32                     enabled;
+
+       /* Interrupt enabled (one bit per IRQ) */
+       struct vgic_bitmap      irq_enabled;
+
+       /* Interrupt 'pin' level */
+       struct vgic_bitmap      irq_state;
+
+       /* Level-triggered interrupt in progress */
+       struct vgic_bitmap      irq_active;
+
+       /* Interrupt priority. Not used yet. */
+       struct vgic_bytemap     irq_priority;
+
+       /* Level/edge triggered */
+       struct vgic_bitmap      irq_cfg;
+
+       /* Source CPU per SGI and target CPU */
+       u8                      irq_sgi_sources[VGIC_MAX_CPUS][16];

VGIC_NR_SGIS

quoted

+static u32 vgic_get_target_reg(struct kvm *kvm, int irq)
+{
+       struct vgic_dist *dist = &kvm->arch.vgic;
+       struct kvm_vcpu *vcpu;
+       int i, c;
+       unsigned long *bmap;
+       u32 val = 0;
+
+       irq -= VGIC_NR_PRIVATE_IRQS;
+
+       kvm_for_each_vcpu(c, vcpu, kvm) {
+               bmap = vgic_bitmap_get_shared_map(&dist->irq_spi_target[c]);
+               for (i = 0; i < GICD_IRQS_PER_ITARGETSR; i++)
+                       if (test_bit(irq + i, bmap))
+                               val |= 1 << (c + i * 8);
+       }
+
+       return val;
+}
+
+static void vgic_set_target_reg(struct kvm *kvm, u32 val, int irq)
+{
+       struct vgic_dist *dist = &kvm->arch.vgic;
+       struct kvm_vcpu *vcpu;
+       int i, c;
+       unsigned long *bmap;
+       u32 target;
+
+       BUG_ON(irq & 3);
+       BUG_ON(irq < VGIC_NR_PRIVATE_IRQS);

This is now different to vgic_Get_target_reg, which doesn't have the
BUG_ONs. Can we remove these ones too?

quoted

+       irq -= VGIC_NR_PRIVATE_IRQS;
+
+       /*
+        * Pick the LSB in each byte. This ensures we target exactly
+        * one vcpu per IRQ. If the byte is null, assume we target
+        * CPU0.
+        */
+       for (i = 0; i < GICD_IRQS_PER_ITARGETSR; i++) {
+               int shift = i * GICD_CPUTARGETS_BITS;
+               target = ffs((val >> shift) & 0xffU);
+               target = target ? (target - 1) : 0;
+               dist->irq_spi_cpu[irq + i] = target;
+               kvm_for_each_vcpu(c, vcpu, kvm) {
+                       bmap = vgic_bitmap_get_shared_map(&dist->irq_spi_target[c]);
+                       if (c == target)
+                               set_bit(irq + i, bmap);
+                       else
+                               clear_bit(irq + i, bmap);
+               }
+       }
+}

[...]

quoted

 static const struct mmio_range vgic_ranges[] = {
+       {                       /* CTRL, TYPER, IIDR */
+               .base           = 0,
+               .len            = 12,
+               .handle_mmio    = handle_mmio_misc,
+       },
+       {                       /* IGROUPRn */
+               .base           = 0x80,
+               .len            = VGIC_NR_IRQS / 8,
+               .handle_mmio    = handle_mmio_raz_wi,
+       },
+       {                       /* ISENABLERn */
+               .base           = 0x100,
+               .len            = VGIC_NR_IRQS / 8,
+               .handle_mmio    = handle_mmio_set_enable_reg,
+       },
+       {                       /* ICENABLERn */
+               .base           = 0x180,
+               .len            = VGIC_NR_IRQS / 8,
+               .handle_mmio    = handle_mmio_clear_enable_reg,
+       },
+       {                       /* ISPENDRn */
+               .base           = 0x200,
+               .len            = VGIC_NR_IRQS / 8,
+               .handle_mmio    = handle_mmio_set_pending_reg,
+       },
+       {                       /* ICPENDRn */
+               .base           = 0x280,
+               .len            = VGIC_NR_IRQS / 8,
+               .handle_mmio    = handle_mmio_clear_pending_reg,
+       },
+       {                       /* ISACTIVERn */
+               .base           = 0x300,
+               .len            = VGIC_NR_IRQS / 8,
+               .handle_mmio    = handle_mmio_raz_wi,
+       },
+       {                       /* ICACTIVERn */
+               .base           = 0x380,
+               .len            = VGIC_NR_IRQS / 8,
+               .handle_mmio    = handle_mmio_raz_wi,
+       },
+       {                       /* IPRIORITYRn */
+               .base           = 0x400,
+               .len            = VGIC_NR_IRQS,
+               .handle_mmio    = handle_mmio_priority_reg,
+       },
+       {                       /* ITARGETSRn */
+               .base           = 0x800,
+               .len            = VGIC_NR_IRQS,
+               .handle_mmio    = handle_mmio_target_reg,
+       },
+       {                       /* ICFGRn */
+               .base           = 0xC00,
+               .len            = VGIC_NR_IRQS / 4,
+               .handle_mmio    = handle_mmio_cfg_reg,
+       },
+       {                       /* SGIRn */
+               .base           = 0xF00,
+               .len            = 4,
+               .handle_mmio    = handle_mmio_sgi_reg,
+       },
        {}
 };

You've added named definitions for these constants to the GIC header file,
so please replace these immediates with those and delete the comments.

The following two commits should address your concerns:

commit ff4648faa6fd3342ce72e537a8068ab21d4085c8
Author: Christoffer Dall [off-list ref]
Date:   Mon Jan 14 16:53:21 2013 -0500

    KVM: ARM: vgic: Use defines instead of hardcoded numbers.

    Address reviewer comments.

    Signed-off-by: Christoffer Dall [off-list ref]

diff --git a/arch/arm/include/asm/kvm_vgic.h b/arch/arm/include/asm/kvm_vgic.h
index f5f270b..1ace491 100644
--- a/arch/arm/include/asm/kvm_vgic.h
+++ b/arch/arm/include/asm/kvm_vgic.h

@@ -99,7 +99,7 @@ struct vgic_dist {
 	struct vgic_bitmap	irq_cfg;

 	/* Source CPU per SGI and target CPU */
-	u8			irq_sgi_sources[VGIC_MAX_CPUS][16];
+	u8			irq_sgi_sources[VGIC_MAX_CPUS][VGIC_NR_SGIS];

 	/* Target CPU for each IRQ */
 	u8			irq_spi_cpu[VGIC_NR_SHARED_IRQS];

diff --git a/arch/arm/kvm/vgic.c b/arch/arm/kvm/vgic.c
index 25daa07..a0d283c 100644
--- a/arch/arm/kvm/vgic.c
+++ b/arch/arm/kvm/vgic.c

@@ -447,9 +447,6 @@ static void vgic_set_target_reg(struct kvm *kvm,

u32 val, int irq)
 	unsigned long *bmap;
 	u32 target;

-	BUG_ON(irq & 3);
-	BUG_ON(irq < VGIC_NR_PRIVATE_IRQS);
-
 	irq -= VGIC_NR_PRIVATE_IRQS;

 	/*

@@ -598,63 +595,63 @@ struct mmio_range {
 };

 static const struct mmio_range vgic_ranges[] = {
-	{			/* CTRL, TYPER, IIDR */
-		.base		= 0,
+	{
+		.base		= GIC_DIST_CTRL,
 		.len		= 12,
 		.handle_mmio	= handle_mmio_misc,
 	},
-	{			/* IGROUPRn */
-		.base		= 0x80,
+	{
+		.base		= GIC_DIST_IGROUP,
 		.len		= VGIC_NR_IRQS / 8,
 		.handle_mmio	= handle_mmio_raz_wi,
 	},
-	{			/* ISENABLERn */
-		.base		= 0x100,
+	{
+		.base		= GIC_DIST_ENABLE_SET,
 		.len		= VGIC_NR_IRQS / 8,
 		.handle_mmio	= handle_mmio_set_enable_reg,
 	},
-	{			/* ICENABLERn */
-		.base		= 0x180,
+	{
+		.base		= GIC_DIST_ENABLE_CLEAR,
 		.len		= VGIC_NR_IRQS / 8,
 		.handle_mmio	= handle_mmio_clear_enable_reg,
 	},
-	{			/* ISPENDRn */
-		.base		= 0x200,
+	{
+		.base		= GIC_DIST_PENDING_SET,
 		.len		= VGIC_NR_IRQS / 8,
 		.handle_mmio	= handle_mmio_set_pending_reg,
 	},
-	{			/* ICPENDRn */
-		.base		= 0x280,
+	{
+		.base		= GIC_DIST_PENDING_CLEAR,
 		.len		= VGIC_NR_IRQS / 8,
 		.handle_mmio	= handle_mmio_clear_pending_reg,
 	},
-	{			/* ISACTIVERn */
-		.base		= 0x300,
+	{
+		.base		= GIC_DIST_ACTIVE_SET,
 		.len		= VGIC_NR_IRQS / 8,
 		.handle_mmio	= handle_mmio_raz_wi,
 	},
-	{			/* ICACTIVERn */
-		.base		= 0x380,
+	{
+		.base		= GIC_DIST_ACTIVE_CLEAR,
 		.len		= VGIC_NR_IRQS / 8,
 		.handle_mmio	= handle_mmio_raz_wi,
 	},
-	{			/* IPRIORITYRn */
-		.base		= 0x400,
+	{
+		.base		= GIC_DIST_PRI,
 		.len		= VGIC_NR_IRQS,
 		.handle_mmio	= handle_mmio_priority_reg,
 	},
-	{			/* ITARGETSRn */
-		.base		= 0x800,
+	{
+		.base		= GIC_DIST_TARGET,
 		.len		= VGIC_NR_IRQS,
 		.handle_mmio	= handle_mmio_target_reg,
 	},
-	{			/* ICFGRn */
-		.base		= 0xC00,
+	{
+		.base		= GIC_DIST_CONFIG,
 		.len		= VGIC_NR_IRQS / 4,
 		.handle_mmio	= handle_mmio_cfg_reg,
 	},
-	{			/* SGIRn */
-		.base		= 0xF00,
+	{
+		.base		= GIC_DIST_SOFTINT,
 		.len		= 4,
 		.handle_mmio	= handle_mmio_sgi_reg,
 	},

@@ -856,7 +853,7 @@ static bool vgic_queue_irq(struct kvm_vcpu *vcpu,

u8 sgi_source_id, int irq)

 	/* Sanitize the input... */
 	BUG_ON(sgi_source_id & ~7);
-	BUG_ON(sgi_source_id && irq > 15);
+	BUG_ON(sgi_source_id && irq >= VGIC_NR_SGIS);
 	BUG_ON(irq >= VGIC_NR_IRQS);

 	kvm_debug("Queue IRQ%d\n", irq);
--


commit 940c2382e1d1cb6831d35ceeccb02c3d3f76a45c
Author: Christoffer Dall [off-list ref]
Date:   Mon Jan 14 16:51:30 2013 -0500

    ARM: gic: add missing distributor defintions

    Add missing register map offsets for the distributor and rename
    GIC_DIST_ACTIVE_BIT to GIC_DIST_ACTIVE_SET to be consistent.

    Cc: Marc Zyniger [off-list ref]
    Signed-off-by: Christoffer Dall [off-list ref]

diff --git a/arch/arm/include/asm/hardware/gic.h

b/arch/arm/include/asm/hardware/gic.h
index dd1add1..6cad421 100644

--- a/arch/arm/include/asm/hardware/gic.h
+++ b/arch/arm/include/asm/hardware/gic.h

@@ -22,11 +22,13 @@

 #define GIC_DIST_CTRL			0x000
 #define GIC_DIST_CTR			0x004
+#define GIC_DIST_IGROUP			0x080
 #define GIC_DIST_ENABLE_SET		0x100
 #define GIC_DIST_ENABLE_CLEAR		0x180
 #define GIC_DIST_PENDING_SET		0x200
 #define GIC_DIST_PENDING_CLEAR		0x280
-#define GIC_DIST_ACTIVE_BIT		0x300
+#define GIC_DIST_ACTIVE_SET		0x300
+#define GIC_DIST_ACTIVE_CLEAR		0x380
 #define GIC_DIST_PRI			0x400
 #define GIC_DIST_TARGET			0x800
 #define GIC_DIST_CONFIG			0xc00
--

Thanks,
-Christoffer