On Fri, Oct 16, 2020 at 01:12:26PM +0200, Peter Zijlstra wrote:
On Tue, Oct 13, 2020 at 11:31:45AM -0700, Dave Hansen wrote:
+/**
- It should also be noted that the underlying WRMSR(MSR_IA32_PKRS) is not
- serializing but still maintains ordering properties similar to WRPKRU.
- The current SDM section on PKRS needs updating but should be the same as
- that of WRPKRU. So to quote from the WRPKRU text:
- WRPKRU will never execute transiently. Memory accesses
- affected by PKRU register will not execute (even transiently)
- until all prior executions of WRPKRU have completed execution
- and updated the PKRU register.
- */
+void write_pkrs(u32 new_pkrs) +{
- u32 *pkrs;
- if (!static_cpu_has(X86_FEATURE_PKS))
return;- pkrs = get_cpu_ptr(&pkrs_cache);
- if (*pkrs != new_pkrs) {
*pkrs = new_pkrs;wrmsrl(MSR_IA32_PKRS, new_pkrs);- }
- put_cpu_ptr(pkrs);
+}
It bugs me a *bit* that this is being called in a preempt-disabled region, but we still bother with the get/put_cpu jazz. Are there other future call-sites for this that aren't in preempt-disabled regions?
So the previous version had a useful comment that got lost.
Ok Looking back I see what happened... This comment...
/* * PKRS is only temporarily changed during specific code paths. * Only a preemption during these windows away from the default * value would require updating the MSR. */
... was added to pks_sched_in() but that got simplified down because cleaning up write_pkrs() made the code there obsolete.
This stuff needs to fundamentally be preempt disabled,
I agree, the update to the percpu cache value and MSR can not be torn.
so it either needs to explicitly do so, or have an assertion that preemption is indeed disabled.
However, I don't think I understand clearly. Doesn't [get|put]_cpu_ptr() handle the preempt_disable() for us? Is it not sufficient to rely on that?
Dave's comment seems to be the opposite where we need to eliminate preempt disable before calling write_pkrs().
FWIW I think I'm mistaken in my response to Dave regarding the preempt_disable() in pks_update_protection().
Ira