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

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

On 2024-02-21 13:08, Nicola Vetrini wrote:
> 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/

There could be a way of untangling asm/flushtlb.h from xen/mm.h, by 
moving "accumulate_tlbflush" and "filtered_flush_tlb_mask" introduced by 
commit 80943aa40e30 ("replace tlbflush check and operation with inline 
functions") [1].
However, these function should then be part of a generic xen/flushtlb.h 
header, since they are used in common code (e.g., common/page_alloc) and 
a bunch of common code source files should move their includes (see [2] 
for a partial non-working patch). Do you feel that this is a feasible 
route?
In passing it should be noted that the header ordering in 
x86/alternative.c is not the one usually prescribed, so that may be 
taken care of as well.

[1] 
https://lore.kernel.org/xen-devel/1474338664-5054-1-git-send-email-dongli.zhang@oracle.com/
[2] 
https://gitlab.com/xen-project/people/bugseng/xen/-/commit/a2be0927f724e7e9f891d1e00831739137c29041

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