Re: [PATCH v2 30/39] xen/riscv: prepare new IMSIC VS-file

[Jan Beulich <jbeulich@xxxxxxxx>]

On 17.09.2026 16:50, Oleksii Kurochko wrote:
> On 9/14/26 3:13 PM, Jan Beulich wrote:
>> On 27.08.2026 17:21, Oleksii Kurochko wrote:
>>> +#define imsic_switchcase_break(ireg, op, v) \
>>> +    case ireg:                              \
>>> +        op(ireg, v);                        \
>>> +        break;
>>> +
>>> +#define imsic_switchcase_ret(ireg, op, ...) \
>>> +    case ireg:                              \
>>> +        return op(ireg, ##__VA_ARGS__);
>>> +
>>> +#define imsic_switchcase_2(F, ireg, ...)    \
>>> +    F(ireg + 0, ##__VA_ARGS__)              \
>>> +    F(ireg + 1, ##__VA_ARGS__)
>>
>> Ah, there is an F here.
>>
>> This (recurring below) shows another problem: The two F invocations
>> look syntacticlly incorrect, due to the missing semicolon. Semicolon
>> use wants redoing everywhere here.
> 
> I will drop then ';' from imsic_switchcase_{break,ret}.

Provided that works, i.e. you have no cae where __VA_ARGS__ expands to
nothing.

>>> +static void cf_check imsic_vsfile_local_clear(void *data)
>>> +{
>>> +    unsigned int i;
>>> +    const struct imsic_vsfile_data *idata = data;
>>> +    unsigned long new_hstatus, old_hstatus, old_vsiselect;
>>> +
>>> +    /* We can only zero-out if we have a IMSIC VS-file */
>>> +    if ( !idata->hgei )
>>> +        return;
>>
>> Wouldn't it make sense to avoid the call here altogether then?
> 
> I think it is better to have this if () here instead of the caller side 
> as this function one day could be just directly (w/ imsic_call_on_cpu) 
> and even the way how it is called now and in the case of 
> imsic_call_on_cpu() is executed on local cpu then it will be basically 
> just direct call of imsic_vsfile_local_clear(). So in the case I am not 
> missing something I prefer to have a check here.
> 
>>
>>> +    old_vsiselect = csr_read(CSR_VSISELECT);
>>
>> Likely obvious to you, but I can't spot why vsiselect would need saving
>> here. If you want me to ack such code, please add at least brief comments.
> 
> I think then it will be better to put the comment once above struct 
> imsic_vsfile_data and then just point here to that comment as basically 
> it will be needed for all imsic_vsfile_local_* helpers.
> 
> So basically I am suggesting:
> 
> --- a/xen/arch/riscv/imsic.c
> +++ b/xen/arch/riscv/imsic.c
> @@ ... @@
>   /*
>    * Arguments of the imsic_vsfile_local_*() helpers, which are executed 
> by the
>    * pCPU owning the interrupt file, thereby through imsic_call_on_cpu().
> + *
> + * The helpers interrupt whatever vCPU context is loaded on that pCPU, 
> which
> + * generally isn't the vCPU the interrupt file belongs to. To reach the 
> file
> + * they retarget hstatus.VGEIN and select the file's registers through
> + * vsiselect. Both CSRs are live state of the interrupted vCPU 
> (vsiselect is
> + * saved to struct arch_vcpu only on context switch, but the 
> interrupted vCPU
> + * may resume guest execution without one), hence the helpers have to 
> restore
> + * them before returning.
>    */
>   struct imsic_vsfile_data {
>       unsigned int hgei;
>       unsigned int nr_eix;
>       struct imsic_mrif *mrif;
>   };
> @@ ... @@ static void cf_check imsic_vsfile_local_clear(void *data)
>       /* We can only zero-out if we have a IMSIC VS-file */
>       if ( !idata->hgei )
>           return;
> 
> +    /* See the comment ahead of struct imsic_vsfile_data. */
>       old_vsiselect = csr_read(CSR_VSISELECT);
>       old_hstatus = csr_read(CSR_HSTATUS);
> @@ ... @@ static void cf_check imsic_vsfile_local_read_clear(void *data)
>       csr_clear(CSR_HGEIE, BIT(idata->hgei, UL));
> 
> +    /* See the comment ahead of struct imsic_vsfile_data. */
>       old_vsiselect = csr_read(CSR_VSISELECT);
>       old_hstatus = csr_read(CSR_HSTATUS);
> @@ ... @@ static void cf_check imsic_vsfile_local_update(void *data)
>        * stack.
>        */
> 
> +    /* See the comment ahead of struct imsic_vsfile_data. */
>       old_vsiselect = csr_read(CSR_VSISELECT);
>       old_hstatus = csr_read(CSR_HSTATUS);
> 
> 
> Does it look clear now?

Not really, I'm afraid. A much shorter comment mentioning that
imsic_..._write() alter vsiselect (if I got things right) would imo
be both more direct, more clear, and could easily live at all three
sites individually.

>>> +    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);
>>> +
>>> +    imsic_vs_csr_write(IMSIC_EIDELIVERY, 0);
>>> +    imsic_vs_csr_write(IMSIC_EITHRESHOLD, 0);
>>> +
>>> +    for ( i = 0; i < idata->nr_eix; i++ )
>>> +    {
>>> +        imsic_eix_write(IMSIC_EIP0 + i * 2, 0);
>>> +        imsic_eix_write(IMSIC_EIE0 + i * 2, 0);
>>> +#ifdef CONFIG_RISCV_32
>>> +        imsic_eix_write(IMSIC_EIP0 + i * 2 + 1, 0);
>>> +        imsic_eix_write(IMSIC_EIE0 + i * 2 + 1, 0);
>>> +#endif
>>
>> In asm/imsic.h I see
>>
>> #define IMSIC_EIPx_BITS         32
>>
>> Why is the number of CSR writes different here for RV32 vs RV64? 
> 
> IMSIC_EIPx_BITS is the unit the AIA spec numbers the eip<k>/eie<k> 
> registers by. On RV32 all of eip0..eip63 exist and are 32 bits wide. On 
> RV64 only the even-numbered ones exist, each being 64 bits wide and 
> covering what eip<k> and eip<k+1> cover on RV32; accessing an 
> odd-numbered one is an illegal instruction. Hence one 64-bit group of 
> interrupt identities takes one register on RV64, but two on RV32.
> 
> I will add some small comments:
> 
>      for ( i = 0; i < idata->nr_eix; i++ )
>      {
>          /* On RV64 a 64-bit EIx group is the even-numbered register 
> alone. */
>          imsic_eix_write(IMSIC_EIP0 + i * 2, 0);
>          imsic_eix_write(IMSIC_EIE0 + i * 2, 0);
> #ifdef CONFIG_RISCV_32
>          /*
>           * On RV32 it is split into the even-numbered (low half) and the
>           * following odd-numbered (high half) register.
>           */
>          imsic_eix_write(IMSIC_EIP0 + i * 2 + 1, 0);
>          imsic_eix_write(IMSIC_EIE0 + i * 2 + 1, 0);
> #endif
>      }
> 
>> And
>> if so, why would you not use the 64-bit write function, allowing the
>> #ifdef to be omitted?
> 
> I can introduce something like:
> 
> /*
>   * On RV64 a 64-bit EIx group is the even-numbered register alone, whereas
>   * on RV32 it is split into the even-numbered (low half) and the following
>   * odd-numbered (high half) register.
>   */
> static void imsic_eix_write64(unsigned int ireg, uint64_t val)
> {
>      imsic_eix_write(ireg, val);
>      if ( IS_ENABLED(CONFIG_RISCV_32) )
>          imsic_eix_write(ireg + 1, val >> 32);
> }
> 
> And then:
> 
>      for ( i = 0; i < idata->nr_eix; i++ )
>      {
>          imsic_eix_write64(IMSIC_EIP0 + i * 2, 0);
>          imsic_eix_write64(IMSIC_EIE0 + i * 2, 0);
>      }
> 
> Would it be better?

Imo yes. That said, when making the original comment, I was (apparently
wrongly, as per the stuff further up) assuming these are direct CSR
writes.

Jan