Re: [PATCH v2 07/16] xen/riscv: introduce platform_get_irq()

[Jan Beulich <jbeulich@xxxxxxxx>]

On 06.05.2025 18:51, Oleksii Kurochko wrote:
> platform_get_irq() recieves information about device's irq ( type
> and irq number ) from device tree node and using this information
> update irq descriptor in irq_desc[] array.
> 
> Introduce dt_irq_xlate and initialize with aplic_irq_xlate() as
> it is used by dt_device_get_irq() which is called by
> platform_get_irq().
> 
> Co-developed-by: Romain Caritey <Romain.Caritey@xxxxxxxxxxxxx>
> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>
> ---
> Changes in V2:
>  - Add cf_check for aplic_irq_xlate().
>  - Ident label in irq_set_type().
>  - Return proper -E... values for platform_get_irq().
> ---
>  xen/arch/riscv/aplic.c           | 20 +++++++++++++++
>  xen/arch/riscv/include/asm/irq.h |  3 +++
>  xen/arch/riscv/irq.c             | 42 ++++++++++++++++++++++++++++++++
>  3 files changed, 65 insertions(+)
> 
> diff --git a/xen/arch/riscv/aplic.c b/xen/arch/riscv/aplic.c
> index caba8f8993..10ae81f7ac 100644
> --- a/xen/arch/riscv/aplic.c
> +++ b/xen/arch/riscv/aplic.c
> @@ -11,6 +11,7 @@
>  
>  #include <xen/errno.h>
>  #include <xen/init.h>
> +#include <xen/irq.h>
>  #include <xen/sections.h>
>  #include <xen/types.h>
>  
> @@ -21,6 +22,23 @@ static struct intc_info __ro_after_init aplic_info = {
>      .hw_version = INTC_APLIC,
>  };
>  
> +static int cf_check aplic_irq_xlate(const uint32_t *intspec,
> +                                    unsigned int intsize,
> +                                    unsigned int *out_hwirq,
> +                                    unsigned int *out_type)
> +{
> +    if ( intsize < 2 )
> +        return -EINVAL;
> +
> +    /* Mapping 1:1 */
> +    *out_hwirq = intspec[0];
> +
> +    if ( out_type )
> +        *out_type = intspec[1] & IRQ_TYPE_SENSE_MASK;
> +
> +    return 0;
> +}
> +
>  static int __init aplic_preinit(struct dt_device_node *node, const void *dat)
>  {
>      if ( aplic_info.node )
> @@ -35,6 +53,8 @@ static int __init aplic_preinit(struct dt_device_node 
> *node, const void *dat)
>  
>      aplic_info.node = node;
>  
> +    dt_irq_xlate = aplic_irq_xlate;
> +
>      return 0;
>  }
>  
> diff --git a/xen/arch/riscv/include/asm/irq.h 
> b/xen/arch/riscv/include/asm/irq.h
> index f609df466e..6223bbbed5 100644
> --- a/xen/arch/riscv/include/asm/irq.h
> +++ b/xen/arch/riscv/include/asm/irq.h
> @@ -30,6 +30,9 @@ static inline void arch_move_irqs(struct vcpu *v)
>      BUG_ON("unimplemented");
>  }
>  
> +struct dt_device_node;
> +int platform_get_irq(const struct dt_device_node *device, int index);
> +
>  void init_IRQ(void);
>  
>  #endif /* ASM__RISCV__IRQ_H */
> diff --git a/xen/arch/riscv/irq.c b/xen/arch/riscv/irq.c
> index 26a8556b2c..4c518bbd97 100644
> --- a/xen/arch/riscv/irq.c
> +++ b/xen/arch/riscv/irq.c
> @@ -7,11 +7,53 @@
>   */
>  
>  #include <xen/bug.h>
> +#include <xen/device_tree.h>
> +#include <xen/errno.h>
>  #include <xen/init.h>
>  #include <xen/irq.h>
>  
>  static irq_desc_t irq_desc[NR_IRQS];
>  
> +static bool irq_validate_new_type(unsigned int curr, unsigned int new)
> +{
> +    return (curr == IRQ_TYPE_INVALID || curr == new );

Nit: Stray blank. In fact you could omit the parentheses as well.

> +}
> +
> +static int irq_set_type(unsigned int irq, unsigned int type)
> +{
> +    unsigned long flags;
> +    struct irq_desc *desc = irq_to_desc(irq);
> +    int ret = -EBUSY;
> +
> +    spin_lock_irqsave(&desc->lock, flags);
> +
> +    if ( !irq_validate_new_type(desc->arch.type, type) )
> +        goto err;
> +
> +    desc->arch.type = type;
> +
> +    ret = 0;
> +
> + err:
> +    spin_unlock_irqrestore(&desc->lock, flags);
> +
> +    return ret;
> +}
> +
> +int platform_get_irq(const struct dt_device_node *device, int index)
> +{
> +    struct dt_irq dt_irq;
> +    int ret;
> +
> +    if ( (ret = dt_device_get_irq(device, index, &dt_irq)) != 0 )
> +        return ret;
> +
> +    if ( (ret = irq_set_type(dt_irq.irq, dt_irq.type)) != 0 )
> +        return ret;
> +
> +    return dt_irq.irq;

What guarantees the value to be at most INT_MAX (i.e. no silent conversion to
a negative value, signaling an error to the caller)? Actually, looking at
irq_set_type(), what guarantees irq_to_desc() there to not overrun irq_desc[]?
There are no bounds checks in aplic_irq_xlate().

Jan