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.
