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 <[email protected]>
Signed-off-by: Nicola Vetrini <[email protected]>
---
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/[email protected]/
--
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)
