Re: [PATCH v2] x86/HVM: restrict use of pinned cache attributes as well as associated flushing

["Roger Pau Monné" <roger.pau@citrix.com>]

On Tue, Jun 10, 2025 at 09:40:38AM +0200, Jan Beulich wrote:
> On 09.06.2025 12:36, Roger Pau Monné wrote:
> > On Wed, Jun 04, 2025 at 11:48:00AM +0200, Jan Beulich wrote:
> >> @@ -605,31 +606,35 @@ int hvm_set_mem_pinned_cacheattr(struct
> >>  
> >>                  type = range->type;
> >>                  call_rcu(&range->rcu, free_pinned_cacheattr_entry);
> >> -                p2m_memory_type_changed(d);
> >>                  switch ( type )
> >>                  {
> >> -                case X86_MT_UCM:
> >> +                case X86_MT_WB:
> >> +                case X86_MT_WP:
> >> +                case X86_MT_WT:
> >>                      /*
> >> -                     * For EPT we can also avoid the flush in this case;
> >> -                     * see epte_get_entry_emt().
> >> +                     * Flush since we don't know what the cachability is going
> >> +                     * to be.
> >>                       */
> >> -                    if ( hap_enabled(d) && cpu_has_vmx )
> >> -                case X86_MT_UC:
> >> -                        break;
> >> -                    /* fall through */
> >> -                default:
> >> -                    flush_all(FLUSH_CACHE);
> >> +                    if ( is_iommu_enabled(d) || cache_flush_permitted(d) )
> >> +                        flush = true;
> > 
> > Is the check here required?  memory_type_changed() will already check
> > for is_iommu_enabled() and cache_flush_permitted(), and hence you
> > could just set flush to true unconditionally here IMO.
> 
> The behavioral difference is when both predicates are false: The way I have
> it now, p2m_memory_type_changed() will then still be called (conditionally),
> better matching prior behavior.

I see.  Yes, p2m_memory_type_changed() needs to be called.

> 
> >>                      break;
> >>                  }
> >> -                return 0;
> >> +                rc = 0;
> >> +                goto finish;
> >>              }
> >>          domain_unlock(d);
> >>          return -ENOENT;
> >>  
> >>      case X86_MT_UCM:
> >>      case X86_MT_UC:
> >> -    case X86_MT_WB:
> >>      case X86_MT_WC:
> >> +        /* Flush since we don't know what the cachability was. */
> >> +        if ( !is_iommu_enabled(d) && !cache_flush_permitted(d) )
> >> +            return -EPERM;

When assigning IO resources without an IOMMU enabled we likely need
to allow the pinned cache attributes to be set, but there's no need to
propagate the changes to the p2m, as the EMT calculation won't take
into account the pinned attributes.

IOW: I don't think we can safely short-circuit and return -EPERM here
without agreeing that it's a behavioral difference form the previous
implementation.

> >> +        flush = true;
> >> +        break;
> >> +
> >> +    case X86_MT_WB:
> >>      case X86_MT_WP:
> >>      case X86_MT_WT:
> >>          break;
> >> @@ -682,9 +687,11 @@ int hvm_set_mem_pinned_cacheattr(struct
> >>  
> >>      xfree(newr);
> >>  
> >> -    p2m_memory_type_changed(d);
> >> -    if ( type != X86_MT_WB )
> >> -        flush_all(FLUSH_CACHE);
> >> + finish:
> >> +    if ( flush )
> >> +        memory_type_changed(d);
> >> +    else if ( d->vcpu && d->vcpu[0] )
> >> +        p2m_memory_type_changed(d);
> > 
> > FWIW, I would just call memory_type_changed() unconditionally
> > regardless of the change.
> 
> In which case the need for the "flush" local var would go away, if I
> understand your suggestion correctly. Like above, there'll then be
> more of a behavioral change than intended. In particular ...

There will be a behavioral change, but not one that the guest would
notice IMO.

> >  We suspect the hypercall is only used at
> > domain creation time (where memory_type_changed() won't do a cache
> > flush anyway).
> 
> ... "suspect" is not enough for my taste. The only alternative there
> that I see (as mentioned in a post-commit-message remark) is to
> refuse such "late" changes altogether. Yet for that we need to be
> sure, which it looks like no-one of us is.

Why do you say only alternative?

Calling memory_type_changed() unconditionally (without taking into
account the previous or new cache attributes) would also be an
acceptable solution, that might wide the cache flushing a bit, but
would still be correct and much simpler IMO.

Thanks, Roger.