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

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

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.

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