Recent patch [1] disabled a self-snoop feature on a list of processor
models with a known errata, so we are confident that the feature
should work on remaining models also for other purposes than to speed
up MTRR programming.
I would like to resurrect an old patch [2] that avoids calling clflush
and wbinvd
to invalidate caches when CPU supports selfsnoop.
The patch was ported to latest Fedora kernel (5.1.16) and tested with
CONFIG_CPA_DEBUG on INTEL_FAM6_IVYBRIDGE_X. The relevant ports of
dmesg show:
...
< hundreds of CPA protect messages, resulting from set_memory_rw CPA
undo test in mm/init_64.c >
CPA protect Rodata RO: 0xffffffffbd1fe000 - 0xffffffffbd1fefff PFN
1461fe req 8000000000000063 prevent 0000000000000002
CPA protect Rodata RO: 0xffff889c461fe000 - 0xffff889c461fefff PFN
1461fe req 8000000000000063 prevent 0000000000000002
Testing CPA: again
Freeing unused kernel image memory: 2016K
Freeing unused kernel image memory: 4K
x86/mm: Checked W+X mappings: passed, no W+X pages found.
rodata_test: all tests were successful
x86/mm: Checking user space page tables
x86/mm: Checked W+X mappings: passed, no W+X pages found.
and from CPA selftest:
CPA self-test:
4k 36352 large 4021 gb 0 x 81[ffff889b00098000-ffff889bdf7ff000] miss 133120
4k 180224 large 3740 gb 0 x 81[ffff889b00098000-ffff889bdf7ff000] miss 133120
4k 180224 large 3740 gb 0 x 81[ffff889b00098000-ffff889bdf7ff000] miss 133120
ok.
[1] https://lkml.org/lkml/2019/6/27/828
[2] https://lkml.org/lkml/2009/4/8/508
Uros.
On Thu, Jul 11, 2019 at 1:13 AM Uros Bizjak <[email protected]> wrote:
>
> Recent patch [1] disabled a self-snoop feature on a list of processor
> models with a known errata, so we are confident that the feature
> should work on remaining models also for other purposes than to speed
> up MTRR programming.
>
> I would like to resurrect an old patch [2] that avoids calling clflush
> and wbinvd
> to invalidate caches when CPU supports selfsnoop.
The big question here is: what are all the reasons that we might need
to flush? Certainly, for stuff like SEV and MKTME, we need to flush
regardless of any self-snoop capability.
On Thu, Jul 11, 2019 at 4:39 PM Andy Lutomirski <[email protected]> wrote:
>
> On Thu, Jul 11, 2019 at 1:13 AM Uros Bizjak <[email protected]> wrote:
> >
> > Recent patch [1] disabled a self-snoop feature on a list of processor
> > models with a known errata, so we are confident that the feature
> > should work on remaining models also for other purposes than to speed
> > up MTRR programming.
> >
> > I would like to resurrect an old patch [2] that avoids calling clflush
> > and wbinvd
> > to invalidate caches when CPU supports selfsnoop.
>
> The big question here is: what are all the reasons that we might need
> to flush? Certainly, for stuff like SEV and MKTME, we need to flush
> regardless of any self-snoop capability.
No AMD target defines self-snoop capability, and set_memory_encrypted
forces cache clearing in __set_memory_enc_dec:
/*
* Before changing the encryption attribute, we need to flush caches.
*/
cpa_flush(&cpa, 1);
Uros.
Uros,
On Thu, 11 Jul 2019, Uros Bizjak wrote:
> Recent patch [1] disabled a self-snoop feature on a list of processor
> models with a known errata, so we are confident that the feature
> should work on remaining models also for other purposes than to speed
> up MTRR programming.
>
> I would like to resurrect an old patch [2] that avoids calling clflush
> and wbinvd
> to invalidate caches when CPU supports selfsnoop.
Please do not attach patches, send them inline and please add a proper
changelog. Just saying 'Disable CPA cache flush for selfsnoop targets' in
the subject line then nada gives absolutely zero information.
> The patch was ported to latest Fedora kernel (5.1.16) and tested with
> CONFIG_CPA_DEBUG on INTEL_FAM6_IVYBRIDGE_X. The relevant ports of
> dmesg show:
>
> ...
> < hundreds of CPA protect messages, resulting from set_memory_rw CPA
> undo test in mm/init_64.c >
> CPA protect Rodata RO: 0xffffffffbd1fe000 - 0xffffffffbd1fefff PFN
> 1461fe req 8000000000000063 prevent 0000000000000002
> CPA protect Rodata RO: 0xffff889c461fe000 - 0xffff889c461fefff PFN
> 1461fe req 8000000000000063 prevent 0000000000000002
> Testing CPA: again
> Freeing unused kernel image memory: 2016K
> Freeing unused kernel image memory: 4K
> x86/mm: Checked W+X mappings: passed, no W+X pages found.
> rodata_test: all tests were successful
> x86/mm: Checking user space page tables
> x86/mm: Checked W+X mappings: passed, no W+X pages found.
>
> and from CPA selftest:
>
> CPA self-test:
> 4k 36352 large 4021 gb 0 x 81[ffff889b00098000-ffff889bdf7ff000] miss 133120
> 4k 180224 large 3740 gb 0 x 81[ffff889b00098000-ffff889bdf7ff000] miss 133120
> 4k 180224 large 3740 gb 0 x 81[ffff889b00098000-ffff889bdf7ff000] miss 133120
> ok.
These outputs are pretty useless simply because the selftest only verifies
the inner workings of CPA itself, but has nothing to do with the
correctness vs. cache flushing.
Thanks,
tglx
On Mon, Jul 15, 2019 at 10:24 AM Thomas Gleixner <[email protected]> wrote:
>
> Uros,
>
> On Thu, 11 Jul 2019, Uros Bizjak wrote:
> > Recent patch [1] disabled a self-snoop feature on a list of processor
> > models with a known errata, so we are confident that the feature
> > should work on remaining models also for other purposes than to speed
> > up MTRR programming.
> >
> > I would like to resurrect an old patch [2] that avoids calling clflush
> > and wbinvd
> > to invalidate caches when CPU supports selfsnoop.
>
> Please do not attach patches, send them inline and please add a proper
> changelog. Just saying 'Disable CPA cache flush for selfsnoop targets' in
> the subject line then nada gives absolutely zero information.
Thanks for your remarks and instructions!
I'll send a new revision of the patch with expanded ChangeLog later today,
saying something along the lines of:
"CPUs which have self-snooping capability can handle conflicting
memory type across CPUs by snooping its own cache. Commit #fd329f276ecaa
("x86/mtrr: Skip cache flushes on CPUs with cache self-snooping")
avoids cache flushes when MTRR registers are programmed. The Page
Attribute Table (PAT) is a companion feature to the MTRRs, and according
to section 11.12.4 of the Intel 64 and IA 32 Architectures Software
Developer's Manual, if the CPU supports cache self-snooping, it is not
necessary to flush caches when remapping a page that was previously
mapped as a different memory type.
Note that commit #1e03bff360010
("x86/cpu/intel: Clear cache self-snoop capability in CPUs with known errata")
cleared cache self-snoop capability for CPUs where conflicting memory types
lead to unpredictable behavior, machine check errors, or hangs."
> > The patch was ported to latest Fedora kernel (5.1.16) and tested with
> > CONFIG_CPA_DEBUG on INTEL_FAM6_IVYBRIDGE_X. The relevant ports of
> > dmesg show:
> >
> > ...
> > < hundreds of CPA protect messages, resulting from set_memory_rw CPA
> > undo test in mm/init_64.c >
> > CPA protect Rodata RO: 0xffffffffbd1fe000 - 0xffffffffbd1fefff PFN
> > 1461fe req 8000000000000063 prevent 0000000000000002
> > CPA protect Rodata RO: 0xffff889c461fe000 - 0xffff889c461fefff PFN
> > 1461fe req 8000000000000063 prevent 0000000000000002
> > Testing CPA: again
> > Freeing unused kernel image memory: 2016K
> > Freeing unused kernel image memory: 4K
> > x86/mm: Checked W+X mappings: passed, no W+X pages found.
> > rodata_test: all tests were successful
> > x86/mm: Checking user space page tables
> > x86/mm: Checked W+X mappings: passed, no W+X pages found.
> >
> > and from CPA selftest:
> >
> > CPA self-test:
> > 4k 36352 large 4021 gb 0 x 81[ffff889b00098000-ffff889bdf7ff000] miss 133120
> > 4k 180224 large 3740 gb 0 x 81[ffff889b00098000-ffff889bdf7ff000] miss 133120
> > 4k 180224 large 3740 gb 0 x 81[ffff889b00098000-ffff889bdf7ff000] miss 133120
> > ok.
>
> These outputs are pretty useless simply because the selftest only verifies
> the inner workings of CPA itself, but has nothing to do with the
> correctness vs. cache flushing.
Please note that CONFIG_CPA_DEBUG also spawns a pageattr-test kthread
which remaps a memory page every 30 seconds. I was confident enough to
run the patched kernel (with CONFIG_CPA_DEBUG) on my main workstation
(Ivybridge-X, Fedora 30), already for a week without a single problem.
Uros.
Uros Bizjak <[email protected]> writes:
> Recent patch [1] disabled a self-snoop feature on a list of processor
> models with a known errata, so we are confident that the feature
> should work on remaining models also for other purposes than to speed
> up MTRR programming.
MTRR is very different than TLBs.
From my understanding not flushing with PAT is only safe everywhere when
the memory is only used for coherent devices (like the Internal GPU on
Intel CPUs). We don't have any infrastructure to track or enforce
this unfortunately.
-Andi
On Mon, 15 Jul 2019, Andi Kleen wrote:
> Uros Bizjak <[email protected]> writes:
>
> > Recent patch [1] disabled a self-snoop feature on a list of processor
> > models with a known errata, so we are confident that the feature
> > should work on remaining models also for other purposes than to speed
> > up MTRR programming.
>
> MTRR is very different than TLBs.
>
> >From my understanding not flushing with PAT is only safe everywhere when
> the memory is only used for coherent devices (like the Internal GPU on
> Intel CPUs). We don't have any infrastructure to track or enforce
> this unfortunately.
Right, we don't know where the PAT invocation comes from and whether they
are safe to omit flushing the cache. The module load code would be one
obvious candidate.
But unless there is some really worthwhile speedup, e.g. for boot, then
adding some flag to let CPA know about the safe 'no flush' operation might
be not worth it.
Thanks,
tglx
On Mon, Jul 15, 2019 at 8:41 PM Thomas Gleixner <[email protected]> wrote:
>
> On Mon, 15 Jul 2019, Andi Kleen wrote:
> > Uros Bizjak <[email protected]> writes:
> >
> > > Recent patch [1] disabled a self-snoop feature on a list of processor
> > > models with a known errata, so we are confident that the feature
> > > should work on remaining models also for other purposes than to speed
> > > up MTRR programming.
> >
> > MTRR is very different than TLBs.
> >
> > >From my understanding not flushing with PAT is only safe everywhere when
> > the memory is only used for coherent devices (like the Internal GPU on
> > Intel CPUs). We don't have any infrastructure to track or enforce
> > this unfortunately.
>
> Right, we don't know where the PAT invocation comes from and whether they
> are safe to omit flushing the cache. The module load code would be one
> obvious candidate.
>
> But unless there is some really worthwhile speedup, e.g. for boot, then
> adding some flag to let CPA know about the safe 'no flush' operation might
> be not worth it.
For the reference, FreeBSD implements this approach, later changed to
use pmap_invalidate_cache_range ifunc (that calls
pmap_invalidate_cache_range_selfsnoop for targets with self-snoop
capability) and pmap_force_invalidate_cache_range [1]. The full
cross-referenced source is at [2].
[1] https://reviews.freebsd.org/D16736
[2] http://fxr.watson.org/fxr/source/amd64/amd64/pmap.c
Uros.
On Mon, 15 Jul 2019, Uros Bizjak wrote:
> On Mon, Jul 15, 2019 at 8:41 PM Thomas Gleixner <[email protected]> wrote:
> >
> > On Mon, 15 Jul 2019, Andi Kleen wrote:
> > > Uros Bizjak <[email protected]> writes:
> > >
> > > > Recent patch [1] disabled a self-snoop feature on a list of processor
> > > > models with a known errata, so we are confident that the feature
> > > > should work on remaining models also for other purposes than to speed
> > > > up MTRR programming.
> > >
> > > MTRR is very different than TLBs.
> > >
> > > >From my understanding not flushing with PAT is only safe everywhere when
> > > the memory is only used for coherent devices (like the Internal GPU on
> > > Intel CPUs). We don't have any infrastructure to track or enforce
> > > this unfortunately.
> >
> > Right, we don't know where the PAT invocation comes from and whether they
> > are safe to omit flushing the cache. The module load code would be one
> > obvious candidate.
> >
> > But unless there is some really worthwhile speedup, e.g. for boot, then
> > adding some flag to let CPA know about the safe 'no flush' operation might
> > be not worth it.
>
> For the reference, FreeBSD implements this approach, later changed to
> use pmap_invalidate_cache_range ifunc (that calls
> pmap_invalidate_cache_range_selfsnoop for targets with self-snoop
> capability) and pmap_force_invalidate_cache_range [1]. The full
> cross-referenced source is at [2].
That does not answer the question whether it's worthwhile to do that.
Thanks,
tglx
>
> That does not answer the question whether it's worthwhile to do that.
It's likely worthwhile for (Intel integrated) graphics.
There was also a recent issue with 3dxp/dax, which uses ioremap in some
cases.
-Andi
> Right, we don't know where the PAT invocation comes from and whether they
> are safe to omit flushing the cache. The module load code would be one
> obvious candidate.
Module load just changes the writable/executable status, right? That shouldn't
need to flush in any case because it doesn't change the caching attributes.
-Andi
On Mon, Jul 15, 2019 at 12:39 PM Andi Kleen <[email protected]> wrote:
>
> > Right, we don't know where the PAT invocation comes from and whether they
> > are safe to omit flushing the cache. The module load code would be one
> > obvious candidate.
>
> Module load just changes the writable/executable status, right? That shouldn't
> need to flush in any case because it doesn't change the caching attributes.
>
Indeed. module load should require a single TLB flush and no cache
flushes. I don't think we're currently efficient enough to do it with
a single TLB flush, but we should be able to...
On Mon, 15 Jul 2019, Andi Kleen wrote:
> > Right, we don't know where the PAT invocation comes from and whether they
> > are safe to omit flushing the cache. The module load code would be one
> > obvious candidate.
>
> Module load just changes the writable/executable status, right? That shouldn't
> need to flush in any case because it doesn't change the caching attributes.
Ah right. We don't flush when the caching attributes are not changed.
Thanks,
tglx
On Mon, Jul 15, 2019 at 12:38 PM Andi Kleen <[email protected]> wrote:
>
> >
> > That does not answer the question whether it's worthwhile to do that.
>
> It's likely worthwhile for (Intel integrated) graphics.
>
> There was also a recent issue with 3dxp/dax, which uses ioremap in some
> cases.
>
FWIW, I applied this simpler patch:
diff --git a/arch/x86/mm/pageattr.c b/arch/x86/mm/pageattr.c
index 6a9a77a403c9..a933f99b176a 100644
--- a/arch/x86/mm/pageattr.c
+++ b/arch/x86/mm/pageattr.c
@@ -1729,6 +1729,7 @@ static int change_page_attr_set_clr(unsigned
long *addr, int numpages,
* attributes:
*/
cache = !!pgprot2cachemode(mask_set);
+ WARN_ON(cache);
/*
* On error; flush everything to be sure.
and booted a VM, including loading a module. The warning did not
fire. For the most part, we use PAT for things like ioremap_wc(), but
there's no flush, since there's no preexisting mapping at all.
I haven't tested on a real kernel with i915. Does i915 really hit
this code path? Does it happen more than once or twice at boot?
The only case I can think of where this would really matter is DAX, if
anyone uses WT for DAX.
> I haven't tested on a real kernel with i915. Does i915 really hit
> this code path? Does it happen more than once or twice at boot?
Yes some workloads allocate/free a lot of write combined memory
for graphics objects.
-Andi
On Mon, Jul 15, 2019 at 3:53 PM Andi Kleen <[email protected]> wrote:
>
> > I haven't tested on a real kernel with i915. Does i915 really hit
> > this code path? Does it happen more than once or twice at boot?
>
> Yes some workloads allocate/free a lot of write combined memory
> for graphics objects.
>
But where does that memory come from? If it's from device memory
(i.e. memory that's not in the kernel direct map), then, unless I
missed something, we're never changing the cache mode per se -- we're
just ioremap_wc-ing it, which doesn't require a flush.
IOW I'm wondering if there's any workload where this patch makes a difference.
On Mon, Jul 15, 2019 at 04:10:36PM -0700, Andy Lutomirski wrote:
> On Mon, Jul 15, 2019 at 3:53 PM Andi Kleen <[email protected]> wrote:
> >
> > > I haven't tested on a real kernel with i915. Does i915 really hit
> > > this code path? Does it happen more than once or twice at boot?
> >
> > Yes some workloads allocate/free a lot of write combined memory
> > for graphics objects.
> >
>
> But where does that memory come from? If it's from device memory
> (i.e. memory that's not in the kernel direct map), then, unless I
> missed something, we're never changing the cache mode per se -- we're
> just ioremap_wc-ing it, which doesn't require a flush.
Integraded graphics doesn't have device memory. There's an reserved
memory area, but a lot of the buffers the GPU works with
come from main memory.
-Andi