When processor enters power-save state it releases resources shared with other
cpu threads which makes other cores working much faster.
This patch also implements saving and restoring registers that might get
corrupted in power-save state.
Details
Details
Diff Detail
Diff Detail
- Repository
- rS FreeBSD src repository - subversion
- Lint
Lint Skipped - Unit
Tests Skipped
Event Timeline
| sys/powerpc/aim/mp_cpudep.c | ||
|---|---|---|
| 97–101 | Can this OR'd expression be broken out to a #define somewhere? It's used in here and one other place, so if they should be kept in sync, it's easiest to make it a #define somewhere (maybe in spr.h) | |
| sys/powerpc/aim/trap_subr64.S | ||
| 321 | You don't need this as 'bne', since the 'beq' immediately preceded. 'bl' should suffice. | |
| 378 | Minor typo here. | |
| 997 | Is there any significance for this being initialized to 0xfeedface? If not, just make it 0. | |
| sys/powerpc/include/spr.h | ||
| 204–209 | I can't tell from Phabricator if Phabricator messed up the spacing here, or if it's in the diff, so please make sure the spacing matches the rest of the file here. | |
Comment Actions
Thanks for your response.
I've updated diff (checked spacing too) so everything should be OK now.
Comment Actions
Is cpu_idle() the right place for this? I'm not sure what the wake-up latency of this code is, but, if it involves restoring registers in software, it seems like it might be heavier-weight than intended for cpu_idle().
| sys/powerpc/aim/trap_subr64.S | ||
|---|---|---|
| 362 | Do we need to restore the SLB here? | |
| sys/powerpc/powernv/platform_powernv.c | ||
| 471 | This is racy: if the thread becomes runnable between here and enter_power_save(), the thread will not wake up. This check has to be done with interrupts off. | |
Comment Actions
Actually, this is the only way to release resources by idling CPUs within the same SMT group - Linux does the same.
Sleep mode used here has the shortest wakeup delay, so the only performance hit is from saving and restoring context. However, it's still less complex than the one in cpu_switch.
All these are because Power does not implement "wfi"-like instruction and waking up from sleep modes has to be handled by entering EXC_RST vector.
| sys/powerpc/powernv/platform_powernv.c | ||
|---|---|---|
| 471 | Fixed. Thanks. | |
Comment Actions
There is a yield instruction ("yield"), but it is a bit of a different thing. Earlier versions of POWER also do implement instructions that do this without going through reset (MSR[POW], notably). As a result, I have somewhat mixed feelings implementing this in the platform layer. Is there a reason to do it there rather than cpu.c, where the code for MSR[POW] etc. lives?
| sys/powerpc/aim/trap_subr64.S | ||
|---|---|---|
| 318 | We branch here in software from trap_subr64.S in certain circumstances with random SRR1, so that code also needs to be updated to make sure that it does not randomly have the bit set. | |
| 319 | Would be good to spell out what bit (the name) this is in a comment, rather than duplicating the numerical value in the code. | |
| 327 | This breaks the alignment of the cpu_reset_handler .llong to an 8-byte boundary (noted in the comment) a few lines below, which will break the kernel by emitting unpatchable relocations. Please fix. | |
| 997 | These embedded .llongs need to be aligned to 8-byte boundaries. There is no explicit check for that (a .p2align, for example), which needs to be fixed. | |
| sys/powerpc/include/spr.h | ||
| 210 | I have missed feelings about this aggregate definition. | |
| sys/powerpc/powernv/platform_powernv.c | ||
| 58 | This should definitely be in a header. | |
| sys/powerpc/aim/trap_subr64.S | ||
|---|---|---|
| 327 | I don't see where the problem is. On hardware everything works perfectly. cpu_reset_handler .llong address is multiple of 8. | |
| sys/powerpc/include/spr.h | ||
| 210 | IMO it is ok to have only one aggregate definition (if i have to change it i'll change in this one place). One can still use separate bit definition for eg. debugging purposes. | |
Comment Actions
Could you tell me more about scenario in which we branch to rstcode from software? I was looking for it but I found nothing.
BTW "yield", "mdoio" and "mdoom" instructions were never implemented on POWER8
Comment Actions
Line 99 of locore64.S.
BTW "yield", "mdoio" and "mdoom" instructions were never implemented on POWER8
OK. I have only the architecture manual, which defines them as non-optional. I guess they are just no-ops?
| sys/powerpc/aim/trap_subr64.S | ||
|---|---|---|
| 327 | It's the *second* instance below that is broken (also, why are there two?). The issue is that it can change the type of relocation emitted to a kind the early kernel cannot process, resulting in it acquiring a value of a zero. | |
| sys/powerpc/include/spr.h | ||
| 210 | I would just suggest a different name, basically. | |
Comment Actions
Thanks. I will clear whole register, right?
BTW "yield", "mdoio" and "mdoom" instructions were never implemented on POWER8
OK. I have only the architecture manual, which defines them as non-optional. I guess they are just no-ops?
Yes, they are no-ops. I don't know how it looks like in older versions of POWER architecture, but these instructions are removed from PowerISA 3.0. On the other side, they are doing no harm.
| sys/powerpc/include/spr.h | ||
|---|---|---|
| 210 | Hm, this value is set of exceptions which can wake up core, so maybe LPCR_PECE_WAKESET? I don't want it to be longer. | |
| sys/powerpc/aim/trap_subr64.S | ||
|---|---|---|
| 327 | Nathan, could you please describe what's wrong with this part and/or how should we handle it correctly? We're trying to think of a scenario, but for us it looks like all .llongs are aligned to 8b. I'm affraid we don't have a knowledge about various relocation options which a compiler can generate. | |
Comment Actions
Yes, that seems like the right approach.
BTW "yield", "mdoio" and "mdoom" instructions were never implemented on POWER8
OK. I have only the architecture manual, which defines them as non-optional. I guess they are just no-ops?
Yes, they are no-ops. I don't know how it looks like in older versions of POWER architecture, but these instructions are removed from PowerISA 3.0. On the other side, they are doing no harm.
OK.
| sys/powerpc/aim/trap_subr64.S | ||
|---|---|---|
| 327 | I seem to have miscounted the number of instructions between the two (I thought there were 5, but there are 6), so things are fully aligned. Apologies. | |
| sys/powerpc/powerpc/cpu.S | ||
|---|---|---|
| 1 ↗ | (On Diff #39528) | This file needs to be renamed, there already is a cpu.c, so builds would most certainly fail. |
| sys/powerpc/powerpc/cpu.S | ||
|---|---|---|
| 1 ↗ | (On Diff #39528) | This file needs to be included in locore64.S to be compiled into kernel. I've been building it many times per day. |
| sys/powerpc/powerpc/cpu.S | ||
|---|---|---|
| 1 ↗ | (On Diff #39528) | I will rename it to cpu_subr64.S |
Comment Actions
cpu_subr64.S isn't really a great name, since it doesn't convey a ton of information. How about cpu_idle64.S? In any case, is there a reason it needs to be #included into locore64.S rather than built as an independent file?
Comment Actions
I wanted this file to be cpu.c equivalent but in assembler, that's why it was named cpu.S
I'd rather keep it cpu_subr64.S because in the near future I want to put highly optimized memcpy into it.
Comment Actions
Added some checks, which detects situation when SRR1 is set to inappropriate value (eg. QEMU)