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

[Julien Grall <julien.grall@xxxxxxx>]

Hi Stefano,

On 5/20/19 10:04 PM, Stefano Stabellini wrote:
> On Mon, 20 May 2019, Julien Grall wrote:
> > On 17/04/2019 18:27, Stefano Stabellini wrote:
> > > On Wed, 17 Apr 2019, Julien Grall wrote:
> > > > 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)) ?
> > > even better
> > Do you want the change to be in this patch or separately?
> In this patch please
Ok, I will respin the patch.

Cheers,

--
Julien Grall

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