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

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

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.

> Maybe none of these barriers is necessary.
>>>> I don't really see why such barriers are needed indeed. Are they needed
<>>>> to actually push the values out?
>>>
>>> That would, I think, require an isb() (instruction barrier) whereas
>>> wmb() turns into a dsb() (data barrier). I expect you are write and
>>> these rmb/wmb are not needed, but an isb may be needed in the caller if
>>> they want to rely on the affect of a write (e.g. enabling the
>>> controller)
>>
>> isb is useful for cache and system control registers. We don't use such
>> things in the GIC code, because the GIC register is memory mapped. We
>> only need to ensure that the write are ordered when it's necessary (only
>> few places).
>>
>> Regards,
>>
>> --
>> Julien Grall
> 
> 
> 


-- 
Julien Grall

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