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

[Jan Beulich <jbeulich@suse.com>]

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

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.

Jan