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

[Julien Grall <julien.grall@xxxxxxxxxx>]

On 10/28/2014 03:43 PM, Thomas Leonard wrote:
> On 28 October 2014 15:25, Julien Grall <julien.grall@xxxxxxxxxx> wrote:
>> On 10/28/2014 03:15 PM, Thomas Leonard wrote:
>>> 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.").
>>
>> 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.

> - reading and acking an interrupt (in gic_handler)

You don't care for this part as it's per CPU.

[..]

> 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.

Regards,

-- 
Julien Grall

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