Re: [PATCH v8 3/8] xen/arm: return int *_dcache_va_range

[Ian Campbell <Ian.Campbell@citrix.com>]

On Mon, 2014-10-20 at 17:34 +0100, Stefano Stabellini wrote:
> On Mon, 20 Oct 2014, Jan Beulich wrote:
> > >>> On 20.10.14 at 16:57, <Ian.Campbell@citrix.com> wrote:
> > >> --- a/xen/arch/arm/mm.c
> > >> +++ b/xen/arch/arm/mm.c
> > >> @@ -388,7 +388,7 @@ void flush_page_to_ram(unsigned long mfn)
> > >>  {
> > >>      void *v = map_domain_page(mfn);
> > >>  
> > >> -    clean_and_invalidate_dcache_va_range(v, PAGE_SIZE);
> > >> +    ASSERT(clean_and_invalidate_dcache_va_range(v, PAGE_SIZE) == 0);
> > > 
> > > Just this one assert? What about all the other callers of
> > > *_dcache_va_range?
> 
> There are many many callers under xen/arch/arm, should I really add
> ASSERT everywhere?

I dunno, but you should certainly be consistent, i.e. everywhere or
nowhere.

> Maybe I could just add an ASSERT on the return value within
> clean_dcache_va_range and clean_and_invalidate_dcache_va_range?

I don't think an ASSERT() is necessary, but a comment before the return
0 like:
    /*
     * These cannot fail and many callers from within the ARM 
     * architecture code do not check the result.
     */

That's assuming there's no possibility of us needing to propagate
failure in the future. Since we are talking about a h/w instruction with
no possibility of an error return I think we can assert (no pun
intended) that this won't happen.

> Or should I introduce wrappers with the ASSERT?

I don't think so.

Ian.