[RFC PATCH] xen/arm: Deliver interrupts to vcpu specified in IROUTER

[RFC PATCH] xen/arm: Deliver interrupts to vcpu specified in IROUTER

[vijay.kilari]

From: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>

In GICv3 use IROUTER register contents to
deliver irq to specified vcpu's.

In error scenario fallback to vcpu 0.

This patch is similar to Stefano's commit
5b3a817ea33b891caf7d7d788da9ce6deffa82a1 for GICv2

Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
---
 xen/arch/arm/vgic-v3.c |  115 ++++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 106 insertions(+), 9 deletions(-)

diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
index ecda672..b01a201 100644
--- a/xen/arch/arm/vgic-v3.c
+++ b/xen/arch/arm/vgic-v3.c
@@ -45,6 +45,35 @@
 #define GICV3_GICR_PIDR2  GICV3_GICD_PIDR2
 #define GICV3_GICR_PIDR4  GICV3_GICD_PIDR4
 
+static struct vcpu *vgicv3_irouter_to_vcpu(struct vcpu *v,
+                                           uint64_t irouter_aff)
+{
+    int i;
+    uint64_t cpu_affinity;
+    struct vcpu *v_target = NULL;
+
+    /* If routing mode is set. Fallback to vcpu0 */
+    if ( irouter_aff & GICD_IROUTER_SPI_MODE_ANY )
+        return v->domain->vcpu[0];
+
+    for ( i = 0; i < v->domain->max_vcpus; i++ )
+    {
+        v_target = v->domain->vcpu[i];
+        cpu_affinity = (MPIDR_AFFINITY_LEVEL(v_target->arch.vmpidr, 3) << 32 |
+                        MPIDR_AFFINITY_LEVEL(v_target->arch.vmpidr, 2) << 16 |
+                        MPIDR_AFFINITY_LEVEL(v_target->arch.vmpidr, 1) << 8  |
+                        MPIDR_AFFINITY_LEVEL(v_target->arch.vmpidr, 0));
+
+        if ( irouter_aff == cpu_affinity )
+            return v_target;
+    }
+    gdprintk(XENLOG_WARNING,
+             "d%d: No vcpu match found for irouter 0x%lx\n",
+             v->domain->domain_id, irouter_aff);
+    /* XXX: Fallback to vcpu0? */
+    return v->domain->vcpu[0];
+}
+
 static int __vgic_v3_rdistr_rd_mmio_read(struct vcpu *v, mmio_info_t *info,
                                          uint32_t gicr_reg)
 {
@@ -347,9 +376,9 @@ static int __vgic_v3_distr_common_mmio_write(struct vcpu *v, mmio_info_t *info,
         vgic_lock_rank(v, rank, flags);
         tr = rank->ienable;
         rank->ienable |= *r;
-        vgic_unlock_rank(v, rank, flags);
         /* The irq number is extracted from offset. so shift by register size */
         vgic_enable_irqs(v, (*r) & (~tr), (reg - GICD_ISENABLER) >> DABT_WORD);
+        vgic_unlock_rank(v, rank, flags);
         return 1;
     case GICD_ICENABLER ... GICD_ICENABLERN:
         if ( dabt.size != DABT_WORD ) goto bad_width;
@@ -358,9 +387,9 @@ static int __vgic_v3_distr_common_mmio_write(struct vcpu *v, mmio_info_t *info,
         vgic_lock_rank(v, rank, flags);
         tr = rank->ienable;
         rank->ienable &= ~*r;
-        vgic_unlock_rank(v, rank, flags);
         /* The irq number is extracted from offset. so shift by register size */
         vgic_disable_irqs(v, (*r) & tr, (reg - GICD_ICENABLER) >> DABT_WORD);
+        vgic_unlock_rank(v, rank, flags);
         return 1;
     case GICD_ISPENDR ... GICD_ISPENDRN:
         if ( dabt.size != DABT_WORD ) goto bad_width;
@@ -749,6 +778,9 @@ static int vgic_v3_distr_mmio_write(struct vcpu *v, mmio_info_t *info)
     register_t *r = select_user_reg(regs, dabt.reg);
     struct vgic_irq_rank *rank;
     unsigned long flags;
+    uint64_t new_target, old_target;
+    struct vcpu *old_vcpu, *new_vcpu;
+    int irq;
     int gicd_reg = (int)(info->gpa - v->domain->arch.vgic.dbase);
 
     switch ( gicd_reg )
@@ -804,16 +836,42 @@ static int vgic_v3_distr_mmio_write(struct vcpu *v, mmio_info_t *info)
         if ( dabt.size != DABT_DOUBLE_WORD ) goto bad_width;
         rank = vgic_rank_offset(v, 64, gicd_reg - GICD_IROUTER, DABT_DOUBLE_WORD);
         if ( rank == NULL) goto write_ignore_64;
-        if ( *r )
+        new_target = *r;
+        vgic_lock_rank(v, rank, flags);
+        old_target = rank->v3.irouter[REG_RANK_INDEX(64,
+                              (gicd_reg - GICD_IROUTER), DABT_DOUBLE_WORD)];
+        if ( *r & GICD_IROUTER_SPI_MODE_ANY )
         {
-            /* TODO: Ignored. We don't support irq delivery for vcpu != 0 */
-            gdprintk(XENLOG_DEBUG,
-                     "SPI delivery to secondary cpus not supported\n");
-            goto write_ignore_64;
+            /* 
+             * IRQ routing mode set. Route any one processor in the entire
+             * system. We chose vcpu 0
+             */
+            rank->v3.irouter[REG_RANK_INDEX(64,
+                     (gicd_reg - GICD_IROUTER), DABT_DOUBLE_WORD)] = *r;
+            vgic_unlock_rank(v, rank, flags);
+            return 1;
         }
-        vgic_lock_rank(v, rank, flags);
+        irq = (gicd_reg - GICD_IROUTER)/8;
+        /* IROUTER now specifies only one cpu affinity for this irq */
+        /* Migrate from any core(vcpu0) to new_target */
+        if ( (old_target & GICD_IROUTER_SPI_MODE_ANY) &&
+             !(new_target & GICD_IROUTER_SPI_MODE_ANY) )
+        {
+            new_vcpu = vgicv3_irouter_to_vcpu(v, new_target);
+            vgic_migrate_irq(v->domain->vcpu[0], new_vcpu, irq);
+        }
+        else
+        {
+            if ( old_target != new_target )
+            {
+                old_vcpu = vgicv3_irouter_to_vcpu(v, old_target);
+                new_vcpu = vgicv3_irouter_to_vcpu(v, new_target);
+                vgic_migrate_irq(old_vcpu, new_vcpu, irq);
+            }
+        }
+ 
         rank->v3.irouter[REG_RANK_INDEX(64,
-                      (gicd_reg - GICD_IROUTER), DABT_DOUBLE_WORD)] = *r;
+                      (gicd_reg - GICD_IROUTER), DABT_DOUBLE_WORD)] = new_target;
         vgic_unlock_rank(v, rank, flags);
         return 1;
     case GICD_NSACR ... GICD_NSACRN:
@@ -928,6 +986,44 @@ static const struct mmio_handler_ops vgic_distr_mmio_handler = {
     .write_handler = vgic_v3_distr_mmio_write,
 };
 
+static struct vcpu *vgicv3_get_target_vcpu(struct vcpu *v, unsigned int irq)
+{
+    int i;
+    uint64_t irq_affinity, cpu_affinity;
+    struct vcpu *v_target;
+    struct vgic_irq_rank *rank = vgic_rank_irq(v, irq);
+
+    ASSERT(spin_is_locked(&rank->lock));
+
+    irq_affinity = rank->v3.irouter[irq % 32];
+    if ( irq_affinity & GICD_IROUTER_SPI_MODE_ANY )
+    {
+        /* 
+         * IRQ routing mode set. Route any one processor in the entire
+         * system. We chose vcpu 0
+         */
+        return v->domain->vcpu[0];
+    }
+     
+    for ( i = 0; i < v->domain->max_vcpus; i++ )
+    {
+        v_target = v->domain->vcpu[i];
+        cpu_affinity = (MPIDR_AFFINITY_LEVEL(v_target->arch.vmpidr, 3) << 32 |
+                        MPIDR_AFFINITY_LEVEL(v_target->arch.vmpidr, 2) << 16 |
+                        MPIDR_AFFINITY_LEVEL(v_target->arch.vmpidr, 1) << 8  |
+                        MPIDR_AFFINITY_LEVEL(v_target->arch.vmpidr, 0));
+
+        if ( irq_affinity == cpu_affinity )
+            return v_target;
+    }
+    gdprintk(XENLOG_WARNING,
+             "d%d: No vcpu match found for irq %d with irouter 0x%lx\n",
+             v->domain->domain_id, irq, irq_affinity );
+
+    /* XXX:Fallback to vcpu0 ? */
+    return v->domain->vcpu[0];
+}
+
 static int vgicv3_vcpu_init(struct vcpu *v)
 {
     int i;
@@ -968,6 +1064,7 @@ static int vgicv3_domain_init(struct domain *d)
 static const struct vgic_ops v3_ops = {
     .vcpu_init   = vgicv3_vcpu_init,
     .domain_init = vgicv3_domain_init,
+    .get_target_vcpu = vgicv3_get_target_vcpu,
     .emulate_sysreg  = vgicv3_emulate_sysreg,
 };
 
-- 
1.7.9.5

Re: [RFC PATCH] xen/arm: Deliver interrupts to vcpu specified in IROUTER

[Stefano Stabellini]

On Thu, 4 Sep 2014, vijay.kilari@gmail.com wrote:
> From: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
> 
> In GICv3 use IROUTER register contents to
> deliver irq to specified vcpu's.
> 
> In error scenario fallback to vcpu 0.
> 
> This patch is similar to Stefano's commit
> 5b3a817ea33b891caf7d7d788da9ce6deffa82a1 for GICv2
> 
> Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
> ---
>  xen/arch/arm/vgic-v3.c |  115 ++++++++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 106 insertions(+), 9 deletions(-)
> 
> diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
> index ecda672..b01a201 100644
> --- a/xen/arch/arm/vgic-v3.c
> +++ b/xen/arch/arm/vgic-v3.c
> @@ -45,6 +45,35 @@
>  #define GICV3_GICR_PIDR2  GICV3_GICD_PIDR2
>  #define GICV3_GICR_PIDR4  GICV3_GICD_PIDR4
>  
> +static struct vcpu *vgicv3_irouter_to_vcpu(struct vcpu *v,
> +                                           uint64_t irouter_aff)
> +{
> +    int i;
> +    uint64_t cpu_affinity;
> +    struct vcpu *v_target = NULL;
> +
> +    /* If routing mode is set. Fallback to vcpu0 */
> +    if ( irouter_aff & GICD_IROUTER_SPI_MODE_ANY )
> +        return v->domain->vcpu[0];
> +
> +    for ( i = 0; i < v->domain->max_vcpus; i++ )
> +    {
> +        v_target = v->domain->vcpu[i];
> +        cpu_affinity = (MPIDR_AFFINITY_LEVEL(v_target->arch.vmpidr, 3) << 32 |
> +                        MPIDR_AFFINITY_LEVEL(v_target->arch.vmpidr, 2) << 16 |
> +                        MPIDR_AFFINITY_LEVEL(v_target->arch.vmpidr, 1) << 8  |
> +                        MPIDR_AFFINITY_LEVEL(v_target->arch.vmpidr, 0));
> +
> +        if ( irouter_aff == cpu_affinity )
> +            return v_target;
> +    }

Using a loop is pretty bad. Couldn't you just calculate the lowest vcpu
number from irouter_aff (maybe using find_first_bit)?


> +    gdprintk(XENLOG_WARNING,
> +             "d%d: No vcpu match found for irouter 0x%lx\n",
> +             v->domain->domain_id, irouter_aff);
> +    /* XXX: Fallback to vcpu0? */
> +    return v->domain->vcpu[0];
> +}
> +
>  static int __vgic_v3_rdistr_rd_mmio_read(struct vcpu *v, mmio_info_t *info,
>                                           uint32_t gicr_reg)
>  {
> @@ -347,9 +376,9 @@ static int __vgic_v3_distr_common_mmio_write(struct vcpu *v, mmio_info_t *info,
>          vgic_lock_rank(v, rank, flags);
>          tr = rank->ienable;
>          rank->ienable |= *r;
> -        vgic_unlock_rank(v, rank, flags);
>          /* The irq number is extracted from offset. so shift by register size */
>          vgic_enable_irqs(v, (*r) & (~tr), (reg - GICD_ISENABLER) >> DABT_WORD);
> +        vgic_unlock_rank(v, rank, flags);
>          return 1;
>      case GICD_ICENABLER ... GICD_ICENABLERN:
>          if ( dabt.size != DABT_WORD ) goto bad_width;
> @@ -358,9 +387,9 @@ static int __vgic_v3_distr_common_mmio_write(struct vcpu *v, mmio_info_t *info,
>          vgic_lock_rank(v, rank, flags);
>          tr = rank->ienable;
>          rank->ienable &= ~*r;
> -        vgic_unlock_rank(v, rank, flags);
>          /* The irq number is extracted from offset. so shift by register size */
>          vgic_disable_irqs(v, (*r) & tr, (reg - GICD_ICENABLER) >> DABT_WORD);
> +        vgic_unlock_rank(v, rank, flags);
>          return 1;
>      case GICD_ISPENDR ... GICD_ISPENDRN:
>          if ( dabt.size != DABT_WORD ) goto bad_width;
> @@ -749,6 +778,9 @@ static int vgic_v3_distr_mmio_write(struct vcpu *v, mmio_info_t *info)
>      register_t *r = select_user_reg(regs, dabt.reg);
>      struct vgic_irq_rank *rank;
>      unsigned long flags;
> +    uint64_t new_target, old_target;
> +    struct vcpu *old_vcpu, *new_vcpu;
> +    int irq;
>      int gicd_reg = (int)(info->gpa - v->domain->arch.vgic.dbase);
>  
>      switch ( gicd_reg )
> @@ -804,16 +836,42 @@ static int vgic_v3_distr_mmio_write(struct vcpu *v, mmio_info_t *info)
>          if ( dabt.size != DABT_DOUBLE_WORD ) goto bad_width;
>          rank = vgic_rank_offset(v, 64, gicd_reg - GICD_IROUTER, DABT_DOUBLE_WORD);
>          if ( rank == NULL) goto write_ignore_64;
> -        if ( *r )
> +        new_target = *r;
> +        vgic_lock_rank(v, rank, flags);
> +        old_target = rank->v3.irouter[REG_RANK_INDEX(64,
> +                              (gicd_reg - GICD_IROUTER), DABT_DOUBLE_WORD)];
> +        if ( *r & GICD_IROUTER_SPI_MODE_ANY )
>          {
> -            /* TODO: Ignored. We don't support irq delivery for vcpu != 0 */
> -            gdprintk(XENLOG_DEBUG,
> -                     "SPI delivery to secondary cpus not supported\n");
> -            goto write_ignore_64;
> +            /* 
> +             * IRQ routing mode set. Route any one processor in the entire
> +             * system. We chose vcpu 0
> +             */
> +            rank->v3.irouter[REG_RANK_INDEX(64,
> +                     (gicd_reg - GICD_IROUTER), DABT_DOUBLE_WORD)] = *r;
> +            vgic_unlock_rank(v, rank, flags);
> +            return 1;
>          }
> -        vgic_lock_rank(v, rank, flags);
> +        irq = (gicd_reg - GICD_IROUTER)/8;
> +        /* IROUTER now specifies only one cpu affinity for this irq */
> +        /* Migrate from any core(vcpu0) to new_target */
> +        if ( (old_target & GICD_IROUTER_SPI_MODE_ANY) &&
> +             !(new_target & GICD_IROUTER_SPI_MODE_ANY) )
> +        {
> +            new_vcpu = vgicv3_irouter_to_vcpu(v, new_target);
> +            vgic_migrate_irq(v->domain->vcpu[0], new_vcpu, irq);
> +        }
> +        else
> +        {
> +            if ( old_target != new_target )
> +            {
> +                old_vcpu = vgicv3_irouter_to_vcpu(v, old_target);
> +                new_vcpu = vgicv3_irouter_to_vcpu(v, new_target);
> +                vgic_migrate_irq(old_vcpu, new_vcpu, irq);
> +            }
> +        }

The issue is that if irouter is GICD_IROUTER_SPI_MODE_ANY, you always
return vcpu0 below.
So if the kernel:

1) move the irq to vcpu1
2) set the irq as GICD_IROUTER_SPI_MODE_ANY

you now have migrated the irq to vcpu1 but you are returning vcpu0 from
vgicv3_get_target_vcpu.

You can either:
- vgic_migrate_irq the irq back to vcpu0 when the kernel sets the irq as
  GICD_IROUTER_SPI_MODE_ANY
- return the current vcpu for the irq rather than vcpu0 if
  GICD_IROUTER_SPI_MODE_ANY

Either way it would work. Maybe the second option might be better.


>          rank->v3.irouter[REG_RANK_INDEX(64,
> -                      (gicd_reg - GICD_IROUTER), DABT_DOUBLE_WORD)] = *r;
> +                      (gicd_reg - GICD_IROUTER), DABT_DOUBLE_WORD)] = new_target;
>          vgic_unlock_rank(v, rank, flags);
>          return 1;
>      case GICD_NSACR ... GICD_NSACRN:
> @@ -928,6 +986,44 @@ static const struct mmio_handler_ops vgic_distr_mmio_handler = {
>      .write_handler = vgic_v3_distr_mmio_write,
>  };
>  
> +static struct vcpu *vgicv3_get_target_vcpu(struct vcpu *v, unsigned int irq)

for consistency I would call it vgic_v3_get_target_vcpu


> +{
> +    int i;
> +    uint64_t irq_affinity, cpu_affinity;
> +    struct vcpu *v_target;
> +    struct vgic_irq_rank *rank = vgic_rank_irq(v, irq);
> +
> +    ASSERT(spin_is_locked(&rank->lock));
> +
> +    irq_affinity = rank->v3.irouter[irq % 32];
> +    if ( irq_affinity & GICD_IROUTER_SPI_MODE_ANY )
> +    {
> +        /* 
> +         * IRQ routing mode set. Route any one processor in the entire
> +         * system. We chose vcpu 0
> +         */
> +        return v->domain->vcpu[0];
> +    }
> +     
> +    for ( i = 0; i < v->domain->max_vcpus; i++ )
> +    {
> +        v_target = v->domain->vcpu[i];
> +        cpu_affinity = (MPIDR_AFFINITY_LEVEL(v_target->arch.vmpidr, 3) << 32 |
> +                        MPIDR_AFFINITY_LEVEL(v_target->arch.vmpidr, 2) << 16 |
> +                        MPIDR_AFFINITY_LEVEL(v_target->arch.vmpidr, 1) << 8  |
> +                        MPIDR_AFFINITY_LEVEL(v_target->arch.vmpidr, 0));
> +
> +        if ( irq_affinity == cpu_affinity )
> +            return v_target;
> +    }

Same comment about the loop.


> +    gdprintk(XENLOG_WARNING,
> +             "d%d: No vcpu match found for irq %d with irouter 0x%lx\n",
> +             v->domain->domain_id, irq, irq_affinity );
> +
> +    /* XXX:Fallback to vcpu0 ? */
> +    return v->domain->vcpu[0];

This is wrong: only valid values should be allowed in rank->v3.irouter.
If the kernel writes a bad value to IROUTER we should catch it in
vgic_v3_distr_mmio_write and avoid saving it.


> +}
> +
>  static int vgicv3_vcpu_init(struct vcpu *v)
>  {
>      int i;
> @@ -968,6 +1064,7 @@ static int vgicv3_domain_init(struct domain *d)
>  static const struct vgic_ops v3_ops = {
>      .vcpu_init   = vgicv3_vcpu_init,
>      .domain_init = vgicv3_domain_init,
> +    .get_target_vcpu = vgicv3_get_target_vcpu,
>      .emulate_sysreg  = vgicv3_emulate_sysreg,
>  };
>  
> -- 
> 1.7.9.5
> 

Re: [RFC PATCH] xen/arm: Deliver interrupts to vcpu specified in IROUTER

[Vijay Kilari]

On Fri, Sep 5, 2014 at 4:59 AM, Stefano Stabellini
<stefano.stabellini@eu.citrix.com> wrote:
> On Thu, 4 Sep 2014, vijay.kilari@gmail.com wrote:
>> From: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
>>
>> In GICv3 use IROUTER register contents to
>> deliver irq to specified vcpu's.
>>
>> In error scenario fallback to vcpu 0.
>>
>> This patch is similar to Stefano's commit
>> 5b3a817ea33b891caf7d7d788da9ce6deffa82a1 for GICv2
>>
>> Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
>> ---
>>  xen/arch/arm/vgic-v3.c |  115 ++++++++++++++++++++++++++++++++++++++++++++----
>>  1 file changed, 106 insertions(+), 9 deletions(-)
>>
>> diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
>> index ecda672..b01a201 100644
>> --- a/xen/arch/arm/vgic-v3.c
>> +++ b/xen/arch/arm/vgic-v3.c
>> @@ -45,6 +45,35 @@
>>  #define GICV3_GICR_PIDR2  GICV3_GICD_PIDR2
>>  #define GICV3_GICR_PIDR4  GICV3_GICD_PIDR4
>>
>> +static struct vcpu *vgicv3_irouter_to_vcpu(struct vcpu *v,
>> +                                           uint64_t irouter_aff)
>> +{
>> +    int i;
>> +    uint64_t cpu_affinity;
>> +    struct vcpu *v_target = NULL;
>> +
>> +    /* If routing mode is set. Fallback to vcpu0 */
>> +    if ( irouter_aff & GICD_IROUTER_SPI_MODE_ANY )
>> +        return v->domain->vcpu[0];
>> +
>> +    for ( i = 0; i < v->domain->max_vcpus; i++ )
>> +    {
>> +        v_target = v->domain->vcpu[i];
>> +        cpu_affinity = (MPIDR_AFFINITY_LEVEL(v_target->arch.vmpidr, 3) << 32 |
>> +                        MPIDR_AFFINITY_LEVEL(v_target->arch.vmpidr, 2) << 16 |
>> +                        MPIDR_AFFINITY_LEVEL(v_target->arch.vmpidr, 1) << 8  |
>> +                        MPIDR_AFFINITY_LEVEL(v_target->arch.vmpidr, 0));
>> +
>> +        if ( irouter_aff == cpu_affinity )
>> +            return v_target;
>> +    }
>
> Using a loop is pretty bad. Couldn't you just calculate the lowest vcpu
> number from irouter_aff (maybe using find_first_bit)?

IMO,  If GICD_IROUTER_SPI_MODE_ANY bit is set, then it specifies any cpu.
If GICD_IROUTER_SPI_MODE_ANY is not set, it register specifies only
one cpu number.

So we cannot use find_first_bit() unlike GICv2 where ITARGET specifies
irq affinity
to more than one CPU.

If at all we want to avoid this for loop, then we have to maintain one
more variable for
every IROUTERn and corresponding vpcu number, which is updated on
IROUTERn update

>
>
>> +    gdprintk(XENLOG_WARNING,
>> +             "d%d: No vcpu match found for irouter 0x%lx\n",
>> +             v->domain->domain_id, irouter_aff);
>> +    /* XXX: Fallback to vcpu0? */
>> +    return v->domain->vcpu[0];
>> +}
>> +
>>  static int __vgic_v3_rdistr_rd_mmio_read(struct vcpu *v, mmio_info_t *info,
>>                                           uint32_t gicr_reg)
>>  {
>> @@ -347,9 +376,9 @@ static int __vgic_v3_distr_common_mmio_write(struct vcpu *v, mmio_info_t *info,
>>          vgic_lock_rank(v, rank, flags);
>>          tr = rank->ienable;
>>          rank->ienable |= *r;
>> -        vgic_unlock_rank(v, rank, flags);
>>          /* The irq number is extracted from offset. so shift by register size */
>>          vgic_enable_irqs(v, (*r) & (~tr), (reg - GICD_ISENABLER) >> DABT_WORD);
>> +        vgic_unlock_rank(v, rank, flags);
>>          return 1;
>>      case GICD_ICENABLER ... GICD_ICENABLERN:
>>          if ( dabt.size != DABT_WORD ) goto bad_width;
>> @@ -358,9 +387,9 @@ static int __vgic_v3_distr_common_mmio_write(struct vcpu *v, mmio_info_t *info,
>>          vgic_lock_rank(v, rank, flags);
>>          tr = rank->ienable;
>>          rank->ienable &= ~*r;
>> -        vgic_unlock_rank(v, rank, flags);
>>          /* The irq number is extracted from offset. so shift by register size */
>>          vgic_disable_irqs(v, (*r) & tr, (reg - GICD_ICENABLER) >> DABT_WORD);
>> +        vgic_unlock_rank(v, rank, flags);
>>          return 1;
>>      case GICD_ISPENDR ... GICD_ISPENDRN:
>>          if ( dabt.size != DABT_WORD ) goto bad_width;
>> @@ -749,6 +778,9 @@ static int vgic_v3_distr_mmio_write(struct vcpu *v, mmio_info_t *info)
>>      register_t *r = select_user_reg(regs, dabt.reg);
>>      struct vgic_irq_rank *rank;
>>      unsigned long flags;
>> +    uint64_t new_target, old_target;
>> +    struct vcpu *old_vcpu, *new_vcpu;
>> +    int irq;
>>      int gicd_reg = (int)(info->gpa - v->domain->arch.vgic.dbase);
>>
>>      switch ( gicd_reg )
>> @@ -804,16 +836,42 @@ static int vgic_v3_distr_mmio_write(struct vcpu *v, mmio_info_t *info)
>>          if ( dabt.size != DABT_DOUBLE_WORD ) goto bad_width;
>>          rank = vgic_rank_offset(v, 64, gicd_reg - GICD_IROUTER, DABT_DOUBLE_WORD);
>>          if ( rank == NULL) goto write_ignore_64;
>> -        if ( *r )
>> +        new_target = *r;
>> +        vgic_lock_rank(v, rank, flags);
>> +        old_target = rank->v3.irouter[REG_RANK_INDEX(64,
>> +                              (gicd_reg - GICD_IROUTER), DABT_DOUBLE_WORD)];
>> +        if ( *r & GICD_IROUTER_SPI_MODE_ANY )
>>          {
>> -            /* TODO: Ignored. We don't support irq delivery for vcpu != 0 */
>> -            gdprintk(XENLOG_DEBUG,
>> -                     "SPI delivery to secondary cpus not supported\n");
>> -            goto write_ignore_64;
>> +            /*
>> +             * IRQ routing mode set. Route any one processor in the entire
>> +             * system. We chose vcpu 0
>> +             */
>> +            rank->v3.irouter[REG_RANK_INDEX(64,
>> +                     (gicd_reg - GICD_IROUTER), DABT_DOUBLE_WORD)] = *r;
>> +            vgic_unlock_rank(v, rank, flags);
>> +            return 1;
>>          }
>> -        vgic_lock_rank(v, rank, flags);
>> +        irq = (gicd_reg - GICD_IROUTER)/8;
>> +        /* IROUTER now specifies only one cpu affinity for this irq */
>> +        /* Migrate from any core(vcpu0) to new_target */
>> +        if ( (old_target & GICD_IROUTER_SPI_MODE_ANY) &&
>> +             !(new_target & GICD_IROUTER_SPI_MODE_ANY) )
>> +        {
>> +            new_vcpu = vgicv3_irouter_to_vcpu(v, new_target);
>> +            vgic_migrate_irq(v->domain->vcpu[0], new_vcpu, irq);
>> +        }
>> +        else
>> +        {
>> +            if ( old_target != new_target )
>> +            {
>> +                old_vcpu = vgicv3_irouter_to_vcpu(v, old_target);
>> +                new_vcpu = vgicv3_irouter_to_vcpu(v, new_target);
>> +                vgic_migrate_irq(old_vcpu, new_vcpu, irq);
>> +            }
>> +        }
>
> The issue is that if irouter is GICD_IROUTER_SPI_MODE_ANY, you always
> return vcpu0 below.
> So if the kernel:
>
> 1) move the irq to vcpu1
> 2) set the irq as GICD_IROUTER_SPI_MODE_ANY
>
> you now have migrated the irq to vcpu1 but you are returning vcpu0 from
> vgicv3_get_target_vcpu.
>
> You can either:
> - vgic_migrate_irq the irq back to vcpu0 when the kernel sets the irq as
>   GICD_IROUTER_SPI_MODE_ANY

   When irq is set to vcpu1 and later when GICD_IROUTER_SPI_MODE_ANY
then I assume that vcpu1 is also valid for the moment if irq is pending.
So no need to do any migration.

Also, as per GICv3 spec, when GICD_IROUTER_SPI_MODE_ANY is set which is
previous not set ( 0 -> 1 ), the affinity value in IROUTER is invalid/unknown.
So later when vgicv3_get_target_vcpu() is called, it always returns vcpu0.

> - return the current vcpu for the irq rather than vcpu0 if
>   GICD_IROUTER_SPI_MODE_ANY
>
> Either way it would work. Maybe the second option might be better.
>
>
>>          rank->v3.irouter[REG_RANK_INDEX(64,
>> -                      (gicd_reg - GICD_IROUTER), DABT_DOUBLE_WORD)] = *r;
>> +                      (gicd_reg - GICD_IROUTER), DABT_DOUBLE_WORD)] = new_target;
>>          vgic_unlock_rank(v, rank, flags);
>>          return 1;
>>      case GICD_NSACR ... GICD_NSACRN:
>> @@ -928,6 +986,44 @@ static const struct mmio_handler_ops vgic_distr_mmio_handler = {
>>      .write_handler = vgic_v3_distr_mmio_write,
>>  };
>>
>> +static struct vcpu *vgicv3_get_target_vcpu(struct vcpu *v, unsigned int irq)
>
> for consistency I would call it vgic_v3_get_target_vcpu
>
>
>> +{
>> +    int i;
>> +    uint64_t irq_affinity, cpu_affinity;
>> +    struct vcpu *v_target;
>> +    struct vgic_irq_rank *rank = vgic_rank_irq(v, irq);
>> +
>> +    ASSERT(spin_is_locked(&rank->lock));
>> +
>> +    irq_affinity = rank->v3.irouter[irq % 32];
>> +    if ( irq_affinity & GICD_IROUTER_SPI_MODE_ANY )
>> +    {
>> +        /*
>> +         * IRQ routing mode set. Route any one processor in the entire
>> +         * system. We chose vcpu 0
>> +         */
>> +        return v->domain->vcpu[0];
>> +    }
>> +
>> +    for ( i = 0; i < v->domain->max_vcpus; i++ )
>> +    {
>> +        v_target = v->domain->vcpu[i];
>> +        cpu_affinity = (MPIDR_AFFINITY_LEVEL(v_target->arch.vmpidr, 3) << 32 |
>> +                        MPIDR_AFFINITY_LEVEL(v_target->arch.vmpidr, 2) << 16 |
>> +                        MPIDR_AFFINITY_LEVEL(v_target->arch.vmpidr, 1) << 8  |
>> +                        MPIDR_AFFINITY_LEVEL(v_target->arch.vmpidr, 0));
>> +
>> +        if ( irq_affinity == cpu_affinity )
>> +            return v_target;
>> +    }
>
> Same comment about the loop.
>
>
>> +    gdprintk(XENLOG_WARNING,
>> +             "d%d: No vcpu match found for irq %d with irouter 0x%lx\n",
>> +             v->domain->domain_id, irq, irq_affinity );
>> +
>> +    /* XXX:Fallback to vcpu0 ? */
>> +    return v->domain->vcpu[0];
>
> This is wrong: only valid values should be allowed in rank->v3.irouter.
> If the kernel writes a bad value to IROUTER we should catch it in
> vgic_v3_distr_mmio_write and avoid saving it.

   0x0 is also valid value. I found that VMPIDR for cpu0 is 0x80000000.
Bit[31] is RES1 bit which is set. If we ignore bit[31], 0x0 is also valid
value.

Question: Shouldn't recover with wrong irouter value by choosing vcpu0?

Note: In next version, I will try to move common code in
vgicv3_get_target_vcpu()
 and vgicv3_irouter_to_vcpu() to one single function

>
>
>> +}
>> +
>>  static int vgicv3_vcpu_init(struct vcpu *v)
>>  {
>>      int i;
>> @@ -968,6 +1064,7 @@ static int vgicv3_domain_init(struct domain *d)
>>  static const struct vgic_ops v3_ops = {
>>      .vcpu_init   = vgicv3_vcpu_init,
>>      .domain_init = vgicv3_domain_init,
>> +    .get_target_vcpu = vgicv3_get_target_vcpu,
>>      .emulate_sysreg  = vgicv3_emulate_sysreg,
>>  };
>>
>> --
>> 1.7.9.5
>>

Re: [RFC PATCH] xen/arm: Deliver interrupts to vcpu specified in IROUTER

[Stefano Stabellini]

On Fri, 5 Sep 2014, Vijay Kilari wrote:
> On Fri, Sep 5, 2014 at 4:59 AM, Stefano Stabellini
> <stefano.stabellini@eu.citrix.com> wrote:
> > On Thu, 4 Sep 2014, vijay.kilari@gmail.com wrote:
> >> From: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
> >>
> >> In GICv3 use IROUTER register contents to
> >> deliver irq to specified vcpu's.
> >>
> >> In error scenario fallback to vcpu 0.
> >>
> >> This patch is similar to Stefano's commit
> >> 5b3a817ea33b891caf7d7d788da9ce6deffa82a1 for GICv2
> >>
> >> Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
> >> ---
> >>  xen/arch/arm/vgic-v3.c |  115 ++++++++++++++++++++++++++++++++++++++++++++----
> >>  1 file changed, 106 insertions(+), 9 deletions(-)
> >>
> >> diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
> >> index ecda672..b01a201 100644
> >> --- a/xen/arch/arm/vgic-v3.c
> >> +++ b/xen/arch/arm/vgic-v3.c
> >> @@ -45,6 +45,35 @@
> >>  #define GICV3_GICR_PIDR2  GICV3_GICD_PIDR2
> >>  #define GICV3_GICR_PIDR4  GICV3_GICD_PIDR4
> >>
> >> +static struct vcpu *vgicv3_irouter_to_vcpu(struct vcpu *v,
> >> +                                           uint64_t irouter_aff)
> >> +{
> >> +    int i;
> >> +    uint64_t cpu_affinity;
> >> +    struct vcpu *v_target = NULL;
> >> +
> >> +    /* If routing mode is set. Fallback to vcpu0 */
> >> +    if ( irouter_aff & GICD_IROUTER_SPI_MODE_ANY )
> >> +        return v->domain->vcpu[0];
> >> +
> >> +    for ( i = 0; i < v->domain->max_vcpus; i++ )
> >> +    {
> >> +        v_target = v->domain->vcpu[i];
> >> +        cpu_affinity = (MPIDR_AFFINITY_LEVEL(v_target->arch.vmpidr, 3) << 32 |
> >> +                        MPIDR_AFFINITY_LEVEL(v_target->arch.vmpidr, 2) << 16 |
> >> +                        MPIDR_AFFINITY_LEVEL(v_target->arch.vmpidr, 1) << 8  |
> >> +                        MPIDR_AFFINITY_LEVEL(v_target->arch.vmpidr, 0));
> >> +
> >> +        if ( irouter_aff == cpu_affinity )
> >> +            return v_target;
> >> +    }
> >
> > Using a loop is pretty bad. Couldn't you just calculate the lowest vcpu
> > number from irouter_aff (maybe using find_first_bit)?
> 
> IMO,  If GICD_IROUTER_SPI_MODE_ANY bit is set, then it specifies any cpu.
> If GICD_IROUTER_SPI_MODE_ANY is not set, it register specifies only
> one cpu number.
> 
> So we cannot use find_first_bit() unlike GICv2 where ITARGET specifies
> irq affinity
> to more than one CPU.
> 
> If at all we want to avoid this for loop, then we have to maintain one
> more variable for
> every IROUTERn and corresponding vpcu number, which is updated on
> IROUTERn update

Given that at the moment AFF0 is just set to vcpu_id (see
vcpu_initialise), I would simply do a direct calculation, adding a TODO
comment so that we remember to fix this when we implement smarter mpidr
schemes.

Even better you could introduce two simple functions that convert vmpidr
to vcpu_id and vice versa, so that we keep all the hacks in one place.


> >> +    gdprintk(XENLOG_WARNING,
> >> +             "d%d: No vcpu match found for irouter 0x%lx\n",
> >> +             v->domain->domain_id, irouter_aff);
> >> +    /* XXX: Fallback to vcpu0? */
> >> +    return v->domain->vcpu[0];
> >> +}
> >> +
> >>  static int __vgic_v3_rdistr_rd_mmio_read(struct vcpu *v, mmio_info_t *info,
> >>                                           uint32_t gicr_reg)
> >>  {
> >> @@ -347,9 +376,9 @@ static int __vgic_v3_distr_common_mmio_write(struct vcpu *v, mmio_info_t *info,
> >>          vgic_lock_rank(v, rank, flags);
> >>          tr = rank->ienable;
> >>          rank->ienable |= *r;
> >> -        vgic_unlock_rank(v, rank, flags);
> >>          /* The irq number is extracted from offset. so shift by register size */
> >>          vgic_enable_irqs(v, (*r) & (~tr), (reg - GICD_ISENABLER) >> DABT_WORD);
> >> +        vgic_unlock_rank(v, rank, flags);
> >>          return 1;
> >>      case GICD_ICENABLER ... GICD_ICENABLERN:
> >>          if ( dabt.size != DABT_WORD ) goto bad_width;
> >> @@ -358,9 +387,9 @@ static int __vgic_v3_distr_common_mmio_write(struct vcpu *v, mmio_info_t *info,
> >>          vgic_lock_rank(v, rank, flags);
> >>          tr = rank->ienable;
> >>          rank->ienable &= ~*r;
> >> -        vgic_unlock_rank(v, rank, flags);
> >>          /* The irq number is extracted from offset. so shift by register size */
> >>          vgic_disable_irqs(v, (*r) & tr, (reg - GICD_ICENABLER) >> DABT_WORD);
> >> +        vgic_unlock_rank(v, rank, flags);
> >>          return 1;
> >>      case GICD_ISPENDR ... GICD_ISPENDRN:
> >>          if ( dabt.size != DABT_WORD ) goto bad_width;
> >> @@ -749,6 +778,9 @@ static int vgic_v3_distr_mmio_write(struct vcpu *v, mmio_info_t *info)
> >>      register_t *r = select_user_reg(regs, dabt.reg);
> >>      struct vgic_irq_rank *rank;
> >>      unsigned long flags;
> >> +    uint64_t new_target, old_target;
> >> +    struct vcpu *old_vcpu, *new_vcpu;
> >> +    int irq;
> >>      int gicd_reg = (int)(info->gpa - v->domain->arch.vgic.dbase);
> >>
> >>      switch ( gicd_reg )
> >> @@ -804,16 +836,42 @@ static int vgic_v3_distr_mmio_write(struct vcpu *v, mmio_info_t *info)
> >>          if ( dabt.size != DABT_DOUBLE_WORD ) goto bad_width;
> >>          rank = vgic_rank_offset(v, 64, gicd_reg - GICD_IROUTER, DABT_DOUBLE_WORD);
> >>          if ( rank == NULL) goto write_ignore_64;
> >> -        if ( *r )
> >> +        new_target = *r;
> >> +        vgic_lock_rank(v, rank, flags);
> >> +        old_target = rank->v3.irouter[REG_RANK_INDEX(64,
> >> +                              (gicd_reg - GICD_IROUTER), DABT_DOUBLE_WORD)];
> >> +        if ( *r & GICD_IROUTER_SPI_MODE_ANY )
> >>          {
> >> -            /* TODO: Ignored. We don't support irq delivery for vcpu != 0 */
> >> -            gdprintk(XENLOG_DEBUG,
> >> -                     "SPI delivery to secondary cpus not supported\n");
> >> -            goto write_ignore_64;
> >> +            /*
> >> +             * IRQ routing mode set. Route any one processor in the entire
> >> +             * system. We chose vcpu 0
> >> +             */
> >> +            rank->v3.irouter[REG_RANK_INDEX(64,
> >> +                     (gicd_reg - GICD_IROUTER), DABT_DOUBLE_WORD)] = *r;
> >> +            vgic_unlock_rank(v, rank, flags);
> >> +            return 1;
> >>          }
> >> -        vgic_lock_rank(v, rank, flags);
> >> +        irq = (gicd_reg - GICD_IROUTER)/8;
> >> +        /* IROUTER now specifies only one cpu affinity for this irq */
> >> +        /* Migrate from any core(vcpu0) to new_target */
> >> +        if ( (old_target & GICD_IROUTER_SPI_MODE_ANY) &&
> >> +             !(new_target & GICD_IROUTER_SPI_MODE_ANY) )
> >> +        {
> >> +            new_vcpu = vgicv3_irouter_to_vcpu(v, new_target);
> >> +            vgic_migrate_irq(v->domain->vcpu[0], new_vcpu, irq);
> >> +        }
> >> +        else
> >> +        {
> >> +            if ( old_target != new_target )
> >> +            {
> >> +                old_vcpu = vgicv3_irouter_to_vcpu(v, old_target);
> >> +                new_vcpu = vgicv3_irouter_to_vcpu(v, new_target);
> >> +                vgic_migrate_irq(old_vcpu, new_vcpu, irq);
> >> +            }
> >> +        }
> >
> > The issue is that if irouter is GICD_IROUTER_SPI_MODE_ANY, you always
> > return vcpu0 below.
> > So if the kernel:
> >
> > 1) move the irq to vcpu1
> > 2) set the irq as GICD_IROUTER_SPI_MODE_ANY
> >
> > you now have migrated the irq to vcpu1 but you are returning vcpu0 from
> > vgicv3_get_target_vcpu.
> >
> > You can either:
> > - vgic_migrate_irq the irq back to vcpu0 when the kernel sets the irq as
> >   GICD_IROUTER_SPI_MODE_ANY
> 
>    When irq is set to vcpu1 and later when GICD_IROUTER_SPI_MODE_ANY
> then I assume that vcpu1 is also valid for the moment if irq is pending.
> So no need to do any migration.
 
That would be OK, except that vgicv3_get_target_vcpu is actually
returning vcpu0. We should try to match physical and virtual irq
delivery as much as possible. So if the physical irq is delivered to
pcpu1 (or whatever is the pcpu currently running vcpu1), then we should
try hard to deliver the corresponding virq to vcpu1.

Calling vgic_migrate_irq changes the physical irq affinity to the pcpu
running the destination vcpu. So step 1 above moves physical irq
delivery to pcpu1 (assuming that pcpu1 is running vcpu1 for simplicity).
After the guest kernel sets the irq as GICD_IROUTER_SPI_MODE_ANY, even
though we could deliver the irq to any vcpu, we should actually delivery
the irq to the vcpu running on the pcpu that received the interrupt. In
my example it would still be vcpu1. Therefore we need to return vcpu1
from vgicv3_get_target_vcpu.

Instead as it is now, we would cause an SGI to be sent from pcpu1 to
pcpu0 in order to inject the irq to vcpu0 (assuming that vcpu0 is
running on pcpu0 for simplicity).


> Also, as per GICv3 spec, when GICD_IROUTER_SPI_MODE_ANY is set which is
> previous not set ( 0 -> 1 ), the affinity value in IROUTER is invalid/unknown.
> So later when vgicv3_get_target_vcpu() is called, it always returns vcpu0.

Could you please tell me which one is the relevant chapter of the GICv3
spec? I couldn't find the statement you mentioned. Maybe my version of
the spec is outdated.


> > - return the current vcpu for the irq rather than vcpu0 if
> >   GICD_IROUTER_SPI_MODE_ANY
> >
> > Either way it would work. Maybe the second option might be better.
> >
> >
> >>          rank->v3.irouter[REG_RANK_INDEX(64,
> >> -                      (gicd_reg - GICD_IROUTER), DABT_DOUBLE_WORD)] = *r;
> >> +                      (gicd_reg - GICD_IROUTER), DABT_DOUBLE_WORD)] = new_target;
> >>          vgic_unlock_rank(v, rank, flags);
> >>          return 1;
> >>      case GICD_NSACR ... GICD_NSACRN:
> >> @@ -928,6 +986,44 @@ static const struct mmio_handler_ops vgic_distr_mmio_handler = {
> >>      .write_handler = vgic_v3_distr_mmio_write,
> >>  };
> >>
> >> +static struct vcpu *vgicv3_get_target_vcpu(struct vcpu *v, unsigned int irq)
> >
> > for consistency I would call it vgic_v3_get_target_vcpu
> >
> >
> >> +{
> >> +    int i;
> >> +    uint64_t irq_affinity, cpu_affinity;
> >> +    struct vcpu *v_target;
> >> +    struct vgic_irq_rank *rank = vgic_rank_irq(v, irq);
> >> +
> >> +    ASSERT(spin_is_locked(&rank->lock));
> >> +
> >> +    irq_affinity = rank->v3.irouter[irq % 32];
> >> +    if ( irq_affinity & GICD_IROUTER_SPI_MODE_ANY )
> >> +    {
> >> +        /*
> >> +         * IRQ routing mode set. Route any one processor in the entire
> >> +         * system. We chose vcpu 0
> >> +         */
> >> +        return v->domain->vcpu[0];
> >> +    }
> >> +
> >> +    for ( i = 0; i < v->domain->max_vcpus; i++ )
> >> +    {
> >> +        v_target = v->domain->vcpu[i];
> >> +        cpu_affinity = (MPIDR_AFFINITY_LEVEL(v_target->arch.vmpidr, 3) << 32 |
> >> +                        MPIDR_AFFINITY_LEVEL(v_target->arch.vmpidr, 2) << 16 |
> >> +                        MPIDR_AFFINITY_LEVEL(v_target->arch.vmpidr, 1) << 8  |
> >> +                        MPIDR_AFFINITY_LEVEL(v_target->arch.vmpidr, 0));
> >> +
> >> +        if ( irq_affinity == cpu_affinity )
> >> +            return v_target;
> >> +    }
> >
> > Same comment about the loop.
> >
> >
> >> +    gdprintk(XENLOG_WARNING,
> >> +             "d%d: No vcpu match found for irq %d with irouter 0x%lx\n",
> >> +             v->domain->domain_id, irq, irq_affinity );
> >> +
> >> +    /* XXX:Fallback to vcpu0 ? */
> >> +    return v->domain->vcpu[0];
> >
> > This is wrong: only valid values should be allowed in rank->v3.irouter.
> > If the kernel writes a bad value to IROUTER we should catch it in
> > vgic_v3_distr_mmio_write and avoid saving it.
> 
>    0x0 is also valid value. I found that VMPIDR for cpu0 is 0x80000000.
> Bit[31] is RES1 bit which is set. If we ignore bit[31], 0x0 is also valid
> value.
> 
> Question: Shouldn't recover with wrong irouter value by choosing vcpu0?

It is simpler and safer to avoid having any wrong irouter values in the
first place. So for example we could write cpu0 to irouter in  
vgic_v3_distr_mmio_write if/when we catch an invalid guest write.


> Note: In next version, I will try to move common code in
> vgicv3_get_target_vcpu()
>  and vgicv3_irouter_to_vcpu() to one single function
> 
> >
> >
> >> +}
> >> +
> >>  static int vgicv3_vcpu_init(struct vcpu *v)
> >>  {
> >>      int i;
> >> @@ -968,6 +1064,7 @@ static int vgicv3_domain_init(struct domain *d)
> >>  static const struct vgic_ops v3_ops = {
> >>      .vcpu_init   = vgicv3_vcpu_init,
> >>      .domain_init = vgicv3_domain_init,
> >> +    .get_target_vcpu = vgicv3_get_target_vcpu,
> >>      .emulate_sysreg  = vgicv3_emulate_sysreg,
> >>  };
> >>
> >> --
> >> 1.7.9.5
> >>
> 

Re: [RFC PATCH] xen/arm: Deliver interrupts to vcpu specified in IROUTER

[Vijay Kilari]

Hi Stefano,

On Sat, Sep 6, 2014 at 12:14 AM, Stefano Stabellini
<stefano.stabellini@eu.citrix.com> wrote:
> On Fri, 5 Sep 2014, Vijay Kilari wrote:
>> On Fri, Sep 5, 2014 at 4:59 AM, Stefano Stabellini
>> <stefano.stabellini@eu.citrix.com> wrote:
>> > On Thu, 4 Sep 2014, vijay.kilari@gmail.com wrote:
>> >> From: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
>> >>
>> >> In GICv3 use IROUTER register contents to
>> >> deliver irq to specified vcpu's.
>> >>
>> >> In error scenario fallback to vcpu 0.
>> >>
>> >> This patch is similar to Stefano's commit
>> >> 5b3a817ea33b891caf7d7d788da9ce6deffa82a1 for GICv2
>> >>
>> >> Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
>> >> ---
>> >>  xen/arch/arm/vgic-v3.c |  115 ++++++++++++++++++++++++++++++++++++++++++++----
>> >>  1 file changed, 106 insertions(+), 9 deletions(-)
>> >>
>> >> diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
>> >> index ecda672..b01a201 100644
>> >> --- a/xen/arch/arm/vgic-v3.c
>> >> +++ b/xen/arch/arm/vgic-v3.c
>> >> @@ -45,6 +45,35 @@
>> >>  #define GICV3_GICR_PIDR2  GICV3_GICD_PIDR2
>> >>  #define GICV3_GICR_PIDR4  GICV3_GICD_PIDR4
>> >>
>> >> +static struct vcpu *vgicv3_irouter_to_vcpu(struct vcpu *v,
>> >> +                                           uint64_t irouter_aff)
>> >> +{
>> >> +    int i;
>> >> +    uint64_t cpu_affinity;
>> >> +    struct vcpu *v_target = NULL;
>> >> +
>> >> +    /* If routing mode is set. Fallback to vcpu0 */
>> >> +    if ( irouter_aff & GICD_IROUTER_SPI_MODE_ANY )
>> >> +        return v->domain->vcpu[0];
>> >> +
>> >> +    for ( i = 0; i < v->domain->max_vcpus; i++ )
>> >> +    {
>> >> +        v_target = v->domain->vcpu[i];
>> >> +        cpu_affinity = (MPIDR_AFFINITY_LEVEL(v_target->arch.vmpidr, 3) << 32 |
>> >> +                        MPIDR_AFFINITY_LEVEL(v_target->arch.vmpidr, 2) << 16 |
>> >> +                        MPIDR_AFFINITY_LEVEL(v_target->arch.vmpidr, 1) << 8  |
>> >> +                        MPIDR_AFFINITY_LEVEL(v_target->arch.vmpidr, 0));
>> >> +
>> >> +        if ( irouter_aff == cpu_affinity )
>> >> +            return v_target;
>> >> +    }
>> >
>> > Using a loop is pretty bad. Couldn't you just calculate the lowest vcpu
>> > number from irouter_aff (maybe using find_first_bit)?
>>
>> IMO,  If GICD_IROUTER_SPI_MODE_ANY bit is set, then it specifies any cpu.
>> If GICD_IROUTER_SPI_MODE_ANY is not set, it register specifies only
>> one cpu number.
>>
>> So we cannot use find_first_bit() unlike GICv2 where ITARGET specifies
>> irq affinity
>> to more than one CPU.
>>
>> If at all we want to avoid this for loop, then we have to maintain one
>> more variable for
>> every IROUTERn and corresponding vpcu number, which is updated on
>> IROUTERn update
>
> Given that at the moment AFF0 is just set to vcpu_id (see
> vcpu_initialise), I would simply do a direct calculation, adding a TODO
> comment so that we remember to fix this when we implement smarter mpidr
> schemes.

Instead of storing vcpu_id in AFF0 of vgic irouter[], we could rather
store vcpu_id in irouter[]
similar to v2. One bit per vcpu, though in v3 always only one bit is set.

The conversion function irouter_to_vcpu & vcpu_to_irouter will manage
mmio_{read,write}
of IROUTER regs.

Is this OK?

>
> Even better you could introduce two simple functions that convert vmpidr
> to vcpu_id and vice versa, so that we keep all the hacks in one place.
>
>
>> >>      switch ( gicd_reg )
>> >> @@ -804,16 +836,42 @@ static int vgic_v3_distr_mmio_write(struct vcpu *v, mmio_info_t *info)
>> >>          if ( dabt.size != DABT_DOUBLE_WORD ) goto bad_width;
>> >>          rank = vgic_rank_offset(v, 64, gicd_reg - GICD_IROUTER, DABT_DOUBLE_WORD);
>> >>          if ( rank == NULL) goto write_ignore_64;
>> >> -        if ( *r )
>> >> +        new_target = *r;
>> >> +        vgic_lock_rank(v, rank, flags);
>> >> +        old_target = rank->v3.irouter[REG_RANK_INDEX(64,
>> >> +                              (gicd_reg - GICD_IROUTER), DABT_DOUBLE_WORD)];
>> >> +        if ( *r & GICD_IROUTER_SPI_MODE_ANY )
>> >>          {
>> >> -            /* TODO: Ignored. We don't support irq delivery for vcpu != 0 */
>> >> -            gdprintk(XENLOG_DEBUG,
>> >> -                     "SPI delivery to secondary cpus not supported\n");
>> >> -            goto write_ignore_64;
>> >> +            /*
>> >> +             * IRQ routing mode set. Route any one processor in the entire
>> >> +             * system. We chose vcpu 0
>> >> +             */
>> >> +            rank->v3.irouter[REG_RANK_INDEX(64,
>> >> +                     (gicd_reg - GICD_IROUTER), DABT_DOUBLE_WORD)] = *r;
>> >> +            vgic_unlock_rank(v, rank, flags);
>> >> +            return 1;
>> >>          }
>> >> -        vgic_lock_rank(v, rank, flags);
>> >> +        irq = (gicd_reg - GICD_IROUTER)/8;
>> >> +        /* IROUTER now specifies only one cpu affinity for this irq */
>> >> +        /* Migrate from any core(vcpu0) to new_target */
>> >> +        if ( (old_target & GICD_IROUTER_SPI_MODE_ANY) &&
>> >> +             !(new_target & GICD_IROUTER_SPI_MODE_ANY) )
>> >> +        {
>> >> +            new_vcpu = vgicv3_irouter_to_vcpu(v, new_target);
>> >> +            vgic_migrate_irq(v->domain->vcpu[0], new_vcpu, irq);
>> >> +        }
>> >> +        else
>> >> +        {
>> >> +            if ( old_target != new_target )
>> >> +            {
>> >> +                old_vcpu = vgicv3_irouter_to_vcpu(v, old_target);
>> >> +                new_vcpu = vgicv3_irouter_to_vcpu(v, new_target);
>> >> +                vgic_migrate_irq(old_vcpu, new_vcpu, irq);
>> >> +            }
>> >> +        }
>> >
>> > The issue is that if irouter is GICD_IROUTER_SPI_MODE_ANY, you always
>> > return vcpu0 below.
>> > So if the kernel:
>> >
>> > 1) move the irq to vcpu1
>> > 2) set the irq as GICD_IROUTER_SPI_MODE_ANY
>> >
>> > you now have migrated the irq to vcpu1 but you are returning vcpu0 from
>> > vgicv3_get_target_vcpu.
>> >
>> > You can either:
>> > - vgic_migrate_irq the irq back to vcpu0 when the kernel sets the irq as
>> >   GICD_IROUTER_SPI_MODE_ANY
>>
>>    When irq is set to vcpu1 and later when GICD_IROUTER_SPI_MODE_ANY
>> then I assume that vcpu1 is also valid for the moment if irq is pending.
>> So no need to do any migration.
>
> That would be OK, except that vgicv3_get_target_vcpu is actually
> returning vcpu0. We should try to match physical and virtual irq
> delivery as much as possible. So if the physical irq is delivered to
> pcpu1 (or whatever is the pcpu currently running vcpu1), then we should
> try hard to deliver the corresponding virq to vcpu1.
>
> Calling vgic_migrate_irq changes the physical irq affinity to the pcpu
> running the destination vcpu. So step 1 above moves physical irq
> delivery to pcpu1 (assuming that pcpu1 is running vcpu1 for simplicity).
> After the guest kernel sets the irq as GICD_IROUTER_SPI_MODE_ANY, even
> though we could deliver the irq to any vcpu, we should actually delivery
> the irq to the vcpu running on the pcpu that received the interrupt. In
> my example it would still be vcpu1. Therefore we need to return vcpu1
> from vgicv3_get_target_vcpu.
>
> Instead as it is now, we would cause an SGI to be sent from pcpu1 to
> pcpu0 in order to inject the irq to vcpu0 (assuming that vcpu0 is
> running on pcpu0 for simplicity).
>
>
>> Also, as per GICv3 spec, when GICD_IROUTER_SPI_MODE_ANY is set which is
>> previous not set ( 0 -> 1 ), the affinity value in IROUTER is invalid/unknown.
>> So later when vgicv3_get_target_vcpu() is called, it always returns vcpu0.
>
> Could you please tell me which one is the relevant chapter of the GICv3
> spec? I couldn't find the statement you mentioned. Maybe my version of
> the spec is outdated.

See 20.0 version section 5.3.4

"When this bit is written from zero to one, the Affinity fields become UNKNOWN."

>
>
>> > - return the current vcpu for the irq rather than vcpu0 if
>> >   GICD_IROUTER_SPI_MODE_ANY
>> >
>> > Either way it would work. Maybe the second option might be better.
>> >
>> >
>> >>          rank->v3.irouter[REG_RANK_INDEX(64,
>> >> -                      (gicd_reg - GICD_IROUTER), DABT_DOUBLE_WORD)] = *r;
>> >> +                      (gicd_reg - GICD_IROUTER), DABT_DOUBLE_WORD)] = new_target;
>> >>          vgic_unlock_rank(v, rank, flags);
>> >>          return 1;
>> >>      case GICD_NSACR ... GICD_NSACRN:
>> >> @@ -928,6 +986,44 @@ static const struct mmio_handler_ops vgic_distr_mmio_handler = {
>> >>      .write_handler = vgic_v3_distr_mmio_write,
>> >>  };
>> >>
>> >> +static struct vcpu *vgicv3_get_target_vcpu(struct vcpu *v, unsigned int irq)
>> >
>> > for consistency I would call it vgic_v3_get_target_vcpu
>> >
>> >
>> >> +{
>> >> +    int i;
>> >> +    uint64_t irq_affinity, cpu_affinity;
>> >> +    struct vcpu *v_target;
>> >> +    struct vgic_irq_rank *rank = vgic_rank_irq(v, irq);
>> >> +
>> >> +    ASSERT(spin_is_locked(&rank->lock));
>> >> +
>> >> +    irq_affinity = rank->v3.irouter[irq % 32];
>> >> +    if ( irq_affinity & GICD_IROUTER_SPI_MODE_ANY )
>> >> +    {
>> >> +        /*
>> >> +         * IRQ routing mode set. Route any one processor in the entire
>> >> +         * system. We chose vcpu 0
>> >> +         */
>> >> +        return v->domain->vcpu[0];
>> >> +    }
>> >> +
>> >> +    for ( i = 0; i < v->domain->max_vcpus; i++ )
>> >> +    {
>> >> +        v_target = v->domain->vcpu[i];
>> >> +        cpu_affinity = (MPIDR_AFFINITY_LEVEL(v_target->arch.vmpidr, 3) << 32 |
>> >> +                        MPIDR_AFFINITY_LEVEL(v_target->arch.vmpidr, 2) << 16 |
>> >> +                        MPIDR_AFFINITY_LEVEL(v_target->arch.vmpidr, 1) << 8  |
>> >> +                        MPIDR_AFFINITY_LEVEL(v_target->arch.vmpidr, 0));
>> >> +
>> >> +        if ( irq_affinity == cpu_affinity )
>> >> +            return v_target;
>> >> +    }
>> >
>> > Same comment about the loop.
>> >
>> >
>> >> +    gdprintk(XENLOG_WARNING,
>> >> +             "d%d: No vcpu match found for irq %d with irouter 0x%lx\n",
>> >> +             v->domain->domain_id, irq, irq_affinity );
>> >> +
>> >> +    /* XXX:Fallback to vcpu0 ? */
>> >> +    return v->domain->vcpu[0];
>> >
>> > This is wrong: only valid values should be allowed in rank->v3.irouter.
>> > If the kernel writes a bad value to IROUTER we should catch it in
>> > vgic_v3_distr_mmio_write and avoid saving it.
>>
>>    0x0 is also valid value. I found that VMPIDR for cpu0 is 0x80000000.
>> Bit[31] is RES1 bit which is set. If we ignore bit[31], 0x0 is also valid
>> value.
>>
>> Question: Shouldn't recover with wrong irouter value by choosing vcpu0?
>
> It is simpler and safer to avoid having any wrong irouter values in the
> first place. So for example we could write cpu0 to irouter in
> vgic_v3_distr_mmio_write if/when we catch an invalid guest write.
>
>

Re: [RFC PATCH] xen/arm: Deliver interrupts to vcpu specified in IROUTER

[Vijay Kilari]

Hi Stefano,

 Apart from your comments below, I encounter below scenario.

The below function vgic_irq_rank() in vgic.c is not generic. The rank is always
depends on register type it is going to access. So it cannot be
just hardcoded.

struct vgic_irq_rank *(struct vcpu *v, unsigned int irq)
{
    return vgic_rank_offset(v, 8, irq, DABT_WORD);
}

This function works ok for GIC v2. But this cannot be used for
GICv3 to access registers like IROUTER which are u64. The rank
calcuation goes wrong and there by takes wrong rank lock.

This function is used in vgic_get_target_vcpu()
and vgic_vcpu_inject_irq() generic code to access
ipriority and itarget.

One option is use to move functions that use this rank locking mechanism
to v2 or v3 driver as a callback.
But if we move this to vgic-v2/v3 driver, then there are some
scenario's where these callbacks are  called with already rank
locks held might fail.
Ex: vgic_v2_get_target_vcpu get called from vgic_enable_irqs()

May be we can create two version of callbacks locked & unlocked
and use appropriately?


On Mon, Sep 8, 2014 at 6:37 PM, Vijay Kilari <vijay.kilari@gmail.com> wrote:
> Hi Stefano,
>
> On Sat, Sep 6, 2014 at 12:14 AM, Stefano Stabellini
> <stefano.stabellini@eu.citrix.com> wrote:
>> On Fri, 5 Sep 2014, Vijay Kilari wrote:
>>> On Fri, Sep 5, 2014 at 4:59 AM, Stefano Stabellini
>>> <stefano.stabellini@eu.citrix.com> wrote:
>>> > On Thu, 4 Sep 2014, vijay.kilari@gmail.com wrote:
>>> >> From: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
>>> >>
>>> >> In GICv3 use IROUTER register contents to
>>> >> deliver irq to specified vcpu's.
>>> >>
>>> >> In error scenario fallback to vcpu 0.
>>> >>
>>> >> This patch is similar to Stefano's commit
>>> >> 5b3a817ea33b891caf7d7d788da9ce6deffa82a1 for GICv2
>>> >>
>>> >> Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
>>> >> ---
>>> >>  xen/arch/arm/vgic-v3.c |  115 ++++++++++++++++++++++++++++++++++++++++++++----
>>> >>  1 file changed, 106 insertions(+), 9 deletions(-)
>>> >>
>>> >> diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
>>> >> index ecda672..b01a201 100644
>>> >> --- a/xen/arch/arm/vgic-v3.c
>>> >> +++ b/xen/arch/arm/vgic-v3.c
>>> >> @@ -45,6 +45,35 @@
>>> >>  #define GICV3_GICR_PIDR2  GICV3_GICD_PIDR2
>>> >>  #define GICV3_GICR_PIDR4  GICV3_GICD_PIDR4
>>> >>
>>> >> +static struct vcpu *vgicv3_irouter_to_vcpu(struct vcpu *v,
>>> >> +                                           uint64_t irouter_aff)
>>> >> +{
>>> >> +    int i;
>>> >> +    uint64_t cpu_affinity;
>>> >> +    struct vcpu *v_target = NULL;
>>> >> +
>>> >> +    /* If routing mode is set. Fallback to vcpu0 */
>>> >> +    if ( irouter_aff & GICD_IROUTER_SPI_MODE_ANY )
>>> >> +        return v->domain->vcpu[0];
>>> >> +
>>> >> +    for ( i = 0; i < v->domain->max_vcpus; i++ )
>>> >> +    {
>>> >> +        v_target = v->domain->vcpu[i];
>>> >> +        cpu_affinity = (MPIDR_AFFINITY_LEVEL(v_target->arch.vmpidr, 3) << 32 |
>>> >> +                        MPIDR_AFFINITY_LEVEL(v_target->arch.vmpidr, 2) << 16 |
>>> >> +                        MPIDR_AFFINITY_LEVEL(v_target->arch.vmpidr, 1) << 8  |
>>> >> +                        MPIDR_AFFINITY_LEVEL(v_target->arch.vmpidr, 0));
>>> >> +
>>> >> +        if ( irouter_aff == cpu_affinity )
>>> >> +            return v_target;
>>> >> +    }
>>> >
>>> > Using a loop is pretty bad. Couldn't you just calculate the lowest vcpu
>>> > number from irouter_aff (maybe using find_first_bit)?
>>>
>>> IMO,  If GICD_IROUTER_SPI_MODE_ANY bit is set, then it specifies any cpu.
>>> If GICD_IROUTER_SPI_MODE_ANY is not set, it register specifies only
>>> one cpu number.
>>>
>>> So we cannot use find_first_bit() unlike GICv2 where ITARGET specifies
>>> irq affinity
>>> to more than one CPU.
>>>
>>> If at all we want to avoid this for loop, then we have to maintain one
>>> more variable for
>>> every IROUTERn and corresponding vpcu number, which is updated on
>>> IROUTERn update
>>
>> Given that at the moment AFF0 is just set to vcpu_id (see
>> vcpu_initialise), I would simply do a direct calculation, adding a TODO
>> comment so that we remember to fix this when we implement smarter mpidr
>> schemes.
>
> Instead of storing vcpu_id in AFF0 of vgic irouter[], we could rather
> store vcpu_id in irouter[]
> similar to v2. One bit per vcpu, though in v3 always only one bit is set.
>
> The conversion function irouter_to_vcpu & vcpu_to_irouter will manage
> mmio_{read,write}
> of IROUTER regs.
>
> Is this OK?
>
>>
>> Even better you could introduce two simple functions that convert vmpidr
>> to vcpu_id and vice versa, so that we keep all the hacks in one place.
>>
>>
>>> >>      switch ( gicd_reg )
>>> >> @@ -804,16 +836,42 @@ static int vgic_v3_distr_mmio_write(struct vcpu *v, mmio_info_t *info)
>>> >>          if ( dabt.size != DABT_DOUBLE_WORD ) goto bad_width;
>>> >>          rank = vgic_rank_offset(v, 64, gicd_reg - GICD_IROUTER, DABT_DOUBLE_WORD);
>>> >>          if ( rank == NULL) goto write_ignore_64;
>>> >> -        if ( *r )
>>> >> +        new_target = *r;
>>> >> +        vgic_lock_rank(v, rank, flags);
>>> >> +        old_target = rank->v3.irouter[REG_RANK_INDEX(64,
>>> >> +                              (gicd_reg - GICD_IROUTER), DABT_DOUBLE_WORD)];
>>> >> +        if ( *r & GICD_IROUTER_SPI_MODE_ANY )
>>> >>          {
>>> >> -            /* TODO: Ignored. We don't support irq delivery for vcpu != 0 */
>>> >> -            gdprintk(XENLOG_DEBUG,
>>> >> -                     "SPI delivery to secondary cpus not supported\n");
>>> >> -            goto write_ignore_64;
>>> >> +            /*
>>> >> +             * IRQ routing mode set. Route any one processor in the entire
>>> >> +             * system. We chose vcpu 0
>>> >> +             */
>>> >> +            rank->v3.irouter[REG_RANK_INDEX(64,
>>> >> +                     (gicd_reg - GICD_IROUTER), DABT_DOUBLE_WORD)] = *r;
>>> >> +            vgic_unlock_rank(v, rank, flags);
>>> >> +            return 1;
>>> >>          }
>>> >> -        vgic_lock_rank(v, rank, flags);
>>> >> +        irq = (gicd_reg - GICD_IROUTER)/8;
>>> >> +        /* IROUTER now specifies only one cpu affinity for this irq */
>>> >> +        /* Migrate from any core(vcpu0) to new_target */
>>> >> +        if ( (old_target & GICD_IROUTER_SPI_MODE_ANY) &&
>>> >> +             !(new_target & GICD_IROUTER_SPI_MODE_ANY) )
>>> >> +        {
>>> >> +            new_vcpu = vgicv3_irouter_to_vcpu(v, new_target);
>>> >> +            vgic_migrate_irq(v->domain->vcpu[0], new_vcpu, irq);
>>> >> +        }
>>> >> +        else
>>> >> +        {
>>> >> +            if ( old_target != new_target )
>>> >> +            {
>>> >> +                old_vcpu = vgicv3_irouter_to_vcpu(v, old_target);
>>> >> +                new_vcpu = vgicv3_irouter_to_vcpu(v, new_target);
>>> >> +                vgic_migrate_irq(old_vcpu, new_vcpu, irq);
>>> >> +            }
>>> >> +        }
>>> >
>>> > The issue is that if irouter is GICD_IROUTER_SPI_MODE_ANY, you always
>>> > return vcpu0 below.
>>> > So if the kernel:
>>> >
>>> > 1) move the irq to vcpu1
>>> > 2) set the irq as GICD_IROUTER_SPI_MODE_ANY
>>> >
>>> > you now have migrated the irq to vcpu1 but you are returning vcpu0 from
>>> > vgicv3_get_target_vcpu.
>>> >
>>> > You can either:
>>> > - vgic_migrate_irq the irq back to vcpu0 when the kernel sets the irq as
>>> >   GICD_IROUTER_SPI_MODE_ANY
>>>
>>>    When irq is set to vcpu1 and later when GICD_IROUTER_SPI_MODE_ANY
>>> then I assume that vcpu1 is also valid for the moment if irq is pending.
>>> So no need to do any migration.
>>
>> That would be OK, except that vgicv3_get_target_vcpu is actually
>> returning vcpu0. We should try to match physical and virtual irq
>> delivery as much as possible. So if the physical irq is delivered to
>> pcpu1 (or whatever is the pcpu currently running vcpu1), then we should
>> try hard to deliver the corresponding virq to vcpu1.
>>
>> Calling vgic_migrate_irq changes the physical irq affinity to the pcpu
>> running the destination vcpu. So step 1 above moves physical irq
>> delivery to pcpu1 (assuming that pcpu1 is running vcpu1 for simplicity).
>> After the guest kernel sets the irq as GICD_IROUTER_SPI_MODE_ANY, even
>> though we could deliver the irq to any vcpu, we should actually delivery
>> the irq to the vcpu running on the pcpu that received the interrupt. In
>> my example it would still be vcpu1. Therefore we need to return vcpu1
>> from vgicv3_get_target_vcpu.
>>
>> Instead as it is now, we would cause an SGI to be sent from pcpu1 to
>> pcpu0 in order to inject the irq to vcpu0 (assuming that vcpu0 is
>> running on pcpu0 for simplicity).
>>
>>
>>> Also, as per GICv3 spec, when GICD_IROUTER_SPI_MODE_ANY is set which is
>>> previous not set ( 0 -> 1 ), the affinity value in IROUTER is invalid/unknown.
>>> So later when vgicv3_get_target_vcpu() is called, it always returns vcpu0.
>>
>> Could you please tell me which one is the relevant chapter of the GICv3
>> spec? I couldn't find the statement you mentioned. Maybe my version of
>> the spec is outdated.
>
> See 20.0 version section 5.3.4
>
> "When this bit is written from zero to one, the Affinity fields become UNKNOWN."
>
>>
>>
>>> > - return the current vcpu for the irq rather than vcpu0 if
>>> >   GICD_IROUTER_SPI_MODE_ANY
>>> >
>>> > Either way it would work. Maybe the second option might be better.
>>> >
>>> >
>>> >>          rank->v3.irouter[REG_RANK_INDEX(64,
>>> >> -                      (gicd_reg - GICD_IROUTER), DABT_DOUBLE_WORD)] = *r;
>>> >> +                      (gicd_reg - GICD_IROUTER), DABT_DOUBLE_WORD)] = new_target;
>>> >>          vgic_unlock_rank(v, rank, flags);
>>> >>          return 1;
>>> >>      case GICD_NSACR ... GICD_NSACRN:
>>> >> @@ -928,6 +986,44 @@ static const struct mmio_handler_ops vgic_distr_mmio_handler = {
>>> >>      .write_handler = vgic_v3_distr_mmio_write,
>>> >>  };
>>> >>
>>> >> +static struct vcpu *vgicv3_get_target_vcpu(struct vcpu *v, unsigned int irq)
>>> >
>>> > for consistency I would call it vgic_v3_get_target_vcpu
>>> >
>>> >
>>> >> +{
>>> >> +    int i;
>>> >> +    uint64_t irq_affinity, cpu_affinity;
>>> >> +    struct vcpu *v_target;
>>> >> +    struct vgic_irq_rank *rank = vgic_rank_irq(v, irq);
>>> >> +
>>> >> +    ASSERT(spin_is_locked(&rank->lock));
>>> >> +
>>> >> +    irq_affinity = rank->v3.irouter[irq % 32];
>>> >> +    if ( irq_affinity & GICD_IROUTER_SPI_MODE_ANY )
>>> >> +    {
>>> >> +        /*
>>> >> +         * IRQ routing mode set. Route any one processor in the entire
>>> >> +         * system. We chose vcpu 0
>>> >> +         */
>>> >> +        return v->domain->vcpu[0];
>>> >> +    }
>>> >> +
>>> >> +    for ( i = 0; i < v->domain->max_vcpus; i++ )
>>> >> +    {
>>> >> +        v_target = v->domain->vcpu[i];
>>> >> +        cpu_affinity = (MPIDR_AFFINITY_LEVEL(v_target->arch.vmpidr, 3) << 32 |
>>> >> +                        MPIDR_AFFINITY_LEVEL(v_target->arch.vmpidr, 2) << 16 |
>>> >> +                        MPIDR_AFFINITY_LEVEL(v_target->arch.vmpidr, 1) << 8  |
>>> >> +                        MPIDR_AFFINITY_LEVEL(v_target->arch.vmpidr, 0));
>>> >> +
>>> >> +        if ( irq_affinity == cpu_affinity )
>>> >> +            return v_target;
>>> >> +    }
>>> >
>>> > Same comment about the loop.
>>> >
>>> >
>>> >> +    gdprintk(XENLOG_WARNING,
>>> >> +             "d%d: No vcpu match found for irq %d with irouter 0x%lx\n",
>>> >> +             v->domain->domain_id, irq, irq_affinity );
>>> >> +
>>> >> +    /* XXX:Fallback to vcpu0 ? */
>>> >> +    return v->domain->vcpu[0];
>>> >
>>> > This is wrong: only valid values should be allowed in rank->v3.irouter.
>>> > If the kernel writes a bad value to IROUTER we should catch it in
>>> > vgic_v3_distr_mmio_write and avoid saving it.
>>>
>>>    0x0 is also valid value. I found that VMPIDR for cpu0 is 0x80000000.
>>> Bit[31] is RES1 bit which is set. If we ignore bit[31], 0x0 is also valid
>>> value.
>>>
>>> Question: Shouldn't recover with wrong irouter value by choosing vcpu0?
>>
>> It is simpler and safer to avoid having any wrong irouter values in the
>> first place. So for example we could write cpu0 to irouter in
>> vgic_v3_distr_mmio_write if/when we catch an invalid guest write.
>>
>>

Re: [RFC PATCH] xen/arm: Deliver interrupts to vcpu specified in IROUTER

[Vijay Kilari]

On Mon, Sep 8, 2014 at 7:17 PM, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> On Mon, 2014-09-08 at 19:11 +0530, Vijay Kilari wrote:
>
> Why have we dropped the list?

I have not dropped the list. I still see list in my email.

>
>> Hi Stefano,
>>
>>  Apart from your comments below, I encounter below scenario.
>>
>> The below function vgic_irq_rank() in vgic.c is not generic. The rank is always
>> depends on register type it is going to access. So it cannot be
>> just hardcoded.
>>
>> struct vgic_irq_rank *(struct vcpu *v, unsigned int irq)
>> {
>>     return vgic_rank_offset(v, 8, irq, DABT_WORD);
>> }
>>
>> This function works ok for GIC v2. But this cannot be used for
>> GICv3 to access registers like IROUTER which are u64. The rank
>> calcuation goes wrong and there by takes wrong rank lock.
>
> This sounds to me like a bug which should be fixed. A rank is just a
> group of 32 consecutive IRQs and their associated register values, it
> shouldn't matter whether those registers are 8-, 16-, 32- or 64-bits.

 Now I think of creating a callback to return the right rank for given irq
based on v2 or v3. This will fix and keep the old code intact.

>
> Now, if we have registers which aren't associated with a bank of
> registers, or which cross between banks then we have a problem, but I
> don't think that is the case with IROUTER, is it?
>
> (I'll let Stefano comment on the rest, since he understands the
> target/inject stuff better than I)
>
> Ian.
>

Re: [RFC PATCH] xen/arm: Deliver interrupts to vcpu specified in IROUTER

[Vijay Kilari]

On Mon, Sep 8, 2014 at 7:33 PM, Vijay Kilari <vijay.kilari@gmail.com> wrote:
> On Mon, Sep 8, 2014 at 7:17 PM, Ian Campbell <Ian.Campbell@citrix.com> wrote:
>> On Mon, 2014-09-08 at 19:11 +0530, Vijay Kilari wrote:
>>
>> Why have we dropped the list?
>
> I have not dropped the list. I still see list in my email.
>
>>
>>> Hi Stefano,
>>>
>>>  Apart from your comments below, I encounter below scenario.
>>>
>>> The below function vgic_irq_rank() in vgic.c is not generic. The rank is always
>>> depends on register type it is going to access. So it cannot be
>>> just hardcoded.
>>>
>>> struct vgic_irq_rank *(struct vcpu *v, unsigned int irq)
>>> {
>>>     return vgic_rank_offset(v, 8, irq, DABT_WORD);
>>> }
>>>
>>> This function works ok for GIC v2. But this cannot be used for
>>> GICv3 to access registers like IROUTER which are u64. The rank
>>> calcuation goes wrong and there by takes wrong rank lock.
>>
>> This sounds to me like a bug which should be fixed. A rank is just a
>> group of 32 consecutive IRQs and their associated register values, it
>> shouldn't matter whether those registers are 8-, 16-, 32- or 64-bits.
>
>  Now I think of creating a callback to return the right rank for given irq
> based on v2 or v3. This will fix and keep the old code intact.

This should fix the issue.

 struct vgic_irq_rank *vgic_rank_irq(struct vcpu *v, unsigned int irq)
 {
-    return vgic_rank_offset(v, 8, irq, DABT_WORD);
+    int rank = irq/32;

+    if ( rank == 0 )
+        return v->arch.vgic.private_irqs;
+    else if ( rank <= DOMAIN_NR_RANKS(v->domain) )
+        return &v->domain->arch.vgic.shared_irqs[rank - 1];
+    else
+        return NULL;
}

Re: [RFC PATCH] xen/arm: Deliver interrupts to vcpu specified in IROUTER

[Ian Campbell]

On Mon, 2014-09-08 at 19:33 +0530, Vijay Kilari wrote:
> On Mon, Sep 8, 2014 at 7:17 PM, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> > On Mon, 2014-09-08 at 19:11 +0530, Vijay Kilari wrote:
> >
> > Why have we dropped the list?
> 
> I have not dropped the list. I still see list in my email.

How strange, my copy of it has:
        From: Vijay Kilari <vijay.kilari@gmail.com>
        To: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
        CC: Ian Campbell <Ian.Campbell@citrix.com>, Julien Grall
(i.e. CC is truncated even before Julien's email address)

I suspect my MUA is playing up in some unfathomable way.

Ian.

Re: [RFC PATCH] xen/arm: Deliver interrupts to vcpu specified in IROUTER

[Ian Campbell]

On Mon, 2014-09-08 at 19:55 +0530, Vijay Kilari wrote:
> On Mon, Sep 8, 2014 at 7:33 PM, Vijay Kilari <vijay.kilari@gmail.com> wrote:
> > On Mon, Sep 8, 2014 at 7:17 PM, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> >> On Mon, 2014-09-08 at 19:11 +0530, Vijay Kilari wrote:
> >>
> >> Why have we dropped the list?
> >
> > I have not dropped the list. I still see list in my email.
> >
> >>
> >>> Hi Stefano,
> >>>
> >>>  Apart from your comments below, I encounter below scenario.
> >>>
> >>> The below function vgic_irq_rank() in vgic.c is not generic. The rank is always
> >>> depends on register type it is going to access. So it cannot be
> >>> just hardcoded.
> >>>
> >>> struct vgic_irq_rank *(struct vcpu *v, unsigned int irq)
> >>> {
> >>>     return vgic_rank_offset(v, 8, irq, DABT_WORD);
> >>> }
> >>>
> >>> This function works ok for GIC v2. But this cannot be used for
> >>> GICv3 to access registers like IROUTER which are u64. The rank
> >>> calcuation goes wrong and there by takes wrong rank lock.
> >>
> >> This sounds to me like a bug which should be fixed. A rank is just a
> >> group of 32 consecutive IRQs and their associated register values, it
> >> shouldn't matter whether those registers are 8-, 16-, 32- or 64-bits.
> >
> >  Now I think of creating a callback to return the right rank for given irq
> > based on v2 or v3. This will fix and keep the old code intact.
> 
> This should fix the issue.

Looks plausible. Will you fold it into your series?

> 
>  struct vgic_irq_rank *vgic_rank_irq(struct vcpu *v, unsigned int irq)
>  {
> -    return vgic_rank_offset(v, 8, irq, DABT_WORD);
> +    int rank = irq/32;
> 
> +    if ( rank == 0 )
> +        return v->arch.vgic.private_irqs;
> +    else if ( rank <= DOMAIN_NR_RANKS(v->domain) )
> +        return &v->domain->arch.vgic.shared_irqs[rank - 1];
> +    else
> +        return NULL;
> }

Re: [RFC PATCH] xen/arm: Deliver interrupts to vcpu specified in IROUTER

[Stefano Stabellini]

On Mon, 8 Sep 2014, Vijay Kilari wrote:
> Hi Stefano,
> 
> On Sat, Sep 6, 2014 at 12:14 AM, Stefano Stabellini
> <stefano.stabellini@eu.citrix.com> wrote:
> > On Fri, 5 Sep 2014, Vijay Kilari wrote:
> >> On Fri, Sep 5, 2014 at 4:59 AM, Stefano Stabellini
> >> <stefano.stabellini@eu.citrix.com> wrote:
> >> > On Thu, 4 Sep 2014, vijay.kilari@gmail.com wrote:
> >> >> From: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
> >> >>
> >> >> In GICv3 use IROUTER register contents to
> >> >> deliver irq to specified vcpu's.
> >> >>
> >> >> In error scenario fallback to vcpu 0.
> >> >>
> >> >> This patch is similar to Stefano's commit
> >> >> 5b3a817ea33b891caf7d7d788da9ce6deffa82a1 for GICv2
> >> >>
> >> >> Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
> >> >> ---
> >> >>  xen/arch/arm/vgic-v3.c |  115 ++++++++++++++++++++++++++++++++++++++++++++----
> >> >>  1 file changed, 106 insertions(+), 9 deletions(-)
> >> >>
> >> >> diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
> >> >> index ecda672..b01a201 100644
> >> >> --- a/xen/arch/arm/vgic-v3.c
> >> >> +++ b/xen/arch/arm/vgic-v3.c
> >> >> @@ -45,6 +45,35 @@
> >> >>  #define GICV3_GICR_PIDR2  GICV3_GICD_PIDR2
> >> >>  #define GICV3_GICR_PIDR4  GICV3_GICD_PIDR4
> >> >>
> >> >> +static struct vcpu *vgicv3_irouter_to_vcpu(struct vcpu *v,
> >> >> +                                           uint64_t irouter_aff)
> >> >> +{
> >> >> +    int i;
> >> >> +    uint64_t cpu_affinity;
> >> >> +    struct vcpu *v_target = NULL;
> >> >> +
> >> >> +    /* If routing mode is set. Fallback to vcpu0 */
> >> >> +    if ( irouter_aff & GICD_IROUTER_SPI_MODE_ANY )
> >> >> +        return v->domain->vcpu[0];
> >> >> +
> >> >> +    for ( i = 0; i < v->domain->max_vcpus; i++ )
> >> >> +    {
> >> >> +        v_target = v->domain->vcpu[i];
> >> >> +        cpu_affinity = (MPIDR_AFFINITY_LEVEL(v_target->arch.vmpidr, 3) << 32 |
> >> >> +                        MPIDR_AFFINITY_LEVEL(v_target->arch.vmpidr, 2) << 16 |
> >> >> +                        MPIDR_AFFINITY_LEVEL(v_target->arch.vmpidr, 1) << 8  |
> >> >> +                        MPIDR_AFFINITY_LEVEL(v_target->arch.vmpidr, 0));
> >> >> +
> >> >> +        if ( irouter_aff == cpu_affinity )
> >> >> +            return v_target;
> >> >> +    }
> >> >
> >> > Using a loop is pretty bad. Couldn't you just calculate the lowest vcpu
> >> > number from irouter_aff (maybe using find_first_bit)?
> >>
> >> IMO,  If GICD_IROUTER_SPI_MODE_ANY bit is set, then it specifies any cpu.
> >> If GICD_IROUTER_SPI_MODE_ANY is not set, it register specifies only
> >> one cpu number.
> >>
> >> So we cannot use find_first_bit() unlike GICv2 where ITARGET specifies
> >> irq affinity
> >> to more than one CPU.
> >>
> >> If at all we want to avoid this for loop, then we have to maintain one
> >> more variable for
> >> every IROUTERn and corresponding vpcu number, which is updated on
> >> IROUTERn update
> >
> > Given that at the moment AFF0 is just set to vcpu_id (see
> > vcpu_initialise), I would simply do a direct calculation, adding a TODO
> > comment so that we remember to fix this when we implement smarter mpidr
> > schemes.
> 
> Instead of storing vcpu_id in AFF0 of vgic irouter[], we could rather
> store vcpu_id in irouter[]
> similar to v2. One bit per vcpu, though in v3 always only one bit is set.
> 
> The conversion function irouter_to_vcpu & vcpu_to_irouter will manage
> mmio_{read,write}
> of IROUTER regs.
> 
> Is this OK?

Sounds good in principle, but of course I would need to see the code to
get a better idea.


> >
> > Even better you could introduce two simple functions that convert vmpidr
> > to vcpu_id and vice versa, so that we keep all the hacks in one place.
> >
> >
> >> >>      switch ( gicd_reg )
> >> >> @@ -804,16 +836,42 @@ static int vgic_v3_distr_mmio_write(struct vcpu *v, mmio_info_t *info)
> >> >>          if ( dabt.size != DABT_DOUBLE_WORD ) goto bad_width;
> >> >>          rank = vgic_rank_offset(v, 64, gicd_reg - GICD_IROUTER, DABT_DOUBLE_WORD);
> >> >>          if ( rank == NULL) goto write_ignore_64;
> >> >> -        if ( *r )
> >> >> +        new_target = *r;
> >> >> +        vgic_lock_rank(v, rank, flags);
> >> >> +        old_target = rank->v3.irouter[REG_RANK_INDEX(64,
> >> >> +                              (gicd_reg - GICD_IROUTER), DABT_DOUBLE_WORD)];
> >> >> +        if ( *r & GICD_IROUTER_SPI_MODE_ANY )
> >> >>          {
> >> >> -            /* TODO: Ignored. We don't support irq delivery for vcpu != 0 */
> >> >> -            gdprintk(XENLOG_DEBUG,
> >> >> -                     "SPI delivery to secondary cpus not supported\n");
> >> >> -            goto write_ignore_64;
> >> >> +            /*
> >> >> +             * IRQ routing mode set. Route any one processor in the entire
> >> >> +             * system. We chose vcpu 0
> >> >> +             */
> >> >> +            rank->v3.irouter[REG_RANK_INDEX(64,
> >> >> +                     (gicd_reg - GICD_IROUTER), DABT_DOUBLE_WORD)] = *r;
> >> >> +            vgic_unlock_rank(v, rank, flags);
> >> >> +            return 1;
> >> >>          }
> >> >> -        vgic_lock_rank(v, rank, flags);
> >> >> +        irq = (gicd_reg - GICD_IROUTER)/8;
> >> >> +        /* IROUTER now specifies only one cpu affinity for this irq */
> >> >> +        /* Migrate from any core(vcpu0) to new_target */
> >> >> +        if ( (old_target & GICD_IROUTER_SPI_MODE_ANY) &&
> >> >> +             !(new_target & GICD_IROUTER_SPI_MODE_ANY) )
> >> >> +        {
> >> >> +            new_vcpu = vgicv3_irouter_to_vcpu(v, new_target);
> >> >> +            vgic_migrate_irq(v->domain->vcpu[0], new_vcpu, irq);
> >> >> +        }
> >> >> +        else
> >> >> +        {
> >> >> +            if ( old_target != new_target )
> >> >> +            {
> >> >> +                old_vcpu = vgicv3_irouter_to_vcpu(v, old_target);
> >> >> +                new_vcpu = vgicv3_irouter_to_vcpu(v, new_target);
> >> >> +                vgic_migrate_irq(old_vcpu, new_vcpu, irq);
> >> >> +            }
> >> >> +        }
> >> >
> >> > The issue is that if irouter is GICD_IROUTER_SPI_MODE_ANY, you always
> >> > return vcpu0 below.
> >> > So if the kernel:
> >> >
> >> > 1) move the irq to vcpu1
> >> > 2) set the irq as GICD_IROUTER_SPI_MODE_ANY
> >> >
> >> > you now have migrated the irq to vcpu1 but you are returning vcpu0 from
> >> > vgicv3_get_target_vcpu.
> >> >
> >> > You can either:
> >> > - vgic_migrate_irq the irq back to vcpu0 when the kernel sets the irq as
> >> >   GICD_IROUTER_SPI_MODE_ANY
> >>
> >>    When irq is set to vcpu1 and later when GICD_IROUTER_SPI_MODE_ANY
> >> then I assume that vcpu1 is also valid for the moment if irq is pending.
> >> So no need to do any migration.
> >
> > That would be OK, except that vgicv3_get_target_vcpu is actually
> > returning vcpu0. We should try to match physical and virtual irq
> > delivery as much as possible. So if the physical irq is delivered to
> > pcpu1 (or whatever is the pcpu currently running vcpu1), then we should
> > try hard to deliver the corresponding virq to vcpu1.
> >
> > Calling vgic_migrate_irq changes the physical irq affinity to the pcpu
> > running the destination vcpu. So step 1 above moves physical irq
> > delivery to pcpu1 (assuming that pcpu1 is running vcpu1 for simplicity).
> > After the guest kernel sets the irq as GICD_IROUTER_SPI_MODE_ANY, even
> > though we could deliver the irq to any vcpu, we should actually delivery
> > the irq to the vcpu running on the pcpu that received the interrupt. In
> > my example it would still be vcpu1. Therefore we need to return vcpu1
> > from vgicv3_get_target_vcpu.
> >
> > Instead as it is now, we would cause an SGI to be sent from pcpu1 to
> > pcpu0 in order to inject the irq to vcpu0 (assuming that vcpu0 is
> > running on pcpu0 for simplicity).
> >
> >
> >> Also, as per GICv3 spec, when GICD_IROUTER_SPI_MODE_ANY is set which is
> >> previous not set ( 0 -> 1 ), the affinity value in IROUTER is invalid/unknown.
> >> So later when vgicv3_get_target_vcpu() is called, it always returns vcpu0.
> >
> > Could you please tell me which one is the relevant chapter of the GICv3
> > spec? I couldn't find the statement you mentioned. Maybe my version of
> > the spec is outdated.
> 
> See 20.0 version section 5.3.4
> 
> "When this bit is written from zero to one, the Affinity fields become UNKNOWN."

Yeah, my version is outdated :-/

In any case I would still recommend always keeping a valid value in
irouter. Maybe we just want to revert to vcpu0, but still it should be a
valid configuration.

Re: [RFC PATCH] xen/arm: Deliver interrupts to vcpu specified in IROUTER

[Stefano Stabellini]

On Mon, 8 Sep 2014, Ian Campbell wrote:
> On Mon, 2014-09-08 at 19:55 +0530, Vijay Kilari wrote:
> > On Mon, Sep 8, 2014 at 7:33 PM, Vijay Kilari <vijay.kilari@gmail.com> wrote:
> > > On Mon, Sep 8, 2014 at 7:17 PM, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> > >> On Mon, 2014-09-08 at 19:11 +0530, Vijay Kilari wrote:
> > >>
> > >> Why have we dropped the list?
> > >
> > > I have not dropped the list. I still see list in my email.
> > >
> > >>
> > >>> Hi Stefano,
> > >>>
> > >>>  Apart from your comments below, I encounter below scenario.
> > >>>
> > >>> The below function vgic_irq_rank() in vgic.c is not generic. The rank is always
> > >>> depends on register type it is going to access. So it cannot be
> > >>> just hardcoded.
> > >>>
> > >>> struct vgic_irq_rank *(struct vcpu *v, unsigned int irq)
> > >>> {
> > >>>     return vgic_rank_offset(v, 8, irq, DABT_WORD);
> > >>> }
> > >>>
> > >>> This function works ok for GIC v2. But this cannot be used for
> > >>> GICv3 to access registers like IROUTER which are u64. The rank
> > >>> calcuation goes wrong and there by takes wrong rank lock.
> > >>
> > >> This sounds to me like a bug which should be fixed. A rank is just a
> > >> group of 32 consecutive IRQs and their associated register values, it
> > >> shouldn't matter whether those registers are 8-, 16-, 32- or 64-bits.
> > >
> > >  Now I think of creating a callback to return the right rank for given irq
> > > based on v2 or v3. This will fix and keep the old code intact.
> > 
> > This should fix the issue.
> 
> Looks plausible. Will you fold it into your series?

It looks OK to me too.


> >  struct vgic_irq_rank *vgic_rank_irq(struct vcpu *v, unsigned int irq)
> >  {
> > -    return vgic_rank_offset(v, 8, irq, DABT_WORD);
> > +    int rank = irq/32;
> > 
> > +    if ( rank == 0 )
> > +        return v->arch.vgic.private_irqs;
> > +    else if ( rank <= DOMAIN_NR_RANKS(v->domain) )
> > +        return &v->domain->arch.vgic.shared_irqs[rank - 1];
> > +    else
> > +        return NULL;
> > }
> 
> 

Re: [RFC PATCH] xen/arm: Deliver interrupts to vcpu specified in IROUTER

[Vijay Kilari]

Hi Stefano,

 Is there way to see irq stats at vcpu level for a domain similar to
/proc/interrupts?

Regards
Vijay

On Tue, Sep 9, 2014 at 4:52 AM, Stefano Stabellini
<stefano.stabellini@eu.citrix.com> wrote:
> On Mon, 8 Sep 2014, Vijay Kilari wrote:
>> Hi Stefano,
>>
>> On Sat, Sep 6, 2014 at 12:14 AM, Stefano Stabellini
>> <stefano.stabellini@eu.citrix.com> wrote:
>> > On Fri, 5 Sep 2014, Vijay Kilari wrote:
>> >> On Fri, Sep 5, 2014 at 4:59 AM, Stefano Stabellini
>> >> <stefano.stabellini@eu.citrix.com> wrote:
>> >> > On Thu, 4 Sep 2014, vijay.kilari@gmail.com wrote:
>> >> >> From: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
>> >> >>
>> >> >> In GICv3 use IROUTER register contents to
>> >> >> deliver irq to specified vcpu's.
>> >> >>
>> >> >> In error scenario fallback to vcpu 0.
>> >> >>
>> >> >> This patch is similar to Stefano's commit
>> >> >> 5b3a817ea33b891caf7d7d788da9ce6deffa82a1 for GICv2
>> >> >>
>> >> >> Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
>> >> >> ---
>> >> >>  xen/arch/arm/vgic-v3.c |  115 ++++++++++++++++++++++++++++++++++++++++++++----
>> >> >>  1 file changed, 106 insertions(+), 9 deletions(-)
>> >> >>
>> >> >> diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
>> >> >> index ecda672..b01a201 100644
>> >> >> --- a/xen/arch/arm/vgic-v3.c
>> >> >> +++ b/xen/arch/arm/vgic-v3.c
>> >> >> @@ -45,6 +45,35 @@
>> >> >>  #define GICV3_GICR_PIDR2  GICV3_GICD_PIDR2
>> >> >>  #define GICV3_GICR_PIDR4  GICV3_GICD_PIDR4
>> >> >>
>> >> >> +static struct vcpu *vgicv3_irouter_to_vcpu(struct vcpu *v,
>> >> >> +                                           uint64_t irouter_aff)
>> >> >> +{
>> >> >> +    int i;
>> >> >> +    uint64_t cpu_affinity;
>> >> >> +    struct vcpu *v_target = NULL;
>> >> >> +
>> >> >> +    /* If routing mode is set. Fallback to vcpu0 */
>> >> >> +    if ( irouter_aff & GICD_IROUTER_SPI_MODE_ANY )
>> >> >> +        return v->domain->vcpu[0];
>> >> >> +
>> >> >> +    for ( i = 0; i < v->domain->max_vcpus; i++ )
>> >> >> +    {
>> >> >> +        v_target = v->domain->vcpu[i];
>> >> >> +        cpu_affinity = (MPIDR_AFFINITY_LEVEL(v_target->arch.vmpidr, 3) << 32 |
>> >> >> +                        MPIDR_AFFINITY_LEVEL(v_target->arch.vmpidr, 2) << 16 |
>> >> >> +                        MPIDR_AFFINITY_LEVEL(v_target->arch.vmpidr, 1) << 8  |
>> >> >> +                        MPIDR_AFFINITY_LEVEL(v_target->arch.vmpidr, 0));
>> >> >> +
>> >> >> +        if ( irouter_aff == cpu_affinity )
>> >> >> +            return v_target;
>> >> >> +    }
>> >> >
>> >> > Using a loop is pretty bad. Couldn't you just calculate the lowest vcpu
>> >> > number from irouter_aff (maybe using find_first_bit)?
>> >>
>> >> IMO,  If GICD_IROUTER_SPI_MODE_ANY bit is set, then it specifies any cpu.
>> >> If GICD_IROUTER_SPI_MODE_ANY is not set, it register specifies only
>> >> one cpu number.
>> >>
>> >> So we cannot use find_first_bit() unlike GICv2 where ITARGET specifies
>> >> irq affinity
>> >> to more than one CPU.
>> >>
>> >> If at all we want to avoid this for loop, then we have to maintain one
>> >> more variable for
>> >> every IROUTERn and corresponding vpcu number, which is updated on
>> >> IROUTERn update
>> >
>> > Given that at the moment AFF0 is just set to vcpu_id (see
>> > vcpu_initialise), I would simply do a direct calculation, adding a TODO
>> > comment so that we remember to fix this when we implement smarter mpidr
>> > schemes.
>>
>> Instead of storing vcpu_id in AFF0 of vgic irouter[], we could rather
>> store vcpu_id in irouter[]
>> similar to v2. One bit per vcpu, though in v3 always only one bit is set.
>>
>> The conversion function irouter_to_vcpu & vcpu_to_irouter will manage
>> mmio_{read,write}
>> of IROUTER regs.
>>
>> Is this OK?
>
> Sounds good in principle, but of course I would need to see the code to
> get a better idea.
>
>
>> >
>> > Even better you could introduce two simple functions that convert vmpidr
>> > to vcpu_id and vice versa, so that we keep all the hacks in one place.
>> >
>> >
>> >> >>      switch ( gicd_reg )
>> >> >> @@ -804,16 +836,42 @@ static int vgic_v3_distr_mmio_write(struct vcpu *v, mmio_info_t *info)
>> >> >>          if ( dabt.size != DABT_DOUBLE_WORD ) goto bad_width;
>> >> >>          rank = vgic_rank_offset(v, 64, gicd_reg - GICD_IROUTER, DABT_DOUBLE_WORD);
>> >> >>          if ( rank == NULL) goto write_ignore_64;
>> >> >> -        if ( *r )
>> >> >> +        new_target = *r;
>> >> >> +        vgic_lock_rank(v, rank, flags);
>> >> >> +        old_target = rank->v3.irouter[REG_RANK_INDEX(64,
>> >> >> +                              (gicd_reg - GICD_IROUTER), DABT_DOUBLE_WORD)];
>> >> >> +        if ( *r & GICD_IROUTER_SPI_MODE_ANY )
>> >> >>          {
>> >> >> -            /* TODO: Ignored. We don't support irq delivery for vcpu != 0 */
>> >> >> -            gdprintk(XENLOG_DEBUG,
>> >> >> -                     "SPI delivery to secondary cpus not supported\n");
>> >> >> -            goto write_ignore_64;
>> >> >> +            /*
>> >> >> +             * IRQ routing mode set. Route any one processor in the entire
>> >> >> +             * system. We chose vcpu 0
>> >> >> +             */
>> >> >> +            rank->v3.irouter[REG_RANK_INDEX(64,
>> >> >> +                     (gicd_reg - GICD_IROUTER), DABT_DOUBLE_WORD)] = *r;
>> >> >> +            vgic_unlock_rank(v, rank, flags);
>> >> >> +            return 1;
>> >> >>          }
>> >> >> -        vgic_lock_rank(v, rank, flags);
>> >> >> +        irq = (gicd_reg - GICD_IROUTER)/8;
>> >> >> +        /* IROUTER now specifies only one cpu affinity for this irq */
>> >> >> +        /* Migrate from any core(vcpu0) to new_target */
>> >> >> +        if ( (old_target & GICD_IROUTER_SPI_MODE_ANY) &&
>> >> >> +             !(new_target & GICD_IROUTER_SPI_MODE_ANY) )
>> >> >> +        {
>> >> >> +            new_vcpu = vgicv3_irouter_to_vcpu(v, new_target);
>> >> >> +            vgic_migrate_irq(v->domain->vcpu[0], new_vcpu, irq);
>> >> >> +        }
>> >> >> +        else
>> >> >> +        {
>> >> >> +            if ( old_target != new_target )
>> >> >> +            {
>> >> >> +                old_vcpu = vgicv3_irouter_to_vcpu(v, old_target);
>> >> >> +                new_vcpu = vgicv3_irouter_to_vcpu(v, new_target);
>> >> >> +                vgic_migrate_irq(old_vcpu, new_vcpu, irq);
>> >> >> +            }
>> >> >> +        }
>> >> >
>> >> > The issue is that if irouter is GICD_IROUTER_SPI_MODE_ANY, you always
>> >> > return vcpu0 below.
>> >> > So if the kernel:
>> >> >
>> >> > 1) move the irq to vcpu1
>> >> > 2) set the irq as GICD_IROUTER_SPI_MODE_ANY
>> >> >
>> >> > you now have migrated the irq to vcpu1 but you are returning vcpu0 from
>> >> > vgicv3_get_target_vcpu.
>> >> >
>> >> > You can either:
>> >> > - vgic_migrate_irq the irq back to vcpu0 when the kernel sets the irq as
>> >> >   GICD_IROUTER_SPI_MODE_ANY
>> >>
>> >>    When irq is set to vcpu1 and later when GICD_IROUTER_SPI_MODE_ANY
>> >> then I assume that vcpu1 is also valid for the moment if irq is pending.
>> >> So no need to do any migration.
>> >
>> > That would be OK, except that vgicv3_get_target_vcpu is actually
>> > returning vcpu0. We should try to match physical and virtual irq
>> > delivery as much as possible. So if the physical irq is delivered to
>> > pcpu1 (or whatever is the pcpu currently running vcpu1), then we should
>> > try hard to deliver the corresponding virq to vcpu1.
>> >
>> > Calling vgic_migrate_irq changes the physical irq affinity to the pcpu
>> > running the destination vcpu. So step 1 above moves physical irq
>> > delivery to pcpu1 (assuming that pcpu1 is running vcpu1 for simplicity).
>> > After the guest kernel sets the irq as GICD_IROUTER_SPI_MODE_ANY, even
>> > though we could deliver the irq to any vcpu, we should actually delivery
>> > the irq to the vcpu running on the pcpu that received the interrupt. In
>> > my example it would still be vcpu1. Therefore we need to return vcpu1
>> > from vgicv3_get_target_vcpu.
>> >
>> > Instead as it is now, we would cause an SGI to be sent from pcpu1 to
>> > pcpu0 in order to inject the irq to vcpu0 (assuming that vcpu0 is
>> > running on pcpu0 for simplicity).
>> >
>> >
>> >> Also, as per GICv3 spec, when GICD_IROUTER_SPI_MODE_ANY is set which is
>> >> previous not set ( 0 -> 1 ), the affinity value in IROUTER is invalid/unknown.
>> >> So later when vgicv3_get_target_vcpu() is called, it always returns vcpu0.
>> >
>> > Could you please tell me which one is the relevant chapter of the GICv3
>> > spec? I couldn't find the statement you mentioned. Maybe my version of
>> > the spec is outdated.
>>
>> See 20.0 version section 5.3.4
>>
>> "When this bit is written from zero to one, the Affinity fields become UNKNOWN."
>
> Yeah, my version is outdated :-/
>
> In any case I would still recommend always keeping a valid value in
> irouter. Maybe we just want to revert to vcpu0, but still it should be a
> valid configuration.
>

