Re: [PATCH v2 39/39] xen/riscv: introduce IMSIC h/w interrupt file attaching to vcpu

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

> Introduce imsic_vsfile_attach() to initialize the AIA-related state needed
> for a vCPU to have a working guest interrupt file.
> 
> A guest (VS) interrupt file must be mapped to one of a pCPU's
> hardware interrupt files (if they exist), so the pCPU a vCPU will actually
> run on needs to be known first. arch_vcpu_create() is therefore not a
> suitable place to call vcpu_aia_init(), since the pCPU assigned to a
> vCPU can still change before it is first scheduled. To avoid
> reassigning the VS interrupt file id and remapping it to a different
> pCPU's hardware interrupt file, imsic_vsfile_attach() is called from a
> later point in the scheduling path (e.g. continue_new_vcpu()). Since
> it will end up being called from a non-__init context, it is not
> itself marked __init.
> 
> Introduce imsic_update_state() to update a vCPU's guest IMSIC state
> (the guest interrupt file id and the pCPU whose hardware interrupt
> file it is mapped to) as a single consistent unit. This state can be
> read concurrently, e.g. by a future helper that checks whether a
> vCPU has a pending IMSIC interrupt, though no such consumer exists
> yet at this stage, so it is protected by a lock.
I have difficulties to clearly understand the aim of this function. In
fact, if we want to know if a vCPU has a pending interrupts, wouldn't be
enough to check hgeip[guest_file_id]?

