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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

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

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

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.

Jan