Re: [PATCH v3 11/14] xen/riscv: implementation of aplic and imsic operations

[Jan Beulich <jbeulich@xxxxxxxx>]

On 21.05.2025 18:03, Oleksii Kurochko wrote:
> +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;

Nit: Comma vs blank placement.

> +    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));
> +
> +    ASSERT(spin_is_locked(&desc->lock));
> +
> +    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);

Nit: Line length.

I'm also puzzled by the various uses of IMSIC_MMIO_PAGE_SHIFT. Why do you
subtract double the value when calculating hhxs, just to add the value
back in here? There's no other usee of the variable afaics.

> +    value = desc->irq;
> +    value |= cpu << APLIC_TARGET_HART_IDX_SHIFT;
> +    value |= group_index << (lhxw + APLIC_TARGET_HART_IDX_SHIFT) ;

Nit: Stray blank.

> +    spin_lock(&aplic.lock);
> +
> +    writel(value, &aplic.regs->target[desc->irq - 1]);
> +
> +    spin_unlock(&aplic.lock);
> +}
> +
> +static const hw_irq_controller aplic_xen_irq_type = {
> +    .typename     = "aplic",
> +    .startup      = aplic_irq_startup,
> +    .shutdown     = aplic_irq_disable,
> +    .enable       = aplic_irq_enable,
> +    .disable      = aplic_irq_disable,
> +    .set_affinity = aplic_set_irq_affinity,

As indicated before, for functions you use as hooks you want to save
yourself (or someone else) future work by marking them cf_check right
away.

> --- a/xen/arch/riscv/imsic.c
> +++ b/xen/arch/riscv/imsic.c
> @@ -22,7 +22,124 @@
>  
>  #include <asm/imsic.h>
>  
> -static struct imsic_config imsic_cfg;
> +static struct imsic_config imsic_cfg = {
> +    .lock = SPIN_LOCK_UNLOCKED,
> +};
> +
> +#define IMSIC_DISABLE_EIDELIVERY    0
> +#define IMSIC_ENABLE_EIDELIVERY     1
> +#define IMSIC_DISABLE_EITHRESHOLD   1
> +#define IMSIC_ENABLE_EITHRESHOLD    0
> +
> +#define imsic_csr_write(c, v)   \
> +do {                            \
> +    csr_write(CSR_SISELECT, c); \
> +    csr_write(CSR_SIREG, v);    \
> +} while (0)
> +
> +#define imsic_csr_set(c, v)     \
> +do {                            \
> +    csr_write(CSR_SISELECT, c); \
> +    csr_set(CSR_SIREG, v);      \
> +} while (0)
> +
> +#define imsic_csr_clear(c, v)   \
> +do {                            \
> +    csr_write(CSR_SISELECT, c); \
> +    csr_clear(CSR_SIREG, v);    \
> +} while (0)
> +
> +void __init imsic_ids_local_delivery(bool enable)
> +{
> +    if ( enable )
> +    {
> +        imsic_csr_write(IMSIC_EITHRESHOLD, IMSIC_ENABLE_EITHRESHOLD);
> +        imsic_csr_write(IMSIC_EIDELIVERY, IMSIC_ENABLE_EIDELIVERY);
> +    }
> +    else
> +    {
> +        imsic_csr_write(IMSIC_EITHRESHOLD, IMSIC_DISABLE_EITHRESHOLD);
> +        imsic_csr_write(IMSIC_EIDELIVERY, IMSIC_DISABLE_EIDELIVERY);
> +    }
> +}
> +
> +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;

Any reason you open-code min() here?

> +        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)
> +{
> +    /*
> +    * The only caller of imsic_irq_enable() is aplic_irq_enable(), which
> +    * already runs with IRQs disabled. Therefore, there's no need to use
> +    * spin_lock_irqsave() in this function.
> +    *
> +    * This ASSERT is added as a safeguard: if imsic_irq_enable() is ever
> +    * called from a context where IRQs are not disabled,
> +    * spin_lock_irqsave() should be used instead of spin_lock().
> +    */
> +    ASSERT(!local_irq_is_enabled());
> +
> +    spin_lock(&imsic_cfg.lock);
> +    /*
> +     * 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.
> +     */
> +    imsic_local_eix_update(irq, 1, false, true);
> +    spin_unlock(&imsic_cfg.lock);
> +}
> +
> +void imsic_irq_disable(unsigned int irq)
> +{
> +   /*
> +    * The only caller of imsic_irq_disable() is aplic_irq_enable(), which

s/enable/disable/ ?

Jan