Page MenuHomeFreeBSD
Differential D37981

ddb: have 'reset' command use normal reboot path
ClosedPublic
Actions

Authored by mhorne on Jan 8 2023, 6:18 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sun, Apr 20, 10:41 PM
Unknown Object (File)
Tue, Apr 8, 8:02 AM
Unknown Object (File)
Thu, Mar 27, 6:19 AM
Unknown Object (File)
Thu, Mar 27, 4:44 AM
Unknown Object (File)
Mar 10 2025, 4:39 AM
Unknown Object (File)
Jan 27 2025, 4:32 PM
Unknown Object (File)
Jan 24 2025, 3:57 PM
Unknown Object (File)
Jan 23 2025, 6:49 PM
View All 96 Files
Subscribers
imp

Details

Reviewers
jhb
markj
bz
imp
Commits
rGf0e49ab720d3: ddb: have 'reset' command use normal reboot path
rG2f12145a37be: kern_reboot: unconditionally call shutdown_reset()
rG5644850620ae: ddb: have 'reset' command use normal reboot path
rG627ca221c311: kern_reboot: unconditionally call shutdown_reset()
Summary

This conditionally gives all registered shutdown handlers a chance to
perform the reboot, with cpu_reset() being the fallback. The
debug.ddb.full_reboot sysctl/tunable restores the previous behaviour.

The motivation is that some platforms may not be able do anything
meaningful via cpu_reset(), due to a lack of standardized reset
mechanism and/or firmware shortcomings. However, they may have a
separate device driver attached that normally performs the reboot. Such
is the case for some versions of the Raspberry Pi.

Test Plan

We get some new console output with this change:

