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