Page MenuHomeFreeBSD
Differential D13985 Diff 38207 sys/amd64/amd64/mp_machdep.c

Changeset View

Standalone View

View Options

sys/amd64/amd64/mp_machdep.c

Show First 20 Lines • Show All 129 Lines • ▼ Show 20 Lines for (i = 0; i < MAXCPU; i++) {
cpu_ipi_pending[i] = 0; cpu_ipi_pending[i] = 0;
} }
/* Install an inter-CPU IPI for TLB invalidation */ /* Install an inter-CPU IPI for TLB invalidation */
if (pmap_pcid_enabled) { if (pmap_pcid_enabled) {
if (invpcid_works) { if (invpcid_works) {
setidt(IPI_INVLTLB, pti ? IDTVEC(invltlb_invpcid_pti) : setidt(IPI_INVLTLB, pti ? IDTVEC(invltlb_invpcid_pti) :
IDTVEC(invltlb_invpcid), SDT_SYSIGT, SEL_KPL, 0); IDTVEC(invltlb_invpcid), SDT_SYSIGT, SEL_KPL, 0);
setidt(IPI_INVLPG, pti ? IDTVEC(invlpg_invpcid_pti_pti) :
IDTVEC(invlpg), SDT_SYSIGT, SEL_KPL, 0);
setidt(IPI_INVLRNG, pti ?
IDTVEC(invlrng_invpcid_pti_pti) : IDTVEC(invlrng),
SDT_SYSIGT, SEL_KPL, 0);
} else { } else {
alcUnsubmitted
Done
Inline Actions

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

alc: I don't think that you need a continuation line here.
setidt(IPI_INVLTLB, pti ? IDTVEC(invltlb_pcid_pti) : setidt(IPI_INVLTLB, pti ? IDTVEC(invltlb_pcid_pti) :
IDTVEC(invltlb_pcid), SDT_SYSIGT, SEL_KPL, 0); IDTVEC(invltlb_pcid), SDT_SYSIGT, SEL_KPL, 0);
setidt(IPI_INVLPG, pti ? IDTVEC(invlpg_pcid_pti_pti) :
IDTVEC(invlpg), SDT_SYSIGT, SEL_KPL, 0);
setidt(IPI_INVLRNG, pti ? IDTVEC(invlrng_pcid_pti_pti) :
IDTVEC(invlrng), SDT_SYSIGT, SEL_KPL, 0);
} }
} else { } else {
setidt(IPI_INVLTLB, pti ? IDTVEC(invltlb_pti) : IDTVEC(invltlb), setidt(IPI_INVLTLB, pti ? IDTVEC(invltlb_pti) : IDTVEC(invltlb),
SDT_SYSIGT, SEL_KPL, 0); SDT_SYSIGT, SEL_KPL, 0);
}
setidt(IPI_INVLPG, pti ? IDTVEC(invlpg_pti) : IDTVEC(invlpg), setidt(IPI_INVLPG, pti ? IDTVEC(invlpg_pti) : IDTVEC(invlpg),
SDT_SYSIGT, SEL_KPL, 0); SDT_SYSIGT, SEL_KPL, 0);
setidt(IPI_INVLRNG, pti ? IDTVEC(invlrng_pti) : IDTVEC(invlrng), setidt(IPI_INVLRNG, pti ? IDTVEC(invlrng_pti) : IDTVEC(invlrng),
SDT_SYSIGT, SEL_KPL, 0); SDT_SYSIGT, SEL_KPL, 0);
}
/* Install an inter-CPU IPI for cache invalidation. */ /* Install an inter-CPU IPI for cache invalidation. */
setidt(IPI_INVLCACHE, pti ? IDTVEC(invlcache_pti) : IDTVEC(invlcache), setidt(IPI_INVLCACHE, pti ? IDTVEC(invlcache_pti) : IDTVEC(invlcache),
SDT_SYSIGT, SEL_KPL, 0); SDT_SYSIGT, SEL_KPL, 0);
/* Install an inter-CPU IPI for all-CPU rendezvous */ /* Install an inter-CPU IPI for all-CPU rendezvous */
setidt(IPI_RENDEZVOUS, pti ? IDTVEC(rendezvous_pti) : setidt(IPI_RENDEZVOUS, pti ? IDTVEC(rendezvous_pti) :
IDTVEC(rendezvous), SDT_SYSIGT, SEL_KPL, 0); IDTVEC(rendezvous), SDT_SYSIGT, SEL_KPL, 0);
▲ Show 20 Lines • Show All 277 Lines • ▼ Show 20 Lines#endif /* COUNT_IPIS */
d.pad = 0; d.pad = 0;
d.addr = 0; d.addr = 0;
invpcid(&d, smp_tlb_pmap == kernel_pmap ? INVPCID_CTXGLOB : invpcid(&d, smp_tlb_pmap == kernel_pmap ? INVPCID_CTXGLOB :
INVPCID_CTX); INVPCID_CTX);
PCPU_SET(smp_tlb_done, generation); PCPU_SET(smp_tlb_done, generation);
} }
void void
invltlb_invpcid_pti_handler(void)
{
struct invpcid_descr d;
uint32_t generation;
#ifdef COUNT_XINVLTLB_HITS
xhits_gbl[PCPU_GET(cpuid)]++;
#endif /* COUNT_XINVLTLB_HITS */
#ifdef COUNT_IPIS
(*ipi_invltlb_counts[PCPU_GET(cpuid)])++;
#endif /* COUNT_IPIS */
generation = smp_tlb_generation;
d.pcid = smp_tlb_pmap->pm_pcids[PCPU_GET(cpuid)].pm_pcid;
d.pad = 0;
d.addr = 0;
invpcid(&d, smp_tlb_pmap == kernel_pmap ? INVPCID_CTXGLOB :
markjUnsubmitted
Done
Inline Actions

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);
}
markj: IMHO it would be clearer to write ``` if (smp_tlb_pmap == kernel_pmap) { d.pcid = 0…
INVPCID_CTX);
if (smp_tlb_pmap != kernel_pmap) {
d.pcid |= PMAP_PCID_USER_PT;
invpcid(&d, INVPCID_CTX);
}
PCPU_SET(smp_tlb_done, generation);
}
void
invltlb_pcid_handler(void) invltlb_pcid_handler(void)
{ {
uint64_t kcr3, ucr3;
uint32_t generation; uint32_t generation;
#ifdef COUNT_XINVLTLB_HITS #ifdef COUNT_XINVLTLB_HITS
xhits_gbl[PCPU_GET(cpuid)]++; xhits_gbl[PCPU_GET(cpuid)]++;
#endif /* COUNT_XINVLTLB_HITS */ #endif /* COUNT_XINVLTLB_HITS */
#ifdef COUNT_IPIS #ifdef COUNT_IPIS
(*ipi_invltlb_counts[PCPU_GET(cpuid)])++; (*ipi_invltlb_counts[PCPU_GET(cpuid)])++;
#endif /* COUNT_IPIS */ #endif /* COUNT_IPIS */
generation = smp_tlb_generation; /* Overlap with serialization */ generation = smp_tlb_generation; /* Overlap with serialization */
if (smp_tlb_pmap == kernel_pmap) { if (smp_tlb_pmap == kernel_pmap) {
invltlb_glob(); invltlb_glob();
} else { } else {
/* /*
* The current pmap might not be equal to * The current pmap might not be equal to
* smp_tlb_pmap. The clearing of the pm_gen in * smp_tlb_pmap. The clearing of the pm_gen in
* pmap_invalidate_all() takes care of TLB * pmap_invalidate_all() takes care of TLB
* invalidation when switching to the pmap on this * invalidation when switching to the pmap on this
markjUnsubmitted
Done
Inline Actions

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

markj: "achieve it" or "effect it" sound more natural than "reach it" to me.
alcUnsubmitted
Done
Inline Actions

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

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

alc: "... to clear kernel mappings from the TLB in the ..." Also, I'm confused. The "if"…
* CPU. * CPU.
alcUnsubmitted
Not Done
Inline Actions

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?

alc: I fear that no one (besides us) will understand this comment. Isn't the issue here that there…
kibAuthorUnsubmitted
Not Done
Inline Actions

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.

kib: No, the kernel mappings as well as user mappings are always flushed on context switch, even…
*/ */
if (PCPU_GET(curpmap) == smp_tlb_pmap) { if (PCPU_GET(curpmap) == smp_tlb_pmap) {
load_cr3(smp_tlb_pmap->pm_cr3 | kcr3 = smp_tlb_pmap->pm_cr3 |
smp_tlb_pmap->pm_pcids[PCPU_GET(cpuid)].pm_pcid); smp_tlb_pmap->pm_pcids[PCPU_GET(cpuid)].pm_pcid;
markjUnsubmitted
Done
Inline Actions

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

markj: Perhaps cache the cpuid here and in the other handlers, rather than fetching from pcpu area…
if (pti) {
ucr3 = smp_tlb_pmap->pm_ucr3 |
smp_tlb_pmap->pm_pcids[PCPU_GET(cpuid)].
pm_pcid;
markjUnsubmitted
Done
Inline Actions

Need the user bit here too.

markj: Need the user bit here too.
pmap_pti_pcid_invalidate(ucr3, kcr3);
} else
load_cr3(kcr3);
} }
} }
PCPU_SET(smp_tlb_done, generation);
}
void
invlpg_invpcid_pti_handler(void)
{
struct invpcid_descr d;
uint32_t generation;
#ifdef COUNT_XINVLTLB_HITS
xhits_pg[PCPU_GET(cpuid)]++;
#endif /* COUNT_XINVLTLB_HITS */
#ifdef COUNT_IPIS
(*ipi_invlpg_counts[PCPU_GET(cpuid)])++;
#endif /* COUNT_IPIS */
generation = smp_tlb_generation; /* Overlap with serialization */
invlpg(smp_tlb_addr1);
if (smp_tlb_pmap != kernel_pmap) {
d.pcid = smp_tlb_pmap->pm_pcids[PCPU_GET(cpuid)].pm_pcid |
PMAP_PCID_USER_PT;
d.pad = 0;
d.addr = smp_tlb_addr1;
invpcid(&d, INVPCID_ADDR);
}
PCPU_SET(smp_tlb_done, generation);
}
void
invlpg_pcid_pti_handler(void)
{
uint64_t kcr3, ucr3;
uint32_t generation;
#ifdef COUNT_XINVLTLB_HITS
xhits_pg[PCPU_GET(cpuid)]++;
#endif /* COUNT_XINVLTLB_HITS */
#ifdef COUNT_IPIS
(*ipi_invlpg_counts[PCPU_GET(cpuid)])++;
#endif /* COUNT_IPIS */
generation = smp_tlb_generation; /* Overlap with serialization */
invlpg(smp_tlb_addr1);
if (smp_tlb_pmap != kernel_pmap) {
kcr3 = smp_tlb_pmap->pm_cr3 | smp_tlb_pmap->pm_pcids[
PCPU_GET(cpuid)].pm_pcid | CR3_PCID_SAVE;
ucr3 = smp_tlb_pmap->pm_ucr3 | smp_tlb_pmap->pm_pcids[
PCPU_GET(cpuid)].pm_pcid | CR3_PCID_SAVE;
markjUnsubmitted
Done
Inline Actions

Seems we are missing the user PCID bit here?

markj: Seems we are missing the user PCID bit here?
pmap_pti_pcid_invlpg(ucr3, kcr3, smp_tlb_addr1);
}
PCPU_SET(smp_tlb_done, generation);
}
void
invlrng_invpcid_pti_handler(void)
{
struct invpcid_descr d;
vm_offset_t addr, addr2;
uint32_t generation;
#ifdef COUNT_XINVLTLB_HITS
xhits_rng[PCPU_GET(cpuid)]++;
#endif /* COUNT_XINVLTLB_HITS */
#ifdef COUNT_IPIS
(*ipi_invlrng_counts[PCPU_GET(cpuid)])++;
#endif /* COUNT_IPIS */
addr = smp_tlb_addr1;
addr2 = smp_tlb_addr2;
generation = smp_tlb_generation; /* Overlap with serialization */
do {
invlpg(addr);
addr += PAGE_SIZE;
} while (addr < addr2);
if (smp_tlb_pmap != kernel_pmap) {
d.pcid = smp_tlb_pmap->pm_pcids[PCPU_GET(cpuid)].pm_pcid |
PMAP_PCID_USER_PT;
d.pad = 0;
d.addr = smp_tlb_addr1;
do {
invpcid(&d, INVPCID_ADDR);
d.addr += PAGE_SIZE;
} while (d.addr < addr2);
}
markjUnsubmitted
Done
Inline Actions

Extra newline.

markj: Extra newline.
PCPU_SET(smp_tlb_done, generation);
}
void
invlrng_pcid_pti_handler(void)
{
vm_offset_t addr, addr2;
uint64_t kcr3, ucr3;
uint32_t generation;
#ifdef COUNT_XINVLTLB_HITS
xhits_rng[PCPU_GET(cpuid)]++;
#endif /* COUNT_XINVLTLB_HITS */
#ifdef COUNT_IPIS
(*ipi_invlrng_counts[PCPU_GET(cpuid)])++;
#endif /* COUNT_IPIS */
addr = smp_tlb_addr1;
addr2 = smp_tlb_addr2;
generation = smp_tlb_generation; /* Overlap with serialization */
do {
invlpg(addr);
addr += PAGE_SIZE;
} while (addr < addr2);
if (smp_tlb_pmap != kernel_pmap) {
kcr3 = smp_tlb_pmap->pm_cr3 | smp_tlb_pmap->pm_pcids[
PCPU_GET(cpuid)].pm_pcid | CR3_PCID_SAVE;
ucr3 = smp_tlb_pmap->pm_ucr3 | smp_tlb_pmap->pm_pcids[
PCPU_GET(cpuid)].pm_pcid | CR3_PCID_SAVE;
pmap_pti_pcid_invlrng(ucr3, kcr3, smp_tlb_addr1, addr2);
}
markjUnsubmitted
Not Done
Inline Actions

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.

markj: Logically it feels like pmap_pti_pcid_invl{pg,rng}() should be setting CR3_PCID_SAVE rather…
kibAuthorUnsubmitted
Not Done
Inline Actions

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: I did not the bit set in assembly to keep the assembler source as small as possible. If you…
markjUnsubmitted
Not Done
Inline Actions

I don't think it's important.

markj: I don't think it's important.
markjUnsubmitted
Done
Inline Actions

Missing user bit. :)

markj: Missing user bit. :)
PCPU_SET(smp_tlb_done, generation); PCPU_SET(smp_tlb_done, generation);
} }
markjUnsubmitted
Done
Inline Actions

Extra newline.

markj: Extra newline.