Re: [XEN PATCH v2 4/5] xen/arm: allow dynamically assigned SGI handlers

[Julien Grall <julien@xxxxxxx>]

Hi Jens,

On 22/04/2024 08:37, Jens Wiklander wrote:
> Updates so request_irq() can be used with a dynamically assigned SGI irq
> as input. This prepares for a later patch where an FF-A schedule
> receiver interrupt handler is installed for an SGI generated by the
> secure world.
I would like to understand the use-case a bit more. Who is responsible 
to decide the SGI number? Is it Xen or the firmware?
If the later, how can we ever guarantee the ID is not going to clash 
with what the OS/hypervisor is using? Is it described in a 
specification? If so, please give a pointer.
> gic_route_irq_to_xen() don't gic_set_irq_type() for SGIs since they are
> always edge triggered.
>
> gic_interrupt() is updated to route the dynamically assigned SGIs to
> do_IRQ() instead of do_sgi(). The latter still handles the statically
> assigned SGI handlers like for instance GIC_SGI_CALL_FUNCTION.
>
> Signed-off-by: Jens Wiklander <jens.wiklander@xxxxxxxxxx>
> ---
> v1->v2
> - Update patch description as requested
> ---
>   xen/arch/arm/gic.c | 5 +++--
>   xen/arch/arm/irq.c | 7 +++++--
I am not sure where to write the comment. But I think the comment on top 
of irq_set_affinity() in setup_irq() should also be updated.
>   2 files changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> index 44c40e86defe..e9aeb7138455 100644
> --- a/xen/arch/arm/gic.c
> +++ b/xen/arch/arm/gic.c
> @@ -117,7 +117,8 @@ void gic_route_irq_to_xen(struct irq_desc *desc, unsigned 
> int priority)
>   
>       desc->handler = gic_hw_ops->gic_host_irq_type;
>   
> -    gic_set_irq_type(desc, desc->arch.type);
> +    if ( desc->irq >= NR_GIC_SGI)
> +        gic_set_irq_type(desc, desc->arch.type);
So above, you say that the SGIs are always edge-triggered interrupt. So 
I assume desc->arch.type. So are you skipping the call because it is 
unnessary or it could do the wrong thing?
Ideally, the outcome of the answer be part of the comment on top of the 
check.
>       gic_set_irq_priority(desc, priority);
>   }
>   
> @@ -375,7 +376,7 @@ void gic_interrupt(struct cpu_user_regs *regs, int is_fiq)
>           /* Reading IRQ will ACK it */
>           irq = gic_hw_ops->read_irq();
>   
> -        if ( likely(irq >= 16 && irq < 1020) )
> +        if ( likely(irq >= GIC_SGI_MAX && irq < 1020) )
This check is now rather confusing as one could think that do_IRQ() 
would still not be reached for dynamic SGI. I think it would be clearer 
GIC_SGI_MAX needs to be renamed to GIC_SGI_STATIC_MAX and do_sgi() to 
do_static_sgi().
>           {
>               isb();
>               do_IRQ(regs, irq, is_fiq);
> diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
> index bcce80a4d624..fdb214560978 100644
> --- a/xen/arch/arm/irq.c
> +++ b/xen/arch/arm/irq.c
> @@ -224,9 +224,12 @@ void do_IRQ(struct cpu_user_regs *regs, unsigned int irq, 
> int is_fiq)
>   
>       perfc_incr(irqs);
>   
> -    ASSERT(irq >= 16); /* SGIs do not come down this path */
> +    /* Statically assigned SGIs do not come down this path */
> +    ASSERT(irq >= GIC_SGI_MAX);

With this change, I think the path with vgic_inject_irq() now needs to 
gain an ASSERT(irq >= NR_GIC_SGI) because the path is not supposed to be 
taken for SGIs.
>   
> -    if ( irq < 32 )
> +    if ( irq < NR_GIC_SGI )
> +        perfc_incr(ipis);
> +    else if ( irq < NR_GIC_LOCAL_IRQS )
>           perfc_incr(ppis);
>       else
>           perfc_incr(spis);
Cheers,

--
Julien Grall