On 10.09.2026 14:44, Oleksii Kurochko wrote: > > > On 9/10/26 1:23 PM, Jan Beulich wrote: >> On 10.09.2026 12:59, Oleksii Kurochko wrote: >>> On 9/9/26 4:52 PM, Jan Beulich wrote: >>>> On 27.08.2026 17:20, Oleksii Kurochko wrote: >>>>> @@ -340,27 +338,11 @@ static void cf_check aplic_set_irq_affinity(struct >>>>> irq_desc *desc, const cpumask >>>>> >>>>> 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; >>>>> - /* >>>>> - * Although this variable is used only once in the calculation of >>>>> - * group_index, and it might seem that hhxs could be defined as: >>>>> - * hhxs = imsic->group_index_shift - IMSIC_MMIO_PAGE_SHIFT; >>>>> - * and then the addition of IMSIC_MMIO_PAGE_SHIFT could be omitted >>>>> - * when calculating the group index. >>>>> - * It was done intentionally this way to follow the formula from >>>>> - * the AIA specification for calculating the MSI address. >>>>> - */ >>>>> - 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; >>>> >>>> Hmm, only after sending the ack I noticed that there's no masking here, ... >>>> >>>>> - value |= cpu << APLIC_TARGET_HART_IDX_SHIFT; >>>>> - value |= group_index << (lhxw + APLIC_TARGET_HART_IDX_SHIFT); >>>>> + cpu = aplic_get_cpu_from_mask(mask); >>>>> + >>>>> + /* Update hart index and EIID in the target register */ >>>>> + value = MASK_INSR(aplic_hart_field(cpu), APLIC_TARGET_HART_IDX) | >>>>> + (desc->irq & APLIC_TARGET_EIID); >>>> >>>> ... but there is masking here. Chopping off bits doesn't look as if it can >>>> lead to anything good. What's the deal here? >>> >>> The mask is a no-op: desc->irq < NR_IRQS (1024) always fits the 11-bit >>> EIID field, so I'll drop it and add a BUILD_BUG_ON() instead. >>> >>> Would it be better to: >>> >>> + /* desc->irq < NR_IRQS, so it always fits the EIID field */ >>> + BUILD_BUG_ON(NR_IRQS - 1 > APLIC_TARGET_EIID); >> >> If the question is whether to prefer BUILD_BUG_ON() over BUG_ON(), then: >> Yes please. However, APLIC_TARGET_EIID is a mask (despite its name not >> indicating that), which only happens to start at bit 0. This kind of >> assumption would better be avoided. > > Then BUILD_BUG_on should be updated to: > > /* desc->irq < NR_IRQS, so it always fits the EIID field */ > BUILD_BUG_ON(NR_IRQS - 1 > MASK_EXTR(~0U, APLIC_TARGET_EIID)); > > >> >> This raises another question though: No matter how big a RISC-V system >> is, it can only ever have 1k IRQs? How does that work with a single >> MSI-X device having up to 2k MSIs? > > Device MSIs never pass through the APLIC. They are written straight into > an IMSIC interrupt file. The ID space belongs to each file, so each hart > has up to 2047 IDs (IMSIC_MAX_ID). > > 1k it is limitation for wired interrupts (which could be delivered in > MSI mode where APLIC + IMSIC is needed) which are going through APLIC.
But desc->irq and NR_IRQS have to represent both. Which then puts the BUILD_BUG_ON() above under question. Jan
