Re: [PATCH v2 10/39] xen/riscv: build the target hart index via aplic_hart_field()

[Jan Beulich <jbeulich@xxxxxxxx>]

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