On Tue, Jul 14, 2020 at 12:42:11PM -0700, Dave Hansen wrote:

On 7/14/20 12:29 PM, Peter Zijlstra wrote:

quoted

On Tue, Jul 14, 2020 at 12:06:16PM -0700, Ira Weiny wrote:

quoted

On Tue, Jul 14, 2020 at 10:44:51AM +0200, Peter Zijlstra wrote:

quoted

So, if I followed along correctly, you're proposing to do a WRMSR per
k{,un}map{_atomic}(), sounds like excellent performance all-round :-(

Only to pages which have this additional protection, ie not DRAM.

User mappings of this memory is not affected (would be covered by User PKeys if
desired).  User mappings to persistent memory are the primary use case and the
performant path.

Because performance to non-volatile memory doesn't matter? I think Dave
has a better answer here ...

So, these WRMSRs are less evil than normal.  They're architecturally
non-serializing instructions, just like the others in the SDM WRMSR
documentation:

	Note that WRMSR to the IA32_TSC_DEADLINE MSR (MSR index 6E0H)
	and the X2APIC MSRs (MSR indices 802H to 83FH) are  not
	serializing.

This section of the SDM needs to be updated for the PKRS.  Also note
that the PKRS WRMSR is similar in its ordering properties to WRPKRU:

	WRPKRU will never execute speculatively. Memory accesses
	affected by PKRU register will not execute (even speculatively)
	until all prior executions of WRPKRU have completed execution
	and updated the PKRU register.

Which means we don't have to do silliness like LFENCE before WRMSR to
get ordering *back*.  This is another tidbit that needs to get added to
the SDM.  It should probably also get captured in the changelog.

But, either way, this *will* make accessing PMEM more expensive from the
kernel.  No escaping that.  But, we've also got customers saying they
won't deploy PMEM until we mitigate this stray write issue.  Those folks
are quite willing to pay the increased in-kernel cost for increased
protection from stray kernel writes.  Intel is also quite motivated
because we really like increasing the number of PMEM deployments. :)

Ira, can you make sure this all gets pulled into the changelogs somewhere?

Yes of course.  Thanks for writing that up.

Ira