Page MenuHomeFreeBSD

Only set up the interrupts that will actually be used in arm generic_timer.
ClosedPublic

Authored by ian on Apr 10 2019, 12:06 AM.

Details

Summary

The code previously set up interrupt handlers for all the interrupt resources available, including for timers that are not in use. That could lead to interrupt storms. For example, if boot firmware enabled the virtual timer but the kernel is using the physical timer, it could get flooded with interrupts on the virtual timer which it cannot shut off. By only setting up an interrupt handler for the hardware that will actually be used, any interrupts from other timer units will remain masked in the interrupt controller.

Diff Detail

Repository
rS 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

ian created this revision.Apr 10 2019, 12:06 AM
imp accepted this revision.Apr 10 2019, 3:35 AM
This revision is now accepted and ready to land.Apr 10 2019, 3:35 AM

Do we need something similar to intr_pic_init_secondary to also disable the timers on non-boot CPUs?

ian added a comment.Apr 10 2019, 2:21 PM

Do we need something similar to intr_pic_init_secondary to also disable the timers on non-boot CPUs?

That would be the other way to solve the same problem, and I spent some time looking into it, but all in all it would yield exactly the same net effect as this change. That is, I couldn't see any benefit to disabling the timer or masking its interrupt via the timer control register instead of at the PIC like this.

Part of what makes the other solution so unsatisfactory is how badly the event timer folks botched the naming of their new functions. There is a cpu_initclocks() to allow platforms to init their clocks, and all the existing functions are implemented by calling cpu_initclocks_bsp() and cpu_initclocks_ap(). Those are the botched names -- they begin with cpu_, and thus should be the per-platform init code -- cpu_initclocks_ap() should be our new function to init the generic timer on APs. But instead it's an MI init routine usurping a name in the MD namespace. Grrr. I got so depressed by how much work it would be to fix all that mess at this late date that I decided to just keep it simple.

This revision was automatically updated to reflect the committed changes.