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

[Thomas Leonard <talex5@xxxxxxxxx>]

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)
- reading and acking an interrupt (in gic_handler)

I don't see that other processors are involved here,

If enabling interrupts is delayed slightly, it shouldn't have any
effect (even if we get to block_domain, wfi will flush the writes).

We might need a dsb after acking the interrupt (the wmb call currently
does this, but it could be more explicit that we need a flush).


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