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

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

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())
> - 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!

~ Oleksii