Re: [PATCH v2 33/39] xen/riscv: dump old interrupt file to memory

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

> At the old interrupt file, dump to memory all the eip and eie arrays).
Typo `)`
> After this step is done, the old interrupt file is no longer in use so
> old intrrupt file VGEIN could be released.
> 
> Restoring of old interrupt file state will be done in follow-up
> patch.
> 


> There are cases where it is needed to specify on which cpu it is
> necessary to VGEIN should be released so update vgein_release() to

The sentence miss a verb, here is a proposal:
```
There are cases where the cpu on which the VGEIN is released needs to
be specified, so update vgein_release() to deal with that.
```
Moreover, could you explain me the cases you are talking about? It's not
clear by reading the commit message in the first place.


> deal with that.




> 


> Keep BUG_ON("unimplemented") placeholder in imsic_migrate_vcpu() to guard


> against silent incorrect behaviour or unexpected panics in guest VMs until
> the function is fully implemented.
> 


> vgein_release() is stub for now and will be introduced later.


> 
> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>
>
> diff --git a/xen/arch/riscv/aia.c b/xen/arch/riscv/aia.c
> index 75c82bcfa1..be3901ec0c 100644
> --- a/xen/arch/riscv/aia.c
> +++ b/xen/arch/riscv/aia.c
> @@ -30,3 +30,8 @@ unsigned int vgein_assign(struct vcpu *v)
>  
>      return 0;
>  }
> +
> +void vgein_release(struct vcpu *v, unsigned int vgein_id, unsigned int cpu)
> +{
> +    BUG_ON("unimplemented\n");
> +}
> diff --git a/xen/arch/riscv/imsic.c b/xen/arch/riscv/imsic.c
> index 5e9f6995e4..3cba58e0c1 100644
> --- a/xen/arch/riscv/imsic.c
> +++ b/xen/arch/riscv/imsic.c
> @@ -56,6 +56,24 @@ static unsigned int __ro_after_init guest_num_msis;
>   */
>  #define GUEST_IMSIC_MAX_MSIS 255U
>  
> +/*
> + * The interrupt identities an IMSIC interrupt file provides are 0 (which is
> + * never valid, but still occupies a bit) up to IMSIC_MAX_ID inclusive, so
> + * IMSIC_MAX_ID + 1 bits have to be covered.
> + */
> +#define IMSIC_MAX_EIX DIV_ROUND_UP(IMSIC_MAX_ID + 1, BITS_PER_TYPE(uint64_t))


> +
> +struct imsic_mrif_eix {
> +    unsigned long eip[BITS_PER_TYPE(uint64_t) / BITS_PER_LONG];
> +    unsigned long eie[BITS_PER_TYPE(uint64_t) / BITS_PER_LONG];


> +};
> +
> +struct imsic_mrif {
> +    struct imsic_mrif_eix eix[IMSIC_MAX_EIX];
> +    unsigned long eithreshold;
> +    unsigned long eidelivery;
> +};
> +
Maybe I didn't get something but I couldn't find anything in the commit
message explaining why do we use mrif here.

Moreover, mrif, as described in aia spec (8.3 Memory-resident interrupt
files), seems to be only usable with IOMMU that Xen doesn't support.

If you want to have something in memory that could store some interrupt
file info, we should take another name to not be confusing.

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