Re: [PATCH v3 31/39] xen/riscv: remap interrupts to new IMSIC VS-file

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 10/26/26 3:06 PM, Baptiste Le Duc wrote:
> > Once the new IMSIC VS-file has been zeroed out, it has to be mapped into
> > the guest's G-stage in place of the old one. Also, if any interrupts at an
> > APLIC are forwarded by MSIs to the old interrupt file, reconfigure the
> > APLIC to send them to the new interrupt file.
> >
> > Taking a h/w guest interrupt file (assigning it, zeroing it out, mapping
> > it into G-stage and recording it in the vCPU's IMSIC state) is factored
> > out into imsic_vsfile_acquire().
> >
> > The relevant translation tables at all IOMMUs would also have to be
> > updated so that MSIs for this virtual interrupt file are sent to the new
> > physical interrupt file, but that is skipped for now as there is no IOMMU
> > support for RISC-V.
> >
> > Synchronize with the APLIC to ensure that no straggler MSIs will arrive at
> > the old interrupt file by running imsic_aplic_sync() on the pCPU which owns
> > that file.
> >
> > If the new file can't be mapped into the guest, release the guest
> > external interrupt number imsic_vsfile_acquire() took for it instead of
> > leaking it. vgein_release() takes the pCPU explicitly instead of relying
> > on v->processor, as on migration v->processor already refers to the new
> > pCPU while the number to be released may belong to the old one.
> >
> > 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.
> >
> > Implement imsic_update_state(), which records the vCPU's guest
> > interrupt file id together with the pCPU owning the file. The two are
> > read as a pair, e.g. by imsic_migrate_vcpu() and the IMSIC context
> > switch hooks, so they are updated under vsfile_lock as a single unit.
> >
> > imsic_map_guest_file() and vgein_release() are stubs for now.
> I have the feeling this patch does more than its subject says: besides
> retargeting the APLIC and adding the sync, it introduces
> imsic_vsfile_acquire() (assign + clear + G-stage map + state update),
> imsic_update_state() and the error path, which relies on two stubs,
> imsic_map_guest_file() and vgein_release().
These are the steps the AIA spec lists for moving the interrupt 
producers to the new file: map the new file into G-stage in place of the 
old one, retarget the APLIC and synchronize with it. 
imsic_vsfile_acquire() and imsic_update_state() are just how the "take 
and map a new file" part is split out, so I'd still say they belong to 
this patch. Your point below, that none of this can run yet, is the real 
issue though.

> At this point vgein_assign()
> is still BUG_ON("unimplemented") too, so none of the new code can
> actually run, and the error path can't be reviewed against the real
> vgein_release().
>
> Also, until "introduce IMSIC h/w interrupt file attaching to vcpu", no
> vCPU ever owns a h/w file, so imsic_migrate_vcpu() can only hit the
> CPU_NONE check.
>
> Wouldn't it be simpler to move the two already-acked patches and the
> attach first, so that no stubs are needed and each migration step is
> written against its real callees when it lands:
> - Add basic VGEIN management for AIA guests (real vgein_assign() and
>    vgein_release(); the reason for the explicit cpu argument of
>    vgein_release() would then have to go into this commit message)
> - Map IMSIC interrupt file for vCPUs (real imsic_map_guest_file())
I think I have such order before and it was changed becuase for locks in 
the first patch it wasn't enough context to understand if they are 
needed or not.
> - Implement continue_new_vcpu() and set hstatus.VSXL
> - Attach a h/w interrupt file to a vCPU on its first run, introducing
>    imsic_vsfile_acquire() and imsic_update_state() with their first user
>    (acquire, vaplic_reconfigure_target(), set hstatus.VGEIN)
> - Prepare new IMSIC VS-file (on top of imsic_vsfile_acquire())
> - Implement APLIC-IMSIC-hart sync barrier for vCPU migration
> - Remap interrupts to the new IMSIC VS-file (APLIC retarget + sync)
> - Save/clear old VS-file, restore, SGEI wake-up as today
>
> A side benefit: attach no longer has to be squeezed into the finished
> migration code, so imsic_vsfile_move() with its first_attach argument
> and the goto set_vgein go away. Attach stays a plain sequence, and
> imsic_migrate_vcpu() simply reuses imsic_vsfile_acquire().
I'm fine with that order. It does mean that "add basic VGEIN management"
and "map IMSIC interrupt file for vCPUs" introduce functions whose first
user only comes with the attach patch a few patches later. If that's
acceptable, I'll add a post-commit note to each of them saying where the
functions get used (as IIUC, they will first introduced and then used in 
later patches).
> Migrating a vCPU which has already run would still hit the trailing
> BUG_ON("unimplemented") until the restore patch, but that is no worse
> than the panic() it hits today.
~ Oleksii