Re: [PATCH v2 13/16] xen/riscv: implementation of aplic and imsic operations

[Oleksii Kurochko]

On 5/15/25 11:44 AM, Jan Beulich wrote:

@@ -159,6 +270,8 @@ static int __init aplic_preinit(struct dt_device_node 
*node, const void *dat)

dt_irq_xlate = aplic_irq_xlate; + spin_lock_init(&aplic.lock);

Can't you have the struct field have a suitable initializer?

Sure, I will use struct initializer:
  static struct aplic_priv aplic = {
      .lock = SPIN_LOCK_UNLOCKED,
  };

+static void imsic_local_eix_update(unsigned long base_id, unsigned long num_id,
+                                   bool pend, bool val)
+{
+    unsigned long id = base_id, last_id = base_id + num_id;
+
+    while ( id < last_id )
+    {
+        unsigned long isel, ireg;
+        unsigned long start_id = id & (__riscv_xlen - 1);
+        unsigned long chunk = __riscv_xlen - start_id;
+        unsigned long count = (last_id - id < chunk) ? last_id - id : chunk;
+
+        isel = id / __riscv_xlen;
+        isel *= __riscv_xlen / IMSIC_EIPx_BITS;
+        isel += pend ? IMSIC_EIP0 : IMSIC_EIE0;
+
+        ireg = GENMASK(start_id + count - 1, start_id);
+
+        id += count;
+
+        if ( val )
+            imsic_csr_set(isel, ireg);
+        else
+            imsic_csr_clear(isel, ireg);
+    }
+}
+
+void imsic_irq_enable(unsigned int irq)
+{
+    unsigned long flags;
+
+    spin_lock_irqsave(&imsic_cfg.lock, flags);
+    /*
+     * There is no irq - 1 here (look at aplic_set_irq_type()) because:
+     * From the spec:
+     *   When an interrupt file supports distinct interrupt identities,
+     *   valid identity numbers are between 1 and inclusive. The identity
+     *   numbers within this range are said to be implemented by the interrupt
+     *   file; numbers outside this range are not implemented. The number zero
+     *   is never a valid interrupt identity.
+     *   ...
+     *   Bit positions in a valid eiek register that don’t correspond to a
+     *   supported interrupt identity (such as bit 0 of eie0) are read-only 
zeros.
+     *
+     * So in EIx registers interrupt i corresponds to bit i in comparison wiht
+     * APLIC's sourcecfg which starts from 0. (l)

What's this 'l' in parentheses here to indicate?

I don't really remember, it seems like I want to point to the spec, but
then just make a quote from the spec instead. I'll just drop it.

+     */
+    imsic_local_eix_update(irq, 1, false, true);
+    spin_unlock_irqrestore(&imsic_cfg.lock, flags);
+}
+
+void imsic_irq_disable(unsigned int irq)
+{
+    unsigned long flags;
+
+    spin_lock_irqsave(&imsic_cfg.lock, flags);
+    imsic_local_eix_update(irq, 1, false, false);
+    spin_unlock_irqrestore(&imsic_cfg.lock, flags);
+}

The sole caller of the function has doubly turned off IRQs already; perhaps
no need to it a 3rd time, unless other callers are to appear? Same for
imsic_irq_enable() as it looks.

I checked a code in private branches and it seems like these functions are 
called
only in aplic_irq_{enable,disable}(), so we could do, at 
least,spin_lock(&imsic_cfg.lock)
+ ASSERT(!local_irq_is_enabled());

~ Oleksii