Microchip icicle kit platform support #165
Conversation
gngram
commented
Jun 8, 2023
gngram
commented
- HardenOS1.0
- Address sanitizer disabled
- By default docker is disabled
|
Thanks a lot for your PR. We're in the process of enabling out of tree ghaf-configurations - which does not mean we could not merge this as RISC-V reference - so I'll share you the a recent outline for the ghaf structure development that has impact to the review of this PR. Please see the outline of coming ghaf changes below: Given above, I'd like you to clarify if you want to introduce this target as new reference and maintain it. |
docs/src/ref_impl/build_and_run.md
Outdated
| In the case of Icicle kit, Ghaf deployment contains of creating a SD Image which includes UBoot and Linux kernel from Microchip and Ghaf based NixOS rootfs: | ||
|
|
||
| 1. To build Ghaf SD image: | ||
| Run the `nix build .#packages.riscv64-linux.mpfs-icicle-kit-release` command. |
There was a problem hiding this comment.
Should we also instruct here to use the -debug target? Considering other reference targets, -release is of very limited usefulness. Also, all the possible debug channels (ssh|serial|..) should go to -debug-targets.
There was a problem hiding this comment.
Added instructions to build image for debug target too.
There was a problem hiding this comment.
This goes against ghaf reference configuration defined packages on host - that will be more minimal in the next phases. See https://tiiuae.github.io/ghaf/features/features.html#next-steps
As these are not essential to the configuration I'd propose two things:
- please refactor this to a development module
- make them user (
ghaf) packages or packages for user passed passed as argument to the module. AssystemPackagesthey are made available to all users (incl.root) which should be avoided.
There was a problem hiding this comment.
Tried to use development modules, for package management need to discuss.
There was a problem hiding this comment.
@brianmcgillion and @mikatammi are in the process of refactoring the ghaf modules behind external API that is configurable instead of static as it has been. They merged a few modules just yesterday. See f9e49ec#diff-802221ea9e0a54520f90631da97263b93204b5dec385457827d48e34b001286bR16 for example.
| /* Networking */ | ||
| /* ========== */ | ||
|
|
||
| networking.hostName = "ghaf-host"; |
There was a problem hiding this comment.
This should be used from common definitions for reference configuration - see
Line 19 in 44a8660
Hardware specifics should not override / conflict / duplicate with the common
networking.hostName - this will keep a ghaf system on different hardware more familiar for developers/users.
There was a problem hiding this comment.
Created a new variant of networking.nix for such platforms which doesn't use vm.
There was a problem hiding this comment.
Thanks, this should be tested by another person. We can come back to it when we've worked with the other structural changes I requested.
There was a problem hiding this comment.
OK, I will push some commits with the structural changes.
There was a problem hiding this comment.
Thanks, please squash (or --amend) the future commits in the branch into one and force push to your PR branch.
Please also note that there was automated nix fmt-check failing and should be fixed.
There was a problem hiding this comment.
And commits must be signed - see https://docs.github.com/en/authentication/managing-commit-signature-verification/about-commit-signature-verification
in addition to DCO Signed-off-by:
Branch protection will block unsigned commits from getting merged.
| @@ -0,0 +1,16 @@ | |||
| {pkgs, ... } : | |||
| { | |||
| environment.systemPackages = with pkgs; [ | |||
There was a problem hiding this comment.
Most of these come from stdenv and can be used in callPackage with whatever nix declared development packages. We should not put them to host and to systemPackages - who ever needs these can use specific nix-shell -p <package>or nix develop with flakes. Leaving them out will also reduce the image size and decrease build time.
+ HardenOS1.0 + Address sanitizer disabled + By default docker is disabled Signed-off-by: Ganga Ram <[email protected]>