debug.kdb.enter: 0KDB: enter: sysctl debug.kdb.enter
[ thread pid 812 tid 100118 ]
Stopped at      kdb_trap+0x448: sd      zero,0(s1)
db> reset
Waiting (max 60 seconds) for system process `vnlru' to stop... done
Uptime: 1m51s

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 48998
Build 45887: arc lint + arc unit

Event Timeline

mhorne created this revision.Jan 8 2023, 6:18 PM
Herald added a subscriber: imp. · View Herald TranscriptJan 8 2023, 6:18 PM
mhorne requested review of this revision.Jan 8 2023, 6:18 PM
Harbormaster completed remote builds in B48991: Diff 114846.Jan 8 2023, 6:18 PM
mhorne added a comment.Jan 8 2023, 6:23 PM

Note that kern_reboot() is called by vpanic(), so it is safe for this context. I'm providing the tunable out of caution, in case there is some unforeseen side effect. If the change seems agreeable I'll document the tunable in ddb(4) as well.

mhorne edited the test plan for this revision. (Show Details)Jan 8 2023, 6:23 PM
imp accepted this revision.Jan 8 2023, 6:46 PM

Looks good to me. RB_NOSYNC is critical here...

This revision is now accepted and ready to land.Jan 8 2023, 6:46 PM
bz added a comment.Jan 8 2023, 11:24 PM

Can we grab a mtx, call eventhandlers, ... etc. when called from ddb (e.g. after a panic)? What about early boot panics - is this all setup then? Given kern_reboot will never return, we'll also never have the chance to try cpu_reset(). Can we, instead of making this a sysctl have it as a /f argument to db_reset? That way it'll be available if cpu_reset fails and one can just call reset /f and give it a try... but in either way, I'd rather not change the default until I am convinced that it won't make it worse in a lot of panic situations...

mhorne added a comment.Jan 9 2023, 3:03 PM
In D37981#862886, @bz wrote:

Can we grab a mtx, call eventhandlers, ... etc. when called from ddb (e.g. after a panic)? What about early boot panics - is this all setup then? Given kern_reboot will never return, we'll also never have the chance to try cpu_reset(). Can we, instead of making this a sysctl have it as a /f argument to db_reset? That way it'll be available if cpu_reset fails and one can just call reset /f and give it a try... but in either way, I'd rather not change the default until I am convinced that it won't make it worse in a lot of panic situations...

Short answer: yes, this function is totally safe for this context. It is already in use today at the very end of vpanic(). Lock acquisitions and attempts to sleep will check if we are in a panic/debugger context and return early if that's the case.

cpu_reset() will still be executed by the kern_reboot() path. It is called by the very last shutdown handler, shutdown_reset().

For early-boot, we do have a small problem. Before any shutdown handlers are registered during SI_SUB_INTRINSIC, calling kern_reboot() will just end up spinning in the infinite loop. This can be observed today if you type continue instead of reset at the prompt after an early panic. To handle this edge-case I think it would be best for kern_reboot() to call shutdown_reset() directly rather than relying on the event handler machinery. That way, the most basic reset is always attempted.

I agree that a command modifier is much more natural than the tunable here, but I still propose the (unifying) change to the default behaviour of reset. I will update the review shortly.

mhorne updated this revision to Diff 114868.Jan 9 2023, 4:27 PM

Switch from a tunable to a ddb command modifier 's'.

Ensure we always call shutdown_reset(), even before event handlers are registered. This bit can be committed separately but I'm including it in this review.

This revision now requires review to proceed.Jan 9 2023, 4:27 PM
Harbormaster completed remote builds in B48998: Diff 114868.Jan 9 2023, 4:28 PM
markj accepted this revision.Jan 10 2023, 9:08 AM
markj added inline comments.
sys/ddb/db_command.c
775

I'd elaborate a bit more on why this might be important, either here or in the man page.

This revision is now accepted and ready to land.Jan 10 2023, 9:08 AM
Closed by commit rG627ca221c311: kern_reboot: unconditionally call shutdown_reset() (authored by mhorne). · Explain WhyJan 23 2023, 7:11 PM
This revision was automatically updated to reflect the committed changes.
mhorne added a commit: rG627ca221c311: kern_reboot: unconditionally call shutdown_reset().
mhorne added a commit: rG5644850620ae: ddb: have 'reset' command use normal reboot path.
mhorne added a commit: rG2f12145a37be: kern_reboot: unconditionally call shutdown_reset().Feb 6 2023, 7:37 PM
mhorne added a commit: rGf0e49ab720d3: ddb: have 'reset' command use normal reboot path.

Revision Contents
Changeset List

PathSize
share/
man/
man4/
14 lines
sys/
ddb/
9 lines
kern/
8 lines

Diff 114868

View Options

share/man/man4/ddb.4

Show First 20 Lines • Show All 54 Lines • ▼ Show 20 Lines
/* The macro expands divider twice */ /* The macro expands divider twice */
#define EFX_DIV_ROUND_UP(_n, _d) (((_n) + (_d) - 1) / (_d)) #define EFX_DIV_ROUND_UP(_n, _d) (((_n) + (_d) - 1) / (_d))
/* Round value up to the nearest power of two. */ /* Round value up to the nearest power of two. */
#define EFX_P2ROUNDUP(_type, _value, _align) \ #define EFX_P2ROUNDUP(_type, _value, _align) \
(-(-(_type)(_value) & -(_type)(_align))) (-(-(_type)(_value) & -(_type)(_align)))
/* Align value down to the nearest power of two. */
#define EFX_P2ALIGN(_type, _value, _align) \
((_type)(_value) & -(_type)(_align))
/* Return codes */ /* Return codes */
typedef __success(return == 0) int efx_rc_t; typedef __success(return == 0) int efx_rc_t;
/* Chip families */ /* Chip families */
typedef enum efx_family_e { typedef enum efx_family_e {
▲ Show 20 Lines • Show All 3,244 Lines • Show Last 20 Lines
View Options

sys/ddb/db_command.c

Show First 20 Lines • Show All 54 Lines • ▼ Show 20 Lines
/* The macro expands divider twice */ /* The macro expands divider twice */
#define EFX_DIV_ROUND_UP(_n, _d) (((_n) + (_d) - 1) / (_d)) #define EFX_DIV_ROUND_UP(_n, _d) (((_n) + (_d) - 1) / (_d))
/* Round value up to the nearest power of two. */ /* Round value up to the nearest power of two. */
#define EFX_P2ROUNDUP(_type, _value, _align) \ #define EFX_P2ROUNDUP(_type, _value, _align) \
(-(-(_type)(_value) & -(_type)(_align))) (-(-(_type)(_value) & -(_type)(_align)))
/* Align value down to the nearest power of two. */
#define EFX_P2ALIGN(_type, _value, _align) \
((_type)(_value) & -(_type)(_align))
/* Return codes */ /* Return codes */
typedef __success(return == 0) int efx_rc_t; typedef __success(return == 0) int efx_rc_t;
/* Chip families */ /* Chip families */
typedef enum efx_family_e { typedef enum efx_family_e {
▲ Show 20 Lines • Show All 3,244 Lines • Show Last 20 Lines
View Options

sys/kern/kern_shutdown.c

Show First 20 Lines • Show All 54 Lines • ▼ Show 20 Lines
/* The macro expands divider twice */ /* The macro expands divider twice */
#define EFX_DIV_ROUND_UP(_n, _d) (((_n) + (_d) - 1) / (_d)) #define EFX_DIV_ROUND_UP(_n, _d) (((_n) + (_d) - 1) / (_d))
/* Round value up to the nearest power of two. */ /* Round value up to the nearest power of two. */
#define EFX_P2ROUNDUP(_type, _value, _align) \ #define EFX_P2ROUNDUP(_type, _value, _align) \
(-(-(_type)(_value) & -(_type)(_align))) (-(-(_type)(_value) & -(_type)(_align)))
/* Align value down to the nearest power of two. */
#define EFX_P2ALIGN(_type, _value, _align) \
((_type)(_value) & -(_type)(_align))
/* Return codes */ /* Return codes */
typedef __success(return == 0) int efx_rc_t; typedef __success(return == 0) int efx_rc_t;
/* Chip families */ /* Chip families */
typedef enum efx_family_e { typedef enum efx_family_e {
▲ Show 20 Lines • Show All 3,244 Lines • Show Last 20 Lines