Page MenuHomeFreeBSD
Differential D13985

Use PCID to optimize PTI.
ClosedPublic
Actions

Authored by kib on Jan 19 2018, 3:04 PM.
Tags
None
Referenced Files
Unknown Object (File)
Mon, Dec 2, 5:16 PM
Unknown Object (File)
Sat, Nov 30, 9:55 AM
Unknown Object (File)
Thu, Nov 28, 11:35 PM
Unknown Object (File)
Wed, Nov 27, 5:09 PM
Unknown Object (File)
Mon, Nov 25, 7:35 AM
Unknown Object (File)
Mon, Nov 25, 5:53 AM
Unknown Object (File)
Mon, Nov 25, 1:08 AM
Unknown Object (File)
Sun, Nov 24, 3:15 PM
View All Files
Subscribers
arshankhanifar_gmail.com
cem
emaste
imp
kbowling
mhorne063_gmail.com
pho
View All 9 Subscribers

Details

Reviewers
alc
markj
Commits
rS328470: Use PCID to optimize PTI.
Summary

Use PCID to avoid complete TLB shootdown when switching between user and kernel mode with PTI enabled.

I use the model close to what I read about KAISER, user-mode PCID has 1:1 correspondence to the kernel-mode PCID, by setting bit 11 in PCID. Full kernel-mode TLB shootdown is performed on context switches, since KVA TLB invalidation only works in the current pmap. On the other hand, on kernel/user switches, CR3_PCID_SAVE bit is set and we do not clear TLB.

I can imagine alternative use of PCID, where there is only one PCID allocated for the kernel pmap. Then, there is no need to shootdown kernel TLB entries on context switch. But copyout(3) would need to either use method similar to proc_rwmem() to access the userspace data, or (in reverse) provide a temporal mapping for the kernel buffer into user mode PCID and use trampoline for copy.

Below is the comparison of the PTI overhead on a haswell machine, which has INVPCID instruction. Patch is not yet tested, I only booted multiuser on haswell (INVPCID) and sandy (PCID but no INVPCID).

./syscall_timing_64 getppid

PCID pti
Clock resolution: 0.000000001
test loop time iterations periteration
getppid 0 1.013932533 2119160 0.000000478
getppid 1 1.018957555 2129562 0.000000478
getppid 2 1.000971159 2091990 0.000000478
getppid 3 1.000953178 2091982 0.000000478
getppid 4 1.020247113 2131976 0.000000478
getppid 5 1.037923614 2169751 0.000000478
getppid 6 1.047713414 2189130 0.000000478
getppid 7 1.038963785 2171784 0.000000478
getppid 8 1.039963943 2173825 0.000000478
getppid 9 1.035961526 2165531 0.000000478

PCID !pti
Clock resolution: 0.000000001
test loop time iterations periteration
getppid 0 1.012187691 2985156 0.000000339
getppid 1 1.023738555 3018773 0.000000339
getppid 2 1.042912187 3075443 0.000000339
getppid 3 1.043958246 3078485 0.000000339
getppid 4 1.043985730 3078480 0.000000339
getppid 5 1.042927727 3075419 0.000000339
getppid 6 1.043948467 3078553 0.000000339
getppid 7 1.035955084 3054875 0.000000339
getppid 8 1.000223259 2949554 0.000000339
getppid 9 1.007950158 2972239 0.000000339

!PCID pti
Clock resolution: 0.000000001
test loop time iterations periteration
getppid 0 1.038952102 1719328 0.000000604
getppid 1 1.024812746 1695689 0.000000604
getppid 2 1.008090182 1667812 0.000000604
getppid 3 1.042990431 1725911 0.000000604
getppid 4 1.043929676 1726714 0.000000604
getppid 5 1.042965631 1725831 0.000000604
getppid 6 1.043961847 1727683 0.000000604
getppid 7 1.042965348 1726560 0.000000604
getppid 8 1.043959939 1727769 0.000000604
getppid 9 1.043970836 1727738 0.000000604

Test Plan

Peter, could you, please, test the patch ?

I did several simple smoke-testing actions, in particular, compiled the openjdk port on debug and production kernel. It is interesting to test this both on machine which reports INVPCID CPU feature, and on machine with PCID but without INVPCID, amd64 only.

Thank you in advance.

Diff Detail

Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