Re: [RFC PATCH] xen/arm: Deliver interrupts to vcpu specified in IROUTER

[Stefano Stabellini]

The only additional info that Xen provides is via the CTRL-AAA menu.
Some details on the GIC are printed but nothing as well organized as
/proc/interrupts.

On Wed, 10 Sep 2014, Vijay Kilari wrote:
> Hi Stefano,
> 
>  Is there way to see irq stats at vcpu level for a domain similar to
> /proc/interrupts?
> 
> Regards
> Vijay
> 
> On Tue, Sep 9, 2014 at 4:52 AM, Stefano Stabellini
> <stefano.stabellini@eu.citrix.com> wrote:
> > On Mon, 8 Sep 2014, Vijay Kilari wrote:
> >> Hi Stefano,
> >>
> >> On Sat, Sep 6, 2014 at 12:14 AM, Stefano Stabellini
> >> <stefano.stabellini@eu.citrix.com> wrote:
> >> > On Fri, 5 Sep 2014, Vijay Kilari wrote:
> >> >> On Fri, Sep 5, 2014 at 4:59 AM, Stefano Stabellini
> >> >> <stefano.stabellini@eu.citrix.com> wrote:
> >> >> > On Thu, 4 Sep 2014, vijay.kilari@gmail.com wrote:
> >> >> >> From: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
> >> >> >>
> >> >> >> In GICv3 use IROUTER register contents to
> >> >> >> deliver irq to specified vcpu's.
> >> >> >>
> >> >> >> In error scenario fallback to vcpu 0.
> >> >> >>
> >> >> >> This patch is similar to Stefano's commit
> >> >> >> 5b3a817ea33b891caf7d7d788da9ce6deffa82a1 for GICv2
> >> >> >>
> >> >> >> Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
> >> >> >> ---
> >> >> >>  xen/arch/arm/vgic-v3.c |  115 ++++++++++++++++++++++++++++++++++++++++++++----
> >> >> >>  1 file changed, 106 insertions(+), 9 deletions(-)
> >> >> >>
> >> >> >> diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
> >> >> >> index ecda672..b01a201 100644
> >> >> >> --- a/xen/arch/arm/vgic-v3.c
> >> >> >> +++ b/xen/arch/arm/vgic-v3.c
> >> >> >> @@ -45,6 +45,35 @@
> >> >> >>  #define GICV3_GICR_PIDR2  GICV3_GICD_PIDR2
> >> >> >>  #define GICV3_GICR_PIDR4  GICV3_GICD_PIDR4
> >> >> >>
> >> >> >> +static struct vcpu *vgicv3_irouter_to_vcpu(struct vcpu *v,
> >> >> >> +                                           uint64_t irouter_aff)
> >> >> >> +{
> >> >> >> +    int i;
> >> >> >> +    uint64_t cpu_affinity;
> >> >> >> +    struct vcpu *v_target = NULL;
> >> >> >> +
> >> >> >> +    /* If routing mode is set. Fallback to vcpu0 */
> >> >> >> +    if ( irouter_aff & GICD_IROUTER_SPI_MODE_ANY )
> >> >> >> +        return v->domain->vcpu[0];
> >> >> >> +
> >> >> >> +    for ( i = 0; i < v->domain->max_vcpus; i++ )
> >> >> >> +    {
> >> >> >> +        v_target = v->domain->vcpu[i];
> >> >> >> +        cpu_affinity = (MPIDR_AFFINITY_LEVEL(v_target->arch.vmpidr, 3) << 32 |
> >> >> >> +                        MPIDR_AFFINITY_LEVEL(v_target->arch.vmpidr, 2) << 16 |
> >> >> >> +                        MPIDR_AFFINITY_LEVEL(v_target->arch.vmpidr, 1) << 8  |
> >> >> >> +                        MPIDR_AFFINITY_LEVEL(v_target->arch.vmpidr, 0));
> >> >> >> +
> >> >> >> +        if ( irouter_aff == cpu_affinity )
> >> >> >> +            return v_target;
> >> >> >> +    }
> >> >> >
> >> >> > Using a loop is pretty bad. Couldn't you just calculate the lowest vcpu
> >> >> > number from irouter_aff (maybe using find_first_bit)?
> >> >>
> >> >> IMO,  If GICD_IROUTER_SPI_MODE_ANY bit is set, then it specifies any cpu.
> >> >> If GICD_IROUTER_SPI_MODE_ANY is not set, it register specifies only
> >> >> one cpu number.
> >> >>
> >> >> So we cannot use find_first_bit() unlike GICv2 where ITARGET specifies
> >> >> irq affinity
> >> >> to more than one CPU.
> >> >>
> >> >> If at all we want to avoid this for loop, then we have to maintain one
> >> >> more variable for
> >> >> every IROUTERn and corresponding vpcu number, which is updated on
> >> >> IROUTERn update
> >> >
> >> > Given that at the moment AFF0 is just set to vcpu_id (see
> >> > vcpu_initialise), I would simply do a direct calculation, adding a TODO
> >> > comment so that we remember to fix this when we implement smarter mpidr
> >> > schemes.
> >>
> >> Instead of storing vcpu_id in AFF0 of vgic irouter[], we could rather
> >> store vcpu_id in irouter[]
> >> similar to v2. One bit per vcpu, though in v3 always only one bit is set.
> >>
> >> The conversion function irouter_to_vcpu & vcpu_to_irouter will manage
> >> mmio_{read,write}
> >> of IROUTER regs.
> >>
> >> Is this OK?
> >
> > Sounds good in principle, but of course I would need to see the code to
> > get a better idea.
> >
> >
> >> >
> >> > Even better you could introduce two simple functions that convert vmpidr
> >> > to vcpu_id and vice versa, so that we keep all the hacks in one place.
> >> >
> >> >
> >> >> >>      switch ( gicd_reg )
> >> >> >> @@ -804,16 +836,42 @@ static int vgic_v3_distr_mmio_write(struct vcpu *v, mmio_info_t *info)
> >> >> >>          if ( dabt.size != DABT_DOUBLE_WORD ) goto bad_width;
> >> >> >>          rank = vgic_rank_offset(v, 64, gicd_reg - GICD_IROUTER, DABT_DOUBLE_WORD);
> >> >> >>          if ( rank == NULL) goto write_ignore_64;
> >> >> >> -        if ( *r )
> >> >> >> +        new_target = *r;
> >> >> >> +        vgic_lock_rank(v, rank, flags);
> >> >> >> +        old_target = rank->v3.irouter[REG_RANK_INDEX(64,
> >> >> >> +                              (gicd_reg - GICD_IROUTER), DABT_DOUBLE_WORD)];
> >> >> >> +        if ( *r & GICD_IROUTER_SPI_MODE_ANY )
> >> >> >>          {
> >> >> >> -            /* TODO: Ignored. We don't support irq delivery for vcpu != 0 */
> >> >> >> -            gdprintk(XENLOG_DEBUG,
> >> >> >> -                     "SPI delivery to secondary cpus not supported\n");
> >> >> >> -            goto write_ignore_64;
> >> >> >> +            /*
> >> >> >> +             * IRQ routing mode set. Route any one processor in the entire
> >> >> >> +             * system. We chose vcpu 0
> >> >> >> +             */
> >> >> >> +            rank->v3.irouter[REG_RANK_INDEX(64,
> >> >> >> +                     (gicd_reg - GICD_IROUTER), DABT_DOUBLE_WORD)] = *r;
> >> >> >> +            vgic_unlock_rank(v, rank, flags);
> >> >> >> +            return 1;
> >> >> >>          }
> >> >> >> -        vgic_lock_rank(v, rank, flags);
> >> >> >> +        irq = (gicd_reg - GICD_IROUTER)/8;
> >> >> >> +        /* IROUTER now specifies only one cpu affinity for this irq */
> >> >> >> +        /* Migrate from any core(vcpu0) to new_target */
> >> >> >> +        if ( (old_target & GICD_IROUTER_SPI_MODE_ANY) &&
> >> >> >> +             !(new_target & GICD_IROUTER_SPI_MODE_ANY) )
> >> >> >> +        {
> >> >> >> +            new_vcpu = vgicv3_irouter_to_vcpu(v, new_target);
> >> >> >> +            vgic_migrate_irq(v->domain->vcpu[0], new_vcpu, irq);
> >> >> >> +        }
> >> >> >> +        else
> >> >> >> +        {
> >> >> >> +            if ( old_target != new_target )
> >> >> >> +            {
> >> >> >> +                old_vcpu = vgicv3_irouter_to_vcpu(v, old_target);
> >> >> >> +                new_vcpu = vgicv3_irouter_to_vcpu(v, new_target);
> >> >> >> +                vgic_migrate_irq(old_vcpu, new_vcpu, irq);
> >> >> >> +            }
> >> >> >> +        }
> >> >> >
> >> >> > The issue is that if irouter is GICD_IROUTER_SPI_MODE_ANY, you always
> >> >> > return vcpu0 below.
> >> >> > So if the kernel:
> >> >> >
> >> >> > 1) move the irq to vcpu1
> >> >> > 2) set the irq as GICD_IROUTER_SPI_MODE_ANY
> >> >> >
> >> >> > you now have migrated the irq to vcpu1 but you are returning vcpu0 from
> >> >> > vgicv3_get_target_vcpu.
> >> >> >
> >> >> > You can either:
> >> >> > - vgic_migrate_irq the irq back to vcpu0 when the kernel sets the irq as
> >> >> >   GICD_IROUTER_SPI_MODE_ANY
> >> >>
> >> >>    When irq is set to vcpu1 and later when GICD_IROUTER_SPI_MODE_ANY
> >> >> then I assume that vcpu1 is also valid for the moment if irq is pending.
> >> >> So no need to do any migration.
> >> >
> >> > That would be OK, except that vgicv3_get_target_vcpu is actually
> >> > returning vcpu0. We should try to match physical and virtual irq
> >> > delivery as much as possible. So if the physical irq is delivered to
> >> > pcpu1 (or whatever is the pcpu currently running vcpu1), then we should
> >> > try hard to deliver the corresponding virq to vcpu1.
> >> >
> >> > Calling vgic_migrate_irq changes the physical irq affinity to the pcpu
> >> > running the destination vcpu. So step 1 above moves physical irq
> >> > delivery to pcpu1 (assuming that pcpu1 is running vcpu1 for simplicity).
> >> > After the guest kernel sets the irq as GICD_IROUTER_SPI_MODE_ANY, even
> >> > though we could deliver the irq to any vcpu, we should actually delivery
> >> > the irq to the vcpu running on the pcpu that received the interrupt. In
> >> > my example it would still be vcpu1. Therefore we need to return vcpu1
> >> > from vgicv3_get_target_vcpu.
> >> >
> >> > Instead as it is now, we would cause an SGI to be sent from pcpu1 to
> >> > pcpu0 in order to inject the irq to vcpu0 (assuming that vcpu0 is
> >> > running on pcpu0 for simplicity).
> >> >
> >> >
> >> >> Also, as per GICv3 spec, when GICD_IROUTER_SPI_MODE_ANY is set which is
> >> >> previous not set ( 0 -> 1 ), the affinity value in IROUTER is invalid/unknown.
> >> >> So later when vgicv3_get_target_vcpu() is called, it always returns vcpu0.
> >> >
> >> > Could you please tell me which one is the relevant chapter of the GICv3
> >> > spec? I couldn't find the statement you mentioned. Maybe my version of
> >> > the spec is outdated.
> >>
> >> See 20.0 version section 5.3.4
> >>
> >> "When this bit is written from zero to one, the Affinity fields become UNKNOWN."
> >
> > Yeah, my version is outdated :-/
> >
> > In any case I would still recommend always keeping a valid value in
> > irouter. Maybe we just want to revert to vcpu0, but still it should be a
> > valid configuration.
> >
> 

