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

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

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

Thanks, Roger.