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