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

[Thomas Leonard <talex5@xxxxxxxxx>]

On 22 October 2014 14:06, Julien Grall <julien.grall@xxxxxxxxxx> 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.").

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



-- 
Dr Thomas Leonard        http://0install.net/
GPG: 9242 9807 C985 3C07 44A6  8B9A AE07 8280 59A5 3CC1
GPG: DA98 25AE CAD0 8975 7CDA  BD8E 0713 3F96 CA74 D8BA

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