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

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

> 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.
> 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;
> +    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.
> [ ... 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/

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