Hi Frank,

Thanks for a lot of good points. 

LIU Zhiwei <zhiwei_liu@c-sky.com> 於 2021年4月9日 週五 下午3:57寫道：

+static uint8_t
+riscv_clic_get_interrupt_level(RISCVCLICState *clic, uint8_t intctl)
+{
+    int nlbits = clic->nlbits;
+
+    uint8_t mask_il = ((1 << nlbits) - 1) << (8 - nlbits);
+    uint8_t mask_padding = (1 << (8 - nlbits)) - 1;
+    /* unused level bits are set to 1 */
+    return (intctl & mask_il) | mask_padding;
+}

According to spec:

  if the nlbits > CLICINTCTLBITS, then the lower bits of the 8-bit

  interrupt level are assumed to be all 1s.

That is, the valid nlbits should be: min(clic->nlbits, CLICINTCTLBITS);

The cliccfg example in spec also shows that:

CLICINTCTLBITS  nlbits  clicintctl[i]  interrupt levels
      0                       2         ........         255
      1                       2         l.......         127,255
      2                       2         ll......         63,127,191,255
      3                       3         lll.....         31,63,95,127,159,191,223,255
      4                       1         lppp....      127,255

 

+ * In a system with multiple harts, the M-mode CLIC regions for all the harts
+ * are placed contiguously in the memory space, followed by the S-mode CLIC
+ * regions for all harts. (Section 3.11)
+ */

The above description is not true any more in the latest spec:

  The CLIC specification does not dictate how CLIC memory-mapped registers are

  split between M/S/U regions as well as the layout of multiple harts as this is generally

  a platform issue and each platform needs to define a discovery mechanism to determine

  the memory map locations.

But I think we can just keep the current design for now anyway, as it's also one of legal memory layout.

Otherwise, the design would be more complicated I think.

We can pass an array containing indexed by hart_id and mode from the machine board, such as

hwaddr clic_memmap[HARTS][MODE] = {

{0x0000, 0x10000, 0x20000},

{0x4000, 0x14000, 0x24000},

{0x8000, 0x18000, 0x28000},

{0xc000, 0x1c000, 0x2c000},

}