In any case, why does updating vCPU's guest IMSIC state would be
required?
> 
> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>
>
> diff --git a/xen/arch/riscv/domain.c b/xen/arch/riscv/domain.c
> index 0782148b72..15b6bfffa9 100644
> --- a/xen/arch/riscv/domain.c
> +++ b/xen/arch/riscv/domain.c
> @@ -155,6 +155,8 @@ static void continue_new_vcpu(struct vcpu *prev)
>          reset_stack_and_jump(idle_loop);
>      else
>      {
> +        imsic_vsfile_attach(current);
> +
>          /*
>           * During a context switch to a new vCPU, interrupts must be disabled
>           * to guarantee that the vCPU's CSR state can be safely restored into
> diff --git a/xen/arch/riscv/imsic.c b/xen/arch/riscv/imsic.c
> index 374a21ace1..ad638d7485 100644
> --- a/xen/arch/riscv/imsic.c
> +++ b/xen/arch/riscv/imsic.c
> @@ -212,7 +212,14 @@ unsigned int vcpu_guest_file_id(const struct vcpu *v)
>  
>  void imsic_update_state(struct vcpu *v, unsigned int guest_file_id)
>  {
> -    BUG_ON("unimplemented\n");
> +    unsigned long flags;
> +    struct vimsic_state *vimsic_state = v->arch.vimsic_state;
> +    unsigned int cpu = guest_file_id ? v->processor : NR_CPUS;
> +
> +    write_lock_irqsave(&vimsic_state->vsfile_lock, flags);
> +    vimsic_state->guest_file_id = guest_file_id;
> +    vimsic_state->vsfile_cpu = cpu;
> +    write_unlock_irqrestore(&vimsic_state->vsfile_lock, flags);
>  }
>  
>  void __init imsic_ids_local_delivery(bool enable)
> @@ -640,6 +647,16 @@ struct imsic_vsfile_data {
>      struct imsic_mrif *mrif;
>  };
>  
> +/*
> + * Number of 64-bit EIx groups needed to cover all the interrupt identities 
> an
> + * IMSIC interrupt file provides, which are 0 (never valid, but it still
> + * occupies a bit) up to and including imsic_cfg.nr_ids.
> + */
> +static unsigned int imsic_nr_eix(void)
> +{
> +    return DIV_ROUND_UP(imsic_cfg.nr_ids + 1, BITS_PER_TYPE(uint64_t));
> +}
> +
>  /*
>   * Execute func() on the pCPU which owns the IMSIC interrupt file func() is
>   * going to work with.
> @@ -1138,20 +1155,90 @@ static void cf_check imsic_vsfile_local_update(void 
> *data)
>      csr_write(CSR_VSISELECT, old_vsiselect);
>  }
>  
> +/*
> + * Point the vCPU's HSTATUS.VGEIN at the guest interrupt file it has been
> + * given. It is applied to the hart when the vCPU's context is restored.
> + */
> +static void vcpu_set_vgein(struct vcpu *v, unsigned int vsfile_id)
> +{
> +    unsigned long hstatus = vcpu_guest_cpu_user_regs(v)->hstatus;
> +
> +    hstatus &= ~HSTATUS_VGEIN;
> +    hstatus |= MASK_INSR(vsfile_id, HSTATUS_VGEIN);
> +
> +    vcpu_guest_cpu_user_regs(v)->hstatus = hstatus;
> +}
> +
> +/*
> + * Take a h/w guest interrupt file of 'cpu' for the vCPU: zero the file out,
> + * map it into the domain's G-stage at the vCPU's virtual IMSIC page and
> + * record the new location in the per-vCPU IMSIC state.
> + *
> + * HSTATUS.VGEIN is deliberately left alone: the vCPU may be pointed at the
> + * file only when the file already holds the vCPU's interrupt state, which in
> + * the case of imsic_migrate_vcpu() happens only after the old file has been
> + * moved to the new one. Thereby it is up to the caller to call
> + * vcpu_set_vgein() at the right moment.
Nit: would clarify the paragraph here:
  HSTATUS.VGEIN is deliberately left alone: it must only point at a file
  which already holds the vCPU's interrupt state. The file taken here is
  zeroed, so for imsic_migrate_vcpu() VGEIN can only be updated once the
  state of the old file has been copied into it. Thereby it is up to the
  caller to call vcpu_set_vgein() at the right moment.
> + *
> + * Returns the id of the taken interrupt file, or 0 if none could be taken, 
> in
> + * which case the domain is crashed.
> + */
> +static unsigned int imsic_vsfile_acquire(struct vcpu *v, unsigned int cpu)
> +{
> +    struct imsic_vsfile_data vsfile_data = { .nr_eix = imsic_nr_eix() };
> +    unsigned int vsfile_id;
> +    int rc;
> +
> +    vsfile_id = vgein_assign(v);
> +    if ( !vsfile_id )
> +    {
> +        /*
> +         * vgein_assign() returns 0 when no free h/w guest interrupt file is
> +         * available. s/w guest interrupt files aren't supported yet, so such
> +         * a vCPU can't be run.
> +         */
> +        domain_crash(v->domain,
> +                     "%pv: no free h/w guest interrupt file on CPU%u\n",
> +                     v, cpu);
> +        return 0;
> +    }
> +
> +    vsfile_data.hgei = vsfile_id;
> +
> +    /* The file could still hold the state of its previous owner */
> +    imsic_call_on_cpu(cpu, imsic_vsfile_local_clear, &vsfile_data);
> +
> +    rc = imsic_map_guest_file(v, vsfile_id);
> +    if ( rc )
> +    {
> +        vgein_release(v, vsfile_id, cpu);
> +
> +        /* Can't continue w/o correctly mapped IMSIC interrupt file */
> +        domain_crash(v->domain,
> +                     "%pv: failed to map h/w guest interrupt file %u: %d\n",
> +                     v, vsfile_id, rc);
> +        return 0;
> +    }
> +
> +    imsic_update_state(v, vsfile_id);
> +
> +    return vsfile_id;
> +}
> +
>  void imsic_migrate_vcpu(struct vcpu *v)
>  {
> -    unsigned int new_vsfile_hgei;
> +    unsigned int new_vsfile_id;
>      unsigned int new_vsfile_cpu;
> -    unsigned int nr_hw_eix = DIV_ROUND_UP(imsic_cfg.nr_ids + 1,
> -                                          BITS_PER_TYPE(uint64_t));
> -    struct imsic_vsfile_data vsfile_data = {
> -        .nr_eix = nr_hw_eix,
> -    };
> +    unsigned int nr_hw_eix = imsic_nr_eix();
>      struct vimsic_state *imsic_state = v->arch.vimsic_state;
>      unsigned long flags;
>      unsigned int old_vsfile_id;
>      unsigned int old_vsfile_cpu;
>      struct imsic_mrif tmrif = { };
> +    struct imsic_vsfile_data vsfile_data = {
> +        .nr_eix = nr_hw_eix,
> +        .mrif = &tmrif,
> +    };
>  
>      /*
>       * The scheduler can mark a freshly created vCPU's unit as migrated and
> @@ -1189,25 +1276,10 @@ void imsic_migrate_vcpu(struct vcpu *v)
>       */
>      new_vsfile_cpu = v->processor;
>  
> -    new_vsfile_hgei = vgein_assign(v);
> -
> -    /* We don't support SW interrupt files at the moment. */
> -    BUG_ON(!new_vsfile_hgei);
> -
> -    vsfile_data.hgei = new_vsfile_hgei;
> -
> -    /* 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");
> -
> +    /* Zero-out, map and start to use the new IMSIC VS-file */
> +    new_vsfile_id = imsic_vsfile_acquire(v, new_vsfile_cpu);
> +    if ( !new_vsfile_id )
>          return;
> -    }
> -
> -    imsic_update_state(v, new_vsfile_hgei);
>  
>      /*
>       * TODO: Modify the relevant translation tables at all IOMMUs so that 
> MSIs
> @@ -1245,7 +1317,7 @@ void imsic_migrate_vcpu(struct vcpu *v)
>      vgein_release(v, old_vsfile_id, old_vsfile_cpu);
>  
>      /* Restore register state in the new IMSIC VS-file */
> -    vsfile_data.mrif = &tmrif;
> +    vsfile_data.hgei = new_vsfile_id;
Isn't this already done by imsic_vsfile_acquire()? Couldn't we pass a
&vsfile_data in their arg so it could fill it instead of allocating one
in the stack?

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