kib created this revision.Jan 19 2018, 3:04 PM
Herald added a subscriber: imp. · View Herald TranscriptJan 19 2018, 3:04 PM
emaste added subscribers: arshankhanifar_gmail.com, mhorne063_gmail.com.Jan 19 2018, 3:30 PM
cem added a subscriber: cem.Jan 19 2018, 5:29 PM
emaste added a comment.Jan 19 2018, 10:29 PM

This passes smoke test on my Sandy Bridge laptop (X220).

kib updated this revision to Diff 38248.Jan 20 2018, 1:39 PM

Fix i386 compilation.

kib edited the test plan for this revision. (Show Details)Jan 20 2018, 1:41 PM
kib added a subscriber: pho.
kib updated this revision to Diff 38251.Jan 20 2018, 3:14 PM

Merge with head.

swills added a subscriber: swills.Jan 20 2018, 4:01 PM
markj added inline comments.Jan 20 2018, 7:47 PM
sys/amd64/amd64/mp_machdep.c
468

IMHO it would be clearer to write

if (smp_tlb_pmap == kernel_pmap) {
    d.pcid = 0;
    invpcid(&d, INVPCID_CTXGLOB);
} else {
    d.pcid = smp_tlb_pmap->pm_pcids[PCPU_GET(cpuid)].pm_pcid;
    invpcid(&d, INVPCID_CTX);
    d.pcid |= PMAP_PCID_USER_PT;
    invpcid(&d, INVPCID_CTX);
}
503

Perhaps cache the cpuid here and in the other handlers, rather than fetching from pcpu area multiple times.

560

Seems we are missing the user PCID bit here?

597

Extra newline.

sys/amd64/amd64/pmap.c
1598

Why does this need to be a critical section?

sys/amd64/amd64/support.S
807

Can't these routines be implemented in C?

kib marked 3 inline comments as done.Jan 20 2018, 8:14 PM
kib added inline comments.
sys/amd64/amd64/pmap.c
1598

pm_pcid are recalculated on the context switch. If we are removed from CPU and then rescheduled (pin still allows that) we would end up using incorrect pcid for the operation.

sys/amd64/amd64/support.S
807

This is quite risky. We are temporary swapping address spaces there, in particular, we enter the AS where the C stack is not mapped. I absolutely need to ensure that the routines do not access anything outside the explicitly allowed addresses while on user page table.

kib marked an inline comment as done.Jan 20 2018, 8:16 PM
kib updated this revision to Diff 38258.Jan 20 2018, 8:27 PM

Shoot down user page table TLB on context switches. It is done automatically only for the kernel twin.
FIx bug pointed out by Mark where wrong PCID was used for user table shoot down on non-INVPCID machine.
Minor editings.

kib updated this revision to Diff 38261.Jan 20 2018, 8:34 PM

When reloading CR3 for shoot down, do not set the save bit.

markj added inline comments.Jan 20 2018, 8:44 PM
sys/amd64/amd64/mp_machdep.c
507

Need the user bit here too.

sys/amd64/amd64/support.S
807

Could you please add prototypes like in the functions above and in cpu_switch.S?

kib updated this revision to Diff 38263.Jan 20 2018, 8:56 PM
kib marked 2 inline comments as done.

Add comments to support.S pmap_pti functions.
One more missed USER_PT bit.

kib updated this revision to Diff 38265.Jan 20 2018, 10:37 PM

Do not call pmap_pti_pcid_invalidate() for pmaps which do not have usermode page tables.

markj added inline comments.Jan 21 2018, 3:47 AM
sys/amd64/amd64/support.S
807

Hm, so isn't this unsafe wrt NMIs?

markj added inline comments.Jan 21 2018, 3:50 AM
sys/amd64/amd64/mp_machdep.c
629

Missing user bit. :)

632

Extra newline.

kbowling added a subscriber: kbowling.Jan 21 2018, 4:20 AM
markj added inline comments.Jan 21 2018, 5:10 AM
sys/amd64/amd64/support.S
807

Never mind, sorry. I forgot that the NMI handler reloads from KCR3.

kib marked 2 inline comments as done.Jan 21 2018, 10:32 AM
kib added inline comments.
sys/amd64/amd64/support.S
807

Yes, there are many more places where kernel is executing with UCR3, and NMI/MCE handlers already handle that.

kib updated this revision to Diff 38273.Jan 21 2018, 1:20 PM

Mark another finding of missed USER_PT.
Update pcpu kcr3 and ucr3 only after shootdown.
Add some comments.

alc added inline comments.Jan 21 2018, 5:44 PM
sys/amd64/amd64/pmap.c
1685

Stray blank line.

1751

