Re: [PATCH v3 32/39] xen/riscv: save and clear the old IMSIC VS-file on vCPU migration

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

> When a vCPU migrates to another pCPU, its guest interrupt file has to
> move as well: the eidelivery, eithreshold and eip/eie state of the old
> IMSIC VS-file has to be carried over to the newly acquired one.
> 
> Once all interrupt producers have been retargeted to the new VS-file,
> so that nothing can reach the old one anymore, read the state of the
> old one and clear it at the same time. This has to be done on the pCPU
> owning the old VS-file, as a guest interrupt file is only accessible
> through vsiselect/vsireg with hstatus.VGEIN selecting it. After that
> the old VS-file is no longer in use and its guest external interrupt
> number can be released.
> 
> The state is saved into struct imsic_mrif. Its eix[] array follows the
> layout of a memory-resident interrupt file (MRIF) as defined by the AIA
> specification: pairs of 64-bit interrupt-pending and interrupt-enable
> doublewords, each pair covering 64 interrupt identities. eithreshold
> and eidelivery aren't part of an MRIF and are stored next to it as they
> belong to the interrupt file state too. Using the MRIF layout allows
> the same structure to hold the interrupt file of a vCPU which doesn't
> own a guest interrupt file, which is how the AIA specification
> describes MRIFs being used by a hypervisor.

I think it's worth mentioning that this MRIF structure could be reused
for software interrupt files later.
> [ ... 172 lines skipped ... ]
> +static void cf_check imsic_vsfile_local_read_clear(void *data)
> +{
> +    unsigned int i;
> +    const struct imsic_vsfile_data *idata = data;
> +    struct imsic_mrif *mrif = idata->mrif;
> +    unsigned long new_hstatus, old_hstatus, old_vsiselect;
> +
> +    /*
> +     * The file is reached by retargeting hstatus.VGEIN, and
> +     * imsic_vs_csr_swap() alters vsiselect. Both belong to the vCPU running
> +     * on this pCPU, so restore them before returning.
> +     */
You're saving the registers here, not restoring them. I'd use something
like:
    /*
     * vsiselect/vsireg only reach the guest interrupt file selected by
     * hstatus.VGEIN, so point VGEIN at this file for the access.
     * hstatus and vsiselect hold the state of the vCPU running on this
     * pCPU; save them here and restore them before returning.
     */

> +    old_vsiselect = csr_read(CSR_VSISELECT);
> +    old_hstatus = csr_read(CSR_HSTATUS);
> +    new_hstatus = old_hstatus & ~HSTATUS_VGEIN;
> +    new_hstatus |= MASK_INSR(idata->hgei, HSTATUS_VGEIN);
> +    csr_write(CSR_HSTATUS, new_hstatus);
> +
> +    /*
> +     * No atomic accessors are needed to store the values into the MRIF here,
> +     * as imsic_vsfile_read_clear() is always called with a pointer to a
> +     * temporary MRIF on the stack.
> +     */
Nit: is this blank line really needed?

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