Re: [PATCH v1 05/14] xen/riscv: introduce platform_get_irq()

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 4/10/25 5:35 PM, Jan Beulich wrote:

> On 08.04.2025 17:57, Oleksii Kurochko wrote:
> > @@ -21,6 +22,22 @@ static struct intc_info __ro_after_init aplic_info = {
> >      .hw_version = INTC_APLIC,
> >  };
> >  
> > +static int aplic_irq_xlate(const uint32_t *intspec, unsigned int intsize,
> >       
> As you start adding functions calling indirectly, please consider adding cf_check
> right away, even if right now this has no effect on RISC-V. That'll save you from
> going through the entire RISC-V subtree later on to find them all.
>     
Sure. I thought that it is a feature for x86 as I haven't seen such attribute for
Arm and RISC-V in GCC manuals.

> > +                           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 +52,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;
> >  }
> >  
> > --- a/xen/arch/riscv/include/asm/irq.h
> > +++ b/xen/arch/riscv/include/asm/irq.h
> > @@ -47,6 +47,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);
> >       
> And I assume callers of this will appear later in the series.
>     
Yes, it will be called ns16550_uart_dt_init() when CONFIG_NS16550 will be enabled for RISC-V.

> > --- a/xen/arch/riscv/irq.c
> > +++ b/xen/arch/riscv/irq.c
> > @@ -7,11 +7,52 @@
> >   */
> >  
> >  #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 );
> > +}
> > +
> > +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:
> >       
> Labels indented by at least one blank please.
>
> > +    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;
> > +
> > +    if ( dt_device_get_irq(device, index, &dt_irq) )
> > +        return -1;
> > +
> > +    if ( irq_set_type(dt_irq.irq, dt_irq.type) )
> > +        return -1;
> >       
> Can you please return proper -E... values, perhaps ones coming back from the
> functions called?
>     
Sure, I will use -EINVAL. (or ,perhaps, it  will be better to introduce ret and
return what dt_device_get_irq()/irq_set_type() returns in the case of failure.

~ Oleksii