Re: [PATCH v2 36/39] xen/riscv: wake up a descheduled vCPU on a guest external interrupt

[Jan Beulich <jbeulich@suse.com>]

On 27.08.2026 17:21, Oleksii Kurochko wrote:
> @@ -62,23 +69,25 @@ static int cf_check cpu_callback(struct notifier_block *nfb,
>                                   unsigned long action, void *hcpu)
>  {
>      unsigned int cpu = (unsigned long)hcpu;
> -    int rc = 0;
>  
>      switch ( action )
>      {
>      case CPU_STARTING:
> -        rc = vgein_init();
> +    {
> +        int rc = vgein_init();
> +
>          if ( rc )
>              printk(XENLOG_ERR "AIA: failed to init vgein for CPU%u: %d\n",
>                     cpu, rc);
>          break;
> +    }
>  
>      case CPU_DYING:
>          vgein_deinit();
>          break;
>      }
>  
> -    return notifier_from_errno(rc);
> +    return NOTIFY_DONE;
>  }

What is this hunk doing in this patch? Was this meant to be merged into the
prior one? But then - why?

> @@ -168,3 +181,36 @@ void vgein_release(struct vcpu *v, unsigned int vgein_id, unsigned int cpu)
>              __func__, v, vgein_id, cpu, vgein->bmp);
>  #endif
>  }
> +
> +void hgei_interrupt(void)
> +{
> +    unsigned long hgei_mask, flags;
> +    struct vgein_ctrl *vgein = &this_cpu(vgein);
> +
> +    hgei_mask = csr_read(CSR_HGEIP) & csr_read(CSR_HGEIE);

Misra, aiui, isn't going to like this. You may want to split it up.

> +    csr_clear(CSR_HGEIE, hgei_mask);
> +
> +    spin_lock_irqsave(&vgein->lock, flags);
> +
> +    for_each_set_bit ( guest_file_id, hgei_mask )
> +    {
> +        /*
> +         * guest_file_id shouldn't be zero, as it will indicate that no
> +         * guest external interrupt source is selected for VS-level external
> +         * interrupts.
> +         */
> +        ASSERT(guest_file_id);

While it only affects debug builds, this check still needlessly is
done on every loop iteration, when doing it once ahead of the loop
would suffice.

> +        if ( vgein->owners[guest_file_id] )
> +        {
> +#ifdef VGEIN_DEBUG
> +            gprintk(XENLOG_DEBUG, "%s: kick ->%pv, hgei_mask(%#lx)\n",
> +                    __func__, vgein->owners[guest_file_id], hgei_mask);
> +#endif

This can ocur very frequently (when VGEIN_DEBUG is defined). A
trace record may be a better alternative.

> --- a/xen/arch/riscv/domain.c
> +++ b/xen/arch/riscv/domain.c
> @@ -136,6 +136,8 @@ static void vcpu_csr_init(struct vcpu *v)
>          v->arch.hstateen0 = (hstateen0 & csr_masks.hstateen0) |
>                              csr_masks.ro_one.hstateen0;
>      }
> +
> +    v->arch.hie = MIP_SGEIP;

Neither part of the rhs identifier has anything to do with the CSR
having its default value set here. That's perhaps again a piece of
RISC-V I'm missing, but I can't make sense of this.

> --- a/xen/arch/riscv/imsic.c
> +++ b/xen/arch/riscv/imsic.c
> @@ -510,12 +510,31 @@ void cf_check imsic_ctxt_switch_from(struct vcpu *v)
>  
>      write_lock_irqsave(&imsic_state->vsfile_lock, flags);
>      imsic_state->vsfile_cpu = v->processor;
> +    /*
> +     * Start to observe the VS-file from HS-mode: while the vCPU isn't
> +     * running an interrupt pending in its VS-file is reported through HGEIP
> +     * instead of being delivered to VS-mode, which lets Xen wake the vCPU up.
> +     */
> +    csr_set(CSR_HGEIE, BIT(imsic_state->guest_file_id, UL));
>      write_unlock_irqrestore(&imsic_state->vsfile_lock, flags);
>  }

It is suspicious for the HGEIE write to be the last step. How's this free
of a window where an interrupt is lost. (Sorry, likely another blind spot
of mine wrt RISC-V.)

>  void cf_check imsic_ctxt_switch_to(struct vcpu *v)
>  {
> -    /* Nothing to do */
> +    struct vimsic_state *imsic_state = v->arch.vimsic_state;
> +    unsigned long flags;
> +
> +    /* A s/w VS-file is never observed through HGEIP. */
> +    if ( !vcpu_guest_file_id(v) )
> +        return;
> +
> +    /*
> +     * The vCPU is about to run, so hstatus.VGEIN delivers the VS-file's
> +     * interrupts to it directly and there is nothing left for Xen to observe.
> +     */
> +    read_lock_irqsave(&imsic_state->vsfile_lock, flags);
> +    csr_clear(CSR_HGEIE, BIT(imsic_state->guest_file_id, UL));
> +    read_unlock_irqrestore(&imsic_state->vsfile_lock, flags);
>  }

How can this be a read-lock when you write a CSR? Or else - why is locking
here necessary in the firt place?

Jan