LIU Zhiwei <zhiwei_...@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_...@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
>
>
>
>>
>>
