Page MenuHomeFreeBSD

PowerNV: Put processor to power-save state in idle thread
ClosedPublic

Authored by pdk_semihalf.com on Feb 12 2018, 4:42 PM.
Referenced Files
Unknown Object (File)
Jan 23 2024, 5:29 AM
Unknown Object (File)
Dec 23 2023, 3:27 AM
Unknown Object (File)
Dec 22 2023, 11:05 PM
Unknown Object (File)
Dec 13 2023, 6:13 AM
Unknown Object (File)
Nov 20 2023, 6:50 AM
Unknown Object (File)
Nov 15 2023, 9:02 PM
Unknown Object (File)
Nov 9 2023, 8:41 PM
Unknown Object (File)
Nov 7 2023, 11:00 AM
Subscribers
None

Details

Summary

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.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

Do you have any objections to this patch?

sys/powerpc/aim/mp_cpudep.c
94–95 ↗(On Diff #39208)

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
312 ↗(On Diff #39208)

You don't need this as 'bne', since the 'beq' immediately preceded. 'bl' should suffice.

369 ↗(On Diff #39208)

Minor typo here.

988 ↗(On Diff #39208)

Is there any significance for this being initialized to 0xfeedface? If not, just make it 0.

sys/powerpc/include/spr.h
204–209 ↗(On Diff #39208)

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.

Thanks for your response.

I've updated diff (checked spacing too) so everything should be OK now.

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
353 ↗(On Diff #39344)

Do we need to restore the SLB here?

sys/powerpc/powernv/platform_powernv.c
468 ↗(On Diff #39344)

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.

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.

pdk_semihalf.com marked an inline comment as done.
pdk_semihalf.com added inline comments.
sys/powerpc/powernv/platform_powernv.c
468 ↗(On Diff #39344)

Fixed. Thanks.

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
309 ↗(On Diff #39379)

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.

310 ↗(On Diff #39379)

Would be good to spell out what bit (the name) this is in a comment, rather than duplicating the numerical value in the code.

318 ↗(On Diff #39379)

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.

988 ↗(On Diff #39379)

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 ↗(On Diff #39379)

I have missed feelings about this aggregate definition.

sys/powerpc/powernv/platform_powernv.c
59 ↗(On Diff #39379)

This should definitely be in a header.

sys/powerpc/aim/trap_subr64.S
318 ↗(On Diff #39379)

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 ↗(On Diff #39379)

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.

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

Could you tell me more about scenario in which we branch to rstcode from software? I was looking for it but I found nothing.

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
318 ↗(On Diff #39379)

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 ↗(On Diff #39379)

I would just suggest a different name, basically.

Could you tell me more about scenario in which we branch to rstcode from software? I was looking for it but I found nothing.

Line 99 of locore64.S.

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 ↗(On Diff #39379)

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
318 ↗(On Diff #39379)

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.

Could you tell me more about scenario in which we branch to rstcode from software? I was looking for it but I found nothing.

Line 99 of locore64.S.

Thanks. I will clear whole register, right?

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
318 ↗(On Diff #39379)

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.

Updated diff. I think it will be the last one

This revision is now accepted and ready to land.Feb 20 2018, 5:55 PM
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.
Of course I will rename this file. Give me only good name.

sys/powerpc/powerpc/cpu.S
1 ↗(On Diff #39528)

I will rename it to cpu_subr64.S

This revision now requires review to proceed.Feb 20 2018, 6:38 PM

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?

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.

Added some checks, which detects situation when SRR1 is set to inappropriate value (eg. QEMU)

This revision is now accepted and ready to land.Feb 21 2018, 12:59 PM
This revision now requires review to proceed.Feb 21 2018, 1:50 PM
This revision was not accepted when it landed; it landed in state Needs Review.Feb 21 2018, 2:28 PM
This revision was automatically updated to reflect the committed changes.