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
