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

[Jan Beulich <jbeulich@xxxxxxxx>]

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

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