Re: [PATCH v2 32/39] xen/riscv: remap interrupts to new IMSIC VS-file

[Jan Beulich <jbeulich@xxxxxxxx>]

On 21.09.2026 10:03, Oleksii Kurochko wrote:
> On 9/14/26 5:02 PM, Jan Beulich wrote:
>> On 27.08.2026 17:21, Oleksii Kurochko wrote:
>>> @@ -242,6 +254,15 @@ void imsic_irq_disable(unsigned int irq)
>>>       spin_unlock(&imsic_cfg.lock);
>>>   }
>>>   
>>> +static bool imsic_local_is_pending(unsigned int id)
>>> +{
>>> +    unsigned long isel =
>>> +        (id / BITS_PER_LONG) * (BITS_PER_LONG / IMSIC_EIPx_BITS) + 
>>> IMSIC_EIP0;
>>> +    unsigned long bit = BIT(id % BITS_PER_LONG, UL);
>>
>> Both can be unsigned int, can't they?
> 
> Yes, agreed. Both isel and bit fit within unsigned int. I will update 
> them in the next version.
> 
>>
>>> +    return !!(imsic_csr_read(isel) & bit);
>>> +}
>>
>> No need for !! here.
>>
>> What about endianness, btw? Does the IMSIC always match the CPU (and
>> its setting)?
> 
> No, the IMSIC does not dynamically adapt its register interfaces based 
> on the CPU's runtime endianness configuration (e.g. mstatus.SBE/MBE):
> 
> - CSR Accesses (imsic_csr_read): Indirect CSR accesses (siselect/sireg 
> or miselect/mireg) operate using standard RISC-V CSR instructions at 
> current XLEN width. Values are read and written directly into 
> architectural GPRs without byte-swapping.

I.e. you need you add endianness conversion.

>> Overall, what does "local" in the function name signify? (For a static
>> function, the "imsic" prefix may also be unnecessary.)
> 
> It signifies that local (on which code is executed now) hart's IMSIC 
> CSRs (isel and ireg in the case of imsic_csr_read()) are touched.

That's the expected thing for CSR access, though.

>>> @@ -848,8 +899,22 @@ void imsic_migrate_vcpu(struct vcpu *v)
>>>       if ( v->arch.last_cpu == NR_CPUS )
>>>           return;
>>>   
>>> +    read_lock_irqsave(&imsic_state->vsfile_lock, flags);
>>> +    old_vsfile_id = imsic_state->guest_file_id;
>>> +    old_vsfile_cpu = imsic_state->vsfile_cpu;
>>> +    read_unlock_irqrestore(&imsic_state->vsfile_lock, flags);
>>> +
>>> +    /*
>>> +     * We don't support SW interrupt files at the moment. Bail out before
>>> +     * anything is touched, as the old file has no owning pCPU in that case
>>> +     * and there is nothing to retarget the producers away from.
>>> +     */
>>> +    if ( old_vsfile_cpu == NR_CPUS )
>>> +        panic("IMSIC SW-file isn't supported\n");
>>> +
>>>       /*
>>>        * At this point, all interrupt producers are still using the old 
>>> IMSIC
>>> +     * VS-file so we first move all interrupt producers to the new IMSIC
>>>        * VS-file.
>>>        */
>>
>> Isn't the new part of the comment premature? Moving doesn't start until ...
>>
>>> @@ -870,5 +935,43 @@ void imsic_migrate_vcpu(struct vcpu *v)
>>>       /* Zero-out new IMSIC VS-file */
>>>       imsic_call_on_cpu(new_vsfile_cpu, imsic_vsfile_local_clear, 
>>> &vsfile_data);
>>>   
>>> +    /* Update G-stage mapping for the new IMSIC VS-file */
>>> +    if ( imsic_map_guest_file(v, new_vsfile_hgei) )
>>> +    {
>>> +        domain_crash(v->domain, "Migration to hw interrupt file failed\n");
>>> +
>>> +        return;
>>> +    }
>>> +
>>> +    imsic_update_state(v, new_vsfile_hgei);
>>> +
>>> +    /*
>>> +     * TODO: Modify the relevant translation tables at all IOMMUs so that 
>>> MSIs
>>> +     *       for this virtual interrupt file are now sent to the new 
>>> physical
>>> +     *       interrupt file.
>>> +     */
>>> +    if ( iommu_enabled )
>>> +        printk_once("IMSIC: IOMMU MSI retargeting is not implemented\n");
>>> +
>>> +    /*
>>> +     * If any interrupts at an APLIC are forwarded by MSIs to the old 
>>> interrupt
>>> +     * file, reconfigure the APLIC to send them to the new interrupt file.
>>> +     */
>>> +    aplic_reconfigure_target(v, old_vsfile_id, old_vsfile_cpu);
>>
>> ... here, as it looks.
> 
> At some point I agree but the steps before are preparation of moving and 
> is a part of moving process.
> 
> Then probably it make sense to reword the comment to:
> 
>     /*
>       * At this point, all interrupt producers are still using the old IMSIC
>       * VS-file.  Allocate and clear the new one before redirecting anything
>       * to it.
>       */
> 
> Would it be better?

Imo yes.

Jan