|
[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: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@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 "
>> + "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?
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |