Re: [PATCH v2 2/6] x86/gnttab: do not implement GNTTABOP_cache_flush

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 5/16/25 11:45 AM, 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é <roger.pau@xxxxxxxxxx>
> ---
> Changes since v1:
>  - Introduce Kconfig option.
>  - Introduce CHANGELOG entry.
> ---
>  CHANGELOG.md                        |  3 +++
>  xen/arch/x86/include/asm/flushtlb.h | 19 -------------------
>  xen/common/Kconfig                  |  5 +++++
>  xen/common/grant_table.c            |  6 ++++++
>  4 files changed, 14 insertions(+), 19 deletions(-)
>
> diff --git a/CHANGELOG.md b/CHANGELOG.md
> index 1ea06524db20..21d7be0aa389 100644
> --- a/CHANGELOG.md
> +++ b/CHANGELOG.md
> @@ -24,6 +24,9 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
>      - Ability to enable stack protector
>  
>  ### Removed
> + - On x86:
> +   - GNTTABOP_cache_flush: it's unused on x86 and the implementation is
> +     broken.
>     
LGTM: Reviewed-By: Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>

Thanks.

~ Oleksii

>  ## [4.20.0](https://xenbits.xenproject.org/gitweb/?p=xen.git;a=shortlog;h=RELEASE-4.20.0) - 2025-03-05
>  
> diff --git a/xen/arch/x86/include/asm/flushtlb.h b/xen/arch/x86/include/asm/flushtlb.h
> index 209ea1e62fae..cd625f911436 100644
> --- a/xen/arch/x86/include/asm/flushtlb.h
> +++ b/xen/arch/x86/include/asm/flushtlb.h
> @@ -184,25 +184,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)
> -{
> -    unsigned int order = get_order_from_bytes(size);
> -
> -    /* sub-page granularity support needs to be added if necessary */
> -    flush_area_local(p, FLUSH_CACHE_WRITEBACK | FLUSH_ORDER(order));
> -    return 0;
> -}
>  
>  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/Kconfig b/xen/common/Kconfig
> index 6d43be2e6e8a..563b036474c0 100644
> --- a/xen/common/Kconfig
> +++ b/xen/common/Kconfig
> @@ -35,6 +35,11 @@ config GRANT_TABLE
>  
>  	  If unsure, say Y.
>  
> +config HAS_GRANT_CACHE_FLUSH
> +	bool
> +	depends on GRANT_TABLE
> +	default ARM
> +
>  config EVTCHN_FIFO
>  	bool "Event Channel Fifo support" if EXPERT
>  	default y
> diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
> index e75ff98aff1c..cf131c43a1f1 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_HAS_GRANT_CACHE_FLUSH
>  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_HAS_GRANT_CACHE_FLUSH */
>  
>  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_HAS_GRANT_CACHE_FLUSH
>  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_HAS_GRANT_CACHE_FLUSH */
>  
>  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_HAS_GRANT_CACHE_FLUSH
>      case GNTTABOP_cache_flush:
>      {
>          XEN_GUEST_HANDLE_PARAM(gnttab_cache_flush_t) cflush =
> @@ -3789,6 +3794,7 @@ long do_grant_table_op(
>          }
>          break;
>      }
> +#endif /* CONFIG_HAS_GRANT_CACHE_FLUSH */
>  
>      default:
>          rc = -ENOSYS;
>