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

2025-05-19 Thread Oleksii Kurochko 


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é
---
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

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

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

2025-05-16 Thread Roger Pau Monné 
On Fri, May 16, 2025 at 12:38:02PM +0200, Jan Beulich wrote:
> On 16.05.2025 12:31, Roger Pau Monné wrote:
> > On Fri, May 16, 2025 at 11:48:48AM +0200, Jan Beulich wrote:
> >> On 16.05.2025 11:45, Roger Pau Monne wrote:
> >>> --- 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
> >>
> >> To keep arch stuff out of common file as much as possible, I think this 
> >> instead
> >> wants to be a "select ..." from ARM. Then:
> >> Reviewed-by: Jan Beulich 
> > 
> > My first attempt was to do it as you suggest, but then if the users
> > disables GRANT_TABLE you get the following warning:
> > 
> > WARNING: unmet direct dependencies detected for HAS_GRANT_CACHE_FLUSH
> >   Depends on [n]: GRANT_TABLE [=n]
> >   Selected by [y]:
> >   - ARM [=y]
> > configuration written to .config
> 
> Right, it needs to be
> 
>   select HAS_GRANT_CACHE_FLUSH if GRANT_TABLE
> 
> (and the "depends on" on the new HAS_* can also go away; HAS_* imo shouldn't
> normally have any dependencies).

Hm, OK, I didn't like it this way because then we force all users of
HAS_GRANT_CACHE_FLUSH to make it depend on GRANT_TABLE, while putting
the dependency in HAS_GRANT_CACHE_FLUSH avoids that.

Anyway, have adjusted according to your suggestion.

Thanks, Roger.

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

2025-05-16 Thread Roger Pau Monné 
On Fri, May 16, 2025 at 11:48:48AM +0200, Jan Beulich wrote:
> On 16.05.2025 11:45, Roger Pau Monne wrote:
> > --- 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
> 
> To keep arch stuff out of common file as much as possible, I think this 
> instead
> wants to be a "select ..." from ARM. Then:
> Reviewed-by: Jan Beulich 

My first attempt was to do it as you suggest, but then if the users
disables GRANT_TABLE you get the following warning:

WARNING: unmet direct dependencies detected for HAS_GRANT_CACHE_FLUSH
  Depends on [n]: GRANT_TABLE [=n]
  Selected by [y]:
  - ARM [=y]
configuration written to .config

And you end up with the following on .config:

# CONFIG_GRANT_TABLE is not set
CONFIG_HAS_GRANT_CACHE_FLUSH=y

That's why I've done it the way presented in this patch.

Thanks, Roger.

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

2025-05-16 Thread Jan Beulich 
On 16.05.2025 12:31, Roger Pau Monné wrote:
> On Fri, May 16, 2025 at 11:48:48AM +0200, Jan Beulich wrote:
>> On 16.05.2025 11:45, Roger Pau Monne wrote:
>>> --- 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
>>
>> To keep arch stuff out of common file as much as possible, I think this 
>> instead
>> wants to be a "select ..." from ARM. Then:
>> Reviewed-by: Jan Beulich 
> 
> My first attempt was to do it as you suggest, but then if the users
> disables GRANT_TABLE you get the following warning:
> 
> WARNING: unmet direct dependencies detected for HAS_GRANT_CACHE_FLUSH
>   Depends on [n]: GRANT_TABLE [=n]
>   Selected by [y]:
>   - ARM [=y]
> configuration written to .config

Right, it needs to be

select HAS_GRANT_CACHE_FLUSH if GRANT_TABLE

(and the "depends on" on the new HAS_* can also go away; HAS_* imo shouldn't
normally have any dependencies).

Jan

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

2025-05-16 Thread Jan Beulich 
On 16.05.2025 11:45, Roger Pau Monne wrote:
> --- 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

To keep arch stuff out of common file as much as possible, I think this instead
wants to be a "select ..." from ARM. Then:
Reviewed-by: Jan Beulich 

Jan

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

2025-05-16 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é 
---
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.
 
 ## 
[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;
-- 
2.48.1