Re: [PATCH v2 34/39] xen/riscv: restore register state in the new IMSIC VS-file

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

> At this point, all interrupt producers have been moved to the new
> IMSIC VS-file so we move register state from the old IMSIC VS/SW-file
> to the new IMSIC VS-file.
> 
> As new IMSIC VS-file is ready to be used update vCPU's hstatus with
> new VGEIN.
> 
> As the whole migration procedure is finished add some extra explanatory
> comments.
> 
> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>
>
> diff --git a/xen/arch/riscv/imsic.c b/xen/arch/riscv/imsic.c
> index 3cba58e0c1..d7b137a1f5 100644
> --- a/xen/arch/riscv/imsic.c
> +++ b/xen/arch/riscv/imsic.c
> @@ -112,6 +112,12 @@ do {                            \
>      r_;                             \
>  })
>  
> +#define imsic_vs_csr_set(c, v)      \
> +do {                                \
> +    csr_write(CSR_VSISELECT, (c));  \
> +    csr_set(CSR_VSIREG, (v));       \
> +} while ( 0 )
> +
>  #define imsic_vs_csr_write(c, v)    \
>  do {                                \
>      csr_write(CSR_VSISELECT, (c));  \
> @@ -185,6 +191,19 @@ static void imsic_eix_write(unsigned int ireg, unsigned 
> long val)
>      }
>  }
>  
> +static void imsic_eix_set(unsigned int ireg, unsigned long val)
> +{
> +    switch ( ireg )
> +    {
> +    imsic_switchcase_64(imsic_switchcase_break, IMSIC_EIP0,
> +                        imsic_vs_csr_set, val)
> +    imsic_switchcase_64(imsic_switchcase_break, IMSIC_EIE0,
> +                        imsic_vs_csr_set, val)
> +    default:
> +        ASSERT_UNREACHABLE();
> +    }
> +}
> +
>  unsigned int vcpu_guest_file_id(const struct vcpu *v)
>  {
>      return ACCESS_ONCE(v->arch.vimsic_state->guest_file_id);
> @@ -630,13 +649,13 @@ static void cf_check imsic_vsfile_local_read_clear(void 
> *data)
>      old_vsiselect = csr_read(CSR_VSISELECT);
>      old_hstatus = csr_read(CSR_HSTATUS);
>      new_hstatus = old_hstatus & ~HSTATUS_VGEIN;
> -    new_hstatus |= ((unsigned long)idata->hgei) << HSTATUS_VGEIN_SHIFT;
> +    new_hstatus |= MASK_INSR(idata->hgei, HSTATUS_VGEIN);


>      csr_write(CSR_HSTATUS, new_hstatus);
>  
>      /*
> -     * There is no need to use atomic functions version to store
> -     * values in MRIF because imsic_vsfile_read_clear() is always called
> -     * with pointer to temporary MRIF on stack.
> +     * No atomic accessors are needed to store the values into the MRIF here,
> +     * as imsic_vsfile_read_clear() is always called with a pointer to a
> +     * temporary MRIF on the stack.
>       */


>  
>      mrif->eidelivery = imsic_vs_csr_swap(IMSIC_EIDELIVERY, 0);
> @@ -972,6 +991,49 @@ int __init vimsic_make_domu_dt_node(struct kernel_info 
> *kinfo,
>      return fdt_end_node(fdt);
>  }
>  
> +static void cf_check imsic_vsfile_local_update(void *data)
> +{
> +    unsigned int i;
> +    struct imsic_mrif_eix *eix;
> +    const struct imsic_vsfile_data *idata = data;
> +    struct imsic_mrif *mrif = idata->mrif;
> +    unsigned long new_hstatus, old_hstatus, old_vsiselect;
> +
> +    /* We can only update if we have a HW IMSIC context */
> +    if ( !idata->hgei )
> +        return;
> +
> +    /*
> +     * No atomic accessors are needed to read the values out of the MRIF 
> here,
> +     * as this is always called with a pointer to a temporary MRIF on the
> +     * stack.
> +     */
idata->mrif may point anywhere, so what the comment really states is a
requirement on callers. It also only makes full sense next to KVM, where
a shared SW-file MRIF is accessed atomically; Xen has neither of those
(yet). Either explicitely explain that callers must declare mrif on
their stack or move the comment directly in call sites.

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