Hi Frank,

Thanks for a lot of good points.

On 2021/6/26 下午11:03, Frank Chang wrote:
> LIU Zhiwei <zhiwei_liu@c-sky.com <mailto: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
Agree, thanks.
>
>
>     + * 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.
>
Agree, thanks.
> 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.
Agree.
>
>     +        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
OK.
>
>     +             * 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?
Good. I think should sync immediately.
>
>     +            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
OK.
>
>     +        /*
>     +         * 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.
Agree.
>
>     +
>     +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

>
>