[PATCH v2 0/6] xen: cache control improvements

[PATCH v2 0/6] xen: cache control improvements

[Roger Pau Monne]

Hello,

Following series contain some fixes for cache control operations, the
main focus is to reduce the load on big systems when cache control
operations are executed.

Patches 1-4 are bugfixes, while patches 5 and 6 are improvements
to the current code.

Thanks, Roger.

Roger Pau Monne (6):
  x86/pv: fix emulation of wb{,no}invd to flush all pCPU caches
  x86/gnttab: do not implement GNTTABOP_cache_flush
  xen/x86: rename cache_flush_permitted() to has_arch_io_resources()
  xen/x86: account for assigned PCI devices in cache_flush_permitted()
  x86/hvm: limit memory type cache flush to running domains
  x86/hvm: reduce the need to flush caches in memory_type_changed()

 CHANGELOG.md                        |  3 +++
 xen/arch/x86/hvm/mtrr.c             | 17 +++++++++++++++--
 xen/arch/x86/include/asm/flushtlb.h | 19 -------------------
 xen/arch/x86/include/asm/iocap.h    |  5 ++++-
 xen/arch/x86/mm.c                   |  4 +---
 xen/arch/x86/mm/p2m-pod.c           |  4 ++--
 xen/arch/x86/pv/emul-priv-op.c      |  6 +++---
 xen/common/Kconfig                  |  5 +++++
 xen/common/grant_table.c            |  6 ++++++
 xen/common/memory.c                 |  2 +-
 xen/include/asm-generic/iocap.h     |  4 +++-
 11 files changed, 43 insertions(+), 32 deletions(-)

-- 
2.48.1

[PATCH v2 1/6] x86/pv: fix emulation of wb{,no}invd to flush all pCPU caches

[Roger Pau Monne]

The current emulation of wb{,no}invd is bogus for PV guests: it will only
flush the current pCPU cache, without taking into account pCPUs where the
vCPU had run previously.  Resort to flushing the cache on all host pCPUs to
make it correct.

Fixes: 799fed0a7cc5 ("Priv-op emulation in Xen, for RDMSR/WRMSR/WBINVD")
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Changes since v1:
 - Use the recently introduced FLUSH_CACHE_WRITEBACK.
---
 xen/arch/x86/pv/emul-priv-op.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/xen/arch/x86/pv/emul-priv-op.c b/xen/arch/x86/pv/emul-priv-op.c
index 295d847ea24c..20a90703c83c 100644
--- a/xen/arch/x86/pv/emul-priv-op.c
+++ b/xen/arch/x86/pv/emul-priv-op.c
@@ -1198,13 +1198,13 @@ static int cf_check cache_op(
     if ( !cache_flush_permitted(current->domain) )
         /*
          * Non-physdev domain attempted WBINVD; ignore for now since
-         * newer linux uses this in some start-of-day timing loops.
+         * Linux uses this in some start-of-day code.
          */
         ;
     else if ( op == x86emul_wbnoinvd /* && cpu_has_wbnoinvd */ )
-        wbnoinvd();
+        flush_all(FLUSH_CACHE_WRITEBACK);
     else
-        wbinvd();
+        flush_all(FLUSH_CACHE);
 
     return X86EMUL_OKAY;
 }
-- 
2.48.1

Re: [PATCH v2 1/6] x86/pv: fix emulation of wb{,no}invd to flush all pCPU caches

[Jan Beulich]

On 16.05.2025 11:45, Roger Pau Monne wrote:
> The current emulation of wb{,no}invd is bogus for PV guests: it will only
> flush the current pCPU cache, without taking into account pCPUs where the
> vCPU had run previously.  Resort to flushing the cache on all host pCPUs to
> make it correct.
> 
> Fixes: 799fed0a7cc5 ("Priv-op emulation in Xen, for RDMSR/WRMSR/WBINVD")
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>

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

