Re: [PATCH v3 14/39] xen/riscv: add IMSIC vCPU context switch handlers

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 10/8/26 11:54 AM, Baptiste Le Duc wrote:
> > IMSIC state currently needs to track only which physical CPU owns a vCPU's
> > IMSIC guest interrupt file, as the CPU id is part of the physical address
> > the file is mapped at.
> >
> > Add imsic_ctxt_switch_from() to record that CPU when a vCPU is switched
> > out. A vCPU running on the s/w VS-file has no h/w file bound to a CPU, so
> > there is nothing to record for it. Nothing reads the recorded value yet.
> >
> > imsic_ctxt_switch_to() has nothing to do: switching a vCPU in on the CPU
> > which owns its guest interrupt file needs no IMSIC update. Only a move to
> > another CPU does (a new VGEIN and a remapped file), and that isn't part of
> > the context switch.
> Nit: I think it could make sense to mention that if a migration happen,
> it would go anyway in imsic_migrate_vcpu() before
> imsic_ctxt_switch_to().
Probably it would be nice but at this moment imsic_migrate_vcpu() 
doesn't exist and it what I tried to cover by "Only a move to another 
CPU does ...". At least, for now I think that this part would be better 
to keep as it is now.
> > Install both as the ctxt_switch_{from,to} hooks of struct vintc_ops. MSI
> > delivery is the only mode Xen supports (aplic_init() panics on an APLIC
> > without an "msi-parent" property) and the only one vAPLIC emulates, so the
> > vAPLIC state to save and restore is always the IMSIC one and no
> > vAPLIC-level forwarder is needed. Being indirect call targets, both
> > handlers get cf_check.
> >
> > Co-developed-by: Romain Caritey <Romain.Caritey@xxxxxxxxxxxxx>
> Should add Signed-off-by Romain Caritey.
> Without that, everything is ok from my side
As we discussed here [1] it is fine to go without Signed-off. And it 
seems like where Xen is different with Linux (where C-o-b should follow 
by S-o-b).
[1] 
https://lore.kernel.org/xen-devel/cover.1790779209.git.oleksii.kurochko@xxxxxxxxx/T/#m5063a41f8b824f28d3f9a5a976488b8308575d93
> > Signed-off-by: Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>
> > --- a/xen/arch/riscv/vaplic.c
> > +++ b/xen/arch/riscv/vaplic.c
> > @@ -456,6 +456,12 @@ static const struct mmio_handler_ops vaplic_mmio_ops = {
> >   static const struct vintc_ops vintc_ops = {
> >       .vcpu_init = vcpu_imsic_init,
> >       .vcpu_deinit = vcpu_imsic_deinit,
> > +    /*
> > +     * MSI delivery is the only supported mode, so the vAPLIC state to save
> Nit: Do we want to always support only MSI delivery? If not, maybe adapt
> the comment.
At the moment I consider MSI delivery the only mode worth supporting:
the AIA-capable platforms I'm aware of come with both APLIC and IMSIC,
and with IMSIC guest interrupt files an interrupt reaches the guest
without trapping into Xen, which direct delivery mode can't offer.

As for the comment, I think it's fine as is, given how the members right
below it are initialized:

    .ctxt_switch_from = imsic_ctxt_switch_from,
    .ctxt_switch_to = imsic_ctxt_switch_to,

The imsic_ prefix makes it explicit that MSI delivery mode is assumed,
and supporting direct delivery mode would mean changing these
initializers anyway, so the comment wouldn't be missed then. I'd prefer
to keep it as it is for now and update it once direct delivery mode
needs to be supported.

> Reviewed-by: Baptiste Le Duc <baptiste.le-duc@xxxxxxxxxx>
Thanks!

~ Oleksii