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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 06.05.2025 18:51, Oleksii Kurochko wrote:
> --- a/xen/arch/riscv/aplic-priv.h
> +++ b/xen/arch/riscv/aplic-priv.h
> @@ -14,6 +14,7 @@
>  #ifndef ASM__RISCV_PRIV_APLIC_H
>  #define ASM__RISCV_PRIV_APLIC_H
>  
> +#include <xen/spinlock.h>
>  #include <xen/types.h>
>  
>  #include <asm/aplic.h>
> @@ -27,6 +28,9 @@ struct aplic_priv {
>      /* registers */
>      volatile struct aplic_regs *regs;
>  
> +    /* lock */
> +    spinlock_t lock;

Nit: I don't see much value in such entirely redundant comments. Useful
contents might be to say what the lock actually is intended to guard.

> @@ -119,9 +121,118 @@ static int __init cf_check aplic_init(void)
>      return 0;
>  }
>  
> +static void aplic_irq_enable(struct irq_desc *desc)
> +{
> +    unsigned long flags;
> +
> +    /*
> +     * TODO: Currently, APLIC is supported only with MSI interrupts.
> +     *       If APLIC without MSI interrupts is required in the future,
> +     *       this function will need to be updated accordingly.
> +     */
> +    ASSERT(readl(&aplic.regs->domaincfg) & APLIC_DOMAINCFG_DM);
> +
> +    ASSERT(spin_is_locked(&desc->lock));

Iirc I said so before: This being an IRQ-safe lock, ...

> +    spin_lock_irqsave(&aplic.lock, flags);

... there's no need to use spin_lock_irqsave() here; plain spin_lock()
will do (and avoid the need for the local variable). Same elsewhere,
obviously.

> +static void aplic_set_irq_affinity(struct irq_desc *desc, const cpumask_t 
> *mask)
> +{
> +    unsigned int cpu;
> +    uint64_t group_index, base_ppn;
> +    uint32_t hhxw, lhxw ,hhxs, value;
> +    const struct imsic_config *imsic = aplic.imsic_cfg;
> +
> +    /*
> +     * TODO: Currently, APLIC is supported only with MSI interrupts.
> +     *       If APLIC without MSI interrupts is required in the future,
> +     *       this function will need to be updated accordingly.
> +     */
> +    ASSERT(readl(&aplic.regs->domaincfg) & APLIC_DOMAINCFG_DM);
> +
> +    ASSERT(!cpumask_empty(mask));
> +
> +    cpu = cpuid_to_hartid(aplic_get_cpu_from_mask(mask));
> +    hhxw = imsic->group_index_bits;
> +    lhxw = imsic->hart_index_bits;
> +    hhxs = imsic->group_index_shift - IMSIC_MMIO_PAGE_SHIFT * 2;
> +    base_ppn = imsic->msi[cpu].base_addr >> IMSIC_MMIO_PAGE_SHIFT;
> +
> +    /* Update hart and EEID in the target register */
> +    group_index = (base_ppn >> (hhxs + IMSIC_MMIO_PAGE_SHIFT)) & (BIT(hhxw, 
> UL) - 1);
> +    value = desc->irq;
> +    value |= cpu << APLIC_TARGET_HART_IDX_SHIFT;
> +    value |= group_index << (lhxw + APLIC_TARGET_HART_IDX_SHIFT) ;
> +
> +    spin_lock(&aplic.lock);
> +
> +    writel(value, &aplic.regs->target[desc->irq - 1]);
> +
> +    spin_unlock(&aplic.lock);

Hmm, interesting, here you use the plain functions, but there's no
assertion as to desc->lock being held. Such aspects would better be
consistent throughout all hooks.

> @@ -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?

> +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?

> +     */
> +    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.

> @@ -274,6 +373,11 @@ int __init imsic_init(const struct dt_device_node *node)
>          goto imsic_init_err;
>      }
>  
> +    spin_lock_init(&imsic_cfg.lock);

Again - suitable initializer for the field instead?

> --- a/xen/arch/riscv/include/asm/aplic.h
> +++ b/xen/arch/riscv/include/asm/aplic.h
> @@ -1,7 +1,7 @@
>  /* SPDX-License-Identifier: MIT */
>  
>  /*
> - * xen/arch/riscv/asm/include/aplic.h
> + * xen/arch/riscv/aplic.h

Please get this right when/where the file is introduced.

Jan