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

[Thomas Leonard <talex5@xxxxxxxxx>]

On 28 October 2014 15:51, Julien Grall <julien.grall@xxxxxxxxxx> wrote:
> 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.

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?

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

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

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


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