Re: [PATCH v3 33/39] xen/riscv: restore register state in the new IMSIC VS-file

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 11/3/26 5:45 PM, Baptiste Le Duc wrote:
> > Complete the migration of a vCPU's guest interrupt file: move the state
> > saved from the old IMSIC VS-file into the new one and make the vCPU use
> > the new VS-file.
> >
> > The interrupt producers have already been retargeted to the new
> > VS-file, so MSIs may land in it before the saved state is restored.
> Could we send an extempore MSI to the hart of the new file once the
> interrupt producers have been retargeted, and spin until it is received?
> That would guarantee that no more MSIs land once we have received it.
I don't think it's needed. The barrier is there so that nothing gets
lost from the old file: once the extempore MSI sent to the hart of the
old file has arrived, every MSI the APLIC sent towards the old file
before the retargeting has landed there, so what's read out of the old
file is complete.

MSIs sent after the retargeting go to the new file, and they can keep
arriving at any time, including while the saved state is being
restored, so an extempore MSI to the hart of the new file wouldn't stop
them anyway: it would only tell that the MSIs sent before it have
arrived. That's why the saved eip/eie bits are ORed into the new file
rather than written to it, so that whatever lands there in the meantime
is kept, and why eidelivery is restored last, so that the new file
doesn't deliver anything until its state is complete.

> > Hence the saved eip/eie bits are ORed into the new VS-file rather than
> > written to it, so that such MSIs aren't lost. eithreshold and
> > eidelivery are restored last: the new VS-file was cleared when it was
> > acquired, so it doesn't deliver interrupts until its pending and enable
> > bits are complete.
> >
> > As with reading the old VS-file, this has to be done on the pCPU owning
> > the new VS-file, with hstatus.VGEIN temporarily selecting it. That is
> > v->processor, the pCPU the vCPU is migrating to.
> >
> > Finally, point hstatus.VGEIN in the vCPU's saved context at the new
> > VS-file, so that it becomes the source of the vCPU's VS-level external
> > interrupts and the file the guest accesses through its siselect/sireg.
> > With that the migration is complete, so drop the BUG_ON("unimplemented")
> Great!
> > [ ... 82 lines skipped ... ]
> > +static void cf_check imsic_vsfile_local_update(void *data)
> > +{
> > +    unsigned int i;
> > +    const struct imsic_vsfile_data *idata = data;
> > +    struct imsic_mrif *mrif = idata->mrif;
> mrif is only read here, so it can be const:
>      const struct imsic_mrif *mrif = idata->mrif;
Good point. I will apply that.

> > +    unsigned long new_hstatus, old_hstatus, old_vsiselect;
> > +
> > +    /* We can only update if we have a HW IMSIC context */
> > +    if ( !idata->hgei )
> > +        return;
> > +
> > +    /*
> > +     * No atomic accessors are needed to read the values out of the MRIF here,
> > +     * as this is always called with a pointer to a temporary MRIF on the
> > +     * stack.
> > +     */
> > +
> > +    /*
> > +     * The file is reached by retargeting hstatus.VGEIN, and the
> > +     * imsic_vs_csr_*() accessors alter vsiselect. Both belong to the vCPU
> > +     * running on this pCPU, so restore them before returning.
> > +     */
> Same comment as on the previous patch: you are not restoring the CSRs
> here but saving them, as imsic_vs_csr_*() will alter vsiselect. Please
> update the comment accordingly.
Sure! As I mentioned in reply to prev. patch I think that it would be 
better to factor out the sequence of work with hstatus&vsiselect to 
separate helpers, put your comment suggestion above the helpers once.
> > [ ... 32 lines skipped ... ]
> > @@ -1152,5 +1222,11 @@ void imsic_migrate_vcpu(struct vcpu *v)
> >       /* Free-up old IMSIC VS-file */
> >       vgein_release(v, old_vsfile_id, old_vsfile_cpu);
> >   
> > -    BUG_ON("unimplemented");
> > +    /* Restore register state in the new IMSIC VS-file */
> > +    imsic_call_on_cpu(v->processor, imsic_vsfile_local_update, &vsfile_data);
> > +
> > +    /* Set VCPU HSTATUS.VGEIN to new IMSIC VS-file */
> Nit: s/VCPU/vCPU/
Acked.

Thanks for review.

~ Oleksii