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

[Baptiste Le Duc <baptiste.le-duc@xxxxxxxxxx>]

> While a vCPU is running, MSIs written to its h/w IMSIC guest interrupt
> file are delivered straight to VS-mode. Once the vCPU is descheduled
> nobody observes that file anymore, so a guest blocked on such an
> interrupt would stay blocked until some unrelated event happens to
> schedule it again.
> 
> Let Xen observe the file in that window: on deschedule set the vCPU's
> bit in HGEIE, which turns an interrupt pending in its VS-file into an
> HS-level SGEI, and clear the bit again on schedule-in. HGEIP only
> reports a file number, so to get from it back to a vCPU keep an
> owners[] map per pCPU, filled by vgein_{assign,release} alongside the
> VGEIN bitmap, and kick the vCPU it points at.
Nit: I would reword the last sentence to make it more clear:

    HGEIP only reports an interrupt file number, so to find the vCPU to
    kick, keep a per-pCPU owners[] array indexed by file number and
    updated in vgein_assign()/vgein_release() together with the VGEIN
    bitmap.

> 
> v->arch.hie is only initialized here and is written to the CSR later,
> on the context switch to the vCPU.
> 
> vgein_release() still has no caller: a vCPU going away has to both free
> its VGEIN slot and drop the owners[] entry, but there is no vCPU
> teardown path to hook it into yet. vgein_deinit() only covers a pCPU
> going offline.
> 
> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>
>
> diff --git a/xen/arch/riscv/aia.c b/xen/arch/riscv/aia.c
> index 1aca07c2f7..9642a9796e 100644
> --- a/xen/arch/riscv/aia.c
> +++ b/xen/arch/riscv/aia.c
> @@ -18,6 +18,13 @@ struct vgein_ctrl {
>      /* The least-significant bits are implemented first, apart from bit 0 */
>      unsigned long bmp;
>      spinlock_t lock;
> +    /*
> +     * Guest interrupt file IDs run from 1 to geilen inclusive (0 means that
> +     * no guest external interrupt source is selected), and geilen can never
> +     * exceed BITS_PER_LONG - 1, so indexing this array by the ID directly
> +     * always fits.
> +     */
> +    struct vcpu *owners[BITS_PER_LONG];
>      unsigned int geilen;
>  };
>  
> @@ -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;
>  }


>  
>  static struct notifier_block cpu_nfb = {
> @@ -138,7 +147,10 @@ unsigned int vgein_assign(struct vcpu *v)
>      if ( vgein_id > vgein->geilen )
>          vgein_id = 0;
>      else
> +    {
>          __set_bit(vgein_id, bmp);
> +        vgein->owners[vgein_id] = v;
> +    }
>  
>      spin_unlock_irqrestore(&vgein->lock, flags);
>  
> @@ -161,6 +173,7 @@ void vgein_release(struct vcpu *v, unsigned int vgein_id, 
> unsigned int cpu)
>      spin_lock_irqsave(&vgein->lock, flags);
>      if ( !__test_and_clear_bit(vgein_id, &vgein->bmp) )
>          ASSERT_UNREACHABLE();
> +    vgein->owners[vgein_id] = NULL;
>      spin_unlock_irqrestore(&vgein->lock, flags);
>  
>  #ifdef VGEIN_DEBUG
> @@ -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);


> +    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);


> +
> +        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


> +
> +            vcpu_kick(vgein->owners[guest_file_id]);
> +        }
> +    }
> +
> +    spin_unlock_irqrestore(&vgein->lock, flags);
> +}
> diff --git a/xen/arch/riscv/domain.c b/xen/arch/riscv/domain.c
> index 2dfe4c2e72..2918196822 100644
> --- 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;


>  }
>  
>  static void continue_new_vcpu(struct vcpu *prev)
> @@ -398,6 +400,7 @@ static void restore_csr_regs(struct vcpu *vcpu)
>      csr_write(CSR_HEDELEG, vcpu->arch.hedeleg);
>      csr_write(CSR_HIDELEG, vcpu->arch.hideleg);
>      csr_write(CSR_HVIP, vcpu->arch.hvip);
> +    csr_write(CSR_HIE, vcpu->arch.hie);
>      csr_write64(CSR_HENVCFG, vcpu->arch.henvcfg);
>      csr_write(CSR_HCOUNTEREN, vcpu->arch.hcounteren);
>      csr_write64(CSR_HTIMEDELTA, vcpu->arch.htimedelta);
> diff --git a/xen/arch/riscv/imsic.c b/xen/arch/riscv/imsic.c
> index d7b137a1f5..0715206611 100644
> --- 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);
>  }


>  
>  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);
>  }


>  
>  int imsic_map_guest_file(struct vcpu *v, unsigned int vsfile_id)
> @@ -646,6 +665,12 @@ static void cf_check imsic_vsfile_local_read_clear(void 
> *data)
>      struct imsic_mrif *mrif = idata->mrif;
>      unsigned long new_hstatus, old_hstatus, old_vsiselect;
>  
> +    /*
> +     * The HGEIE bit imsic_ctxt_switch_from() armed belongs to the old owner
> +     * only.
> +     */
> +    csr_clear(CSR_HGEIE, BIT(idata->hgei, UL));
> +
>      old_vsiselect = csr_read(CSR_VSISELECT);
>      old_hstatus = csr_read(CSR_HSTATUS);
>      new_hstatus = old_hstatus & ~HSTATUS_VGEIN;
> @@ -991,6 +1016,21 @@ int __init vimsic_make_domu_dt_node(struct kernel_info 
> *kinfo,
>      return fdt_end_node(fdt);
>  }
>  
> +/*
> + * Start to observe the interrupt file from HS-mode, the same way
> + * imsic_ctxt_switch_from() does it for a vCPU which is switched out.
> + *
> + * The counterpart, clearing the bit of the interrupt file which is left
> + * behind, is done by imsic_vsfile_local_read_clear(), which already runs on
> + * the pCPU owning that file.
> + */
> +static void cf_check imsic_local_hgeie_set(void *data)
> +{
> +    const struct imsic_vsfile_data *idata = data;
> +
> +    csr_set(CSR_HGEIE, BIT(idata->hgei, UL));
> +}
> +
Sorry I'm a bit lost here, are you doing that for vCPUs migration?
Because in your commit message you only mentioned scheduled-out case
which needs to have Xen observing the descheduled vCPU to re-scheduled
it, but no other case that would need to have interrupt file observed is
explained

-- 
Baptiste Le Duc <baptiste.le-duc@xxxxxxxxxx>