Page MenuHomeFreeBSD
Differential D2463

Add ACPI support for arm64
ClosedPublic
Actions

Authored by andrew on May 6 2015, 4:56 PM.
Tags
None
Referenced Files
Unknown Object (File)
Mon, Apr 14, 10:28 PM
Unknown Object (File)
Mon, Apr 14, 12:02 AM
Unknown Object (File)
Fri, Apr 11, 2:35 AM
Unknown Object (File)
Thu, Apr 10, 1:52 AM
Unknown Object (File)
Mar 16 2025, 7:17 AM
Unknown Object (File)
Feb 23 2025, 5:41 AM
Unknown Object (File)
Feb 21 2025, 9:38 PM
Unknown Object (File)
Feb 17 2025, 5:54 AM
View All Files
Subscribers
andrew
emaste
gnn
imp
jkim
zbb

Details

Reviewers
kib
emaste
jhb
imp
Commits
rS284273: Add basic support for ACPI. It splits out the nexus driver to two new
Summary

This adds basic support for ACPI. It splits out the nexus driver to
two new drivers, one for fdt, one for acpi. It then uses this to decide
if it will use fdt or acpi.

When using acpi a small driver is added to handle the MADT and GTDT. The
former is used to attach the gicv2 driver, the latter the generic timer.
This is needed because both of these need devices to attach to.

A few x86 specific places have been marked as such, and pci support is
missing.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Passed
Unit
No Test Coverage

Event Timeline

andrew updated this revision to Diff 5231.May 6 2015, 4:56 PM
andrew retitled this revision from to Add ACPI support for arm64.
andrew updated this object.
andrew edited the test plan for this revision. (Show Details)
andrew added reviewers: emaste, jhb, imp.
Herald added subscribers: imp, andrew, emaste. · View Herald TranscriptMay 6 2015, 4:56 PM
andrew added a reviewer: kib.May 6 2015, 4:57 PM
andrew added a comment.May 6 2015, 6:24 PM

I should also mention that we still need fdt for now to get the kernel console. The uart driver will need a bit of work to have it decide if it should look at acpi or fdt.

emaste added inline comments.May 6 2015, 8:41 PM
sys/arm64/acpica/acpi_arm64.c
2

Add a (C) and license block?

sys/arm64/arm64/gic.h
35–37

should we make this _MACHINE_GIC_H_?

sys/arm64/arm64/machdep.c
357–361

Unrelated (but reasonable) change, commit separately perhaps?

396

Do we have a constant for this somewhere or is it usually spelled this way in ACPI code?

sys/arm64/arm64/nexus.c
75–76

Do these belong in #ifdef DEV_ACPI guards?
I suspect it doesn't hurt either way, but would expect it to be consistent with #ifdef FDT above

emaste added a subscriber: jkim.May 6 2015, 8:43 PM
emaste added a subscriber: zbb.May 6 2015, 8:49 PM
jkim added a comment.May 6 2015, 8:51 PM

acpi_cpu.c and acpi_throttle.c have "#if defined(i386) || defined(arm64)" and these look very suspicious to me.

andrew updated this revision to Diff 5251.May 7 2015, 10:11 AM
andrew marked 2 inline comments as done.

Update based on feedback

andrew updated this revision to Diff 5252.May 7 2015, 10:14 AM

Missed machdep.c

sys/arm64/arm64/gic.h
35–37

I named it _ARM64_GIC_H_ because of its location. I didn't put it into sys/arm64/include as I'd like to move to a common gic driver under sys/dev.

sys/arm64/arm64/machdep.c
357–361

It from an earlier change that didn't get fully reverted.

396

This is the same on i386 and amd64, we could use UINT_MAX, or something based on it.

sys/arm64/arm64/nexus.c
75–76

Yes

emaste added inline comments.May 7 2015, 2:21 PM
sys/arm64/arm64/gic.h
35–36

ok

sys/arm64/arm64/machdep.c
397

Ok, it's not a magic number in the way that register offsets etc. would be and doesn't bother me too much if i386/amd64 are the same.

emaste edited edge metadata.May 11 2015, 1:00 PM

This seems reasonable to me with minor comments on two copyright blocks; ideally someone with more direct ACPI knowledge can give it a go-ahead.

sys/arm64/acpica/OsdEnvironment.c
2–3

this seems to be new/trivial content?

sys/arm64/arm64/gic.h
6–9

does this apply?

andrew added inline comments.May 11 2015, 1:07 PM
sys/arm64/acpica/OsdEnvironment.c
2–3

