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

Re: [Xen-devel] [PATCH for-next] xen/arm: irq: Don't use _IRQ_PENDING when handling host interrupt



Hi Stefano,

On 4/16/19 10:51 PM, Stefano Stabellini wrote:
On Mon, 28 Jan 2019, Julien Grall wrote:
While SPIs are shared between CPU, it is not possible to receive the
same interrupts on a different CPU while the interrupt is in active
state. The deactivation of the interrupt is done at the end of the
handling.

This means the _IRQ_PENDING logic is unecessary on Arm as a same
interrupt can never come up while in the loop. So remove it to
simplify the interrupt handle code.

Signed-off-by: Julien Grall <julien.grall@xxxxxxx>
---
  xen/arch/arm/irq.c | 32 ++++++++++----------------------
  1 file changed, 10 insertions(+), 22 deletions(-)

diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
index c51cf333ce..3877657a52 100644
--- a/xen/arch/arm/irq.c
+++ b/xen/arch/arm/irq.c
@@ -199,6 +199,7 @@ int request_irq(unsigned int irq, unsigned int irqflags,
  void do_IRQ(struct cpu_user_regs *regs, unsigned int irq, int is_fiq)
  {
      struct irq_desc *desc = irq_to_desc(irq);
+    struct irqaction *action;
perfc_incr(irqs); @@ -242,35 +243,22 @@ void do_IRQ(struct cpu_user_regs *regs, unsigned int irq, int is_fiq)
          goto out_no_end;
      }
- set_bit(_IRQ_PENDING, &desc->status);
-
-    /*
-     * Since we set PENDING, if another processor is handling a different
-     * instance of this same irq, the other processor will take care of it.
-     */
-    if ( test_bit(_IRQ_DISABLED, &desc->status) ||
-         test_bit(_IRQ_INPROGRESS, &desc->status) )
+    if ( test_bit(_IRQ_DISABLED, &desc->status) )
          goto out;

It is a good idea to remove the IRQ_PENDING logic, that is OK.


However, are we sure that we want to remove the _IRQ_INPROGRESS check
too? IRQ handlers shouldn't be called twice in a row. Given that
_IRQ_INPROGRESS can be set manually (gicv2_set_active_state) it seems it
would be a good idea to keep the check anyway?

set_active_state is only used by the vGIC to replicate state from of the virtual interrupt to the physical interrupt. We don't have the virtual interrupt in this path (see above).

Any other user (e.g interrupts routed to Xen) would be pretty broken. At best you would break the interrupt flow. At worst, you may never receive the interrupt again.

So I think we can drop _IRQ_PROGRESS here.

Cheers,

--
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel

 


Rackspace

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