[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



Hi Thomas,

On 14/11/2014 10:22, Thomas Leonard wrote:
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?

Some part of the code maybe necessary: setting PPIs/SGIs... You can give a look to Linux/Xen GIC drivers to have an idea on what is necessary.

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

You don't care if the EOI takes few miliseconds more to reach the GICC interface. The same interrupt won't trigger until the EOI has been handling.

[..]

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.

What do you mean by flushing? Cache?

gic.c may require to put data in RAM too... a barrier in the IRQ enabling code will make sure that the data is seen by the other processor.

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

The GICv2 is divided in 2 parts: GICC and GICD. The former is per-cpu while the latter is shared.

I agree that Device memory is synchronous. But with SMP you never know which processor will write/read first on this region. So you have to take lock to order them.

Regards,

--
Julien Grall

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

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.