No, it's mostly copied from sys/x86/acpica/OsdEnvironment.c with the removal of acpi_get_root_from_memory, but git is claiming otherwise.

sys/arm64/arm64/gic.h
6–9

This was copied from sys/arm/arm/gic.c which has this.

andrew updated this revision to Diff 5324.May 11 2015, 1:23 PM
andrew edited edge metadata.
andrew removed rS FreeBSD src repository - subversion as the repository for this revision.

Update to the latest head, and remove the broken git copy logic

andrew mentioned this in D2377: Introduce ARM GICv3 support.May 11 2015, 1:28 PM
jhb added inline comments.May 12 2015, 12:29 PM
sys/arm64/acpica/acpi_arm64.c
81

Having the structures and methods be "acpi_arm64" and the device name "arm64_acpi" is a bit confusing, perhaps just pick one?

96

Looking at this driver, I feel like this should be ordered a bit differently. That is, I would move the logic to probe the tables into identify routines for the "gic" and timer drivers and let these drivers hang directly off of acpi0. This would remove the arm64_acpi device entirely. The "gic" identify routine would walk the MADT (an identify routine can create multiple devices), and the timer identify routine would walk the GTDT table. (Basically, acpi_arm64_add_gtdt would become the timer's identify routine, and acpi_arm64_add_madt would become "gic"'s identify routine.)

sys/arm64/arm64/nexus.c
328

You only need this method if you are using one of the bus_generic_rl_* methods in your driver. They are the only consumer of this. If you aren't using those, you can remove this.

emaste added a subscriber: gnn.May 14 2015, 12:05 AM
andrew updated this revision to Diff 5526.May 21 2015, 5:34 PM

Move the table parsing code to the identify functions

andrew updated this revision to Diff 5527.May 21 2015, 5:38 PM

Use an uploaded diff, git is having issues where it thinks files have been copied.

andrew added inline comments.May 21 2015, 5:49 PM
sys/arm64/arm64/nexus.c
328

Without this I get:

acpi0: <ARMLTD ARM-JUNO>
acpi0: Power Button (fixed)
acpi0: Sleep Button (fixed)
panic: vm_fault failed: ffffff8000039ae8
Uptime: 1s
Automatic reboot in 15 seconds - press a key on the console to abort

This is because acpi_sysres_alloc calls BUS_GET_RESOURCE_LIST on the nexus device.

jkim added a comment.May 21 2015, 7:35 PM

Please see r283261:

https://reviews.freebsd.org/rS283261

jkim added inline comments.May 21 2015, 8:08 PM
sys/conf/Makefile.arm64
37

This should be moved to acpica_machdep.h.

emaste added a comment.May 26 2015, 7:53 PM

@andrew is a new version of this coming soon?

jhb added inline comments.May 27 2015, 2:03 PM
sys/arm/arm/generic_timer.c
322

One trick you can use here is BUS_PROBE_NOWILDCARD. This will the only attach to devices that are explicitly added with your driver name. I think that means you can remove your other two checks and just have:

device_set_desc(...);
return (BUS_PROBE_NOWILDCARD);
sys/arm64/arm64/gic_acpi.c
149

Same BUS_PROBE_NO_WILDCARD suggestion here.

sys/arm64/arm64/nexus.c
328

Oh, ugh. Ok then. I don't have a good idea on how to do this better currently, but that's an ACPI issue, not an arm64 one.

andrew updated this revision to Diff 5916.Jun 3 2015, 4:31 PM

Rebase and update based on comments

jkim added inline comments.Jun 3 2015, 5:37 PM
sys/dev/acpica/acpi_cpu.c
1148

It is a gratuitous white space change. Actually, this macro is used without a leading blank line in couple of places in the same file. ;-)

emaste added inline comments.Jun 3 2015, 5:42 PM
sys/dev/acpica/acpi_cpu.c
1148

Per style(9) it's supposed to be there, but in this case I think it's just a result of adding and then removing a change here. Agreed that we should drop this change in the committed version.

jhb accepted this revision.Jun 4 2015, 4:41 PM
jhb edited edge metadata.

This looks fine as far as I can tell.

This revision is now accepted and ready to land.Jun 4 2015, 4:41 PM
Closed by commit rS284273: Add basic support for ACPI. It splits out the nexus driver to two new (authored by andrew). · Explain WhyJun 11 2015, 3:45 PM
This revision was automatically updated to reflect the committed changes.
andrew marked 3 inline comments as done.

