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 <jul...@xen.org>
Signed-off-by: Nicola Vetrini <nicola.vetr...@bugseng.com>
---
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-cf485e4b6...@xen.org/
---
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.
diff --git a/xen/arch/arm/include/asm/page.h
b/xen/arch/arm/include/asm/page.h
index 69f817d1e68a..e90c9de3616e 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>
@@ -159,13 +160,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 +189,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 +210,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 +232,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
@@ -266,6 +261,20 @@ static inline int
clean_and_invalidate_dcache_va_range
: : "r" (_p), "m" (*_p));
\
} while (0)
+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;
+}
+
/*
* Write a pagetable entry.
*
diff --git a/xen/arch/x86/include/asm/flushtlb.h
b/xen/arch/x86/include/asm/flushtlb.h
index bb0ad58db49b..c37bf4455714 100644
--- a/xen/arch/x86/include/asm/flushtlb.h
+++ b/xen/arch/x86/include/asm/flushtlb.h
@@ -10,6 +10,7 @@
#ifndef __FLUSHTLB_H__
#define __FLUSHTLB_H__
+#include <public/grant_table.h>
#include <xen/mm.h>
#include <xen/percpu.h>
#include <xen/smp.h>
@@ -182,21 +183,21 @@ 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)
+
+static inline int arch_grant_cache_flush(unsigned int op, const void
*p,
+ unsigned long size)
{
- unsigned int order = get_order_from_bytes(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 */
- flush_area_local(p, FLUSH_CACHE|FLUSH_ORDER(order));
+ (void) 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/common/grant_table.c b/xen/common/grant_table.c
index 5721eab22561..8615ea144bb3 100644
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -3572,14 +3572,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 )
{
--
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)
