Re: [PATCH 5/5] xen/arm: Only enable physical IRQs when the guest asks

[Julien Grall <julien.grall@linaro.org>]

On 06/25/2013 05:19 PM, Stefano Stabellini wrote:

> On Tue, 25 Jun 2013, Julien Grall wrote:
>> If the guest VCPU receives an interrupts when it's disabled, it will throw
>                                    ^interrupt
> 
>> away the IRQ with EOIed it.
> 
> What does this mean? I cannot parse the sentence.

"it will throw away the IRQ without EOIing".

> 
>> This is result to lost IRQ forever.
>> Directly EOIed the interrupt doesn't help because the IRQ could be fired
>> again and result to an infinited loop.
>>
>> It happens during dom0 boot on the versatile express TC2 with the ethernet
>> card.
>>
>> Let the interrupt disabled when Xen setups the route and enable it when Linux
>> asks to enable it.
> 
> Is the problem that Xen keeps the interrupt enabled even when Linux
> disables it at the gic level? Is this what this patch is trying to
> address?

This patch only delays the physical IRQ activation until Linux will
enable it. This patch avoids to lose IRQ when a VCPU is disabled.

I tried to also disable the IRQ when Linux asks to disable it but the
versatile express platform hang.

> 
>> Signed-off-by: Julien Grall <julien.grall@citrix.com>
>> ---
>>  xen/arch/arm/domain_build.c |   14 ++++++++++++++
>>  xen/arch/arm/gic.c          |   25 +++++++++++++++++++++++--
>>  xen/arch/arm/vgic.c         |    7 +++----
>>  xen/include/asm-arm/gic.h   |    4 ++++
>>  xen/include/asm-arm/irq.h   |    6 ++++++
>>  5 files changed, 50 insertions(+), 6 deletions(-)
>>
>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
>> index f5befbd..0470a2d 100644
>> --- a/xen/arch/arm/domain_build.c
>> +++ b/xen/arch/arm/domain_build.c
>> @@ -408,6 +408,20 @@ static int map_device(struct domain *d, const struct dt_device_node *dev)
>>          }
>>  
>>          DPRINT("irq %u = %u type = 0x%x\n", i, irq.irq, irq.type);
>> +
>> +        /*
>> +         * Only map SGI interrupt in the guest as XEN won't handle
>> +         * it correctly.
>> +         * TODO: Fix it
>> +         */
>> +        if ( !irq_is_sgi(irq.irq) )
>> +        {
>> +            printk(XENLOG_WARNING "WARNING: Don't map irq %u for %s has "
>> +                   "XEN doesn't handle properly non-SGI interrupt\n",
>> +                   i, dt_node_full_name(dev));
> 
> do you mean SPI?
> 
> 
>> +            continue;
>> +        }
>> +
>>          /* Don't check return because the IRQ can be use by multiple device */
>>          gic_route_irq_to_guest(d, &irq, dt_node_name(dev));
>>      }
>> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
>> index b16ba8c..e7d082a 100644
>> --- a/xen/arch/arm/gic.c
>> +++ b/xen/arch/arm/gic.c
>> @@ -179,6 +179,17 @@ static hw_irq_controller gic_guest_irq_type = {
>>      .set_affinity = gic_irq_set_affinity,
>>  };
>>  
>> +void gic_irq_enable(struct irq_desc *desc)
>> +{
>> +    spin_lock_irq(&desc->lock);
>> +    spin_lock(&gic.lock);
>> +
>> +    desc->handler->enable(desc);
>> +
>> +    spin_unlock(&gic.lock);
>> +    spin_unlock_irq(&desc->lock);
>> +}
> 
> This function looks a bit too similar to gic_irq_mask and friends,
> except that it takes two locks.
> 
> To make that obvious it's probably better to call it gic_irq_enable_safe
> or gic_irq_enable_locked.
> 
> 
>>  /* needs to be called with gic.lock held */
>>  static void gic_set_irq_properties(unsigned int irq, bool_t level,
>>          unsigned int cpu_mask, unsigned int priority)
>> @@ -543,8 +554,6 @@ static int __setup_irq(struct irq_desc *desc, unsigned int irq,
>>      desc->status &= ~IRQ_DISABLED;
>>      dsb();
>>  
>> -    desc->handler->startup(desc);
>> -
>>      return 0;
>>  }
>>  
>> @@ -560,6 +569,8 @@ int __init setup_dt_irq(const struct dt_irq *irq, struct irqaction *new)
>>  
>>      rc = __setup_irq(desc, irq->irq, new);
>>  
>> +    desc->handler->startup(desc);
>> +
>>      spin_unlock_irqrestore(&desc->lock, flags);
>>  
>>      return rc;
> 
> This two changes make it so guest irqs are not enabled by default, good.
> 
> 
>> @@ -711,6 +722,7 @@ void gic_inject(void)
>>          gic_inject_irq_start();
>>  }
>>  
>> +/* TODO: Handle properly non SGI-interrupt */
>>  int gic_route_irq_to_guest(struct domain *d, const struct dt_irq *irq,
>>                             const char * devname)
>>  {
>> @@ -719,11 +731,18 @@ int gic_route_irq_to_guest(struct domain *d, const struct dt_irq *irq,
>>      unsigned long flags;
>>      int retval;
>>      bool_t level;
>> +    struct pending_irq *p;
>> +    /* XXX: handler other VCPU than 0 */
>> +    struct vcpu *v = d->vcpu[0];
>>  
>>      action = xmalloc(struct irqaction);
>>      if (!action)
>>          return -ENOMEM;
>>  
>> +    /* XXX: Here we assume a 1:1 interrupt mapping between the host and dom0 */
>> +    BUG_ON(irq->irq >= (d->arch.vgic.nr_lines + NR_LOCAL_IRQS));
>> +    p = irq_to_pending(v, irq->irq);
>> +
>>      action->dev_id = d;
>>      action->name = devname;
>>      action->free_on_release = 1;
>> @@ -744,6 +763,8 @@ int gic_route_irq_to_guest(struct domain *d, const struct dt_irq *irq,
>>          goto out;
>>      }
>>  
>> +    p->desc = desc;
>> +
>>  out:
>>      spin_unlock(&gic.lock);
>>      spin_unlock_irqrestore(&desc->lock, flags);
>> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
>> index cea9233..4f3d816 100644
>> --- a/xen/arch/arm/vgic.c
>> +++ b/xen/arch/arm/vgic.c
>> @@ -372,6 +372,9 @@ static void vgic_enable_irqs(struct vcpu *v, uint32_t r, int n)
>>          if ( !list_empty(&p->inflight) && !gic_irq_injected(irq) )
>>              gic_set_guest_irq(v, irq, GICH_LR_PENDING, p->priority);
>>  
>> +        if ( p->desc != NULL )
>> +            gic_irq_enable(p->desc);
>> +
>>          i++;
>>      }
>>  }
> 
> Should we add a gic_irq_disable call when the guest disables irqs?
> 
> 
>> @@ -685,10 +688,6 @@ void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int irq, int virtual)
>>  
>>      n->irq = irq;
>>      n->priority = priority;
>> -    if (!virtual)
>> -        n->desc = irq_to_desc(irq);
>> -    else
>> -        n->desc = NULL;
>>  
>>      /* the irq is enabled */
>>      if ( rank->ienable & (1 << (irq % 32)) )
> 
> I don't quite understand why are you changing where the "desc"
> assignement is done.
> If it is a cleanup you shouldn't mix it with a patch like this that is
> supposed to fix a specific issue. Otherwise please explain why you need
> this change.
> 
> 
>> diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
>> index f9e9ef1..f7f3c1e 100644
>> --- a/xen/include/asm-arm/gic.h
>> +++ b/xen/include/asm-arm/gic.h
>> @@ -134,6 +134,7 @@
>>  
>>  #ifndef __ASSEMBLY__
>>  #include <xen/device_tree.h>
>> +#include <xen/irq.h>
>>  
>>  extern int domain_vgic_init(struct domain *d);
>>  extern void domain_vgic_free(struct domain *d);
>> @@ -154,6 +155,9 @@ extern void gic_inject(void);
>>  extern void gic_clear_pending_irqs(struct vcpu *v);
>>  extern int gic_events_need_delivery(void);
>>  
>> +/* Helper to enable an IRQ and take all the needed locks */
>> +extern void gic_irq_enable(struct irq_desc *desc);
>> +
>>  extern void __cpuinit init_maintenance_interrupt(void);
>>  extern void gic_set_guest_irq(struct vcpu *v, unsigned int irq,
>>          unsigned int state, unsigned int priority);
>> diff --git a/xen/include/asm-arm/irq.h b/xen/include/asm-arm/irq.h
>> index 80ff68d..346dc1d 100644
>> --- a/xen/include/asm-arm/irq.h
>> +++ b/xen/include/asm-arm/irq.h
>> @@ -46,6 +46,12 @@ int __init request_dt_irq(const struct dt_irq *irq,
>>                            void *dev_id);
>>  int __init setup_dt_irq(const struct dt_irq *irq, struct irqaction *new);
>>  
>> +#define FIRST_SGI_IRQ   32
>> +static inline bool_t irq_is_sgi(unsigned int irq)
>> +{
>> +    return (irq >= FIRST_SGI_IRQ);
>> +}
> 
> Aren't SGIs supposed to be between 0 and 15? Aren't you talking about
> SPIs here?