On 21.02.2024 16:46, Nicola Vetrini wrote: > 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 <jul...@xen.org> >>>>> Signed-off-by: Nicola Vetrini <nicola.vetr...@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.coop...@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?
Yeah, introducing xen/flushtlb.h to hold these sounds pretty sensible. > 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. I'm afraid I don't understand this remark. Jan