+static void
+riscv_clic_update_intie(RISCVCLICState *clic, int mode, int hartid,
+                        int irq, uint64_t new_intie)
+{
+    size_t hart_offset = hartid * clic->num_sources;
+    size_t irq_offset = riscv_clic_get_irq_offset(clic, mode, hartid, irq);
+    CLICActiveInterrupt *active_list = &clic->active_list[hart_offset];
+    size_t *active_count = &clic->active_count[hartid];
+
+    uint8_t old_intie = clic->clicintie[irq_offset];
+    clic->clicintie[irq_offset] = !!new_intie;
+
+    /* Add to or remove from list of active interrupts */
+    if (new_intie && !old_intie) {
+        active_list[*active_count].intcfg = (mode << 8) |
+                                            clic->clicintctl[irq_offset];
+        active_list[*active_count].irq = irq;
+        (*active_count)++;
+    } else if (!new_intie && old_intie) {
+        CLICActiveInterrupt key = {
+            (mode << 8) | clic->clicintctl[irq_offset], irq
+        };
+        CLICActiveInterrupt *result = bsearch(&key,
+                                              active_list, *active_count,
+                                              sizeof(CLICActiveInterrupt),
+                                              riscv_clic_active_compare);
+        size_t elem = (result - active_list) / sizeof(CLICActiveInterrupt);

I think what you are trying to do here is to get the number of elements

right after the active interrupt to be deleted in order to calculate the size of

active interrupts to be memmoved.

However, according to C spec:

  When two pointers are subtracted, both shall point to elements of the same array object,

  or one past the last element of the array object; the result is the difference of the
  subscripts of the two array elements.

So I think: (result - active_list) is already the number of elements you want.

You don't have to divide it with sizeof(CLICActiveInterrupt) again.

 

+        size_t sz = (--(*active_count) - elem) * sizeof(CLICActiveInterrupt);
+        assert(result);

Nit: assert(result) can be moved above size_t elem statement.

 

+        memmove(&result[0], &result[1], sz);
+    }
+
+    /* Sort list of active interrupts */
+    qsort(active_list, *active_count,
+          sizeof(CLICActiveInterrupt),
+          riscv_clic_active_compare);
+
+    riscv_clic_next_interrupt(clic, hartid);
+}
+
+static void
+riscv_clic_hart_write(RISCVCLICState *clic, hwaddr addr,
+                      uint64_t value, unsigned size,
+                      int mode, int hartid, int irq)
+{
+    int req = extract32(addr, 0, 2);
+    size_t irq_offset = riscv_clic_get_irq_offset(clic, mode, hartid, irq);
+
+    if (hartid >= clic->num_harts) {
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "clic: invalid hartid %u: 0x%" HWADDR_PRIx "\n",
+                      hartid, addr);
+        return;
+    }
+
+    if (irq >= clic->num_sources) {
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "clic: invalid irq %u: 0x%" HWADDR_PRIx "\n", irq, addr);
+        return;
+    }
+
+    switch (req) {

Spec. says that it's legal to write 32-bit value to set

{clicintctl[i], clicintattr[i], clicintie[i] ,clicintip[i]} at the same time:

  A 32-bit write to {clicintctl,clicintattr,clicintie,clicintip} is legal.

  However, there is no specified order in which the effects of

  the individual byte updates take effect.

 

I miss it. I think it only supports 1 byte access or 4 bytes access. For 4 bytes access,  we need to pass an flag to specify to the order from the machine board.

Thoughts?

+    case 0: /* clicintip[i] */
+        if (riscv_clic_validate_intip(clic, mode, hartid, irq)) {
+            /*
+             * The actual pending bit is located at bit 0 (i.e., the
+             * leastsignificant bit). In case future extensions expand the bit

Typo: leastsignificant => least significant

 

+             * field, from FW perspective clicintip[i]=zero means no interrupt
+             * pending, and clicintip[i]!=0 (not just 1) indicates an
+             * interrupt is pending. (Section 3.4)
+             */
+            if (value != clic->clicintip[irq_offset]) {
+                riscv_clic_update_intip(clic, mode, hartid, irq, value);
+            }
+        }
+        break;
+    case 1: /* clicintie[i] */
+        if (clic->clicintie[irq_offset] != value) {
+            riscv_clic_update_intie(clic, mode, hartid, irq, value);
+        }
+        break;
+    case 2: /* clicintattr[i] */
+        if (riscv_clic_validate_intattr(clic, value)) {
+            if (clic->clicintattr[irq_offset] != value) {
+                /* When nmbits=2, check WARL */
+                bool invalid = (clic->nmbits == 2) &&
+                               (extract64(value, 6, 2) == 0b10);
+                if (invalid) {
+                    uint8_t old_mode = extract32(clic->clicintattr[irq_offset],
+                                                 6, 2);
+                    value = deposit32(value, 6, 2, old_mode);
+                }
+                clic->clicintattr[irq_offset] = value;
+                riscv_clic_next_interrupt(clic, hartid);
+            }
+        }
+        break;
+    case 3: /* clicintctl[i] */
+        if (value != clic->clicintctl[irq_offset]) {
+            clic->clicintctl[irq_offset] = value;

If irq i is already in the active_list, when will its intcfg been synced?

 

+            riscv_clic_next_interrupt(clic, hartid);
+        }
+        break;
+    }
+}
+
+static uint64_t
+riscv_clic_hart_read(RISCVCLICState *clic, hwaddr addr, int mode,
+                     int hartid, int irq)
+{
+    int req = extract32(addr, 0, 2);
+    size_t irq_offset = riscv_clic_get_irq_offset(clic, mode, hartid, irq);
+
+    if (hartid >= clic->num_harts) {
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "clic: invalid hartid %u: 0x%" HWADDR_PRIx "\n",
+                      hartid, addr);
+        return 0;
+    }
+
+    if (irq >= clic->num_sources) {
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "clic: invalid irq %u: 0x%" HWADDR_PRIx "\n", irq, addr);
+        return 0;
+    }
+
+    switch (req) {
+    case 0: /* clicintip[i] */
+        return clic->clicintip[irq_offset];
+    case 1: /* clicintie[i] */
+        return clic->clicintie[irq_offset];
+    case 2: /* clicintattr[i] */
+        /*
+         * clicintattr register layout
+         * Bits Field
+         * 7:6 mode
+         * 5:3 reserved (WPRI 0)
+         * 2:1 trig
+         * 0 shv
+         */
+        return clic->clicintattr[irq_offset] & ~0x38;
+    case 3: /* clicintctrl */

Typo: clicintctl

 

+        /*
+         * The implemented bits are kept left-justified in the most-significant
+         * bits of each 8-bit clicintctl[i] register, with the lower
+         * unimplemented bits treated as hardwired to 1.(Section 3.7)
+         */
+        return clic->clicintctl[irq_offset] |
+               ((1 << (8 - clic->clicintctlbits)) - 1);
+    }
+
+    return 0;
+}
+

