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
