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