Re: [RFC PATCH] xen/arm: Deliver interrupts to vcpu specified in IROUTER

[Ian Campbell]

On Wed, 2014-09-10 at 19:54 +0100, Stefano Stabellini wrote:
> The only additional info that Xen provides is via the CTRL-AAA menu.
> Some details on the GIC are printed but nothing as well organized as
> /proc/interrupts.

I've got the following sat in a local branch somewhere. It's not at all
what you would want here but it might give a hint as to the sort of
approach for adding this stuff. I also posted some perfc stuff back in
http://lists.xen.org/archives/html/xen-devel/2014-05/msg01984.html which
included various IRQ (rather than GIC) level stats IIRC.

Ian.

8<----------------------

commit 2c71c40ef723b90faf14bc513bd71e532beffa08
Author: Ian Campbell <ian.campbell@citrix.com>
Date:   Wed Nov 20 11:54:20 2013 +0000

    xen: arm: Add debug keyhandler to dump the physical GIC state.
    
    Rename the existing gic_dump_info to gic_dump_info_guest reduce confusion.
    
    Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
    ---
    v2: s/gic_dump_info/gic_dump_info_guest/

diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index 256d9cf..d4ddf41 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -27,6 +27,7 @@
 #include <xen/softirq.h>
 #include <xen/list.h>
 #include <xen/device_tree.h>
