[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH ARM v8 2/4] mini-os: arm: interrupt controller
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
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |