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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

>>                      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;
>> +        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 ...

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

Jan