+#include <xen/keyhandler.h>
 #include <asm/p2m.h>
 #include <asm/domain.h>
 #include <asm/platform.h>
@@ -161,6 +162,77 @@ int gic_irq_xlate(const u32 *intspec, unsigned int intsize,
     return 0;
 }
 
+
+static void do_dump_gic(unsigned char key)
+{
+    int irq;
+    printk("'%c' pressed -> dumping GIC state\n", key);
+
+    for ( irq = 0; irq < gic.lines; irq++ )
+    {
+        const char *type;
+        int type_nr, enable, pend, active, priority, target;
+        struct irq_desc *desc = irq_to_desc(irq);
+        uint8_t *bytereg;
+        uint32_t wordreg;
+
+        bytereg = (uint8_t *) (GICD + GICD_ITARGETSR);
+        target = bytereg[irq];
+
+        bytereg = (uint8_t *) (GICD + GICD_IPRIORITYR);
+        priority = bytereg[irq];
+
+        switch ( irq )
+        {
+        case 0 ... 15:
+            type = "SGI";
+            type_nr = irq;
+            target = 0x00; /* these are per-CPU */
+            break;
+        case 16 ... 31:
+            type = "PPI";
+            type_nr = irq - 16;
+            break;
+        default:
+            type = "SPI";
+            type_nr = irq - 32;
+            break;
+        }
+
+        wordreg = GICD[GICD_ISENABLER + irq / 32];
+        enable = !!(wordreg & (1u << (irq % 32)));
+        wordreg = GICD[GICD_ISPENDR + irq / 32];
+        pend = !!(wordreg & (1u << (irq % 32)));
+        wordreg = GICD[GICD_ISACTIVER + irq / 32];
+        active = !!(wordreg & (1u << (irq % 32)));
+
+        printk("IRQ%d %s%d: %c%c%c pri:%02x tgt:%02x ",
+               irq, type, type_nr,
+               enable ? 'e' : '-',
+               pend   ? 'p' : '-',
+               active ? 'a' : '-',
+               priority, target);
+
+        if ( desc->status & IRQ_GUEST )
+        {
+            struct domain *d = desc->action->dev_id;
+            printk("dom%d %s", d->domain_id, desc->action->name);
+        }
+        else
+        {
+            printk("Xen");
+        }
+        printk("\n");
+    }
+
+}
+
+static struct keyhandler dump_gic_keyhandler = {
+    .irq_callback = 0,
+    .u.fn = do_dump_gic,
+    .desc = "dump GIC state"
+};
+
 /* Set up the GIC */
 void __init gic_init(void)
 {
@@ -189,6 +261,8 @@ void __init gic_init(void)
 
     /* Clear LR mask for cpu0 */
     clear_cpu_lr_mask();
+
+    register_keyhandler('G', &dump_gic_keyhandler);
 }
 
 void send_SGI_mask(const cpumask_t *cpumask, enum gic_sgi sgi)
@@ -592,7 +666,7 @@ static void maintenance_interrupt(int irq, void *dev_id, struct cpu_user_regs *r
      */
 }
 
-void gic_dump_info(struct vcpu *v)
+void gic_dump_info_guest(struct vcpu *v)
 {
     struct pending_irq *p;
 
diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
index a0c07bf..f10ad39 100644
--- a/xen/include/asm-arm/gic.h
+++ b/xen/include/asm-arm/gic.h
@@ -245,7 +245,7 @@ extern void send_SGI_self(enum gic_sgi sgi);
 extern void send_SGI_allbutself(enum gic_sgi sgi);
 
 /* print useful debug info */
-extern void gic_dump_info(struct vcpu *v);
+extern void gic_dump_info_guest(struct vcpu *v);
 
 /* Number of interrupt lines */
 extern unsigned int gic_number_lines(void);