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

[LIU Zhiwei <zhiwei_liu@c-sky.com>]

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


On 2021/6/26 下午8:56, Frank Chang wrote:
> On Wed, Jun 16, 2021 at 10:56 AM LIU Zhiwei <zhiwei_liu@c-sky.com 
> <mailto:zhiwei_liu@c-sky.com>> wrote:
>
>
>     On 6/13/21 6:10 PM, Frank Chang wrote:
>>     LIU Zhiwei <zhiwei_liu@c-sky.com <mailto:zhiwei_liu@c-sky.com>> 於
>>     2021年4月9日 週五 下午3:57寫道：
>>
>>         +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);
>>         +    clic->active_count = g_new0(size_t, clic->num_harts);
>>         +    clic->exccode = g_new0(uint32_t, clic->num_harts);
>>         +    clic->cpu_irqs = g_new0(qemu_irq, clic->num_harts);
>>         +    sysbus_init_mmio(SYS_BUS_DEVICE(dev), &clic->mmio);
>>         +
>>         +    /* Allocate irq through gpio, so that we can use qtest */
>>         +    qdev_init_gpio_in(dev, riscv_clic_set_irq, irqs);
>>         +    qdev_init_gpio_out(dev, clic->cpu_irqs, clic->num_harts);
>>         +
>>         +    for (i = 0; i < clic->num_harts; i++) {
>>         +        RISCVCPU *cpu = RISCV_CPU(qemu_get_cpu(i));
>>
>>
>>     As spec says:
>>       Smaller single-core systems might have only a CLIC,
>>       while multicore systems might have a CLIC per-core and a single
>>     shared PLIC.
>>       The PLIC xeip signals are treated as hart-local interrupt
>>     sources by the CLIC at each core.
>>
>>     It looks like it's possible to have one CLIC instance per core.
>
>     If you want to delivery an interrupt to one hart, you should
>     encode the IRQ by the interrupt number
>     , the hart number and the interrupt target privilege, then set the
>     irq.
>
>     I think how to calculate the hart number is the task of PLIC and
>     it can make use of "hartid-base"
>     to calculate it.
>
>     Thanks,
>     Zhiwei
>
>
> Hi Zhiwei,
>
> What I mean is if there are multiple CLIC instances, each per core 
> (CLIC spec allows that).
> If you try to bind CLIC with CPU index start from 0,
> it will be impossible for CLIC instance to bind CPU from index other 
> than 0.
>
> For example, for 4 cores system, it's possible to have 4 CLIC instances:
>   * CLIC 0 binds to CPU 0
>   * CLIC 1 binds to CPU 1
>   * CLIC 2 binds to CPU 2
>   * CLIC 3 binds to CPU 3
>
> and that's why I said it's possible to pass an extra "hartid-base" 
> just like PLIC.
> I know most of hardid are calculated by the requesing address, so most 
> hartid usages should be fine.
> But I saw two places using qemu_get_cpu(),
> which may cause the problem for the scenario I describe above:
> i.e. riscv_clic_next_interrupt() and riscv_clic_realize() as my 
> original reply.

So what's the problem here?

Currently all cores share the same CLIC instance. Do you want to give 
each core  a CLIC instance?

Thanks,
Zhiwei

>
> Regards,
> Frank Chang
>
>>     However if you try to bind CPU reference start from index i = 0.
>>     It's not possible for each per-core CLIC to bind their own CPU
>>     instance in multicore system
>>     as they have to bind from CPU 0.
>>
>>     I'm not sure if we add a new "hartid-base" property just like
>>     what SiFive PLIC is
>>     implemented would be a good idea or not.
>>
>>
>>     Regards,
>>     Frank Chang
>>
>>         + qemu_irq irq = qemu_allocate_irq(riscv_clic_cpu_irq_handler,
>>         +  &cpu->env, 1);
>>         +        qdev_connect_gpio_out(dev, i, irq);
>>         +        cpu->env.clic = clic;
>>         +    }
>>         +}
>>         +
>>
>>

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