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

[Oleksii Kurochko <oleksii.kurochko@gmail.com>]

On 11/3/26 5:02 PM, Baptiste Le Duc wrote:
>> 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.

I'm fine with making that explicit. I tried to cover it with "... a vCPU
which doesn't own a guest interrupt file ...", and adding "hardware"
should make it clearer: "... a vCPU which doesn't own a hardware guest
interrupt file ...". I think that covers your suggestion as well, so
I'll add it.

To be honest, I'm a bit unsure how much to say about software interrupt 
files at this point: some earlier commit messages mentioned them, and I 
was asked to drop that, as they are neither introduced nor used yet.
That's why I'd rather keep the wording implicit as above, but if you
still prefer them to be named explicitly, I can add a sentence.

At this point, I'm honestly getting a bit confused about how commit 
messages are supposed to be written.

>> [ ... 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. 

It was about CSR_HSTATUS, and CSR_VSISELECT which are stored and restored:

+    csr_write(CSR_HSTATUS, old_hstatus);
+    csr_write(CSR_VSISELECT, old_vsiselect);

> 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.
>       */

I agree that this option is clearer.

Also, as the same switch to a guest interrupt file is done in
three more places (in the prev patches), I'll factor it out into 
imsic_vsfile_select() and imsic_vsfile_deselect(), with your wording as 
the comment on them.

> 
>> +    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?
> 

No real need. Lets drop it.

Thanks for review!

~ Oleksii