Revision Contents
Changeset List

PathSize
sys/
arm/
arm/
71 lines
arm64/
acpica/
include/
68 lines
63 lines
acpica/
277 lines
217 lines
arm64/
include/
gic.h
pcpu.h
72 lines
63 lines
arm64/
58 lines
87 lines
2 lines
166 lines
conf/
1 line
include/
94 lines
iodev.h
pcpu.h
71 lines
1 line
3 lines
conf/
3 lines
7 lines
dev/
acpica/
Osd/
10 lines
2 lines
12 lines
2 lines
3 lines

Diff 5252

View Options

sys/arm/arm/generic_timer.c

Show First 20 Lines • Show All 403 Lines • ▼ Show 20 Linesstart_ap(int apic_id)
} }
return 0; /* return FAILURE */ return 0; /* return FAILURE */
} }
void void
invltlb_invpcid_handler(void) invltlb_invpcid_handler(void)
{ {
struct invpcid_descr d; struct invpcid_descr d;
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;
kibUnsubmitted
Not Done
Inline Actions

Did you looked at the code generated ? Compiler might spill 'generation' into memory and then reload it after invpcid, not keeping the intermediary in a register. I expect it more from clang and less from gcc, the later usually generates better code.

And such single-use variable deserves a comment, since my first action when looking at making any changes to such code, is to remove the once-used aliases.

It sounds as if you want to rewrite the handlers back into assembler.

kib: Did you looked at the code generated ? Compiler might spill 'generation' into memory and then…
rang_acm.orgAuthorUnsubmitted
Not Done
Inline Actions

Yes, I did, at least with clang; I didn’t try building the kernel with gcc, but as you say, gcc generally does a better job with x86 code. We’re working with very few values here so they fit easily into the amd64 registers.

I don’t think rewriting the handlers in assembly is worth it; the compiler does a decent job and in the absence of serializing instructions the processor can deal with almost any ordering of instructions reasonably well. However, for some of these handlers the actual invalidation cost is quite low (≈50 cycles) while the cost of serialization is high, and given the frequency of execution & simplicity of code, it seems worthwhile to me to do some of these micro-optimizations. Some of our workloads see 50K invalidations per second, which when multiplied by 31 cores is 1.5 million of these interrupts per second (ugh).

I can add comments describing the use, something like “issue memory read before serializing instructions to avoid stalling afterwards with a read/write dependency”. (The IRET still stalls for the write to complete, unfortunately, but that's about half as long as for a dependent read and write.)

rang_acm.org: Yes, I did, at least with clang; I didn’t try building the kernel with gcc, but as you say, gcc…
d.pcid = smp_tlb_pmap->pm_pcids[PCPU_GET(cpuid)].pm_pcid; d.pcid = smp_tlb_pmap->pm_pcids[PCPU_GET(cpuid)].pm_pcid;
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);
atomic_add_int(&smp_tlb_wait, 1); PCPU_SET(smp_tlb_done, generation);
} }
void void
invltlb_pcid_handler(void) invltlb_pcid_handler(void)
{ {
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;
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
* CPU. * CPU.
*/ */
if (PCPU_GET(curpmap) == smp_tlb_pmap) { if (PCPU_GET(curpmap) == smp_tlb_pmap) {
load_cr3(smp_tlb_pmap->pm_cr3 | load_cr3(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);
} }
} }
atomic_add_int(&smp_tlb_wait, 1); PCPU_SET(smp_tlb_done, generation);
} }
View Options

sys/arm64/acpica/OsdEnvironment.c

