Page MenuHomeFreeBSD

amd64 pmap: add comment explaining TLB invalidation modes.
ClosedPublic

Authored by kib on Jul 26 2020, 4:44 PM.

Diff Detail

Repository
R10 FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
sys/amd64/amd64/pmap.c
2710

"The kernel page table maps the user and kernel address spaces in their entirety. It is identical to the per-process page table allocated in non-PTI ...

2711–2712

I would open the paragraph with this sentence.

kib marked 7 inline comments as done.Nov 8 2020, 8:57 PM
kib added inline comments.
sys/amd64/amd64/pmap.c
2702

I said 'high operational impact' there. It is explained below, at this point intent is only to note to reader that PTI changes a lot. What specifically is changed comparing for !PTI, is described later.

2704–2705

Typo: I mean PTI vs. non-PTI.

kib marked 2 inline comments as done.

Edits.

sys/amd64/amd64/pmap.c
2707

-> "the Meltdown bug ..."

Remove the stray space between PTI and the subsequent comma.

2716

-> "... that the user space ..."

I would drop the "s" at the end of "kernel page tables".

2717

I would take it for granted that TLB coherence must be maintained. Is the real point that you're trying to make here that these TLB invalidations can't be deferred, unlike the user page table ones mentioned below? If so, I think that this paragraph would be clearer if you swapped the order of the sentences, that is, talk about the user page table first.

2718

"... when the CPU is in user mode. Consequently, some TLB invalidations ...

2721

"The existence of a user mode page table ..."

2722

"by a pm_ucr3 value that differs from ..., in which case pm_ucr3 contains

2723

"... for the user mode page table's root.

2725

"... which CPUs currently have the pmap active. A CPU's bit is set on ...

2727

"... For the kernel page table, the pm_active field

2730

"present in hardware" -> "in use by the hardware"

2733

"..., the pmap sends ...

2734

"... recorded as active in pm_active. Updates to and reads from pm_active are not synchronized, and so they may race with each other. Shootdown handlers ...

2740

"... to. This feature provides a limited namespace ..."

(The text that follows uses "PCID" to refer to a "Process ID" so I want to stop using it to refer to the feature.)

2741

"... 12 bits, supporting 4095 simultaneous IDs total.

2743

"Allocation of a PCID to a pmap ...

2744

"section 15.12, "Other TLB Consistency Algorithms", of Vahalia's book "Unix Internals".

2745

"A PCID cannot ... whole lifetime of a pmap in ...

2747

"when the CPU ...

2749

"switch that activates ...

2753

"On invalidation" -> "On TLB invalidation"

and

"is" -> "are"

2754

"... that the previously allocated ..."

2755–2757

"Consequently, if a pmap is inactive on a CPU, then a TLB shootdown for that pmap and CPU can be initiated by an ordinary memory access to reset the target CPU's generation count within the pmap. The CPU initiating the TLB shootdown does not need to send an IPI to the target CPU.

2760

For consistency, "user mode"

2761

"A user PCID ...

2762

"... bit, 11, to ...

2764

"Userspace" -> "User space"
and
"usermode" -> "user mode"

2765–2766

"... clearing bit 63 of the loaded ..."

2767

"usermode" -> "user mode"

Also, can you clarify whether this is a "total invalidation" for one pmap or all pmaps on that CPU.

2768

-> "... in the user ..."

2772

INVTLB?

2775

-> "If the INVPCID ...

2776

-> "from the kernel page table.

2778

-> "... The kernel reserves ...

2779

"usermode" -> "user mode"

2780

-> "... A context switch allocates a new ...

2781

-> "the recorded PCID is zero or the recorded generation does not match the CPU's

2782

-> "..., effectively flushing the TLB ...

2823

"usermode" -> "user mode"

2824

-> "... for the user page table.

kib marked 39 inline comments as done.

39 more edits.

That's all that I have.

sys/amd64/amd64/pmap.c
2699–2700

I would drop the last sentence of this paragraph, i.e., the sentence "For SMP, ...", and then merge this paragraph with the next one.

(The use of IPIs comes up below. Here, I think it is lower level than the rest of this new combined paragraph.)

2701

This line is missing an "*".

2702

"The kernel configuration option that has the greatest operational impact on TLB invalidation is PTI, which ...

2702–2705

PTI is the kernel configuration option that most affects

2703

"CPUs. The most impactful hardware ...

2714

"allocated" -> "used"

2718–2720

"... mode. In contrast, the user space part of the kernel page table is used for copyout(9), so TLB invalidations on this page table cannot be postponed."

2723

"... by a pm_ucr3

2736

For consistency, add another space after the period.

2754–2755

"... count, pm_gen, which is ...

2758

"... valid. Effectively, zeroing any of these counters triggers a TLB shootdown ...

2760

"of a new ...

2775

Drop the first "the": "clearing bit 63 ...

2780–2781

"modes. If the requested invalidation is for a specific address or the total invalidation of a currently active pmap, then the TLB is flushed using INVLPG for a kernel page table, and ..."

I feel like there is a missing qualifying phrase at the end of INVPCID(INVPCID_CTXGLOB)/invltlb_glob().

2798

I think that all of the entries of this form would be clearer if written as "remote user page, inactive pmap:" Otherwise, it reads like the page must be inactive.

kib marked 14 inline comments as done.Jan 6 2021, 7:10 PM
kib added inline comments.
sys/amd64/amd64/pmap.c
2702–2705

Sorry, I cannot get sense of this comment.

Another batch of edits.
Move comment out of #ifdef SMP.

sys/amd64/amd64/pmap.c
2702–2705

I think that this was the start of a comment that I intended to delete because it was superseded by another comment.

sys/amd64/amd64/pmap.c
2702

"mainly" -> "first"

2718

Can shorten to: "... from kernel to user mode."

2720

-> "... be similarly postponed.

2728

-> "... switch to the pmap, and ..."

2732

Add comma, -> "..., e.g., ..."

2752

Add comma, -> "i.e., ..."

2776

-> "... TLB entries for ..."

2777

Can we change "If ucr3_load_mask is set, then local ..." to "In which case, local ..."?

2784

We rarely use the word "process" in this comment. Can we say, "user space", instead?

2800

Here is a technical question as opposed to one about wording: Given the IPI:INVLPG why is pm_gen zeroed? Is this because of aforementioned pm_active race?

2838

Are you planning to replace this?

kib marked 13 inline comments as done.Jan 9 2021, 9:29 PM
kib added inline comments.
sys/amd64/amd64/pmap.c
2800

Yes, due to a possibility to lost the race and have the target pmap no longer active.

I believe that the recent fix where pm_gen zeroing was misplaced and this caused observable issues proves the point.

2838

Not in this change.

I plan to write about generation, emulation of A/D. Might be mention different format of EPT tables.

For SVM, something should be also noted.

kib marked 2 inline comments as done.

Next batch of edits.

sys/amd64/amd64/pmap.c
2800

Then, I would add a parenthetical comment on the next line: "(Both actions are required to handle the aforementioned pm_active races.)"

sys/amd64/amd64/pmap.c
2760

For consistency, "cpu" -> "CPU"

kib marked an inline comment as done.

Add note about INVLPG+pm_gen = 0

kib marked an inline comment as done.Jan 9 2021, 11:38 PM
This revision is now accepted and ready to land.Jan 9 2021, 11:56 PM