Re: [PATCH 5/9] x86/mtrr: use memory_type_changed() in hvm_set_mem_pinned_cacheattr()

["Roger Pau Monné" <roger.pau@citrix.com>]

On Fri, May 16, 2025 at 09:00:42AM +0200, Jan Beulich wrote:
> On 15.05.2025 12:22, Roger Pau Monné wrote:
> > On Mon, May 12, 2025 at 05:04:56PM +0200, Jan Beulich wrote:
> >> On 06.05.2025 10:31, Roger Pau Monne wrote:
> >>> The current logic partially open-codes memory_type_changed(), but doesn't
> >>> check whether the type change or the cache flush is actually needed.
> >>> Instead switch to using memory_type_changed(), at possibly a higher expense
> >>> cost of not exclusively issuing cache flushes when limiting cacheability.
> >>>
> >>> However using memory_type_changed() has the benefit of limiting
> >>> recalculations and cache flushes to strictly only when it's meaningful due
> >>> to the guest configuration, like having devices or IO regions assigned.
> >>>
> >>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> >>
> >> Hmm, I'm not convinced this is a win. This operation isn't normally used on
> >> a running guest, aiui.
> >>
> >> What's more, this heavily conflicts with a patch posted (again) over 2 years
> >> ago [1]. Unless there's something severely wrong with that (and unless your
> >> patch would make that old one unnecessary), I'm again of the opinion that
> >> that one should go in first. It is becoming increasingly noticeable that it's
> >> unhelpful if posted patches aren't being looked at.
> > 
> > I'm happy for your change to go in first, but I think that
> > memory_type_changed() should be adjusted to contain the extra checks
> > that you add in your patch, and then hvm_set_mem_pinned_cacheattr()
> > should be switched to use memory_type_changed() rather than
> > open-coding it.
> 
> Maybe, but see my other reply to your patch.
> 
> >  For once it would miss the adjustment done to
> > memory_type_changed() to only flush the cache when there's a
> > meaningful change to the p2m (iow: p2m_memory_type_changed() is not a
> > no-op).
> 
> That could also be mirrored here, if there were (remaining) reasons to avoid
> use of memory_type_changed() for this function.

Indeed, but it's just more open-coding of memory_type_changed() in the
context of hvm_set_mem_pinned_cacheattr().  IMO I don't know exactly
the usage of this function, it seems like it should be mostly used at
domain build time, or maybe when doing hotplug of a device, so not
very often.

Given how complicated cache management is, I would prefer if the logic
is limited to few places (like memory_type_changed()), instead of
having open-coded implementations of the same logic in multiple
places.

Thanks, Roger.