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

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



On 06/25/2013 05:28 PM, Ian Campbell wrote:

> On Tue, 2013-06-25 at 00:04 +0100, Julien Grall wrote:
>> If the guest VCPU receives an interrupts when it's disabled, it will throw
>> away the IRQ with EOIed it.
> 
> Did you mean "without EOIing it" or perhaps "having EOIed it"?
> 
>>  This is result to lost IRQ forever.
> 
> "This results in losing the IRQ forever".
> 
>> Directly EOIed the interrupt doesn't help because the IRQ could be fired
> 
> EOIing in this context.
> 
>> again and result to an infinited loop.
> 
>                         infinite
> 
>> 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
> 
>   "Lets ... when Xen sets up the route..."
> 
>> asks to enable it.
>>
>> Signed-off-by: Julien Grall <julien.grall@xxxxxxxxxx>
>> ---
>>  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 "
> 
> s/has/as/ I think.
> 
> Did you really mean SGI here? I'd have thought from the context that you
> would mean SPIs. SGIs aren't anything to do with any real devices almost
> by definition -- if you saw on in the device tree I'd be very surprised!


It was SPIs. I will fix it in the next patch series.

> 
>> +                   "XEN doesn't handle properly non-SGI interrupt\n",
>> +                   i, dt_node_full_name(dev));
>> +            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);
>> +}
>> +
>>  /* 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;
>> @@ -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 */
> 
> That should be something like "XXX: handle VCPUs other than 0".
> 
> This only matters if we can route SGIs or PPIs to the guest though I
> think, since they are the only banked interrupts? For SPIs we actually
> want to actively avoid doing this multiple times, don't we?


Yes. Here the VCPU is only used to retrieved the struct pending_irq.

> 
> For the banked interrupts I think we just need a loop here, or for
> p->desc to not be part of the pending_irq struct but actually part of
> some separate per-domain datastructure, since it would be very weird to
> have a domain where the PPIs differed between CPUs. (I'm not sure if
> that is allowed by the hardware, I bet it is, but it would be a
> pathological case IMHO...).

> I think a perdomain irq_desc * array is probably the right answer,
> unless someone can convincingly argue that PPI routing differing between
> VCPUs in a guest is a useful thing...


Until now, I didn't see PPIs on other devices than the arch timers and
the GIC. I don't know if it's possible, but pending_irq are also banked
for PPIs, so it's not an issue.

The issue is how do we link the physical PPI to the virtual PPI? Is a
1:1 mapping. How does Xen handle PPI when a it is coming on VCPUs which
doesn't handle it (for instance a domU)?

>> +    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++;
>>      }
>>  }
>> @@ -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)) )
>> 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);
>> +}
>> +
>>  #endif /* _ASM_HW_IRQ_H */
>>  /*
>>   * Local variables:
> 
> 



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.