+static void
+riscv_clic_write(void *opaque, hwaddr addr, uint64_t value, unsigned size)
+{
+    RISCVCLICState *clic = opaque;
+    hwaddr clic_size = clic->clic_size;
+    int hartid, mode, irq;
+
+    if (addr < clic_size) {

Is this necessary?

I think memory region size already limits the request address to be within the range of clic_size.

At this point, not necessary.

 

+static uint64_t riscv_clic_read(void *opaque, hwaddr addr, unsigned size)
+{
+    RISCVCLICState *clic = opaque;
+    hwaddr clic_size = clic->clic_size;
+    int hartid, mode, irq;
+
+    if (addr < clic_size) {

Same to riscv_clic_write().

Thanks,

Frank Chang

 

+        if (addr < 0x1000) {
+            assert(addr % 4 == 0);
+            int index = addr / 4;
+            switch (index) {
+            case 0: /* cliccfg */
+                return clic->nvbits |
+                       (clic->nlbits << 1) |
+                       (clic->nmbits << 5);
+            case 1: /* clicinfo */
+                /*
+                 * clicinfo register layout
+                 *
+                 * Bits Field
+                 * 31 reserved (WARL 0)
+                 * 30:25 num_trigger
+                 * 24:21 CLICINTCTLBITS
+                 * 20:13 version (for version control)
+                 * 12:0 num_interrupt
+                 */
+                return clic->clicinfo & ~INT32_MAX;

clic->clicinfo should represent the CLIC setting information.

I think it's better to add clic reset function or in riscv_clic_realize() to initialize clic->clicinfo.

 

+
+static void riscv_clic_realize(DeviceState *dev, Error **errp)
+{
+    RISCVCLICState *clic = RISCV_CLIC(dev);
+    size_t harts_x_sources = clic->num_harts * clic->num_sources;
+    int irqs, i;
+
+    if (clic->prv_s && clic->prv_u) {
+        irqs = 3 * harts_x_sources;
+    } else if (clic->prv_s || clic->prv_u) {
+        irqs = 2 * harts_x_sources;
+    } else {
+        irqs = harts_x_sources;
+    }
+
+    clic->clic_size = irqs * 4 + 0x1000;
+    memory_region_init_io(&clic->mmio, OBJECT(dev), &riscv_clic_ops, clic,
+                          TYPE_RISCV_CLIC, clic->clic_size);
+
+    clic->clicintip = g_new0(uint8_t, irqs);
+    clic->clicintie = g_new0(uint8_t, irqs);
+    clic->clicintattr = g_new0(uint8_t, irqs);
+    clic->clicintctl = g_new0(uint8_t, irqs);
+    clic->active_list = g_new0(CLICActiveInterrupt, irqs);

Should the size of clic->active_list be: harts_x_sources?

Every irq can be in the active_list, so just the number of irqs.

Remind we will only use M-mode IRQ in next patch set, I still think we should use the
irqs here, because irq in active_list has privilege information.

Thanks,
Zhiwei