[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é <roger.pau@citrix.com>
---
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

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

[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 <jbeulich@suse.com>

Jan

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

[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 <jbeulich@suse.com>

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

[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 <jbeulich@suse.com>
> 
> 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

[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 <jbeulich@suse.com>
> > 
> > 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

[Oleksii Kurochko]

[-- Attachment #1: Type: text/plain, Size: 5429 bytes --]


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@citrix.com>
> ---
> 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@gmail.com>

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;

[-- Attachment #2: Type: text/html, Size: 6003 bytes --]

[PATCH v2 3/6] xen/x86: rename cache_flush_permitted() to has_arch_io_resources()

[Roger Pau Monne]

To better describe the underlying implementation.  Define
cache_flush_permitted() as an alias of has_arch_io_resources(), so that
current users of cache_flush_permitted() are not effectively modified.

With the introduction of the new handler, change some of the call sites of
cache_flush_permitted() to instead use has_arch_io_resources() as such
callers are not after whether cache flush is enabled, but rather whether
the domain has any IO resources assigned.

No functional change intended.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Changes since v1:
 - Drop adjustment to l1_disallow_mask().
---
 xen/arch/x86/include/asm/iocap.h | 4 +++-
 xen/arch/x86/mm/p2m-pod.c        | 4 ++--
 xen/common/memory.c              | 2 +-
 xen/include/asm-generic/iocap.h  | 4 +++-
 4 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/xen/arch/x86/include/asm/iocap.h b/xen/arch/x86/include/asm/iocap.h
index 53d87ae8a334..61d026dbf5f6 100644
--- a/xen/arch/x86/include/asm/iocap.h
+++ b/xen/arch/x86/include/asm/iocap.h
@@ -15,10 +15,12 @@
 #define ioports_access_permitted(d, s, e)               \
     rangeset_contains_range((d)->arch.ioport_caps, s, e)
 
-#define cache_flush_permitted(d)                        \
+#define has_arch_io_resources(d)                        \
     (!rangeset_is_empty((d)->iomem_caps) ||             \
      !rangeset_is_empty((d)->arch.ioport_caps))
 
+#define cache_flush_permitted has_arch_io_resources
+
 static inline int ioports_permit_access(struct domain *d, unsigned long s,
                                         unsigned long e)
 {
diff --git a/xen/arch/x86/mm/p2m-pod.c b/xen/arch/x86/mm/p2m-pod.c
index df2a1cc0749b..05633fe2ac88 100644
--- a/xen/arch/x86/mm/p2m-pod.c
+++ b/xen/arch/x86/mm/p2m-pod.c
@@ -338,7 +338,7 @@ p2m_pod_set_mem_target(struct domain *d, unsigned long target)
 
     ASSERT( pod_target >= p2m->pod.count );
 
-    if ( has_arch_pdevs(d) || cache_flush_permitted(d) )
+    if ( has_arch_pdevs(d) || has_arch_io_resources(d) )
         ret = -ENOTEMPTY;
     else
         ret = p2m_pod_set_cache_target(p2m, pod_target, 1/*preemptible*/);
@@ -1395,7 +1395,7 @@ guest_physmap_mark_populate_on_demand(struct domain *d, unsigned long gfn,
     if ( !paging_mode_translate(d) )
         return -EINVAL;
 
-    if ( has_arch_pdevs(d) || cache_flush_permitted(d) )
+    if ( has_arch_pdevs(d) || has_arch_io_resources(d) )
         return -ENOTEMPTY;
 
     do {
diff --git a/xen/common/memory.c b/xen/common/memory.c
index 8ca4e1a8425b..46620ed8253d 100644
--- a/xen/common/memory.c
+++ b/xen/common/memory.c
@@ -86,7 +86,7 @@ static unsigned int max_order(const struct domain *d)
     unsigned int order = domu_max_order;
 
 #ifdef CONFIG_HAS_PASSTHROUGH
-    if ( cache_flush_permitted(d) && order < ptdom_max_order )
+    if ( has_arch_io_resources(d) && order < ptdom_max_order )
         order = ptdom_max_order;
 #endif
 
diff --git a/xen/include/asm-generic/iocap.h b/xen/include/asm-generic/iocap.h
index dd7cb45488f7..664bbc8971fe 100644
--- a/xen/include/asm-generic/iocap.h
+++ b/xen/include/asm-generic/iocap.h
@@ -2,9 +2,11 @@
 #ifndef __ASM_GENERIC_IOCAP_H__
 #define __ASM_GENERIC_IOCAP_H__
 
-#define cache_flush_permitted(d)                        \
+#define has_arch_io_resources(d)                        \
     (!rangeset_is_empty((d)->iomem_caps))
 
+#define cache_flush_permitted has_arch_io_resources
+
 #endif /* __ASM_GENERIC_IOCAP_H__ */
 
 /*
-- 
2.48.1

Re: [PATCH v2 3/6] xen/x86: rename cache_flush_permitted() to has_arch_io_resources()

[Jan Beulich]

On 16.05.2025 11:45, Roger Pau Monne wrote:
> To better describe the underlying implementation.  Define
> cache_flush_permitted() as an alias of has_arch_io_resources(), so that
> current users of cache_flush_permitted() are not effectively modified.
> 
> With the introduction of the new handler, change some of the call sites of
> cache_flush_permitted() to instead use has_arch_io_resources() as such
> callers are not after whether cache flush is enabled, but rather whether
> the domain has any IO resources assigned.
> 
> No functional change intended.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
> Changes since v1:
>  - Drop adjustment to l1_disallow_mask().

Moving the change to the next patch instead, as I see. For both
patches:
Reviewed-by: Jan Beulich <jbeulich@suse.com>

Jan

[PATCH v2 4/6] xen/x86: account for assigned PCI devices in cache_flush_permitted()

[Roger Pau Monne]

While unlikely, it's possible for PCI devices to not have any IO resources
assigned, yet in such case the owner domain might still need to issue cache
control operations in case the device performs DMA requests.

Adjust cache_flush_permitted() to account for has_arch_pdevs().

While there also switch l1_disallow_mask() to use cache_flush_permitted().
This should be a non-functional change after the adjustment done to
cache_flush_permitted().

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Changes since v1:
 - New in this version.
---
 xen/arch/x86/include/asm/iocap.h | 3 ++-
 xen/arch/x86/mm.c                | 4 +---
 2 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/xen/arch/x86/include/asm/iocap.h b/xen/arch/x86/include/asm/iocap.h
index 61d026dbf5f6..f948b7186e95 100644
--- a/xen/arch/x86/include/asm/iocap.h
+++ b/xen/arch/x86/include/asm/iocap.h
@@ -19,7 +19,8 @@
     (!rangeset_is_empty((d)->iomem_caps) ||             \
      !rangeset_is_empty((d)->arch.ioport_caps))
 
-#define cache_flush_permitted has_arch_io_resources
+#define cache_flush_permitted(d) \
+    (has_arch_io_resources(d) || has_arch_pdevs(d))
 
 static inline int ioports_permit_access(struct domain *d, unsigned long s,
                                         unsigned long e)
diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index a1703db762e3..657623336c0e 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -172,9 +172,7 @@ static DEFINE_SPINLOCK(subpage_ro_lock);
 
 #define l1_disallow_mask(d)                                     \
     (((d) != dom_io) &&                                         \
-     (rangeset_is_empty((d)->iomem_caps) &&                     \
-      rangeset_is_empty((d)->arch.ioport_caps) &&               \
-      !has_arch_pdevs(d) &&                                     \
+     (!cache_flush_permitted(d) &&                              \
       is_pv_domain(d)) ?                                        \
      L1_DISALLOW_MASK : (L1_DISALLOW_MASK & ~PAGE_CACHE_ATTRS))
 
-- 
2.48.1

[PATCH v2 5/6] x86/hvm: limit memory type cache flush to running domains

[Roger Pau Monne]

Avoid the cache flush if the domain is not yet running.  There shouldn't be
any cached data resulting from domain accesses that need flushing, as the
domain hasn't run yet.

No change in domain observable behavior intended.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Changes since v1:
 - New in this version.
---
 xen/arch/x86/hvm/mtrr.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/xen/arch/x86/hvm/mtrr.c b/xen/arch/x86/hvm/mtrr.c
index 402a1d926337..8e1e15af8d73 100644
--- a/xen/arch/x86/hvm/mtrr.c
+++ b/xen/arch/x86/hvm/mtrr.c
@@ -783,7 +783,13 @@ HVM_REGISTER_SAVE_RESTORE(MTRR, hvm_save_mtrr_msr, NULL, hvm_load_mtrr_msr, 1,
 void memory_type_changed(struct domain *d)
 {
     if ( (is_iommu_enabled(d) || cache_flush_permitted(d)) &&
-         d->vcpu && d->vcpu[0] && p2m_memory_type_changed(d) )
+         d->vcpu && d->vcpu[0] && p2m_memory_type_changed(d) &&
+         /*
+          * Do the p2m type-change, but skip the cache flush if the domain is
+          * not yet running.  The check for creation_finished must strictly be
+          * done after the call to p2m_memory_type_changed().
+          */
+         d->creation_finished )
     {
         flush_all(FLUSH_CACHE);
     }
-- 
2.48.1

Re: [PATCH v2 5/6] x86/hvm: limit memory type cache flush to running domains

[Jan Beulich]

On 16.05.2025 11:45, Roger Pau Monne wrote:
> Avoid the cache flush if the domain is not yet running.  There shouldn't be
> any cached data resulting from domain accesses that need flushing, as the
> domain hasn't run yet.

This again plays into what we started discussing already: There may still be
data in caches due to Xen or toolsstack behavior. Imo to compensate we'd need
to do one flush right before unleashing the domain. Yet of course this makes
sense only when we make sure to not have (cachable) mapping in Xen for any of
the affected ranges. Hence, with that not presently being the case, ...

> No change in domain observable behavior intended.

... I agree here, thus ...

> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>

The situation may want discussing a bit more in the description, though,
which would make me feel less uneasy about that R-b.

Jan

Re: [PATCH v2 5/6] x86/hvm: limit memory type cache flush to running domains

[Roger Pau Monné]

On Sun, May 18, 2025 at 01:38:02PM +0200, Jan Beulich wrote:
> On 16.05.2025 11:45, Roger Pau Monne wrote:
> > Avoid the cache flush if the domain is not yet running.  There shouldn't be
> > any cached data resulting from domain accesses that need flushing, as the
> > domain hasn't run yet.
> 
> This again plays into what we started discussing already: There may still be
> data in caches due to Xen or toolsstack behavior. Imo to compensate we'd need
> to do one flush right before unleashing the domain. Yet of course this makes
> sense only when we make sure to not have (cachable) mapping in Xen for any of
> the affected ranges. Hence, with that not presently being the case, ...
> 
> > No change in domain observable behavior intended.
> 
> ... I agree here, thus ...
> 
> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> 
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> 
> The situation may want discussing a bit more in the description, though,
> which would make me feel less uneasy about that R-b.

I've added:

"There can be data in the caches as a result of Xen and/or toolstack
behavior.  Ideally we would do a cache flush strictly before starting the
domain, however doing so only makes sense once we can guarantee there are
no leftover mappings of the affected ranges with cacheable attributes,
otherwise the CPU can speculatively populate the cache with data from those
ranges."

Thanks, Roger.

[PATCH v2 6/6] x86/hvm: reduce the need to flush caches in memory_type_changed()

[Roger Pau Monne]

The current cache flushing done in memory_type_changed() is too wide, and
this doesn't scale on boxes with high number of CPUs.  Attempt to limit
cache flushes as a result of p2m type changes, and only do them if:

 * The CPU doesn't support (or has broken) self-snoop capability, otherwise
   there would be leftover aliases in the cache.  For guest initiated
   memory changes (like changes to MTRRs) the guest should already do a
   cache flush.
 * The IOMMU cannot force all DMA accesses to be snooping ones.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Changes since v1:
 - New in this version.
---
Not sure whether this attempt to reduce cache flushing should get some
mention in the CHANGELOG.
---
 xen/arch/x86/hvm/mtrr.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/xen/arch/x86/hvm/mtrr.c b/xen/arch/x86/hvm/mtrr.c
index 8e1e15af8d73..9a6f6865a05e 100644
--- a/xen/arch/x86/hvm/mtrr.c
+++ b/xen/arch/x86/hvm/mtrr.c
@@ -782,14 +782,21 @@ HVM_REGISTER_SAVE_RESTORE(MTRR, hvm_save_mtrr_msr, NULL, hvm_load_mtrr_msr, 1,
 
 void memory_type_changed(struct domain *d)
 {
-    if ( (is_iommu_enabled(d) || cache_flush_permitted(d)) &&
+    if ( cache_flush_permitted(d) &&
          d->vcpu && d->vcpu[0] && p2m_memory_type_changed(d) &&
          /*
           * Do the p2m type-change, but skip the cache flush if the domain is
           * not yet running.  The check for creation_finished must strictly be
           * done after the call to p2m_memory_type_changed().
           */
-         d->creation_finished )
+         d->creation_finished &&
+         /*
+          * The cache flush should be done if either: CPU doesn't have
+          * self-snoop in which case there could be aliases left in the cache,
+          * or IOMMUs cannot force all DMA device accesses to be snooped.
+          */
+         (!boot_cpu_has(X86_FEATURE_XEN_SELFSNOOP) ||
+          (is_iommu_enabled(d) && !iommu_snoop)) )
     {
         flush_all(FLUSH_CACHE);
     }
-- 
2.48.1

Re: [PATCH v2 6/6] x86/hvm: reduce the need to flush caches in memory_type_changed()

[Jan Beulich]

On 16.05.2025 11:45, Roger Pau Monne wrote:
> The current cache flushing done in memory_type_changed() is too wide, and
> this doesn't scale on boxes with high number of CPUs.  Attempt to limit
> cache flushes as a result of p2m type changes, and only do them if:
> 
>  * The CPU doesn't support (or has broken) self-snoop capability, otherwise
>    there would be leftover aliases in the cache.  For guest initiated
>    memory changes (like changes to MTRRs) the guest should already do a
>    cache flush.
>  * The IOMMU cannot force all DMA accesses to be snooping ones.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>

> Not sure whether this attempt to reduce cache flushing should get some
> mention in the CHANGELOG.

Err on the side of adding an entry there?

> --- a/xen/arch/x86/hvm/mtrr.c
> +++ b/xen/arch/x86/hvm/mtrr.c
> @@ -782,14 +782,21 @@ HVM_REGISTER_SAVE_RESTORE(MTRR, hvm_save_mtrr_msr, NULL, hvm_load_mtrr_msr, 1,
>  
>  void memory_type_changed(struct domain *d)
>  {
> -    if ( (is_iommu_enabled(d) || cache_flush_permitted(d)) &&
> +    if ( cache_flush_permitted(d) &&
>           d->vcpu && d->vcpu[0] && p2m_memory_type_changed(d) &&
>           /*
>            * Do the p2m type-change, but skip the cache flush if the domain is
>            * not yet running.  The check for creation_finished must strictly be
>            * done after the call to p2m_memory_type_changed().
>            */
> -         d->creation_finished )
> +         d->creation_finished &&
> +         /*
> +          * The cache flush should be done if either: CPU doesn't have
> +          * self-snoop in which case there could be aliases left in the cache,
> +          * or IOMMUs cannot force all DMA device accesses to be snooped.

I think this could do with "some" added ahead of IOMMUs (maybe parenthesized),
to clarify the route to take here if and when we change the global to a finer
grained property.

Jan

> +          */
> +         (!boot_cpu_has(X86_FEATURE_XEN_SELFSNOOP) ||
> +          (is_iommu_enabled(d) && !iommu_snoop)) )
>      {
>          flush_all(FLUSH_CACHE);
>      }

Re: [PATCH v2 6/6] x86/hvm: reduce the need to flush caches in memory_type_changed()

[Roger Pau Monné]

On Sun, May 18, 2025 at 01:44:49PM +0200, Jan Beulich wrote:
> On 16.05.2025 11:45, Roger Pau Monne wrote:
> > The current cache flushing done in memory_type_changed() is too wide, and
> > this doesn't scale on boxes with high number of CPUs.  Attempt to limit
> > cache flushes as a result of p2m type changes, and only do them if:
> > 
> >  * The CPU doesn't support (or has broken) self-snoop capability, otherwise
> >    there would be leftover aliases in the cache.  For guest initiated
> >    memory changes (like changes to MTRRs) the guest should already do a
> >    cache flush.
> >  * The IOMMU cannot force all DMA accesses to be snooping ones.
> > 
> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> 
> Reviewed-by: Jan Beulich <jbeulich@suse.com>

Thanks.

> > Not sure whether this attempt to reduce cache flushing should get some
> > mention in the CHANGELOG.
> 
> Err on the side of adding an entry there?

Oleksii would you be fine with me adding:

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 1ea06524db20..fa971cd9e6ee 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -11,6 +11,9 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
    - For x86, GCC 5.1 and Binutils 2.25, or Clang/LLVM 11
    - For ARM32 and ARM64, GCC 5.1 and Binutils 2.25
  - Linux based device model stubdomains are now fully supported.
+ - On x86:
+   - Restrict the cache flushing done in memory_type_changed() to improve
+     performance.

 ### Added
  - On x86:

Asking whether you provide a RB/Acked-by here so that I don't need to
resend just for the CHANGELOG addition.

> > --- a/xen/arch/x86/hvm/mtrr.c
> > +++ b/xen/arch/x86/hvm/mtrr.c
> > @@ -782,14 +782,21 @@ HVM_REGISTER_SAVE_RESTORE(MTRR, hvm_save_mtrr_msr, NULL, hvm_load_mtrr_msr, 1,
> >  
> >  void memory_type_changed(struct domain *d)
> >  {
> > -    if ( (is_iommu_enabled(d) || cache_flush_permitted(d)) &&
> > +    if ( cache_flush_permitted(d) &&
> >           d->vcpu && d->vcpu[0] && p2m_memory_type_changed(d) &&
> >           /*
> >            * Do the p2m type-change, but skip the cache flush if the domain is
> >            * not yet running.  The check for creation_finished must strictly be
> >            * done after the call to p2m_memory_type_changed().
> >            */
> > -         d->creation_finished )
> > +         d->creation_finished &&
> > +         /*
> > +          * The cache flush should be done if either: CPU doesn't have
> > +          * self-snoop in which case there could be aliases left in the cache,
> > +          * or IOMMUs cannot force all DMA device accesses to be snooped.
> 
> I think this could do with "some" added ahead of IOMMUs (maybe parenthesized),
> to clarify the route to take here if and when we change the global to a finer
> grained property.

Done, thanks.

Re: [PATCH v2 6/6] x86/hvm: reduce the need to flush caches in memory_type_changed()

[Jan Beulich]

On 19.05.2025 13:08, Roger Pau Monné wrote:
> On Sun, May 18, 2025 at 01:44:49PM +0200, Jan Beulich wrote:
>> On 16.05.2025 11:45, Roger Pau Monne wrote:
>>> Not sure whether this attempt to reduce cache flushing should get some
>>> mention in the CHANGELOG.
>>
>> Err on the side of adding an entry there?
> 
> Oleksii would you be fine with me adding:
> 
> diff --git a/CHANGELOG.md b/CHANGELOG.md
> index 1ea06524db20..fa971cd9e6ee 100644
> --- a/CHANGELOG.md
> +++ b/CHANGELOG.md
> @@ -11,6 +11,9 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
>     - For x86, GCC 5.1 and Binutils 2.25, or Clang/LLVM 11
>     - For ARM32 and ARM64, GCC 5.1 and Binutils 2.25
>   - Linux based device model stubdomains are now fully supported.
> + - On x86:
> +   - Restrict the cache flushing done in memory_type_changed() to improve
> +     performance.

Maybe better to mention function names here, saying "after a memory type change
by a guest" instead?

Jan

Re: [PATCH v2 6/6] x86/hvm: reduce the need to flush caches in memory_type_changed()

[Roger Pau Monné]

On Mon, May 19, 2025 at 03:22:32PM +0200, Jan Beulich wrote:
> On 19.05.2025 13:08, Roger Pau Monné wrote:
> > On Sun, May 18, 2025 at 01:44:49PM +0200, Jan Beulich wrote:
> >> On 16.05.2025 11:45, Roger Pau Monne wrote:
> >>> Not sure whether this attempt to reduce cache flushing should get some
> >>> mention in the CHANGELOG.
> >>
> >> Err on the side of adding an entry there?
> > 
> > Oleksii would you be fine with me adding:
> > 
> > diff --git a/CHANGELOG.md b/CHANGELOG.md
> > index 1ea06524db20..fa971cd9e6ee 100644
> > --- a/CHANGELOG.md
> > +++ b/CHANGELOG.md
> > @@ -11,6 +11,9 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
> >     - For x86, GCC 5.1 and Binutils 2.25, or Clang/LLVM 11
> >     - For ARM32 and ARM64, GCC 5.1 and Binutils 2.25
> >   - Linux based device model stubdomains are now fully supported.
> > + - On x86:
> > +   - Restrict the cache flushing done in memory_type_changed() to improve
> > +     performance.
> 
> Maybe better to mention function names here, saying "after a memory type change
> by a guest" instead?

It's not just "after a memory type change by a guest", since
memory_type_changed() is also used for toolstack operations like
io{mem,ports}_{permit,deny}_access(), that strictly speaking are not
memory type changes, neither are done by the guest itself.  I could
reword to:

   - Restrict the cache flushing done as a result of guest physical
     memory map manipulations and memory type changes.

Does that sound better?

Thanks, Roger.

Re: [PATCH v2 6/6] x86/hvm: reduce the need to flush caches in memory_type_changed()

[Jan Beulich]

On 19.05.2025 16:33, Roger Pau Monné wrote:
> On Mon, May 19, 2025 at 03:22:32PM +0200, Jan Beulich wrote:
>> On 19.05.2025 13:08, Roger Pau Monné wrote:
>>> On Sun, May 18, 2025 at 01:44:49PM +0200, Jan Beulich wrote:
>>>> On 16.05.2025 11:45, Roger Pau Monne wrote:
>>>>> Not sure whether this attempt to reduce cache flushing should get some
>>>>> mention in the CHANGELOG.
>>>>
>>>> Err on the side of adding an entry there?
>>>
>>> Oleksii would you be fine with me adding:
>>>
>>> diff --git a/CHANGELOG.md b/CHANGELOG.md
>>> index 1ea06524db20..fa971cd9e6ee 100644
>>> --- a/CHANGELOG.md
>>> +++ b/CHANGELOG.md
>>> @@ -11,6 +11,9 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
>>>     - For x86, GCC 5.1 and Binutils 2.25, or Clang/LLVM 11
>>>     - For ARM32 and ARM64, GCC 5.1 and Binutils 2.25
>>>   - Linux based device model stubdomains are now fully supported.
>>> + - On x86:
>>> +   - Restrict the cache flushing done in memory_type_changed() to improve
>>> +     performance.
>>
>> Maybe better to mention function names here, saying "after a memory type change
>> by a guest" instead?
> 
> It's not just "after a memory type change by a guest", since
> memory_type_changed() is also used for toolstack operations like
> io{mem,ports}_{permit,deny}_access(), that strictly speaking are not
> memory type changes,

Sure, I'm aware. But I didn't think it would matter that much here. Still ...

> neither are done by the guest itself.  I could
> reword to:
> 
>    - Restrict the cache flushing done as a result of guest physical
>      memory map manipulations and memory type changes.
> 
> Does that sound better?

... yes, let's go with this then.

Jan

Re: [PATCH v2 6/6] x86/hvm: reduce the need to flush caches in memory_type_changed()

[Oleksii Kurochko]

On 5/19/25 4:33 PM, Roger Pau Monné wrote:
> On Mon, May 19, 2025 at 03:22:32PM +0200, Jan Beulich wrote:
>> On 19.05.2025 13:08, Roger Pau Monné wrote:
>>> On Sun, May 18, 2025 at 01:44:49PM +0200, Jan Beulich wrote:
>>>> On 16.05.2025 11:45, Roger Pau Monne wrote:
>>>>> Not sure whether this attempt to reduce cache flushing should get some
>>>>> mention in the CHANGELOG.
>>>> Err on the side of adding an entry there?
>>> Oleksii would you be fine with me adding:
>>>
>>> diff --git a/CHANGELOG.md b/CHANGELOG.md
>>> index 1ea06524db20..fa971cd9e6ee 100644
>>> --- a/CHANGELOG.md
>>> +++ b/CHANGELOG.md
>>> @@ -11,6 +11,9 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
>>>      - For x86, GCC 5.1 and Binutils 2.25, or Clang/LLVM 11
>>>      - For ARM32 and ARM64, GCC 5.1 and Binutils 2.25
>>>    - Linux based device model stubdomains are now fully supported.
>>> + - On x86:
>>> +   - Restrict the cache flushing done in memory_type_changed() to improve
>>> +     performance.
>> Maybe better to mention function names here, saying "after a memory type change
>> by a guest" instead?
> It's not just "after a memory type change by a guest", since
> memory_type_changed() is also used for toolstack operations like
> io{mem,ports}_{permit,deny}_access(), that strictly speaking are not
> memory type changes, neither are done by the guest itself.  I could
> reword to:
>
>     - Restrict the cache flushing done as a result of guest physical
>       memory map manipulations and memory type changes.
>
> Does that sound better?

Acked-By: Oleksii Kurochko <oleksii.kurochko@gmail.com>

~ Oleksii