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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 27.08.2026 17:21, Oleksii Kurochko wrote:
> @@ -60,6 +61,12 @@ static unsigned int __ro_after_init guest_num_msis;
>  #define IMSIC_DISABLE_EITHRESHOLD   1
>  #define IMSIC_ENABLE_EITHRESHOLD    0
>  
> +#define imsic_csr_read(c)           \
> +({                                  \
> +    csr_write(CSR_SISELECT, (c));   \

Nit: Excess parentheses again.

> @@ -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?

> +    return !!(imsic_csr_read(isel) & bit);
> +}

No need for !! here.

What about endianness, btw? Does the IMSIC always match the CPU (and
its setting)?

Overall, what does "local" in the function name signify? (For a static
function, the "imsic" prefix may also be unnecessary.)

> @@ -497,6 +523,27 @@ static void imsic_call_on_cpu(unsigned int cpu, void 
> (*func)(void *),
>          on_selected_cpus(cpumask_of(cpu), func, data, 1);
>  }
>  
> +/*
> + * Ensure that all the MSIs the APLIC has already generated for the hart this
> + * runs on have really reached the hart's IMSIC.
> + *
> + * The barrier is the one described by the AIA specification in
> + * "Synchronizing interactions between a hart and the APLIC": ask the APLIC 
> to
> + * send an MSI to the hart itself and wait until it shows up as pending in 
> the
> + * hart's own interrupt file. As it says nothing about MSIs on their way to
> + * any other hart, it has to be executed by the pCPU owning the interrupt 
> file
> + * the MSIs were being sent to.
> + */
> +static void cf_check imsic_aplic_sync(void *data)

If the parameter isn't used, maybe best to name it "unused"?

> @@ -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.

Jan