Re: [PATCH v2 32/39] xen/riscv: remap interrupts to new IMSIC VS-file

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 9/21/26 10:28 AM, Jan Beulich wrote:
> On 21.09.2026 10:03, Oleksii Kurochko wrote:
> > On 9/14/26 5:02 PM, Jan Beulich wrote:
> > > On 27.08.2026 17:21, Oleksii Kurochko wrote:
> > > > @@ -242,6 +254,15 @@ void imsic_irq_disable(unsigned int irq)
> > > >        spin_unlock(&imsic_cfg.lock);
> > > >    }
> > > >    
> > > > +static bool imsic_local_is_pending(unsigned int id)
> > > > +{
> > > > +    unsigned long isel =
> > > > +        (id / BITS_PER_LONG) * (BITS_PER_LONG / IMSIC_EIPx_BITS) + IMSIC_EIP0;
> > > > +    unsigned long bit = BIT(id % BITS_PER_LONG, UL);
> > > Both can be unsigned int, can't they?
> > Yes, agreed. Both isel and bit fit within unsigned int. I will update
> > them in the next version.
Sorry for not noticing that in first reply but local variable `bit` 
should be unsigned long as if id % 64 >= 32 we will have overflow of 
'unsigned int'.
> > > > +    return !!(imsic_csr_read(isel) & bit);
> > > > +}
> > > No need for !! here.
> > >
> > > What about endianness, btw? Does the IMSIC always match the CPU (and
> > > its setting)?
> > No, the IMSIC does not dynamically adapt its register interfaces based
> > on the CPU's runtime endianness configuration (e.g. mstatus.SBE/MBE):
> >
> > - CSR Accesses (imsic_csr_read): Indirect CSR accesses (siselect/sireg
> > or miselect/mireg) operate using standard RISC-V CSR instructions at
> > current XLEN width. Values are read and written directly into
> > architectural GPRs without byte-swapping.
> I.e. you need you add endianness conversion.
I think I don't really get why. My understanding is that endianness is 
about an access to memory. We don't have here load/store instruction or 
an explicit access to memory. We have here only CSR instruction which 
loads value from IMSIC register to GPR w/ an access to any memory.
Probably I wasn't clear here "he IMSIC does not dynamically adapt its 
register interfaces based on the CPU's runtime endianness configuration 
(e.g. mstatus.SBE/MBE):" and it would be better to reply as:
```
endianness (mstatus.SBE/MBE) only governs memory accesses, whereas
eipK is accessed via CSR instructions, which transfer an XLEN-wide
value between the CSR and a GPR with no notion of byte order. The AIA
spec defines eipK in terms of bit significance (bit i of eipK is
identity K*32+i), so BIT(id % BITS_PER_LONG) is correct regardless of
the CPU's data endianness. Endianness only matters for the memory-mapped
seteipnum register, which is why the spec provides both seteipnum_le
and seteipnum_be.
```

> > > Overall, what does "local" in the function name signify? (For a static
> > > function, the "imsic" prefix may also be unnecessary.)
> > It signifies that local (on which code is executed now) hart's IMSIC
> > CSRs (isel and ireg in the case of imsic_csr_read()) are touched.
> That's the expected thing for CSR access, though.
Fair enough. I'll drop both the "imsic_" prefix and "local" and rename
it to irq_is_pending().

~ Oleksii