Re: [PATCH v2 6/6] x86/hvm: reduce the need to flush caches in memory_type_changed()
On 5/19/25 4:33 PM, Roger Pau Monné wrote:
On Mon, May 19, 2025 at 03:22:32PM +0200, Jan Beulich wrote:
On 19.05.2025 13:08, Roger Pau Monné wrote:
On Sun, May 18, 2025 at 01:44:49PM +0200, Jan Beulich wrote:
On 16.05.2025 11:45, Roger Pau Monne wrote:
Not sure whether this attempt to reduce cache flushing should get some
mention in the CHANGELOG.
Err on the side of adding an entry there?
Oleksii would you be fine with me adding:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 1ea06524db20..fa971cd9e6ee 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -11,6 +11,9 @@ The format is based on [Keep a
Changelog](https://keepachangelog.com/en/1.0.0/)
- For x86, GCC 5.1 and Binutils 2.25, or Clang/LLVM 11
- For ARM32 and ARM64, GCC 5.1 and Binutils 2.25
- Linux based device model stubdomains are now fully supported.
+ - On x86:
+ - Restrict the cache flushing done in memory_type_changed() to improve
+ performance.
Maybe better to mention function names here, saying "after a memory type change
by a guest" instead?
It's not just "after a memory type change by a guest", since
memory_type_changed() is also used for toolstack operations like
io{mem,ports}_{permit,deny}_access(), that strictly speaking are not
memory type changes, neither are done by the guest itself. I could
reword to:
- Restrict the cache flushing done as a result of guest physical
memory map manipulations and memory type changes.
Does that sound better?
Acked-By: Oleksii Kurochko
~ Oleksii
Re: [PATCH v2 6/6] x86/hvm: reduce the need to flush caches in memory_type_changed()
On 19.05.2025 16:33, Roger Pau Monné wrote:
> On Mon, May 19, 2025 at 03:22:32PM +0200, Jan Beulich wrote:
>> On 19.05.2025 13:08, Roger Pau Monné wrote:
>>> On Sun, May 18, 2025 at 01:44:49PM +0200, Jan Beulich wrote:
On 16.05.2025 11:45, Roger Pau Monne wrote:
> Not sure whether this attempt to reduce cache flushing should get some
> mention in the CHANGELOG.
Err on the side of adding an entry there?
>>>
>>> Oleksii would you be fine with me adding:
>>>
>>> diff --git a/CHANGELOG.md b/CHANGELOG.md
>>> index 1ea06524db20..fa971cd9e6ee 100644
>>> --- a/CHANGELOG.md
>>> +++ b/CHANGELOG.md
>>> @@ -11,6 +11,9 @@ The format is based on [Keep a
>>> Changelog](https://keepachangelog.com/en/1.0.0/)
>>> - For x86, GCC 5.1 and Binutils 2.25, or Clang/LLVM 11
>>> - For ARM32 and ARM64, GCC 5.1 and Binutils 2.25
>>> - Linux based device model stubdomains are now fully supported.
>>> + - On x86:
>>> + - Restrict the cache flushing done in memory_type_changed() to improve
>>> + performance.
>>
>> Maybe better to mention function names here, saying "after a memory type
>> change
>> by a guest" instead?
>
> It's not just "after a memory type change by a guest", since
> memory_type_changed() is also used for toolstack operations like
> io{mem,ports}_{permit,deny}_access(), that strictly speaking are not
> memory type changes,
Sure, I'm aware. But I didn't think it would matter that much here. Still ...
> neither are done by the guest itself. I could
> reword to:
>
>- Restrict the cache flushing done as a result of guest physical
> memory map manipulations and memory type changes.
>
> Does that sound better?
... yes, let's go with this then.
Jan
Re: [PATCH v2 6/6] x86/hvm: reduce the need to flush caches in memory_type_changed()
On Mon, May 19, 2025 at 03:22:32PM +0200, Jan Beulich wrote:
> On 19.05.2025 13:08, Roger Pau Monné wrote:
> > On Sun, May 18, 2025 at 01:44:49PM +0200, Jan Beulich wrote:
> >> On 16.05.2025 11:45, Roger Pau Monne wrote:
> >>> Not sure whether this attempt to reduce cache flushing should get some
> >>> mention in the CHANGELOG.
> >>
> >> Err on the side of adding an entry there?
> >
> > Oleksii would you be fine with me adding:
> >
> > diff --git a/CHANGELOG.md b/CHANGELOG.md
> > index 1ea06524db20..fa971cd9e6ee 100644
> > --- a/CHANGELOG.md
> > +++ b/CHANGELOG.md
> > @@ -11,6 +11,9 @@ The format is based on [Keep a
> > Changelog](https://keepachangelog.com/en/1.0.0/)
> > - For x86, GCC 5.1 and Binutils 2.25, or Clang/LLVM 11
> > - For ARM32 and ARM64, GCC 5.1 and Binutils 2.25
> > - Linux based device model stubdomains are now fully supported.
> > + - On x86:
> > + - Restrict the cache flushing done in memory_type_changed() to improve
> > + performance.
>
> Maybe better to mention function names here, saying "after a memory type
> change
> by a guest" instead?
It's not just "after a memory type change by a guest", since
memory_type_changed() is also used for toolstack operations like
io{mem,ports}_{permit,deny}_access(), that strictly speaking are not
memory type changes, neither are done by the guest itself. I could
reword to:
- Restrict the cache flushing done as a result of guest physical
memory map manipulations and memory type changes.
Does that sound better?
Thanks, Roger.
Re: [PATCH v2 6/6] x86/hvm: reduce the need to flush caches in memory_type_changed()
On 19.05.2025 13:08, Roger Pau Monné wrote: > On Sun, May 18, 2025 at 01:44:49PM +0200, Jan Beulich wrote: >> On 16.05.2025 11:45, Roger Pau Monne wrote: >>> Not sure whether this attempt to reduce cache flushing should get some >>> mention in the CHANGELOG. >> >> Err on the side of adding an entry there? > > Oleksii would you be fine with me adding: > > diff --git a/CHANGELOG.md b/CHANGELOG.md > index 1ea06524db20..fa971cd9e6ee 100644 > --- a/CHANGELOG.md > +++ b/CHANGELOG.md > @@ -11,6 +11,9 @@ The format is based on [Keep a > Changelog](https://keepachangelog.com/en/1.0.0/) > - For x86, GCC 5.1 and Binutils 2.25, or Clang/LLVM 11 > - For ARM32 and ARM64, GCC 5.1 and Binutils 2.25 > - Linux based device model stubdomains are now fully supported. > + - On x86: > + - Restrict the cache flushing done in memory_type_changed() to improve > + performance. Maybe better to mention function names here, saying "after a memory type change by a guest" instead? Jan
Re: [PATCH v2 6/6] x86/hvm: reduce the need to flush caches in memory_type_changed()
On Sun, May 18, 2025 at 01:44:49PM +0200, Jan Beulich wrote:
> On 16.05.2025 11:45, Roger Pau Monne wrote:
> > The current cache flushing done in memory_type_changed() is too wide, and
> > this doesn't scale on boxes with high number of CPUs. Attempt to limit
> > cache flushes as a result of p2m type changes, and only do them if:
> >
> > * The CPU doesn't support (or has broken) self-snoop capability, otherwise
> >there would be leftover aliases in the cache. For guest initiated
> >memory changes (like changes to MTRRs) the guest should already do a
> >cache flush.
> > * The IOMMU cannot force all DMA accesses to be snooping ones.
> >
> > Signed-off-by: Roger Pau Monné
>
> Reviewed-by: Jan Beulich
Thanks.
> > Not sure whether this attempt to reduce cache flushing should get some
> > mention in the CHANGELOG.
>
> Err on the side of adding an entry there?
Oleksii would you be fine with me adding:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 1ea06524db20..fa971cd9e6ee 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -11,6 +11,9 @@ The format is based on [Keep a
Changelog](https://keepachangelog.com/en/1.0.0/)
- For x86, GCC 5.1 and Binutils 2.25, or Clang/LLVM 11
- For ARM32 and ARM64, GCC 5.1 and Binutils 2.25
- Linux based device model stubdomains are now fully supported.
+ - On x86:
+ - Restrict the cache flushing done in memory_type_changed() to improve
+ performance.
### Added
- On x86:
Asking whether you provide a RB/Acked-by here so that I don't need to
resend just for the CHANGELOG addition.
> > --- a/xen/arch/x86/hvm/mtrr.c
> > +++ b/xen/arch/x86/hvm/mtrr.c
> > @@ -782,14 +782,21 @@ HVM_REGISTER_SAVE_RESTORE(MTRR, hvm_save_mtrr_msr,
> > NULL, hvm_load_mtrr_msr, 1,
> >
> > void memory_type_changed(struct domain *d)
> > {
> > -if ( (is_iommu_enabled(d) || cache_flush_permitted(d)) &&
> > +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 )
> > + 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.
>
> I think this could do with "some" added ahead of IOMMUs (maybe parenthesized),
> to clarify the route to take here if and when we change the global to a finer
> grained property.
Done, thanks.
Re: [PATCH v2 6/6] x86/hvm: reduce the need to flush caches in memory_type_changed()
On 16.05.2025 11:45, Roger Pau Monne wrote:
> The current cache flushing done in memory_type_changed() is too wide, and
> this doesn't scale on boxes with high number of CPUs. Attempt to limit
> cache flushes as a result of p2m type changes, and only do them if:
>
> * The CPU doesn't support (or has broken) self-snoop capability, otherwise
>there would be leftover aliases in the cache. For guest initiated
>memory changes (like changes to MTRRs) the guest should already do a
>cache flush.
> * The IOMMU cannot force all DMA accesses to be snooping ones.
>
> Signed-off-by: Roger Pau Monné
Reviewed-by: Jan Beulich
> Not sure whether this attempt to reduce cache flushing should get some
> mention in the CHANGELOG.
Err on the side of adding an entry there?
> --- a/xen/arch/x86/hvm/mtrr.c
> +++ b/xen/arch/x86/hvm/mtrr.c
> @@ -782,14 +782,21 @@ HVM_REGISTER_SAVE_RESTORE(MTRR, hvm_save_mtrr_msr,
> NULL, hvm_load_mtrr_msr, 1,
>
> void memory_type_changed(struct domain *d)
> {
> -if ( (is_iommu_enabled(d) || cache_flush_permitted(d)) &&
> +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 )
> + 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.
I think this could do with "some" added ahead of IOMMUs (maybe parenthesized),
to clarify the route to take here if and when we change the global to a finer
grained property.
Jan
> + */
> + (!boot_cpu_has(X86_FEATURE_XEN_SELFSNOOP) ||
> + (is_iommu_enabled(d) && !iommu_snoop)) )
> {
> flush_all(FLUSH_CACHE);
> }
