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

[Nicola Vetrini <nicola.vetrini@xxxxxxxxxxx>]

Hi Stefano,

On 2024-02-24 00:05, Stefano Stabellini wrote:
> 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
>
> ?
Yes, this is what was suggested and what I was trying to accomplish.
Basically my plan is:

1. move the arch_grant_cache_flush helper to asm/grant_table.h for both 
architectures
2. pull out of xen/mm.h this hunk (note the inclusion of asm/flushtlb in 
the middle of the file) because there is a build error on 
tlbflush_current_time() induced in some .c file (don't remember which) 
by the earlier movement
-#include <asm/flushtlb.h>
-
-static inline void accumulate_tlbflush(bool *need_tlbflush,
-                                       const struct page_info *page,
-                                       uint32_t *tlbflush_timestamp)
-{
-    if ( page->u.free.need_tlbflush &&
-         page->tlbflush_timestamp <= tlbflush_current_time() &&
-         (!*need_tlbflush ||
-          page->tlbflush_timestamp > *tlbflush_timestamp) )
-    {
-        *need_tlbflush = true;
-        *tlbflush_timestamp = page->tlbflush_timestamp;
-    }
-}
-
-static inline void filtered_flush_tlb_mask(uint32_t tlbflush_timestamp)
-{
-    cpumask_t mask;
-
-    cpumask_copy(&mask, &cpu_online_map);
-    tlbflush_filter(&mask, tlbflush_timestamp);
-    if ( !cpumask_empty(&mask) )
-    {
-        perfc_incr(need_flush_tlb_flush);
-        arch_flush_tlb_mask(&mask);
-    }
-}
-

which is going to be in a new header xen/flushtlb.h
3. replace various inclusions the previously relied on the fact that 
xen/mm.h included asm/flushtlb.h (some even stating this as evidenced 
from the hunk below)
diff --git a/xen/arch/x86/cpu/microcode/amd.c 
b/xen/arch/x86/cpu/microcode/amd.c
index 75fc84e445ce..91ee7e6ec39e 100644
--- a/xen/arch/x86/cpu/microcode/amd.c
+++ b/xen/arch/x86/cpu/microcode/amd.c
@@ -15,8 +15,8 @@
  */

 #include <xen/err.h>
+#include <xen/flushtlb.h>
 #include <xen/init.h>
-#include <xen/mm.h> /* TODO: Fix asm/tlbflush.h breakage */

and then make everything build.
However, the dependencies tied to xen/mm.h are quite numerous on x86, 
and I'm not seeing an obvious way to avoid touching xen/mm.h. See this 
tree [1] for the latest state of the patch.
If anyone has an idea how to tackle this in a smarter way, I'm open to 
suggestions.
Specifically in step (2) there might be a way to avoid doing that 
modification, perhaps.
[1] 
https://gitlab.com/xen-project/people/bugseng/xen/-/commits/cache_helpers
--
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)