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

[Nicola Vetrini <nicola.vetrini@bugseng.com>]

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@xen.org>
>> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
>> ---
>> 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)