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

[Oleksii Kurochko <oleksii.kurochko@gmail.com>]

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