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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Fri, 23 Feb 2024, Nicola Vetrini wrote:
> On 2024-02-19 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@xxxxxxx>
> > Signed-off-by: Nicola Vetrini <nicola.vetrini@xxxxxxxxxxx>
> > ---
> > 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.
> > 
> > [1]
> > https://lore.kernel.org/xen-devel/09589e8f-77b6-47f7-b5bd-cf485e4b60f1@xxxxxxx/
> > ---
> >  xen/arch/arm/include/asm/page.h     | 33 ++++++++++++++++++-----------
> >  xen/arch/x86/include/asm/flushtlb.h | 23 ++++++++++----------
> >  xen/common/grant_table.c            |  9 +-------
> >  3 files changed, 34 insertions(+), 31 deletions(-)
> > 
> 
> I'll put this patch in the backlog at the moment: too many intricacies while
> trying to untangle xen/flushtlb from xen/mm.h, and there are easier cases that
> can be done faster. If someone is interested I can post the partial work I've
> done so far, even though it doesn't
> build on x86.

I understand that the blocker is:

diff --git a/xen/arch/arm/include/asm/page.h b/xen/arch/arm/include/asm/page.h
index 69f817d1e6..e90c9de361 100644
--- 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>
 #include <xen/errno.h>
 #include <xen/types.h>
 #include <xen/lib.h>


And the headers disentagling required to solve it, right?


Let me ask a silly question. public/grant_table.h seems needed by
arch_grant_cache_flush. Can we move arch_grant_cache_flush somewhere
else? It is not like page.h is a perfect fit for it anyway.

For instance, can we move it to 

xen/arch/arm/include/asm/grant_table.h

?