Re: [PATCH] xen/arm: irq: Use appropriate priority for SGIs in setup_irq()

[Mykola Kvach <xakep.amatop@xxxxxxxxx>]

Hi Julien,

Thank you for your review and the helpful comments.
I appreciate your time and feedback.

On Sat, Sep 13, 2025 at 1:01 AM Julien Grall <julien@xxxxxxx> wrote:
>
> Hi Mykola,
>
> On 03/09/2025 03:55, Mykola Kvach wrote:
> > From: Mykola Kvach <mykola_kvach@xxxxxxxx>
> >
> > Use GIC_PRI_IPI priority for SGI interrupts instead of the generic
> > GIC_PRI_IRQ priority in setup_irq().
> >
> > This change ensures that SGIs get the correct priority level when
> > being set up for Xen's use, maintaining proper interrupt precedence
> > in the system.
> >
> > The priority assignment now follows ARM GIC best practices:
> > - SGIs (0-15): GIC_PRI_IPI (higher priority)
> > - PPIs/SPIs (16+): GIC_PRI_IRQ (standard priority)
>
> Please provide a reference to the spec. But I don't follow why we should
> follow exactly what the spec suggest. This is up to us to decide what we
> want. Otherwise what's the point of having more than two priorities?

To clarify, the GIC specification does not require SGIs to have higher
priority than PPIs or SPIs. My reference to “best practices” is based on
how Xen typically configures SGIs with higher priority during initialization.
This is not a strict requirement, but it helps maintain interrupt precedence
in a way that aligns with established implementations.

If needed, PPIs or SPIs could be assigned higher priority depending on
system requirements.

>
> >
> > Signed-off-by: Mykola Kvach <mykola_kvach@xxxxxxxx>
> > ---
> >   xen/arch/arm/irq.c | 8 +++++++-
> >   1 file changed, 7 insertions(+), 1 deletion(-)
> >
> > diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
> > index 02ca82c089..17c7ac92b5 100644
> > --- a/xen/arch/arm/irq.c
> > +++ b/xen/arch/arm/irq.c
> > @@ -397,7 +397,13 @@ int setup_irq(unsigned int irq, unsigned int irqflags, 
> > struct irqaction *new)
> AFAIK, we are not using setup_irq() to handle SGIs because they are all
> static and always enabled. Are you planning to handle dynamic SGIs? If
> yes, then can you provide more details?As far as I know, there can be at 
> least one “dynamic” SGI in Xen.

As far as I know, there is at least one “dynamic” SGI in Xen. For
example, see ffa_notif.c in the functions ffa_notif_init_interrupt
and ffa_notif_init, which handle initialization of such SGIs.

>
> >       /* First time the IRQ is setup */
> >       if ( disabled )
> >       {
> > -        gic_route_irq_to_xen(desc, GIC_PRI_IRQ);
> > +        unsigned int prio = GIC_PRI_IRQ;
> > +
> > +        /* Use appropriate priority based on interrupt type */
> > +        if (desc->irq < NR_GIC_SGI)
> > +            prio = GIC_PRI_IPI;
>
> I am a bit split with this change. I feel static SGI (e.g. EVENT_CHECK,
> CALL_FUNCTION) should have higher priority to the dynamic SGIs because
> they are critical for Xen.

That’s a good point. My intention was to follow the general approach of
assigning higher priority to SGIs, as done during GIC initialization.

>
> Before making my mind, I would like to understand a bit more the use case.
>
> Cheers,
>
> --
> Julien Grall
>

Best regards,
Mykola