Re: [PATCH v5 06/12] ARM: KVM: VGIC distributor handling

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Mon, Jan 14, 2013 at 10:39 AM, Will Deacon <will.deacon@xxxxxxx> wrote:
> On Tue, Jan 08, 2013 at 06:42:04PM +0000, Christoffer Dall wrote:
>> From: Marc Zyngier <marc.zyngier@xxxxxxx>
>>
>> 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 <marc.zyngier@xxxxxxx>
>> Signed-off-by: Christoffer Dall <c.dall@xxxxxxxxxxxxxxxxxxxxxx>
>> ---
>>  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).
>
>> +#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
>
>
>> +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?
>
>> +       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);
>> +               }
>> +       }
>> +}
>
> [...]
>
>>  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 <c.dall@xxxxxxxxxxxxxxxxxxxxxx>
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 <c.dall@xxxxxxxxxxxxxxxxxxxxxx>

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 <c.dall@xxxxxxxxxxxxxxxxxxxxxx>
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 <marc.zyngier@xxxxxxx>
    Signed-off-by: Christoffer Dall <c.dall@xxxxxxxxxxxxxxxxxxxxxx>

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
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[Index of Archives]     [KVM ARM]     [KVM ia64]     [KVM ppc]     [Virtualization Tools]     [Spice Development]     [Libvirt]     [Libvirt Users]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite Questions]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux