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

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

On 2026-09-23 18:02 +0200, Oleksii Kurochko wrote:
> 
> 
> On 9/23/26 5:15 PM, Baptiste Le Duc wrote:
> >> At the old interrupt file, dump to memory all the eip and eie arrays).
> > Typo `)`
> 
> Will drop `)`.
> 
> >> 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.
> > ```
> 
> I think it could be dropped at all as vgein_release() stub is just 
> introduced here and not updated.
> 
> > Moreover, could you explain me the cases you are talking about? It's not
> > clear by reading the commit message in the first place.
> 
> For example, during migration of vCPU, vCPU->processor points to new CPU 
> where it will be run but we still have to free VGEIN on the prev. 
> ->processor.
> 
> > 
> > 
> >> 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.
> 
> It is just convenient way to temporary store h/w interrupt file. Also it 
> could be used not only for ...
> 
> > 
> > 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.
> 
> ... IOMMU but also to support more guest interrupts file implemented by 
> IMSIC (basically what I am calling as software interrupt file). Without 
> memory-resident interrupt files, the number of virtual RISC-V harts that 
> can directly receive MSIs from devices is limited by the total number of 
> guest interrupt files implemented by all IMSICs in the system, because 
> all MSIs to RISC-V harts must go through IMSICs. For a single RISC-V 
> hart, the number of guest interrupt files is the GEILEN parameter 
> defined by the Privileged Architecture, which can be at most 31 for RV32 
> and 63 for RV64.
> 
> > 
> > If you want to have something in memory that could store some interrupt
> > file info, we should take another name to not be confusing.
> 
> It seems like it is okay to use memory residential interrupt file (mrif) 
> here based on KVM's code who are using mrif for the same purpose I 
> described above.
> 
> In short, MRIF is a joint virtualization technology shared between the 
> IOMMU and the hypervisor. The IOMMU uses the MRIF as a memory target to 
> land incoming hardware MSIs, while the hypervisor manages these MRIFs in 
> RAM as software data structures to support an effectively unlimited 
> number of vCPUs that don't currently hold a physical IMSIC guest file slot.
> 
Yes, I read the spec to understand but here, mrif doesn't catch the MSIs
right? so it's not exactly the behaviour mentioned or you have in mind
to add full support when IOMMU will be supported?

> ~ Oleksii
> 
> 
>