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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

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@xxxxxxxxxx>
> 
> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

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.