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

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

On Fri, May 16, 2025 at 08:58:39AM +0200, Jan Beulich wrote:
> On 06.05.2025 10:31, Roger Pau Monne wrote:
> > --- a/xen/arch/x86/hvm/mtrr.c
> > +++ b/xen/arch/x86/hvm/mtrr.c
> > @@ -605,22 +605,8 @@ int hvm_set_mem_pinned_cacheattr(struct domain *d, 
> > uint64_t gfn_start,
> >  
> >                  type = range->type;
> >                  call_rcu(&range->rcu, free_pinned_cacheattr_entry);
> > -                p2m_memory_type_changed(d);
> > -                switch ( type )
> > -                {
> > -                case X86_MT_UCM:
> > -                    /*
> > -                     * For EPT we can also avoid the flush in this case;
> > -                     * see epte_get_entry_emt().
> > -                     */
> > -                    if ( hap_enabled(d) && cpu_has_vmx )
> > -                case X86_MT_UC:
> > -                        break;
> > -                    /* fall through */
> > -                default:
> > -                    flush_all(FLUSH_CACHE);
> > -                    break;
> > -                }
> > +                memory_type_changed(d);
> > +
> >                  return 0;
> >              }
> 
> Hmm, so your consideration to avoid the "goto" in my patch was perhaps this
> part of your change, where you expect the "return 0" could then stay here.
> Maybe. However, you removing the switch() means we'd then also flush in
> cases where currently (or with my change) we avoid doing so.

Oh, yes, just replied to your previous email with this suggestion.

I don't have a strong opinion, but I don't think the fine grained
flush avoidance is really required.  What's more, if we want to call
memory_type_changed() we must do so for any changes, as the call to
p2m_memory_type_changed() must be done unconditionally regardless of
whether the specific MTRR type change could allow us to avoid the
flush.

Overall, I have the impression hvm_set_mem_pinned_cacheattr() should
only be used while building a domain, and hence the flush can likely
be skipped unconditionally, regardless of the type changes.

Thanks, Roger.