We don't permit use of uncachable memory types elsewhere unless a domain
meets certain criteria. Enforce this also during registration of pinned
cache attribute ranges.
Furthermore restrict cache flushing to just uncachable range registration.
While there, also
- take CPU self-snoop as well as IOMMU snoop into account (albeit the
latter still is a global property rather than a per-domain one),
- avoid flushes when the domain isn't running yet (which ought to be the
common case).
Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
At the expense of yet larger a diff it would be possible to get away
without any "goto", by moving the whole "new entry" handling into the
switch(). Personally I'd prefer that, but the larger diff may be
unwelcome.
I have to admit that I can't spot what part of epte_get_entry_emt() the
comment refers to that is being deleted. The function does use
hvm_get_mem_pinned_cacheattr(), yes, but there's nothing there that talks
about cache flushes (and their avoiding) in any way.
Is it really sensible to add/remove ranges once the guest is already
running? (If it is, limiting the scope of the flush would be nice, but
would require knowing dirtyness for the domain wrt the caches, which
currently we don't track.)
This is kind of amending XSA-428.
--- a/xen/arch/x86/hvm/mtrr.c
+++ b/xen/arch/x86/hvm/mtrr.c
@@ -589,6 +589,7 @@ int hvm_set_mem_pinned_cacheattr(struct
{
struct hvm_mem_pinned_cacheattr_range *range, *newr;
unsigned int nr = 0;
+ bool flush = false;
int rc = 1;
if ( !is_hvm_domain(d) )
@@ -612,31 +613,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;
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;
@@ -689,8 +694,12 @@ int hvm_set_mem_pinned_cacheattr(struct
xfree(newr);
+ finish:
p2m_memory_type_changed(d);
- if ( type != X86_MT_WB )
+
+ if ( flush && d->creation_finished &&
+ (!boot_cpu_has(X86_FEATURE_XEN_SELFSNOOP) ||
+ (is_iommu_enabled(d) && !iommu_snoop)) )
flush_all(FLUSH_CACHE);
return rc;On Wed, Mar 22, 2023 at 07:50:09AM +0100, Jan Beulich wrote:
> We don't permit use of uncachable memory types elsewhere unless a domain
> meets certain criteria. Enforce this also during registration of pinned
> cache attribute ranges.
>
> Furthermore restrict cache flushing to just uncachable range registration.
> While there, also
> - take CPU self-snoop as well as IOMMU snoop into account (albeit the
> latter still is a global property rather than a per-domain one),
> - avoid flushes when the domain isn't running yet (which ought to be the
> common case).
>
> Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> At the expense of yet larger a diff it would be possible to get away
> without any "goto", by moving the whole "new entry" handling into the
> switch(). Personally I'd prefer that, but the larger diff may be
> unwelcome.
>
> I have to admit that I can't spot what part of epte_get_entry_emt() the
> comment refers to that is being deleted. The function does use
> hvm_get_mem_pinned_cacheattr(), yes, but there's nothing there that talks
> about cache flushes (and their avoiding) in any way.
>
> Is it really sensible to add/remove ranges once the guest is already
> running? (If it is, limiting the scope of the flush would be nice, but
> would require knowing dirtyness for the domain wrt the caches, which
> currently we don't track.)
>
> This is kind of amending XSA-428.
>
> --- a/xen/arch/x86/hvm/mtrr.c
> +++ b/xen/arch/x86/hvm/mtrr.c
> @@ -589,6 +589,7 @@ int hvm_set_mem_pinned_cacheattr(struct
> {
> struct hvm_mem_pinned_cacheattr_range *range, *newr;
> unsigned int nr = 0;
> + bool flush = false;
> int rc = 1;
>
> if ( !is_hvm_domain(d) )
> @@ -612,31 +613,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;
> 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;
> @@ -689,8 +694,12 @@ int hvm_set_mem_pinned_cacheattr(struct
>
> xfree(newr);
>
> + finish:
> p2m_memory_type_changed(d);
> - if ( type != X86_MT_WB )
> +
> + if ( flush && d->creation_finished &&
> + (!boot_cpu_has(X86_FEATURE_XEN_SELFSNOOP) ||
> + (is_iommu_enabled(d) && !iommu_snoop)) )
> flush_all(FLUSH_CACHE);
I think it would be better if we could add those checks to
memory_type_changed() rather than open-coding them here, and just call
memory_type_changed() then, which would also avoid the goto AFAICT.
Thanks, Roger.
On 15.05.2025 12:04, Roger Pau Monné wrote:
> On Wed, Mar 22, 2023 at 07:50:09AM +0100, Jan Beulich wrote:
>> We don't permit use of uncachable memory types elsewhere unless a domain
>> meets certain criteria. Enforce this also during registration of pinned
>> cache attribute ranges.
>>
>> Furthermore restrict cache flushing to just uncachable range registration.
>> While there, also
>> - take CPU self-snoop as well as IOMMU snoop into account (albeit the
>> latter still is a global property rather than a per-domain one),
>> - avoid flushes when the domain isn't running yet (which ought to be the
>> common case).
>>
>> Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> ---
>> At the expense of yet larger a diff it would be possible to get away
>> without any "goto", by moving the whole "new entry" handling into the
>> switch(). Personally I'd prefer that, but the larger diff may be
>> unwelcome.
>>
>> I have to admit that I can't spot what part of epte_get_entry_emt() the
>> comment refers to that is being deleted. The function does use
>> hvm_get_mem_pinned_cacheattr(), yes, but there's nothing there that talks
>> about cache flushes (and their avoiding) in any way.
>>
>> Is it really sensible to add/remove ranges once the guest is already
>> running? (If it is, limiting the scope of the flush would be nice, but
>> would require knowing dirtyness for the domain wrt the caches, which
>> currently we don't track.)
>>
>> This is kind of amending XSA-428.
>>
>> --- a/xen/arch/x86/hvm/mtrr.c
>> +++ b/xen/arch/x86/hvm/mtrr.c
>> @@ -589,6 +589,7 @@ int hvm_set_mem_pinned_cacheattr(struct
>> {
>> struct hvm_mem_pinned_cacheattr_range *range, *newr;
>> unsigned int nr = 0;
>> + bool flush = false;
>> int rc = 1;
>>
>> if ( !is_hvm_domain(d) )
>> @@ -612,31 +613,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;
>> 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;
>> @@ -689,8 +694,12 @@ int hvm_set_mem_pinned_cacheattr(struct
>>
>> xfree(newr);
>>
>> + finish:
>> p2m_memory_type_changed(d);
>> - if ( type != X86_MT_WB )
>> +
>> + if ( flush && d->creation_finished &&
>> + (!boot_cpu_has(X86_FEATURE_XEN_SELFSNOOP) ||
>> + (is_iommu_enabled(d) && !iommu_snoop)) )
>> flush_all(FLUSH_CACHE);
>
> I think it would be better if we could add those checks to
> memory_type_changed() rather than open-coding them here, and just call
> memory_type_changed() then, which would also avoid the goto AFAICT.
Hmm, with this last remark, what does "those checks" cover then? I first
read it as meaning the conditions in just this if(), but the "goto" is
needed for a different reason.
Jan
On Fri, May 16, 2025 at 08:54:52AM +0200, Jan Beulich wrote:
> On 15.05.2025 12:04, Roger Pau Monné wrote:
> > On Wed, Mar 22, 2023 at 07:50:09AM +0100, Jan Beulich wrote:
> >> We don't permit use of uncachable memory types elsewhere unless a domain
> >> meets certain criteria. Enforce this also during registration of pinned
> >> cache attribute ranges.
> >>
> >> Furthermore restrict cache flushing to just uncachable range registration.
> >> While there, also
> >> - take CPU self-snoop as well as IOMMU snoop into account (albeit the
> >> latter still is a global property rather than a per-domain one),
> >> - avoid flushes when the domain isn't running yet (which ought to be the
> >> common case).
> >>
> >> Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
> >> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> >> ---
> >> At the expense of yet larger a diff it would be possible to get away
> >> without any "goto", by moving the whole "new entry" handling into the
> >> switch(). Personally I'd prefer that, but the larger diff may be
> >> unwelcome.
> >>
> >> I have to admit that I can't spot what part of epte_get_entry_emt() the
> >> comment refers to that is being deleted. The function does use
> >> hvm_get_mem_pinned_cacheattr(), yes, but there's nothing there that talks
> >> about cache flushes (and their avoiding) in any way.
> >>
> >> Is it really sensible to add/remove ranges once the guest is already
> >> running? (If it is, limiting the scope of the flush would be nice, but
> >> would require knowing dirtyness for the domain wrt the caches, which
> >> currently we don't track.)
> >>
> >> This is kind of amending XSA-428.
> >>
> >> --- a/xen/arch/x86/hvm/mtrr.c
> >> +++ b/xen/arch/x86/hvm/mtrr.c
> >> @@ -589,6 +589,7 @@ int hvm_set_mem_pinned_cacheattr(struct
> >> {
> >> struct hvm_mem_pinned_cacheattr_range *range, *newr;
> >> unsigned int nr = 0;
> >> + bool flush = false;
> >> int rc = 1;
> >>
> >> if ( !is_hvm_domain(d) )
> >> @@ -612,31 +613,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;
> >> 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;
> >> @@ -689,8 +694,12 @@ int hvm_set_mem_pinned_cacheattr(struct
> >>
> >> xfree(newr);
> >>
> >> + finish:
> >> p2m_memory_type_changed(d);
> >> - if ( type != X86_MT_WB )
> >> +
> >> + if ( flush && d->creation_finished &&
> >> + (!boot_cpu_has(X86_FEATURE_XEN_SELFSNOOP) ||
> >> + (is_iommu_enabled(d) && !iommu_snoop)) )
> >> flush_all(FLUSH_CACHE);
> >
> > I think it would be better if we could add those checks to
> > memory_type_changed() rather than open-coding them here, and just call
> > memory_type_changed() then, which would also avoid the goto AFAICT.
>
> Hmm, with this last remark, what does "those checks" cover then?
I have a patches I was going to send today (done some overnight
testing) that do:
if ( cache_flush_permitted(d) &&
d->vcpu && d->vcpu[0] && p2m_memory_type_changed(d) &&
/*
* Do the p2m type-change, but skip the cache flush if the domain is
* not yet running. The check for creation_finished must strictly be
* done after the call to p2m_memory_type_changed().
*/
d->creation_finished &&
/*
* The cache flush should be done if either: CPU doesn't have
* self-snoop in which case there could be aliases left in the cache,
* or IOMMUs cannot force all DMA device accesses to be snooped.
*/
(!boot_cpu_has(X86_FEATURE_XEN_SELFSNOOP) ||
(is_iommu_enabled(d) && !iommu_snoop)) )
{
flush_all(FLUSH_CACHE);
}
As to attempt to limit the cache flushing done in
memory_type_changed().
> I first
> read it as meaning the conditions in just this if(), but the "goto" is
> needed for a different reason.
Maybe I'm missing something, but couldn't
hvm_set_mem_pinned_cacheattr() just call memory_type_changed() (with
the proposed checks above added) if and return then, instead of doing
a goto?
Thanks, Roger.
On 16.05.2025 09:43, Roger Pau Monné wrote:
> On Fri, May 16, 2025 at 08:54:52AM +0200, Jan Beulich wrote:
>> On 15.05.2025 12:04, Roger Pau Monné wrote:
>>> On Wed, Mar 22, 2023 at 07:50:09AM +0100, Jan Beulich wrote:
>>>> We don't permit use of uncachable memory types elsewhere unless a domain
>>>> meets certain criteria. Enforce this also during registration of pinned
>>>> cache attribute ranges.
>>>>
>>>> Furthermore restrict cache flushing to just uncachable range registration.
>>>> While there, also
>>>> - take CPU self-snoop as well as IOMMU snoop into account (albeit the
>>>> latter still is a global property rather than a per-domain one),
>>>> - avoid flushes when the domain isn't running yet (which ought to be the
>>>> common case).
>>>>
>>>> Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>>> ---
>>>> At the expense of yet larger a diff it would be possible to get away
>>>> without any "goto", by moving the whole "new entry" handling into the
>>>> switch(). Personally I'd prefer that, but the larger diff may be
>>>> unwelcome.
>>>>
>>>> I have to admit that I can't spot what part of epte_get_entry_emt() the
>>>> comment refers to that is being deleted. The function does use
>>>> hvm_get_mem_pinned_cacheattr(), yes, but there's nothing there that talks
>>>> about cache flushes (and their avoiding) in any way.
>>>>
>>>> Is it really sensible to add/remove ranges once the guest is already
>>>> running? (If it is, limiting the scope of the flush would be nice, but
>>>> would require knowing dirtyness for the domain wrt the caches, which
>>>> currently we don't track.)
>>>>
>>>> This is kind of amending XSA-428.
>>>>
>>>> --- a/xen/arch/x86/hvm/mtrr.c
>>>> +++ b/xen/arch/x86/hvm/mtrr.c
>>>> @@ -589,6 +589,7 @@ int hvm_set_mem_pinned_cacheattr(struct
>>>> {
>>>> struct hvm_mem_pinned_cacheattr_range *range, *newr;
>>>> unsigned int nr = 0;
>>>> + bool flush = false;
>>>> int rc = 1;
>>>>
>>>> if ( !is_hvm_domain(d) )
>>>> @@ -612,31 +613,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;
>>>> 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;
>>>> @@ -689,8 +694,12 @@ int hvm_set_mem_pinned_cacheattr(struct
>>>>
>>>> xfree(newr);
>>>>
>>>> + finish:
>>>> p2m_memory_type_changed(d);
>>>> - if ( type != X86_MT_WB )
>>>> +
>>>> + if ( flush && d->creation_finished &&
>>>> + (!boot_cpu_has(X86_FEATURE_XEN_SELFSNOOP) ||
>>>> + (is_iommu_enabled(d) && !iommu_snoop)) )
>>>> flush_all(FLUSH_CACHE);
>>>
>>> I think it would be better if we could add those checks to
>>> memory_type_changed() rather than open-coding them here, and just call
>>> memory_type_changed() then, which would also avoid the goto AFAICT.
>>
>> Hmm, with this last remark, what does "those checks" cover then?
>
> I have a patches I was going to send today (done some overnight
> testing) that do:
>
> if ( cache_flush_permitted(d) &&
> d->vcpu && d->vcpu[0] && p2m_memory_type_changed(d) &&
> /*
> * Do the p2m type-change, but skip the cache flush if the domain is
> * not yet running. The check for creation_finished must strictly be
> * done after the call to p2m_memory_type_changed().
> */
> d->creation_finished &&
> /*
> * The cache flush should be done if either: CPU doesn't have
> * self-snoop in which case there could be aliases left in the cache,
> * or IOMMUs cannot force all DMA device accesses to be snooped.
> */
> (!boot_cpu_has(X86_FEATURE_XEN_SELFSNOOP) ||
> (is_iommu_enabled(d) && !iommu_snoop)) )
> {
> flush_all(FLUSH_CACHE);
> }
>
> As to attempt to limit the cache flushing done in
> memory_type_changed().
>
>> I first
>> read it as meaning the conditions in just this if(), but the "goto" is
>> needed for a different reason.
>
> Maybe I'm missing something, but couldn't
> hvm_set_mem_pinned_cacheattr() just call memory_type_changed() (with
> the proposed checks above added) if and return then, instead of doing
> a goto?
As per later replies to your patch - yes, looks like that's possible.
Jan
© 2016 - 2026 Red Hat, Inc.