Re: [PATCH v3 29/39] xen/riscv: prepare new IMSIC VS-file

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

On 2026-11-05 13:49 +0100, Oleksii Kurochko wrote:
> [ ... 79 lines skipped ... ]
> >> +
> > What does the switch buy? ireg only ends up as the value written to
> > vsiselect, not as a CSR number, so it doesn't need to be a compile-time
> > constant. imsic_vs_csr_write(ireg, val) does the same, without the 128 case
> > labels and the macro pyramid. If there's a reason, it wants stating in a
> > comment; otherwise please drop the pyramid (an ASSERT() on the range can
> > stay if you want one).
> 
> Good point.
> 
> I will drop it and use the following instead:
> 
> /*
>   * 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. ireg names the first, even-numbered
>   * register of such a group, among eip0..eip63 or eie0..eie63.
>   */
> static void imsic_eix_check(unsigned int ireg)
> {
>      ASSERT(!(ireg & 1));
>      ASSERT((ireg >= IMSIC_EIP0 && ireg < IMSIC_EIP0 + 64) ||
>             (ireg >= IMSIC_EIE0 && ireg < IMSIC_EIE0 + 64));
> }
> 
> /* Write a whole 64-bit EIx group, laid out as described above. */
Nit: "above" may go stale if the code moves. Better to name the function
you are referring to.

Also, the parameter isn't an *ireg value. It is the register number
written to *iselect; *ireg is only the data window. Per AIA (IMSIC
chapter, "Interrupt files"), values 0x80-0xFF of *iselect select
eip0-63/eie0-63, and imsic_csr_write() uses its first argument that way:

#define imsic_csr_write(c, v)   \
do {                            \
    csr_write(CSR_SISELECT, c); \
    csr_write(CSR_SIREG, v);    \
} while (0)

So I'd rename it to "iselect" (here and in the other helpers taking
the same argument).

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