Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add options for phys mem offset, kernel stack addr and size #44

Closed
wants to merge 1 commit into from

Conversation

64
Copy link
Contributor

@64 64 commented Aug 2, 2019

Allows you to specify these three options through Cargo.toml:

[package.metadata.bootimage]
physical-memory-offset = "0xffff800000000000"
kernel-stack-address = "0xffffff8000000000"
kernel-stack-size = 128

@phil-opp
Copy link
Member

phil-opp commented Aug 2, 2019

Thanks a lot for the pull request!

I know that I proposed flags like this in the gitter chat, but I'm no longer sure about this approach because it effectively locks bootimage to the specific bootloader crate. Perhaps it's better to let the bootloader crate parse their own options.

This could be implemented by passing a KERNEL_MANIFEST environment variable to the bootloader that points to the Cargo.toml of the kernel. The bootloader build script can then look for a package.metadata.bootloader configuration table in it. This would have the advantages that new options can be added to the bootloader without updating bootimage and that different bootloader crates can have different options. However it would be more work and slightly increase the compile times of the bootloader crate. What do you think?

@64
Copy link
Contributor Author

64 commented Aug 2, 2019

I know that I proposed flags like this in the gitter chat, but I'm no longer sure about this approach because it effectively locks bootimage to the specific bootloader crate.

Hm, I don’t exactly see how. If the bootloader doesn’t support these things then it will silently ignore the env vars.

This could be implemented by passing a KERNEL_MANIFEST environment variable to the bootloader that points to the Cargo.toml of the kernel.

Would this be done by bootimage?

Otherwise, it sounds like a good idea.

@phil-opp
Copy link
Member

phil-opp commented Aug 2, 2019

Hm, I don’t exactly see how. If the bootloader doesn’t support these things then it will silently ignore the env vars.

True, but an alternative bootloader implementation could not implement additional options without modifying bootimage, so that the two crates are still tied together. Also, silently ignoring the env variables means silently ignoring a configuration option, which doesn't seem like a good idea since it can lead to surprising behavior (e.g. the kernel stack remains at same place even though the user overrides the location). By letting the bootloader parse its own options, it can throw a proper error if something isn't supported.

This could be implemented by passing a KERNEL_MANIFEST environment variable to the bootloader that points to the Cargo.toml of the kernel.

Would this be done by bootimage?

Yes. Instead of passing each configuration option separately, bootimage would just tell the bootloader where the configuration can be found.

@64
Copy link
Contributor Author

64 commented Aug 2, 2019

Sounds good! Will cook up a PR at a later date.

@64 64 closed this Aug 2, 2019
@phil-opp
Copy link
Member

phil-opp commented Aug 2, 2019

Thanks a lot! Sorry for changing my opinion on this!

@64
Copy link
Contributor Author

64 commented Aug 2, 2019

No worries at all. I think this will need a bootloader breaking change, though, if we’re going to remove the env vars.

@phil-opp
Copy link
Member

phil-opp commented Aug 2, 2019

That's fine with me.

@phil-opp
Copy link
Member

phil-opp commented Aug 2, 2019

We could of course also keep the environment variables if you prefer.

@64 64 deleted the extra-config branch August 5, 2019 20:53
bors bot added a commit that referenced this pull request Aug 5, 2019
45: Pass location of kernel's Cargo.toml to bootloader r=phil-opp a=64

As discussed in #44.

Co-authored-by: Matt Taylor <mstaveleytaylor@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants