Re: [XEN PATCH] xen: cache clearing and invalidation helpers refactoring

[Nicola Vetrini <nicola.vetrini@xxxxxxxxxxx>]

On 2024-02-20 08:45, Jan Beulich wrote:
> On 19.02.2024 16:14, Nicola Vetrini wrote:
> > The cache clearing and invalidation helpers in x86 and Arm didn't
> > comply with MISRA C Rule 17.7: "The value returned by a function
> > having non-void return type shall be used". On Arm they
> > were always returning 0, while some in x86 returned -EOPNOTSUPP
> > and in common/grant_table the return value is saved.
> >
> > As a consequence, a common helper arch_grant_cache_flush that returns
> > an integer is introduced, so that each architecture can choose whether 
> > to
> > return an error value on certain conditions, and the helpers have 
> > either
> > been changed to return void (on Arm) or deleted entirely (on x86).
> >
> > Signed-off-by: Julien Grall <julien@xxxxxxx>
> > Signed-off-by: Nicola Vetrini <nicola.vetrini@xxxxxxxxxxx>
> > ---
> > The original refactor idea came from Julien Grall in [1]; I edited 
> > that proposal
> > to fix build errors.
> >
> > I did introduce a cast to void for the call to flush_area_local on 
> > x86, because
> > even before this patch the return value of that function wasn't 
> > checked in all
> > but one use in x86/smp.c, and in this context the helper (perhaps 
> > incidentally)
> > ignored the return value of flush_area_local.
> I object to such casting to void, at least until there's an overriding
> decision that for Misra purposes such casts may be needed.
There are three choices here:
1. cast to void
2. deviation for flush_area_local, which for the case of the cache 
helpers is what led to this patch; it may still be a viable option, if 
other maintainers agree
3. refactor of flush_area_local; this is not viable here because the 
return value is actually used and useful, as far as I can tell, in smp.c
> > --- a/xen/arch/arm/include/asm/page.h
> > +++ b/xen/arch/arm/include/asm/page.h
> > @@ -123,6 +123,7 @@
> >
> >  #ifndef __ASSEMBLY__
> >
> > +#include <public/grant_table.h>
> This is a no-go, imo (also on x86): Adding this include here 
> effectively
> means that nearly every CU will have a dependency on that header, no
> matter that most are entirely agnostic of grants. Each arch has a
> grant_table.h - is there any reason the new, grant-specific helper 
> can't
> be put there?
I would have to test, but I think that can be done

> > @@ -182,21 +183,21 @@ void flush_area_mask(const cpumask_t *mask, 
> > const void *va,
> >  }
> >
> >  static inline void flush_page_to_ram(unsigned long mfn, bool 
> > sync_icache) {}
> > -static inline int invalidate_dcache_va_range(const void *p,
> > -                                             unsigned long size)
> > -{ return -EOPNOTSUPP; }
> > -static inline int clean_and_invalidate_dcache_va_range(const void *p,
> > -                                                       unsigned long 
> > size)
> > +
> > +static inline int arch_grant_cache_flush(unsigned int op, const void 
> > *p,
> > +                                     unsigned long size)
> >  {
> > -    unsigned int order = get_order_from_bytes(size);
> > +    unsigned int order;
> > +
> > +    if ( !(op & GNTTAB_CACHE_CLEAN) )
> > +        return -EOPNOTSUPP;
> > +
> > +    order = get_order_from_bytes(size);
> >      /* sub-page granularity support needs to be added if necessary */
> > -    flush_area_local(p, FLUSH_CACHE|FLUSH_ORDER(order));
> > +    (void) flush_area_local(p, FLUSH_CACHE|FLUSH_ORDER(order));
> As to my objection to the addition of a cast, did you actually check
> what this function returns, before saying "incidentally" in the
> respective remark? Already the return type being "unsigned int" is
> indicative of the return value not being suitable here for handing
> on.
My "incidentally" was motivated by the fact that the caller doesn't 
check whether
flags_in != flags_out (effectively tests for the execution of a certain 
code path). It may have been deliberate or not, I don't know. If it was 
accidental, then a check of the return value in arch_grant_cache_flush 
will eliminate the need for a void cast.
> In addition there shouldn't be a blank after a cast. Instead, if
> already you were to touch this line, it would have been nice if you
> took the opportunity and added the missing blanks around the binary
> operator involved.
That's true, thanks.

--
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)