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

[Julien Grall <julien.grall@xxxxxxxxxx>]

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