Re: [PATCH 4/9] x86/gnttab: do not implement GNTTABOP_cache_flush

2025-05-12 Thread Jan Beulich
On 06.05.2025 10:31, Roger Pau Monne wrote:
> The current underlying implementation of GNTTABOP_cache_flush on x86 won't
> work as expected.  The provided {clean,invalidate}_dcache_va_range()
> helpers only do a local pCPU cache flush, so the cache of previous pCPUs
> where the vCPU might have run are not flushed.
> 
> However instead of attempting to fix this, make the GNTTABOP_cache_flush
> operation only available to ARM.  There are no known users on x86, the
> implementation is broken, and other architectures don't have grant-table
> support yet.
> 
> With that operation not implemented on x86, the related
> {clean,invalidate}_dcache_va_range() helpers can also be removed.
> 
> Fixes: f62dc81c2df7 ("x86: introduce more cache maintenance operations")
> Fixes: 18e8d22fe750 ("introduce GNTTABOP_cache_flush")
> Signed-off-by: Roger Pau Monné 

Ah, here we go. I think this is what we want, without patch 3. It will want
to come with a CHANGELOG entry, though.

> --- a/xen/common/grant_table.c
> +++ b/xen/common/grant_table.c
> @@ -940,6 +940,7 @@ static void reduce_status_for_pin(struct domain *rd,
>  gnttab_clear_flags(rd, clear_flags, status);
>  }
>  
> +#ifdef CONFIG_ARM

Better introduce a new Kconfig setting (e.g. HAS_GRANT_CACHE_FLUSH) right
away, in case RISC-V and/or PPC would also want such behavior? 

Jan



[PATCH 4/9] x86/gnttab: do not implement GNTTABOP_cache_flush

2025-05-06 Thread Roger Pau Monne
The current underlying implementation of GNTTABOP_cache_flush on x86 won't
work as expected.  The provided {clean,invalidate}_dcache_va_range()
helpers only do a local pCPU cache flush, so the cache of previous pCPUs
where the vCPU might have run are not flushed.

However instead of attempting to fix this, make the GNTTABOP_cache_flush
operation only available to ARM.  There are no known users on x86, the
implementation is broken, and other architectures don't have grant-table
support yet.

With that operation not implemented on x86, the related
{clean,invalidate}_dcache_va_range() helpers can also be removed.

Fixes: f62dc81c2df7 ("x86: introduce more cache maintenance operations")
Fixes: 18e8d22fe750 ("introduce GNTTABOP_cache_flush")
Signed-off-by: Roger Pau Monné 
---
I've attempted to introduce a new arch_do_grant_table_op() in a separate
arch-specific file, but it required exposing too much functionality from
the common grant_table.c, ifdefying is probably better for the time being.
---
 xen/arch/x86/include/asm/flushtlb.h | 15 ---
 xen/common/grant_table.c|  6 ++
 2 files changed, 6 insertions(+), 15 deletions(-)

diff --git a/xen/arch/x86/include/asm/flushtlb.h 
b/xen/arch/x86/include/asm/flushtlb.h
index bb0ad58db49b..d0c9120b5faf 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/common/grant_table.c b/xen/common/grant_table.c
index d874ac5f1241..cc7c7d004065 100644
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -940,6 +940,7 @@ static void reduce_status_for_pin(struct domain *rd,
 gnttab_clear_flags(rd, clear_flags, status);
 }
 
+#ifdef CONFIG_ARM
 static struct active_grant_entry *grant_map_exists(const struct domain *ld,
struct grant_table *rgt,
mfn_t mfn,
@@ -975,6 +976,7 @@ static struct active_grant_entry *grant_map_exists(const 
struct domain *ld,
 
 return ERR_PTR(-EINVAL);
 }
+#endif /* CONFIG_ARM */
 
 union maptrack_node {
 struct {
@@ -3520,6 +3522,7 @@ 
gnttab_swap_grant_ref(XEN_GUEST_HANDLE_PARAM(gnttab_swap_grant_ref_t) uop,
 return 0;
 }
 
+#ifdef CONFIG_ARM
 static int _cache_flush(const gnttab_cache_flush_t *cflush, grant_ref_t 
*cur_ref)
 {
 struct domain *d, *owner;
@@ -3631,6 +3634,7 @@ 
gnttab_cache_flush(XEN_GUEST_HANDLE_PARAM(gnttab_cache_flush_t) uop,
 
 return 0;
 }
+#endif /* CONFIG_ARM */
 
 long do_grant_table_op(
 unsigned int cmd, XEN_GUEST_HANDLE_PARAM(void) uop, unsigned int count)
@@ -3773,6 +3777,7 @@ long do_grant_table_op(
 break;
 }
 
+#ifdef CONFIG_ARM
 case GNTTABOP_cache_flush:
 {
 XEN_GUEST_HANDLE_PARAM(gnttab_cache_flush_t) cflush =
@@ -3794,6 +3799,7 @@ long do_grant_table_op(
 }
 break;
 }
+#endif
 
 default:
 rc = -ENOSYS;
-- 
2.48.1