Re: [PATCH v2 13/16] xen/riscv: implementation of aplic and imsic operations

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 5/15/25 11:44 AM, Jan Beulich wrote:

> > @@ -159,6 +270,8 @@ static int __init aplic_preinit(struct dt_device_node *node, const void *dat)
> >  
> >      dt_irq_xlate = aplic_irq_xlate;
> >  
> > +    spin_lock_init(&aplic.lock);
> >       
> Can't you have the struct field have a suitable initializer?
>     
Sure, I will use struct initializer:
  static struct aplic_priv aplic = {
      .lock = SPIN_LOCK_UNLOCKED,
  };

> > +static void imsic_local_eix_update(unsigned long base_id, unsigned long num_id,
> > +                                   bool pend, bool val)
> > +{
> > +    unsigned long id = base_id, last_id = base_id + num_id;
> > +
> > +    while ( id < last_id )
> > +    {
> > +        unsigned long isel, ireg;
> > +        unsigned long start_id = id & (__riscv_xlen - 1);
> > +        unsigned long chunk = __riscv_xlen - start_id;
> > +        unsigned long count = (last_id - id < chunk) ? last_id - id : chunk;
> > +
> > +        isel = id / __riscv_xlen;
> > +        isel *= __riscv_xlen / IMSIC_EIPx_BITS;
> > +        isel += pend ? IMSIC_EIP0 : IMSIC_EIE0;
> > +
> > +        ireg = GENMASK(start_id + count - 1, start_id);
> > +
> > +        id += count;
> > +
> > +        if ( val )
> > +            imsic_csr_set(isel, ireg);
> > +        else
> > +            imsic_csr_clear(isel, ireg);
> > +    }
> > +}
> > +
> > +void imsic_irq_enable(unsigned int irq)
> > +{
> > +    unsigned long flags;
> > +
> > +    spin_lock_irqsave(&imsic_cfg.lock, flags);
> > +    /*
> > +     * There is no irq - 1 here (look at aplic_set_irq_type()) because:
> > +     * From the spec:
> > +     *   When an interrupt file supports distinct interrupt identities,
> > +     *   valid identity numbers are between 1 and inclusive. The identity
> > +     *   numbers within this range are said to be implemented by the interrupt
> > +     *   file; numbers outside this range are not implemented. The number zero
> > +     *   is never a valid interrupt identity.
> > +     *   ...
> > +     *   Bit positions in a valid eiek register that don’t correspond to a
> > +     *   supported interrupt identity (such as bit 0 of eie0) are read-only zeros.
> > +     *
> > +     * So in EIx registers interrupt i corresponds to bit i in comparison wiht
> > +     * APLIC's sourcecfg which starts from 0. (l)
> >       
> What's this 'l' in parentheses here to indicate?
>     
I don't really remember, it seems like I want to point to the spec, but
then just make a quote from the spec instead. I'll just drop it.

> > +     */
> > +    imsic_local_eix_update(irq, 1, false, true);
> > +    spin_unlock_irqrestore(&imsic_cfg.lock, flags);
> > +}
> > +
> > +void imsic_irq_disable(unsigned int irq)
> > +{
> > +    unsigned long flags;
> > +
> > +    spin_lock_irqsave(&imsic_cfg.lock, flags);
> > +    imsic_local_eix_update(irq, 1, false, false);
> > +    spin_unlock_irqrestore(&imsic_cfg.lock, flags);
> > +}
> >       
> The sole caller of the function has doubly turned off IRQs already; perhaps
> no need to it a 3rd time, unless other callers are to appear? Same for
> imsic_irq_enable() as it looks.
>     
I checked a code in private branches and it seems like these functions are called
only in aplic_irq_{enable,disable}(), so we could do, at least,spin_lock(&imsic_cfg.lock)
+ ASSERT(!local_irq_is_enabled());

~ Oleksii