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

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

On 2024-02-20 09:14, Nicola Vetrini wrote:
> 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
> 

The only blocker so far is that this triggers a build error due to a 
circular dependency between xen/mm.h and asm/flushtlb.h on x86. Also 
found some earlier evidence [1] that there are some oddities around 
asm/flushtlb's inclusion.

[1] 
https://lore.kernel.org/xen-devel/20200318210540.5602-1-andrew.cooper3@citrix.com/

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