From: LIU Zhiwei <zhiwei_liu@c-sky.com>
To: Frank Chang <frank.chang@sifive.com>
Cc: Palmer Dabbelt <palmer@dabbelt.com>,
Alistair Francis <Alistair.Francis@wdc.com>,
"open list:RISC-V" <qemu-riscv@nongnu.org>,
"qemu-devel@nongnu.org Developers" <qemu-devel@nongnu.org>,
wxy194768@alibaba-inc.com
Subject: Re: [RFC PATCH 03/11] hw/intc: Add CLIC device
Date: Tue, 29 Jun 2021 10:43:56 +0800 [thread overview]
Message-ID: <4ca1ca94-52d0-9b67-2b65-c9f48d484c7c@c-sky.com> (raw)
In-Reply-To: <CANzO1D2vPxsOfpfLKROyHhD5rRGPn7YpGwx-PE9vCByzeMG15A@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 14460 bytes --]
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
>
>
[-- Attachment #2: Type: text/html, Size: 25539 bytes --]
next prev parent reply other threads:[~2021-06-29 2:46 UTC|newest]
Thread overview: 81+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-04-09 7:48 [RFC PATCH 00/11] RISC-V: support clic v0.9 specification LIU Zhiwei
2021-04-09 7:48 ` [RFC PATCH 01/11] target/riscv: Add CLIC CSR mintstatus LIU Zhiwei
2021-04-19 23:23 ` Alistair Francis
2021-04-20 0:49 ` LIU Zhiwei
2021-07-01 8:45 ` Frank Chang
2021-07-01 9:38 ` LIU Zhiwei
2021-07-02 5:38 ` Alistair Francis
2021-07-02 6:09 ` LIU Zhiwei
2021-07-02 7:16 ` Alistair Francis
2021-09-28 8:10 ` Frank Chang
2021-09-29 3:55 ` Alistair Francis
2021-04-09 7:48 ` [RFC PATCH 02/11] target/riscv: Update CSR xintthresh in CLIC mode LIU Zhiwei
2021-06-26 17:23 ` Frank Chang
2021-06-27 8:23 ` Frank Chang
2021-04-09 7:48 ` [RFC PATCH 03/11] hw/intc: Add CLIC device LIU Zhiwei
2021-04-19 23:25 ` Alistair Francis
2021-04-20 0:57 ` LIU Zhiwei
2021-04-22 0:16 ` Alistair Francis
2021-06-13 10:10 ` Frank Chang
2021-06-16 2:56 ` LIU Zhiwei
2021-06-26 12:56 ` Frank Chang
2021-06-28 7:15 ` LIU Zhiwei
2021-06-28 7:23 ` Frank Chang
2021-06-28 7:39 ` LIU Zhiwei
2021-06-28 7:49 ` Frank Chang
2021-06-28 8:01 ` LIU Zhiwei
2021-06-28 8:07 ` Frank Chang
2021-06-28 8:11 ` LIU Zhiwei
2021-06-28 8:19 ` Frank Chang
2021-06-28 8:43 ` LIU Zhiwei
2021-06-28 9:11 ` Frank Chang
2021-06-26 15:03 ` Frank Chang
2021-06-26 15:26 ` Frank Chang
2021-06-29 2:52 ` LIU Zhiwei
2021-06-29 2:43 ` LIU Zhiwei [this message]
2021-06-30 5:37 ` Frank Chang
2021-06-26 15:20 ` Frank Chang
2021-06-29 2:50 ` LIU Zhiwei
2021-06-26 17:15 ` Frank Chang
2021-06-26 17:19 ` Frank Chang
2021-06-28 10:16 ` Frank Chang
2021-06-28 12:56 ` LIU Zhiwei
2021-06-28 14:30 ` Frank Chang
2021-06-28 21:36 ` LIU Zhiwei
2021-06-28 10:24 ` Frank Chang
2021-06-28 10:48 ` LIU Zhiwei
2021-07-13 6:53 ` Frank Chang
2021-07-13 6:57 ` Frank Chang
2021-04-09 7:48 ` [RFC PATCH 04/11] target/riscv: Update CSR xie in CLIC mode LIU Zhiwei
2021-06-27 6:50 ` Frank Chang
2021-04-09 7:48 ` [RFC PATCH 05/11] target/riscv: Update CSR xip " LIU Zhiwei
2021-06-27 6:45 ` Frank Chang
2021-04-09 7:48 ` [RFC PATCH 06/11] target/riscv: Update CSR xtvec " LIU Zhiwei
2021-06-27 8:59 ` Frank Chang
2021-07-10 15:04 ` Frank Chang
2021-04-09 7:48 ` [RFC PATCH 07/11] target/riscv: Update CSR xtvt " LIU Zhiwei
2021-06-27 8:33 ` Frank Chang
2021-04-09 7:48 ` [RFC PATCH 08/11] target/riscv: Update CSR xnxti " LIU Zhiwei
2021-06-11 8:15 ` Frank Chang
2021-06-11 8:30 ` LIU Zhiwei
2021-06-11 8:42 ` Frank Chang
2021-06-11 8:56 ` LIU Zhiwei
2021-06-11 9:07 ` Frank Chang
2021-06-11 9:26 ` LIU Zhiwei
2021-06-15 7:45 ` Alistair Francis
2021-06-27 10:07 ` Frank Chang
2021-07-10 14:59 ` Frank Chang
2021-04-09 7:48 ` [RFC PATCH 09/11] target/riscv: Update CSR mclicbase " LIU Zhiwei
2021-06-26 15:31 ` Frank Chang
2021-06-29 2:54 ` LIU Zhiwei
2021-04-09 7:48 ` [RFC PATCH 10/11] target/riscv: Update interrupt handling " LIU Zhiwei
2021-06-27 15:39 ` Frank Chang
2021-04-09 7:48 ` [RFC PATCH 11/11] target/riscv: Update interrupt return " LIU Zhiwei
2021-06-27 12:08 ` Frank Chang
2021-07-13 7:15 ` Frank Chang
2021-04-19 23:30 ` [RFC PATCH 00/11] RISC-V: support clic v0.9 specification Alistair Francis
2021-04-20 1:44 ` LIU Zhiwei
2021-04-20 6:26 ` Alistair Francis
2021-04-20 7:20 ` LIU Zhiwei
2021-04-22 0:21 ` Alistair Francis
2021-06-27 15:55 ` Frank Chang
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=4ca1ca94-52d0-9b67-2b65-c9f48d484c7c@c-sky.com \
--to=zhiwei_liu@c-sky.com \
--cc=Alistair.Francis@wdc.com \
--cc=frank.chang@sifive.com \
--cc=palmer@dabbelt.com \
--cc=qemu-devel@nongnu.org \
--cc=qemu-riscv@nongnu.org \
--cc=wxy194768@alibaba-inc.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).