Show First 20 Lines • Show All 403 Lines • ▼ Show 20 Linesstart_ap(int apic_id)
} }
return 0; /* return FAILURE */ return 0; /* return FAILURE */
} }
void void
invltlb_invpcid_handler(void) invltlb_invpcid_handler(void)
{ {
struct invpcid_descr d; struct invpcid_descr d;
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;
kibUnsubmitted
Not Done
Inline Actions

Did you looked at the code generated ? Compiler might spill 'generation' into memory and then reload it after invpcid, not keeping the intermediary in a register. I expect it more from clang and less from gcc, the later usually generates better code.

And such single-use variable deserves a comment, since my first action when looking at making any changes to such code, is to remove the once-used aliases.

It sounds as if you want to rewrite the handlers back into assembler.

kib: Did you looked at the code generated ? Compiler might spill 'generation' into memory and then…
rang_acm.orgAuthorUnsubmitted
Not Done
Inline Actions

Yes, I did, at least with clang; I didn’t try building the kernel with gcc, but as you say, gcc generally does a better job with x86 code. We’re working with very few values here so they fit easily into the amd64 registers.

I don’t think rewriting the handlers in assembly is worth it; the compiler does a decent job and in the absence of serializing instructions the processor can deal with almost any ordering of instructions reasonably well. However, for some of these handlers the actual invalidation cost is quite low (≈50 cycles) while the cost of serialization is high, and given the frequency of execution & simplicity of code, it seems worthwhile to me to do some of these micro-optimizations. Some of our workloads see 50K invalidations per second, which when multiplied by 31 cores is 1.5 million of these interrupts per second (ugh).

I can add comments describing the use, something like “issue memory read before serializing instructions to avoid stalling afterwards with a read/write dependency”. (The IRET still stalls for the write to complete, unfortunately, but that's about half as long as for a dependent read and write.)

rang_acm.org: Yes, I did, at least with clang; I didn’t try building the kernel with gcc, but as you say, gcc…
d.pcid = smp_tlb_pmap->pm_pcids[PCPU_GET(cpuid)].pm_pcid; d.pcid = smp_tlb_pmap->pm_pcids[PCPU_GET(cpuid)].pm_pcid;
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);
atomic_add_int(&smp_tlb_wait, 1); PCPU_SET(smp_tlb_done, generation);
} }
void void
invltlb_pcid_handler(void) invltlb_pcid_handler(void)
{ {
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;
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
* CPU. * CPU.
*/ */
if (PCPU_GET(curpmap) == smp_tlb_pmap) { if (PCPU_GET(curpmap) == smp_tlb_pmap) {
load_cr3(smp_tlb_pmap->pm_cr3 | load_cr3(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);
} }
} }
atomic_add_int(&smp_tlb_wait, 1); PCPU_SET(smp_tlb_done, generation);
} }
View Options

sys/arm64/acpica/acpi_arm64.c

Show First 20 Lines • Show All 403 Lines • ▼ Show 20 Linesstart_ap(int apic_id)
} }
return 0; /* return FAILURE */ return 0; /* return FAILURE */
} }
void void
invltlb_invpcid_handler(void) invltlb_invpcid_handler(void)
{ {
struct invpcid_descr d; struct invpcid_descr d;
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;
kibUnsubmitted
Not Done
Inline Actions

Did you looked at the code generated ? Compiler might spill 'generation' into memory and then reload it after invpcid, not keeping the intermediary in a register. I expect it more from clang and less from gcc, the later usually generates better code.

And such single-use variable deserves a comment, since my first action when looking at making any changes to such code, is to remove the once-used aliases.

It sounds as if you want to rewrite the handlers back into assembler.

kib: Did you looked at the code generated ? Compiler might spill 'generation' into memory and then…
rang_acm.orgAuthorUnsubmitted
Not Done
Inline Actions

Yes, I did, at least with clang; I didn’t try building the kernel with gcc, but as you say, gcc generally does a better job with x86 code. We’re working with very few values here so they fit easily into the amd64 registers.

I don’t think rewriting the handlers in assembly is worth it; the compiler does a decent job and in the absence of serializing instructions the processor can deal with almost any ordering of instructions reasonably well. However, for some of these handlers the actual invalidation cost is quite low (≈50 cycles) while the cost of serialization is high, and given the frequency of execution & simplicity of code, it seems worthwhile to me to do some of these micro-optimizations. Some of our workloads see 50K invalidations per second, which when multiplied by 31 cores is 1.5 million of these interrupts per second (ugh).

I can add comments describing the use, something like “issue memory read before serializing instructions to avoid stalling afterwards with a read/write dependency”. (The IRET still stalls for the write to complete, unfortunately, but that's about half as long as for a dependent read and write.)

rang_acm.org: Yes, I did, at least with clang; I didn’t try building the kernel with gcc, but as you say, gcc…
d.pcid = smp_tlb_pmap->pm_pcids[PCPU_GET(cpuid)].pm_pcid; d.pcid = smp_tlb_pmap->pm_pcids[PCPU_GET(cpuid)].pm_pcid;
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);
atomic_add_int(&smp_tlb_wait, 1); PCPU_SET(smp_tlb_done, generation);
} }
void void
invltlb_pcid_handler(void) invltlb_pcid_handler(void)
{ {
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;
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
* CPU. * CPU.
*/ */
if (PCPU_GET(curpmap) == smp_tlb_pmap) { if (PCPU_GET(curpmap) == smp_tlb_pmap) {
load_cr3(smp_tlb_pmap->pm_cr3 | load_cr3(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);
} }
} }
atomic_add_int(&smp_tlb_wait, 1); PCPU_SET(smp_tlb_done, generation);
} }
View Options

