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
