[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [XEN PATCH] xen: cache clearing and invalidation helpers refactoring
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 returnsan integer is introduced, so that each architecture can choose whether to return an error value on certain conditions, and the helpers have eitherbeen 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 proposalto 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 void2. 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 effectivelymeans that nearly every CU will have a dependency on that header, no matter that most are entirely agnostic of grants. Each arch has agrant_table.h - is there any reason the new, grant-specific helper can'tbe 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@xxxxxxxxxx/ -- Nicola Vetrini, BSc Software Engineer, BUGSENG srl (https://bugseng.com)
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |