|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v4 03/10] xen/arm: support HW interrupts, do not request maintenance_interrupts
On Wed, 2014-03-19 at 12:31 +0000, Stefano Stabellini wrote:
> If the irq to be injected is an hardware irq (p->desc != NULL), set
> GICH_LR_HW. Do not set GICH_LR_MAINTENANCE_IRQ.
>
> Remove the code to EOI a physical interrupt on behalf of the guest
> because it has become unnecessary.
Stupid question: there is no possibility of a h/w interrupt which for
one reason or another cannot be injected using these GIC facilities?
>
> Introduce a new function, gic_clear_lrs, that goes over the GICH_LR
> registers, clear the invalid ones and free the corresponding interrupts
> from the inflight queue if appropriate. Add the interrupt to lr_pending
> if the GIC_IRQ_GUEST_PENDING is still set.
>
> Call gic_clear_lrs from gic_restore_state and on return to guest
> (gic_inject).
>
> In vgic_vcpu_inject_irq, if the target is a vcpu running on another cpu,
> send and SGI to it to interrupt it and force it to clear the old LRs.
s/and/an/
Is the SGI just there to cause it to flush its LRs? Does it not also
serve the purpose of causing the pcpu to pick up the potential new
interrupt?
>
> Signed-off-by: Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>
>
> ---
>
> Changes in v4:
> - merged patch #3 and #4 into a single patch.
>
> Changes in v2:
> - remove the EOI code, now unnecessary;
> - do not assume physical IRQ == virtual IRQ;
> - refactor gic_set_lr.
> ---
> xen/arch/arm/gic.c | 135
> ++++++++++++++++++++++-----------------------------
> xen/arch/arm/vgic.c | 3 +-
> 2 files changed, 60 insertions(+), 78 deletions(-)
>
> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> index dbba5d3..32d3bea 100644
> --- a/xen/arch/arm/gic.c
> +++ b/xen/arch/arm/gic.c
> @@ -128,6 +130,7 @@ void gic_restore_state(struct vcpu *v)
> GICH[GICH_HCR] = GICH_HCR_EN;
> isb();
>
> + gic_clear_lrs(v);
> gic_restore_pending_irqs(v);
Not related to this patch exactly, but is this function badly named? It
seems to not actually be restoring anything but is actually looking for
any newly pending irqs which it can now inject, is that right?
Both callers of gic_restore_pending_irqs call gic_clear_lrs. Which
suggests that the clear should be done there (which also seems logical).
> }
>
> @@ -625,16 +628,19 @@ int __init setup_dt_irq(const struct dt_irq *irq,
> struct irqaction *new)
> static inline void gic_set_lr(int lr, struct pending_irq *p,
> unsigned int state)
> {
> - int maintenance_int = GICH_LR_MAINTENANCE_IRQ;
> + uint32_t lr_reg;
>
> BUG_ON(lr >= nr_lrs);
> BUG_ON(lr < 0);
> BUG_ON(state & ~(GICH_LR_STATE_MASK<<GICH_LR_STATE_SHIFT));
>
> - GICH[GICH_LR + lr] = state |
> - maintenance_int |
> - ((p->priority >> 3) << GICH_LR_PRIORITY_SHIFT) |
> + lr_reg = state | ((p->priority >> 3) << GICH_LR_PRIORITY_SHIFT) |
> ((p->irq & GICH_LR_VIRTUAL_MASK) << GICH_LR_VIRTUAL_SHIFT);
> + if ( p->desc != NULL )
> + lr_reg |= GICH_LR_HW |
> + ((p->desc->irq & GICH_LR_PHYSICAL_MASK) <<
> GICH_LR_PHYSICAL_SHIFT);
It seems like it would be a silicon design bug if this were ever true.
So BUG_ON(p->desc->irq & ~GICH_LR_PHYSICAL_MASK)?
I think this should be checked at the time the interrupt is routed
instead of here, which gets it out of this hotter path.
Another future cleanup: I think a lot of this register frobbing code
would be cleaner if GICH_LR_FOO_MASK was already shifted.
[...]
> +static void gic_clear_lrs(struct vcpu *v)
> +{
> + struct pending_irq *p;
> + int i = 0, irq;
> + uint32_t lr;
> + bool_t inflight;
> +
> + ASSERT(!local_irq_is_enabled());
> +
> + while ((i = find_next_bit((const long unsigned int *) &this_cpu(lr_mask),
"long unsigned int" is an odd one isn't it?
It seems like all of the other places which do bitops on lr_mask manage
to avoid any case at all.
> + nr_lrs, i)) < nr_lrs) {
> + lr = GICH[GICH_LR + i];
> + if ( !(lr & (GICH_LR_PENDING|GICH_LR_ACTIVE)) )
This state means the lr is "completed" (or inactive, however you want to
think about it)? A little helper macro lr_completed(lr) would clarify
this without needing a comment.
Isn't that condition also true for an LR which was never injected? Won't
we then try and complete a non-existent interrupt? Ah, this is protected
by the lr_mask isn't it.
> + {
> + inflight = 0;
> + GICH[GICH_LR + i] = 0;
> + clear_bit(i, &this_cpu(lr_mask));
> +
> + irq = (lr >> GICH_LR_VIRTUAL_SHIFT) & GICH_LR_VIRTUAL_MASK;
> + spin_lock(&gic.lock);
> + p = irq_to_pending(v, irq);
> + if ( p->desc != NULL )
> + p->desc->status &= ~IRQ_INPROGRESS;
> + clear_bit(GIC_IRQ_GUEST_VISIBLE, &p->status);
> + if ( test_bit(GIC_IRQ_GUEST_PENDING, &p->status) &&
> + test_bit(GIC_IRQ_GUEST_ENABLED, &p->status))
> + {
> + inflight = 1;
> + gic_set_guest_irq(v, irq, GICH_LR_PENDING, p->priority);
> + }
> + spin_unlock(&gic.lock);
Can an interrupt arrive at this point and make this interrupt "inflight"
again, such that the following list remove is actually wrong?
> + if ( !inflight )
> + {
> + spin_lock(&v->arch.vgic.lock);
> + list_del_init(&p->inflight);
> + spin_unlock(&v->arch.vgic.lock);
> + }
> +
> + }
> +
> + i++;
> + }
> +}
> +
> static void gic_restore_pending_irqs(struct vcpu *v)
> {
> int i;
> static void maintenance_interrupt(int irq, void *dev_id, struct
> cpu_user_regs *regs)
> {
[...]
Is now empty but not removed?
> }
>
> void gic_dump_info(struct vcpu *v)
> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
> index aab490c..566f0ff 100644
> --- a/xen/arch/arm/vgic.c
> +++ b/xen/arch/arm/vgic.c
> @@ -701,8 +701,7 @@ void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int
> irq)
> if ( (irq != current->domain->arch.evtchn_irq) ||
> (!test_bit(GIC_IRQ_GUEST_VISIBLE, &n->status)) )
> set_bit(GIC_IRQ_GUEST_PENDING, &n->status);
> - spin_unlock_irqrestore(&v->arch.vgic.lock, flags);
> - return;
> + goto out;
> }
>
> /* vcpu offline */
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |