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

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

Hi Thomas,

On 14/11/2014 10:22, Thomas Leonard wrote:
> On 28 October 2014 15:51, Julien Grall <julien.grall@linaro.org> wrote:
>> On 10/28/2014 03:43 PM, Thomas Leonard wrote:
>>> On 28 October 2014 15:25, Julien Grall <julien.grall@linaro.org> wrote:
>>>> On 10/28/2014 03:15 PM, Thomas Leonard wrote:
>>>>> On 22 October 2014 14:06, Julien Grall <julien.grall@linaro.org> wrote:
>>>>>> On 10/22/2014 10:03 AM, Ian Campbell wrote:
>>>>>>> On Tue, 2014-10-21 at 23:54 +0200, Samuel Thibault wrote:
>>>>>>>> Ian Campbell, le Tue 21 Oct 2014 12:00:18 +0100, a écrit :
>>>>>>>>> On Fri, 2014-10-03 at 10:20 +0100, Thomas Leonard wrote:
>>>>>>>>>> +static inline uint32_t REG_READ32(volatile uint32_t *addr)
>>>>>>>>>> +{
>>>>>>>>>> +    uint32_t value;
>>>>>>>>>> +    __asm__ __volatile__("ldr %0, [%1]":"=&r"(value):"r"(addr));
>>>>>>>>>> +    rmb();
>>>>>>>>>
>>>>>>>>> I'm not 100% convinced that you need this rmb().
>>>>>>
>>>>>> Most the GIC code doesn't require read barrier but...
>>>>>>
>>>>>>>>>
>>>>>>>>>> +    return value;
>>>>>>>>>> +}
>>>>>>>>>> +
>>>>>>>>>> +static inline void REG_WRITE32(volatile uint32_t *addr, unsigned int value)
>>>>>>>>>> +{
>>>>>>>>>> +    __asm__ __volatile__("str %0, [%1]"::"r"(value), "r"(addr));
>>>>>>>>>> +    wmb();
>>>>>>>>>> +}
>>>>>>
>>>>>> write barrier may be necessary on some, where we need to wait that all
>>>>>> write has been done before doing this one (such as enable the GIC ...).
>>>>>>
>>>>>> So this function is buggy. It should be:
>>>>>>
>>>>>> wmb();
>>>>>> __asm__ __volatile__(....).
>>>>>
>>>>> gic_init does an explicit wmb() before enabling the GIC anyway,
>>>>> although I'm not really sure why it's needed (these barriers are from
>>>>> Karim's original code, so I don't know the original reason for them).
>>>>> Xen will have marked the GIC memory as device memory, so I guess we're
>>>>> protected from many effects ("The number, order and sizes of the
>>>>> accesses are maintained.").
>>>>
>>>> Device memory doesn't mean the barrier are not necessary... The barriers
>>>> are there for the whole memory, not only the GIC memory.
>>>>
>>>> A common use case is sending an SGI. You need to ensure that every
>>>> read/write before the SGI will be seen by the other processors.
>>>> Otherwise they may not see correctly the data.
>>>
>>> Right, but I mean in the context of this code. The only things we're doing are:
>>>
>>> - enabling interrupts (in gic_init)
>>
>> You need to take care of the barrier/lock for enabling interrupts. Other
>> processor may be present at that time.
>>
>> Though IIRC, mini-os for ARM is not yet SMP.
>
> It's difficult to see what needs to be done to support code that
> doesn't exist yet. For example, would gic_init be called once, or once
> per processor? Before or after the other processors are started?

Some part of the code maybe necessary: setting PPIs/SGIs... You can give 
a look to Linux/Xen GIC drivers to have an idea on what is necessary.

>>> - reading and acking an interrupt (in gic_handler)
>>
>> You don't care for this part as it's per CPU.
>
> Just to confirm: you mean that writing to the GIC registers happens
> immediately, without buffering? i.e. calling gic_eoir() and then
> reenabling interrupts won't trigger again because the ack hasn't
> reached the GIC yet?

You don't care if the EOI takes few miliseconds more to reach the GICC 
interface. The same interrupt won't trigger until the EOI has been handling.

>> [..]
>>
>>> If enabling interrupts is delayed slightly, it shouldn't have any
>>> effect (even if we get to block_domain, wfi will flush the writes).
>>
>> Relying on a later flush that is currently existing is not the right
>> thing to do. You don't know how mini-os will evolve.
>>
>> You have to check if barriers are needed everywhere.
>
> Could you give a concrete example of where not having a barrier would
> cause a problem? As far as I can see:
>
> - If the caller of gic_init requires something to reach RAM, disk or
> whatever before interrupts are enabled, then that caller should add
> the barrier. gic.c can't know what needs flushing first.

What do you mean by flushing? Cache?

gic.c may require to put data in RAM too... a barrier in the IRQ 
enabling code will make sure that the data is seen by the other processor.

> - Operations performed by gic.c are synchronous, because they write to
> device memory (so the processor can't reorder them) and use __asm__
> __volatile__ (preventing the compiler from reordering them). So there
> should be no need for a barrier afterwards, or in between operations
> either.

The GICv2 is divided in 2 parts: GICC and GICD. The former is per-cpu 
while the latter is shared.

I agree that Device memory is synchronous. But with SMP you never know 
which processor will write/read first on this region. So you have to 
take lock to order them.

Regards,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel