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

[Jan Beulich <jbeulich@xxxxxxxx>]

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@xxxxxxxxxx>

Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

> 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);
>      }