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

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 11/5/26 4:27 PM, Baptiste Le Duc wrote:
> 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.
Agree with that.

> 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).
As to the name: I'd rather keep ireg. The value does go into *iselect,
but what it denotes is a register of the interrupt file: per the AIA
spec, values of *iselect in the range 0x70-0xFF select the registers
of the interrupt file (eidelivery, eithreshold, eip0-63, eie0-63), and
ireg is the number of such a register, eip0-63/eie0-63 here.

~ Oleksii