Re: [Xen-devel] [PATCH 6/7] xen/arm: flush D-cache and I-cache when appropriate

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

On Sat, 27 Oct 2012, Tim Deegan wrote:
> At 19:40 +0100 on 26 Oct (1351280413), Stefano Stabellini wrote:
> > > No!  It's always safe to flush the entire line -- regardless of what
> > > other writes might be happening to it.  After all, the cache controller
> > > might evict that line on its own at any time, so nothing can be relying
> > > on its _not_ being flushed.
> > > 
> > > The only problem with not knowing how big a cacheline is is this: if the
> > > object is _bigger_ than a cache line we might use one DCCMVAC where more
> > > than one is needed.  We can avoid that by a compile-time check on some
> > > sort of architectural minimum cacheline size.
> >  
> > If we want to do that then we need to pass a size argument to
> > flush_xen_dcache_va and have a loop in the function, each iteration
> > flushing a cacheline:
> > 
> > static inline void flush_xen_dcache_va(void *p, unsigned long size)
> > {
> >     unsigned long cacheline_size = READ_CP32(CCSIDR);
> >     unsigned long i;
> >     for (i = 0; i < size; i += cacheline_size, p += cacheline_size) {
> >         asm volatile (
> >             "dsb;"
> >             STORE_CP32(0, DCCMVAC)
> >             "dsb;"
> >             : : "r" (p));
> >     }
> > }
> 
> I think we should have two functions.  One should look almost like that
> and be for flushing large ranges, and one much simpler for flushing
> small items.  Like this (totally untested, uncompiled even):
> 
>  #define MIN_CACHELINE_BYTES 32 // or whatever
> 
>  /* In setup.c somewhere. */
>  if ( READ_CP32(CCSIDR) < MIN_CACHELINE_BYTES )
>      panic("CPU has preposterously small cache lines");
> 
>  /* Function for flushing medium-sized areas.
>   * if 'range' is large enough we might want to use model-specific
>   * full-cache flushes. */
>  static inline void flush_xen_dcache_va_range(void *p, unsigned long size)
>  {
>      void *end;
>      unsigned long cacheline_bytes = READ_CP32(CCSIDR);
>      barrier();       /* So the compiler issues all writes to the range */
>      dsb();           /* So the CPU issues all writes to the range */ 
>      for ( end = p + size; p < end; p += cacheline_bytes )
>          WRITE_CP32(DCCMVAC, p);
>      dsb();           /* So we know the flushes happen before continuing */
>  }
> 
>  /* Macro for flushing a single small item.  The predicate is always 
>   * compile-time constant so this will compile down to 3 instructions in
>   * the common case.  Make sure to call it with the correct type of
>   * pointer! */
>  #define flush_xen_dcache_va(p) do {                       \
>      typeof(p) _p = (p);                                   \
>      if ( (sizeof *_p) > MIN_CACHELINE_BYTES )             \
>          flush_xen_dcache_va_range(_p, sizeof *_p);        \
>      else                                                  \
>          asm volatile (                                    \
>              "dsb;"   /* Finish all earlier writes */      \
>              STORE_CP32(0, DCCMVAC)                        \
>              "dsb;"   /* Finish flush before continuing */ \
>              : : "r" (_p), "m" (*_p));                     \
>  } while (0)
> 
> What do you think?

I think that is OK, but I would like to avoid reading CCSIDR every
single time we need to do a dcache flush, I don't know how slow that
coprocessor read is but I wouldn't want to have to find out :)

I am thinking of introducing a global variable to hold the cacheline
size and initialize it in start_xen.

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