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

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 11/9/26 11:19 AM, Oleksii Kurochko wrote:
> On 11/6/26 2:26 PM, Baptiste Le Duc wrote:
> > On 2026-11-05 17:57 +0100, Oleksii Kurochko wrote:
> > > [ ... 52 lines skipped ... ]
> > >
> > > > 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).
> > Yes, the post-commit notes are fine with me. IMO it's better to introduce
> > the real functions before their first caller, even if that caller only
> > lands a few patches later, than to write callers against stubs. That way
> > each migration step is written against its real callees, and the order
> > follows how IMSIC actually works: a vCPU gets a file on its first run,
> > and only then can that file be migrated.
> >
> > The locks in the VGEIN patch would then get a real user a few patches
> > later (the attach patch), and that patch's commit message can explain
> > why they are needed.
> >
> > What do you think of something like this, after "introduce
> > vaplic_reconfigure_target()"?
> >
> > - Set the guest's XLEN explicitly in hstatus.VSXL (moved before
> >    continue_new_vcpu(), with the vcpu_set_vsxl() call added to its stub,
> >    so that no commit enters a guest with an undefined VSXL)
> > - Implement continue_new_vcpu()
> > - Add basic VGEIN management for AIA guests (acked; the commit message
> >    would have to explain the explicit cpu argument of vgein_release())
Would you be okay with such explanation:

vgein_release() takes the pCPU the VGEIN belongs to explicitly rather
than relying on v->processor, as the two may differ: a vCPU moved to
another pCPU has to release the VGEIN of the pCPU it leaves, while
v->processor already names the new one. As the bitmap of a pCPU may thus
be changed from another pCPU, it is protected by a lock.

I tried to write in the way as it is requirement to API rather than a 
description of the code which will be introduced later somewhere in "=== 
migration"
~ Oleksii

> > - Map IMSIC interrupt file for vCPUs (acked)
> > - 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
> > === migration
> > - Implement APLIC-IMSIC-hart sync barrier for vCPU migration
> > - Remap interrupts to the new IMSIC VS-file ("Prepare new IMSIC VS-file"
> >    folded in, as with imsic_vsfile_acquire() already there it is reduced
> >    to a single call)
> > - Save and clear the old IMSIC VS-file on vCPU migration
> > - Restore register state in the new IMSIC VS-file
> > - Wake up a descheduled vCPU on a guest external interrupt (last, as it
> >    arms HGEIE in imsic_migrate_vcpu() and clears it in
> >    imsic_vsfile_local_read_clear())
> >
> > But I'm not an expert in patch series management, so I may be wrong.
> Let's see how it works. I'll try to reshuffle the patches as suggested.
>
> Thanks for the suggestion!