Re: [PATCH ARM v8 2/4] mini-os: arm: interrupt controller

[Julien Grall <julien.grall@linaro.org>]

Hi Ian and Thomas,

On 10/21/2014 12:00 PM, Ian Campbell wrote:
> On Fri, 2014-10-03 at 10:20 +0100, Thomas Leonard wrote:
>> +static void gic_set_priority(struct gic *gic, int irq_number, unsigned char priority)
>> +{
>> +    uint32_t value;
>> +    uint32_t *addr = REG(gicd(gic, GICD_IPRIORITYR)) + (irq_number >> 2);
>> +    value = REG_READ32(addr);
>> +    value &= ~(0xff << (8 * (irq_number & 0x3))); // clear old priority
>> +    value |= priority << (8 * (irq_number & 0x3)); // set new priority
>> +    REG_WRITE32(addr, value);
> 
> I believe the IPRIORITYR registers are byte addressable, so you can just
> do a strb of the new value without needing to read-modify-write.
> 
>> +static void gic_route_interrupt(struct gic *gic, int irq_number, unsigned char cpu_set)
>> +{
>> +    uint32_t value;
>> +    uint32_t *addr = REG(gicd(gic, GICD_ITARGETSR)) + (irq_number >> 2);
>> +    value = REG_READ32(addr);
>> +    value &= ~(0xff << (8 * (irq_number & 0x3))); // clear old target
>> +    value |= cpu_set << (8 * (irq_number & 0x3)); // set new target
>> +    REG_WRITE32(addr, value);
> 
> Same for ITARGETSR.

Our implementation of ITARGETSR doesn't handle correctly read/write per
byte. If the register is RO (such as for the SGIs and PPIs), our write
ignore is checking that the guest is writing a word.

Even though we need to fix it in Xen (I could send a patch for it). We
need to keep this implementation if we want mini-os to run on Xen 4.4.x.

>> +/* Note: not thread safe (but we only support one CPU for now anyway) */
>> +static void gic_enable_interrupt(struct gic *gic, int irq_number,
>> +        unsigned char cpu_set, unsigned char level_sensitive, unsigned char ppi)
>> +{
>> +    void *set_enable_reg;
>> +    void *cfg_reg;
>> +
>> +    // set priority
>> +    gic_set_priority(gic, irq_number, 0x0);
>> +
>> +    // set target cpus for this interrupt
>> +    gic_route_interrupt(gic, irq_number, cpu_set);
>> +
>> +    // set level/edge triggered
>> +    cfg_reg = (void *)gicd(gic, GICD_ICFGR);
>> +    if (level_sensitive) {
>> +        clear_bit_non_atomic((irq_number * 2) + 1, cfg_reg);
>> +    } else {
>> +        set_bit_non_atomic((irq_number * 2) + 1, cfg_reg);
>> +    }
>> +    if (ppi)
> 
> missing else? Or should be folded in above as level_sensitive||ppi

I would even drop this check. I don't think, we need to have specific
configuration for PPIs.

[..]

>> +    gic_enable_interrupt(&gic, EVENTS_IRQ /* interrupt number */, 0x1 /*cpu_set*/, 1 /*level_sensitive*/, 0 /* ppi */);
>> +    gic_enable_interrupt(&gic, VIRTUALTIMER_IRQ /* interrupt number */, 0x1 /*cpu_set*/, 1 /*level_sensitive*/, 1 /* ppi */);
> 
> BTW, ppi or not is implicit in the irq number.

At the same time, both interrupt are PPIs, why the former as ppi = 0 and
the latter 1?

Regards,

-- 
Julien Grall