sys/arm64/acpica/acpi_machdep.c

Show First 20 Lines • Show All 403 Lines • ▼ Show 20 Linesstart_ap(int apic_id)
} }
return 0; /* return FAILURE */ return 0; /* return FAILURE */
} }
void void
invltlb_invpcid_handler(void) invltlb_invpcid_handler(void)
{ {
struct invpcid_descr d; struct invpcid_descr d;
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;
kibUnsubmitted
Not Done
Inline Actions

Did you looked at the code generated ? Compiler might spill 'generation' into memory and then reload it after invpcid, not keeping the intermediary in a register. I expect it more from clang and less from gcc, the later usually generates better code.

And such single-use variable deserves a comment, since my first action when looking at making any changes to such code, is to remove the once-used aliases.

It sounds as if you want to rewrite the handlers back into assembler.

kib: Did you looked at the code generated ? Compiler might spill 'generation' into memory and then…
rang_acm.orgAuthorUnsubmitted
Not Done
Inline Actions

Yes, I did, at least with clang; I didn’t try building the kernel with gcc, but as you say, gcc generally does a better job with x86 code. We’re working with very few values here so they fit easily into the amd64 registers.

I don’t think rewriting the handlers in assembly is worth it; the compiler does a decent job and in the absence of serializing instructions the processor can deal with almost any ordering of instructions reasonably well. However, for some of these handlers the actual invalidation cost is quite low (≈50 cycles) while the cost of serialization is high, and given the frequency of execution & simplicity of code, it seems worthwhile to me to do some of these micro-optimizations. Some of our workloads see 50K invalidations per second, which when multiplied by 31 cores is 1.5 million of these interrupts per second (ugh).

I can add comments describing the use, something like “issue memory read before serializing instructions to avoid stalling afterwards with a read/write dependency”. (The IRET still stalls for the write to complete, unfortunately, but that's about half as long as for a dependent read and write.)

rang_acm.org: Yes, I did, at least with clang; I didn’t try building the kernel with gcc, but as you say, gcc…
d.pcid = smp_tlb_pmap->pm_pcids[PCPU_GET(cpuid)].pm_pcid; d.pcid = smp_tlb_pmap->pm_pcids[PCPU_GET(cpuid)].pm_pcid;
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);
atomic_add_int(&smp_tlb_wait, 1); PCPU_SET(smp_tlb_done, generation);
} }
void void
invltlb_pcid_handler(void) invltlb_pcid_handler(void)
{ {
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;
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
* CPU. * CPU.
*/ */
if (PCPU_GET(curpmap) == smp_tlb_pmap) { if (PCPU_GET(curpmap) == smp_tlb_pmap) {
load_cr3(smp_tlb_pmap->pm_cr3 | load_cr3(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);
} }
} }
atomic_add_int(&smp_tlb_wait, 1); PCPU_SET(smp_tlb_done, generation);
} }
View Options

sys/arm64/acpica/acpi_wakeup.c

Loading...
View Options

sys/arm64/arm64/gic.h

Loading...
View Options

sys/arm64/arm64/gic.c

Loading...
View Options

sys/arm64/arm64/gic_acpi.c

Loading...
View Options

sys/arm64/arm64/gic_fdt.c

Loading...
View Options

sys/arm64/arm64/machdep.c

Loading...
View Options

sys/arm64/arm64/nexus.c

Loading...
View Options

sys/arm64/conf/GENERIC

Loading...
View Options

sys/arm64/include/acpica_machdep.h

Loading...
View Options

sys/arm64/include/iodev.h

Loading...
View Options

sys/arm64/include/pci_cfgreg.h

Loading...
View Options

sys/arm64/include/pcpu.h

Loading...
View Options

sys/conf/Makefile.arm64

Loading...
View Options

sys/conf/files.arm64

Loading...
View Options

sys/dev/acpica/Osd/OsdHardware.c

Loading...
View Options

sys/dev/acpica/acpi.c

Loading...
View Options

sys/dev/acpica/acpi_cpu.c

Loading...
View Options

sys/dev/acpica/acpi_throttle.c

Loading...
View Options

sys/dev/acpica/acpi_timer.c

Loading...