Re: [RFC PATCH 03/11] hw/intc: Add CLIC device

[Frank Chang <frank.chang@sifive.com>]

[-- Attachment #1: Type: text/plain, Size: 13557 bytes --]

LIU Zhiwei <zhiwei_liu@c-sky.com> 於 2021年6月29日 週二 上午10:45寫道：

> 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> 於 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},
>
> }
>
That would be great.
We can create different memory regions for each memory map
and assign them with the same read/write ops.

Thanks,
Frank Chang


>
>
>>
>> +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
>
>
>
>>
>>

[-- Attachment #2: Type: text/html, Size: 25442 bytes --]