Stray blank line.

kib updated this revision to Diff 38279.Jan 21 2018, 6:01 PM

For PCID+non-INVPCID, only reload kcr3 from pmap in invalidation IPI handler when the pmap is current. Otherwise we end up with invalid translation loaded for usermode part.
Check for valid ucr3 before committing to do the userspace part invalidation.

kib added a comment.Jan 21 2018, 11:18 PM

Diff 38279 survived full make -j 32 tinderbox run on ivy bridge machine (PCID but no INVPCID).

markj accepted this revision.Jan 22 2018, 4:14 PM

I can't see any problems in this version.

sys/amd64/amd64/apic_vector.S
188

Is there a reason for not having a separate PTI entry point, as you do for invlpg and invlrng?

sys/amd64/amd64/mp_machdep.c
498

"achieve it" or "effect it" sound more natural than "reach it" to me.

628

Logically it feels like pmap_pti_pcid_invl{pg,rng}() should be setting CR3_PCID_SAVE rather than having it be set in the caller. It would also make the non-INVPCID handlers more consistent.

sys/amd64/amd64/pmap.c
7341

There's a stray space here.

This revision is now accepted and ready to land.Jan 22 2018, 4:14 PM
alc added inline comments.Jan 22 2018, 5:17 PM
sys/amd64/amd64/mp_machdep.c
498

"... to clear kernel mappings from the TLB in the ..."

Also, I'm confused. The "if" expression is "!=". Shouldn't it be "==".

kib marked 3 inline comments as done.Jan 22 2018, 5:32 PM
kib added inline comments.
sys/amd64/amd64/mp_machdep.c
628

I did not the bit set in assembly to keep the assembler source as small as possible. If you consider this important, I will change the asm.

Note that for the support.S' reader, having CR3_PCID_SAVE set in the caller is more natural (IMHO).

kib marked an inline comment as done.Jan 22 2018, 5:34 PM
kib updated this revision to Diff 38319.Jan 22 2018, 5:42 PM

Regularize IPI handlers names.
Fix if() condition.
Update comments.

This revision now requires review to proceed.Jan 22 2018, 5:42 PM
alc added inline comments.Jan 22 2018, 6:07 PM
sys/amd64/amd64/mp_machdep.c
143

I don't think that you need a continuation line here.

499

I fear that no one (besides us) will understand this comment.

Isn't the issue here that there are now multiple kernel page tables/address spaces, and we want the kernel mapping(s) flushed from all of them?

kib marked an inline comment as done.Jan 22 2018, 6:27 PM
kib added inline comments.
sys/amd64/amd64/mp_machdep.c
499

No, the kernel mappings as well as user mappings are always flushed on context switch, even with PCID enabled. I have to do this because kernel mode PCID are per-pmap. So only the currently active TLB partition is to be flushed.

What I am trying to say there is that I need to know PCID of the pmap where to flush the kernel mappings, this is most likely the PCID for the current pmap. But since the function was asked to do this for the kernel pmap, to not reach out and find the corresponding PCID (can it be non-current after all ?), I use the global flush.

markj added inline comments.Jan 22 2018, 6:29 PM
sys/amd64/amd64/mp_machdep.c
628

I don't think it's important.

This revision was not accepted when it landed; it landed in state Needs Review.Jan 27 2018, 11:50 AM
Closed by commit rS328470: Use PCID to optimize PTI. (authored by kib). · Explain Why
This revision was automatically updated to reflect the committed changes.
Herald added a subscriber: rpokala. · View Herald TranscriptJan 27 2018, 11:50 AM

Revision Contents
Changeset List

PathSize
sys/
amd64/
amd64/
20 lines
172 lines
152 lines
32 lines
include/
6 lines
9 lines
i386/
i386/
4 lines
2 lines
x86/
include/
5 lines
x86/
11 lines

Diff 38251

View Options

sys/amd64/amd64/apic_vector.S

Loading...
View Options

sys/amd64/amd64/mp_machdep.c

Loading...
View Options

sys/amd64/amd64/pmap.c

Loading...
View Options

sys/amd64/amd64/support.S

Loading...
View Options

sys/amd64/include/pmap.h

Loading...
View Options

sys/amd64/include/smp.h

Loading...
View Options

sys/i386/i386/pmap.c

Loading...
View Options

sys/i386/i386/vm_machdep.c

Loading...
View Options

sys/x86/include/x86_smp.h

Loading...
View Options

sys/x86/x86/mp_x86.c

Loading...