Initialize the static kernel boot environment, which makes the riscv boot process honor the environment set via the kernel config file
Sponsored by: Axiado
Details
Details
- Reviewers
philip kp syrinx - Group Reviewers
riscv - Commits
- rS367896: riscv: always initialize the static kernel environment
Compile & boot a riscv system/kernel, with some read only tunables set in the kernel config file, e.g.
envvar "net.fibs=5" envvar "net.pf.source_nodes_hashsize=16384"
Verify the read only tunables' values have been correctly applied.
Diff Detail
Diff Detail
- Lint
Lint Passed - Unit
No Test Coverage - Build Status
Buildable 34698 Build 31761: arc lint + arc unit
Event Timeline
Comment Actions
This looks good -- and is known to work. The size of the environment should probably be a constant rather than a magic number though.
| sys/riscv/riscv/machdep.c | ||
|---|---|---|
| 133 | Can this be a defined constant rather than a magic number? | |
| sys/riscv/riscv/machdep.c | ||
|---|---|---|
| 133 | Would you consider a different name? This has nothing to do with boot1. | |
| 955 | If the only goal is to honor the environment variables from the config, then there is no need to provide a buffer at all; you can simply call init_static_kenv(NULL, 0) and it will perform the initialization. The buffer is useful only if we wanted to manually set some environment variables via calls to kern_setenv(). | |
| sys/riscv/riscv/machdep.c | ||
|---|---|---|
| 133 | arm calls it static_kenv (and also uses 4096 as its size). | |
| 955 | I believe we currently don't have a need for kern_setenv() during boot. I'd keep the buffer for two reasons:
| |
| sys/riscv/riscv/machdep.c | ||
|---|---|---|
| 955 | Sorry, I should have clarified: the buffer is only useful for calls to kern_setenv() that occur early in boot, such as in initriscv. At SI_SUB_KMEM + 1 we switch to a dynamic kenv, after which the two static environments (one from config and the one we would be creating here) are no longer used. This occurs before any driver code is loaded. You can see what I mean in init_dynamic_kenv. | |
| sys/riscv/riscv/machdep.c | ||
|---|---|---|
| 133 | static char static_kenv[PAGE_SIZE]; perhaps? | |
| 955 | Ideally, at some point we should support setting those values via entries in the device tree blob for example, or some u-boot environment area perhaps; for that we need the static buffer if I understand correctly I could remove the buffer and call init_static_kenv(NULL, 0) so we have at least the kernel envvar config option to modify the read-only tunables during kernel build without modifying the source code, and add those other options, incl. the buffer in a later patch... | |
| sys/riscv/riscv/machdep.c | ||
|---|---|---|
| 955 | Those features would be useful, and if you intend on adding them soon then it's up to you if you want to include the buffer in this patch or later. | |
Comment Actions
I've discovered this change should have gone in before rS363345, and is now required to avoid an early panic when trying to parse kenv variables from bootargs. Taking over the review so I can update with a rebased version.
Comment Actions
It does. I find this placement slightly more intuitive, but if you disagree then I can move it to that function.