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

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

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

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.

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