On Sat, 24 Feb 2024, Nicola Vetrini wrote:
> 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 <[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.
> > > >
> > > > [1]
> > > >
> > > https://lore.kernel.org/xen-devel/[email protected]/
> > > > ---
> > > > 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
It looks like it would be easier to resolve the build error on
tlbflush_current_time() in another way. What's the build error exactly?
Looking at the implementation of tlbflush_current_time on the various
arches I couldn't find any potential issues.
I just moved the implementation of arch_grant_cache_flush to arch
specific headers and it seemed to have worked for me. See below. Maybe I
am building with a different kconfig.
diff --git a/xen/arch/arm/include/asm/grant_table.h
b/xen/arch/arm/include/asm/grant_table.h
index d3c518a926..2c5c07e061 100644
--- a/xen/arch/arm/include/asm/grant_table.h
+++ b/xen/arch/arm/include/asm/grant_table.h
@@ -76,6 +76,20 @@ int replace_grant_host_mapping(uint64_t gpaddr, mfn_t frame,
#define gnttab_need_iommu_mapping(d) \
(is_domain_direct_mapped(d) && is_iommu_enabled(d))
+static inline int arch_grant_cache_flush(unsigned int op, const void *p,
+ unsigned long size)
+{
+ if ( (op & GNTTAB_CACHE_INVAL) && (op & GNTTAB_CACHE_CLEAN) )
+ clean_and_invalidate_dcache_va_range(p, size);
+ else if ( op & GNTTAB_CACHE_INVAL )
+ invalidate_dcache_va_range(p, size);
+ else if ( op & GNTTAB_CACHE_CLEAN )
+ clean_dcache_va_range(p, size);
+
+ /* ARM callers assume that dcache_* functions cannot fail. */
+ return 0;
+}
+
#endif /* __ASM_GRANT_TABLE_H__ */
/*
* Local variables:
diff --git a/xen/arch/arm/include/asm/page.h b/xen/arch/arm/include/asm/page.h
index 69f817d1e6..aea692a24d 100644
--- a/xen/arch/arm/include/asm/page.h
+++ b/xen/arch/arm/include/asm/page.h
@@ -159,13 +159,13 @@ static inline size_t read_dcache_line_bytes(void)
* if 'range' is large enough we might want to use model-specific
* full-cache flushes. */
-static inline int invalidate_dcache_va_range(const void *p, unsigned long size)
+static inline void invalidate_dcache_va_range(const void *p, unsigned long
size)
{
size_t cacheline_mask = dcache_line_bytes - 1;
unsigned long idx = 0;
if ( !size )
- return 0;
+ return;
/* Passing a region that wraps around is illegal */
ASSERT(((uintptr_t)p + size - 1) >= (uintptr_t)p);
@@ -188,17 +188,15 @@ static inline int invalidate_dcache_va_range(const void
*p, unsigned long size)
asm volatile (__clean_and_invalidate_dcache_one(0) : : "r" (p + idx));
dsb(sy); /* So we know the flushes happen before continuing */
-
- return 0;
}
-static inline int clean_dcache_va_range(const void *p, unsigned long size)
+static inline void clean_dcache_va_range(const void *p, unsigned long size)
{
size_t cacheline_mask = dcache_line_bytes - 1;
unsigned long idx = 0;
if ( !size )
- return 0;
+ return;
/* Passing a region that wraps around is illegal */
ASSERT(((uintptr_t)p + size - 1) >= (uintptr_t)p);
@@ -211,18 +209,16 @@ static inline int clean_dcache_va_range(const void *p,
unsigned long size)
idx += dcache_line_bytes, size -= dcache_line_bytes )
asm volatile (__clean_dcache_one(0) : : "r" (p + idx));
dsb(sy); /* So we know the flushes happen before continuing */
- /* ARM callers assume that dcache_* functions cannot fail. */
- return 0;
}
-static inline int clean_and_invalidate_dcache_va_range
+static inline void clean_and_invalidate_dcache_va_range
(const void *p, unsigned long size)
{
size_t cacheline_mask = dcache_line_bytes - 1;
unsigned long idx = 0;
if ( !size )
- return 0;
+ return;
/* Passing a region that wraps around is illegal */
ASSERT(((uintptr_t)p + size - 1) >= (uintptr_t)p);
@@ -235,8 +231,6 @@ static inline int clean_and_invalidate_dcache_va_range
idx += dcache_line_bytes, size -= dcache_line_bytes )
asm volatile (__clean_and_invalidate_dcache_one(0) : : "r" (p + idx));
dsb(sy); /* So we know the flushes happen before continuing */
- /* ARM callers assume that dcache_* functions cannot fail. */
- return 0;
}
/* Macros for flushing a single small item. The predicate is always
diff --git a/xen/arch/x86/include/asm/flushtlb.h
b/xen/arch/x86/include/asm/flushtlb.h
index bb0ad58db4..d0c9120b5f 100644
--- a/xen/arch/x86/include/asm/flushtlb.h
+++ b/xen/arch/x86/include/asm/flushtlb.h
@@ -182,21 +182,6 @@ void flush_area_mask(const cpumask_t *mask, const void *va,
}
static inline void flush_page_to_ram(unsigned long mfn, bool sync_icache) {}
-static inline int invalidate_dcache_va_range(const void *p,
- unsigned long size)
-{ return -EOPNOTSUPP; }
-static inline int clean_and_invalidate_dcache_va_range(const void *p,
- unsigned long size)
-{
- unsigned int order = get_order_from_bytes(size);
- /* sub-page granularity support needs to be added if necessary */
- flush_area_local(p, FLUSH_CACHE|FLUSH_ORDER(order));
- return 0;
-}
-static inline int clean_dcache_va_range(const void *p, unsigned long size)
-{
- return clean_and_invalidate_dcache_va_range(p, size);
-}
unsigned int guest_flush_tlb_flags(const struct domain *d);
void guest_flush_tlb_mask(const struct domain *d, const cpumask_t *mask);
diff --git a/xen/arch/x86/include/asm/grant_table.h
b/xen/arch/x86/include/asm/grant_table.h
index 5c23cec90c..60a6dbb231 100644
--- a/xen/arch/x86/include/asm/grant_table.h
+++ b/xen/arch/x86/include/asm/grant_table.h
@@ -72,4 +72,20 @@ static inline void gnttab_clear_flags(struct domain *d,
#define gnttab_need_iommu_mapping(d) \
(!paging_mode_translate(d) && need_iommu_pt_sync(d))
+static inline int arch_grant_cache_flush(unsigned int op, const void *p,
+ unsigned long size)
+{
+ unsigned int order;
+
+ if ( !(op & GNTTAB_CACHE_CLEAN) )
+ return -EOPNOTSUPP;
+
+ order = get_order_from_bytes(size);
+ /* sub-page granularity support needs to be added if necessary */
+ (void) flush_area_local(p, FLUSH_CACHE|FLUSH_ORDER(order));
+
+ return 0;
+}
+
+
#endif /* __ASM_GRANT_TABLE_H__ */
diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
index 37b178a67b..0df663944f 100644
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -3575,14 +3575,7 @@ static int _cache_flush(const gnttab_cache_flush_t
*cflush, grant_ref_t *cur_ref
v = map_domain_page(mfn);
v += cflush->offset;
- if ( (cflush->op & GNTTAB_CACHE_INVAL) && (cflush->op &
GNTTAB_CACHE_CLEAN) )
- ret = clean_and_invalidate_dcache_va_range(v, cflush->length);
- else if ( cflush->op & GNTTAB_CACHE_INVAL )
- ret = invalidate_dcache_va_range(v, cflush->length);
- else if ( cflush->op & GNTTAB_CACHE_CLEAN )
- ret = clean_dcache_va_range(v, cflush->length);
- else
- ret = 0;
+ ret = arch_grant_cache_flush(cflush->op, v, cflush->length);
if ( d != owner )
{
