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

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

On Tue, Jun 10, 2025 at 01:59:52PM +0200, Jan Beulich wrote:
> On 10.06.2025 12:44, Roger Pau Monné wrote:
> > 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.

This is complicated.  Looking at memory_type_changed() it's true that
p2m_memory_type_changed() will only be called when
cache_flush_permitted() == true, however by cache_flush_permitted()
returning false we could also imply that there are no p2m_mmio_direct
entries in the p2m, and hence effectively nothing to flush?

> >>
> >>>>                      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.
> 
> Why would it not do so? Am I overlooking a conditional there that would
> cause hvm_get_mem_pinned_cacheattr() to not be called? The only related
> one I see is
> 
>     if ( type != p2m_mmio_direct && !is_iommu_enabled(d) &&
>          !cache_flush_permitted(d) )

Sorry, what I wrote is nonsense.  I wanted to note that it should be
possible to set cache attributes ahead of assigning IO resources, and
so returning -EPERM might alter existing users of the interface.

However setting cache attributes ahead of having actually assigned IO
resources would seem like an out of order operation logic, and hence
there's no point in supporting it.

> covering the without-IOMMU case just the same as the "with" one. (The
> "without" case looks dubious to me, as I don't think we arrange for
> any identity mapping, but that's a separate topic.)
> 
> > 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.
> 
> There's no question there is a behavioral change here. Without I/O
> resources (and without IOMMU) we simply don't accept cache attributes
> other then WB elsewhere; the change is to avoid doing so here as well,
> to get things to be consistent. Hence the -EPERM return.
> 
> >>>> @@ -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?
> 
> Oh, sorry, I meant "only" just in regard to options keeping the main
> code structure of the change. I agree ...
> 
> > 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.
> 
> ... that this, too, is a possibility. It would, however, go against the
> stated purpose of the change (in the subject "... as well as associated
> flushing"), which - after all - was the main goal here, seeing the
> series this was originally part of.

Given the recently added logic in memory_type_changed() to limit the
flushing, I'm not sure it would go against the patch subject.  Just
using memory_type_changed() as-is will already prevent flushing when
the guest is not yet started, and would also limit the flushing to the
cases where snooping accesses cannot be forced from the IOMMU.
It will mostly likely result in a reduction of the flushing, even if
the attribute type checking was removed.

IMO the added complexity here is not worth the performance
improvement, not without a clear justification that it's needed.

Thanks, Roger.