[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,

On 17/04/2019 18:12, Stefano Stabellini wrote:
On Tue, 16 Apr 2019, Julien Grall wrote:
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.

I gave it a close look. You are right, it is safe to remove the
_IRQ_PROGRESS check here.

Reviewed-by: Stefano Stabellini <sstabellini@xxxxxxxxxx>


The thing that worries me a bit is that technically set_active_state is
part of the gic_hw_operations functions which are not necessarily guest
specific: we haven't written down anywhere that set_active_state cannot
be called passing one of the xen irqs as parameter. I agree it would be
broken to do so, but still... Maybe we should add a comment?

How about adding an ASSERT(test_bit(_IRQ_GUEST, &desc->status)) ?

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®.