|
[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 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!
> + "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?
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...
> + 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
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |