feat: Configure dvsim and FuseSoC from a config file outside the repository (or the command line) - #250
Conversation
5481778 to
4d7bb0d
Compare
gautschimi
left a comment
There was a problem hiding this comment.
The way I understand the dvsim.hjson selection:
example setup:
folder_a/ (no dvsim.hjson)
└── folder_b/
├── dvsim.hjson
└── folder_c/ (no dvsim.hjson)
It depends which hjson is selected based on the invocation directory:
From folder_a → does not find anything, walks up towards the root and falls back to $XDG_CONFIG_HOME/lowRISC/dvsim/config.hjson
From folder_b → uses the dvsim.hjson from folder_b
From folder_c → no file here, walks up → finds folder_b's dvsim.hjson
Is this the indented behavior? I think it would be better if we walk down into a directory.
|
Thanks @gautschimi, yes this is by design. It enables the following use case: You can then invoke |
Ok but this would also be possible if you walked into the directory. In your case invoking dvsim from |
I think that would unfortunately require bigger/breaking changes to DVSim, because the relative paths in the DVSim config files (e.g., in the OT repo) are relative to the directory in which you execute DVSim. You can reproduce that by trying to run I suggest this is something we consider for a next major version. |
|
@gautschimi As we just discussed offline, it would be even better if |
|
Could we also add a log message that shows which e.g. [I 260825 09:32:47 run:990] [proj_root]: /home/mgautschi/opentitan
[I 260825 09:32:47 run:990] [dvsim_config]: /home/mgautschi/dvsim.hjson
[I 260825 09:32:47 flow:229] [scratch_path]: [chip] [/home/mgautschi/opentitan/scratch/rram_macro_virtual/chip_earlgrey_asic-sim-vcs]
[I 260825 09:35:41 report:489] [results]: [chip] |
|
@gautschimi I pushed a new commit implementing your two feature requests (being able to set |
There was a problem hiding this comment.
This LGTM. It's slightly unfortunate we have to have fusesoc-specific CLI args, given that in theory this should be a run tool that is generic over the underlying utilities it can call, but I think it's acceptable to incorporate this change now and work on migrating back towards being fully generic as a future task.
I have two suggestions, neither of which should block merging as-is.
-
Along with @gautschimi's suggestion of logging when we have resolved options from a config file, it would be good to log at the INFO level when we are substituting values over the top of in-tree values. E.g.
[I 260825 14:03:09 fusesoc:275] FuseSoC --mapping override in uart_lint build_opts [/proj/hw/ip/uart/dv/uart_sim_cfg.hjson]: lowrisc:prim_generic:all:0.1 -> lowrisc:prim_my_tech:all:0.1One way to view the external config file is as a source of build impurity, which is entirely the point of this change, but can also contribute to making it harder to understand what configuration changes have been applied. In my memory, commands added to
.bazelrchas been the source of confusion in the past, forgetting they had been added for a previous debugging journey. Printing every time an override is applied makes it more difficult to forget this mechanism is being exercised. -
IMO the user default config file in XDG_CONFIG_HOME should also be named
dvsim.hjsonfor consistency with the out-of-tree file. A minor subjective nit.
In testing I made some local changes working towards 1) so I'll submit that for a future patch release.
|
@hcallahan-lowrisc, thanks for your feeback! I'll implement it. Just to ensure I understand you correctly: Would you be OK with |
Great. Yes, that is fine, or |
Flow configs describe a flow; there has been no way to configure the tool itself. Settings that belong to a workspace rather than to a flow had to be repeated on every command line. This commit adds `dvsim.config`, which finds and parses an hjson configuration file: an explicit path if given, else the nearest `dvsim.hjson` walking up from the working directory, else `$XDG_CONFIG_HOME/lowRISC/dvsim/config.hjson`. Walking upwards matters because dvsim is normally invoked from inside the project it builds, while a file describing a workspace sits alongside that project. Each feature owns a section of the file. Unknown keys within a section are rejected, so that a typo fails at the point it is made rather than silently doing nothing. No feature reads a section yet. Signed-off-by: Andreas Kurth <adk@lowrisc.org>
Flow configs hardcode the FuseSoC arguments they pass, including the `--mapping` that selects which technology library a design is built against, so building an existing config tree against a different library meant editing those configs. The hjson `overrides:` key cannot do it either: a primary config loads its children before processing its own overrides, so an override written in a wrapper config never reaches them. This commit adds `--fusesoc-mapping [OLD=]NEW` and `--fusesoc-extra-cores-root PATH`, both repeatable, together with the `fusesoc` section of the configuration file and the `--dvsim-config` option that selects the file. Command-line values are appended to those from the file. The rewrite runs at the end of `FlowCfg._expand()`. By then the option lists hold literal arguments rather than wildcards, and it still precedes the `_create_objects()` call that subclasses make from their own `_expand()`, whose objects copy those lists. Because every child of a primary config is constructed with the same arguments object, the rewrite reaches all of them. Only lists whose command is FuseSoC are touched. `--cores-root` is a global option and is inserted before the `run` subcommand; `--mapping` belongs to `run` and is inserted after it. Repeated mappings are dropped, because configs build these lists by appending and FuseSoC rejects the same mapping given twice. Every substitution is logged at INFO, naming the config whose arguments were changed. These options come from outside the project, so an unlogged one would be a change to the build that leaves no trace in the tree. Signed-off-by: Andreas Kurth <adk@lowrisc.org>
Selecting a technology library from a config file above the repository still required running dvsim from inside that repository, because proj_root was only ever discovered from the working directory or given on the command line. This commit lets the config file set `proj_root`, so that dvsim can be invoked from the workspace that holds the config file rather than from the project it builds. A relative `--proj-root` is now resolved against the working directory, and a relative `proj_root` in the config file against the directory holding that file; both may be absolute. The discovery order for the config file itself is unchanged. Resolving these to absolute paths matters because the value is handed on to flows, and from there to tools such as FuseSoC, which run with a different working directory. The branch name is now read from the git repository at proj_root rather than from the working directory, which has none to read when dvsim is invoked from outside the project. The config file is loaded once, in main(), rather than separately by each feature that reads a section of it, and its path is logged alongside proj_root. Signed-off-by: Andreas Kurth <adk@lowrisc.org>
e75505f to
4f2ec6b
Compare
|
@hcallahan-lowrisc updated, PTAL |
hcallahan-lowrisc
left a comment
There was a problem hiding this comment.
Looks great, just ran the tests and I can see the logs being emitted. Thanks!
Motivation
This enables DVSim users to write configuration files outside the tree of the repository using DVSim (e.g., OpenTitan), in which they can define FuseSoC mappings, additional 'cores' search paths, and the project root to operate on. Example
dvsim.hjson:The reason this cannot be done with the flow configs today is that they hardcode the FuseSoC arguments they pass, including the
--mappingthat selects which technology library a design is built against. Building an existing config tree against a different library therefore meant editing those configs. The hjsonoverrides:key cannot do it either: a primary config loads its children before processing its own overrides, so an override written in a wrapper config never reaches them.Setting
proj_rootin the same file completes the picture: with it,dvsimcan be invoked from the workspace that holds the configuration, rather than only from inside the repository it builds.Usage
With this PR, one can place a
dvsim.hjsonconfig file like the example above in a local workspace directory outside/above the repository using DVSim (e.g., OpenTitan), and then simply runeither in the repository or in the workspace directory above it, to simulate against
prim_mytechinstead ofprim_generic.The config file is located as follows:
--dvsim-config FILE, if given;dvsim.hjson, walking up from the working directory;$XDG_CONFIG_HOME/lowRISC/dvsim/dvsim.hjson.Walking upwards is what makes the workspace layout above work:
dvsimis normally invoked from inside the repository it builds, while a file describing the workspace sits alongside that repository rather than inside it.Paths are resolved so that both invocation styles mean the same thing. A relative
proj_rootorextra_cores_rootin the config file is relative to the directory holding that file, while a relative--proj-rooton the command line is relative to the working directory. Either may be given as an absolute path.proj_rootis taken from--proj-rootif present, otherwise from the config file, otherwise by searching for a git repository containing the working directory, as before.The path of the config file in use is logged alongside
proj_root, so it is clear which one was picked up:The FuseSoC settings are also available on the command line, which is useful for one-off runs:
Both options are repeatable, and both apply to every config in a run, including all the blocks of a primary config. Values from the file are applied first, and command-line values are appended to them.
Implementation
Three commits, all
feat:feat: add a configuration file for dvsim itselfaddsdvsim.config, which locates and parses the file. Each feature owns a section of it, so the module itself knows nothing about FuseSoC. Unknown keys within a section are rejected, so a typo fails at the point it is made rather than silently doing nothing. Relative paths are resolved against the config file's own directory, which is what lets a checked-in file stay valid whereverdvsimis invoked from.feat: select the FuseSoC mapping and cores-root from dvsimaddsdvsim.fusesoc, the two--fusesoc-*options and the--dvsim-configoption that selects the file. Three points are worth calling out for review:FlowCfg._expand(). By then the option lists hold literal arguments rather than wildcards, so the--mappingand--cores-roottokens are visible whichever config key they came from. It also has to happen inside_expand()rather than after it, because subclasses call_create_objects()from their own_expand()and the build modes created there take a copy of these lists. Rewriting afterwards updates the config attribute but has no effect on the command that actually runs.overrides:key. Every child of a primary config is constructed with the sameargsobject, so an args-driven rewrite reaches all of them, which is exactly whatoverrides:cannot do.--cores-rootis a FuseSoC global option and is inserted before therunsubcommand, while--mappingbelongs torunand is inserted after it. A mapping is given as[OLD=]NEW: theOLD=form replaces an existing--mapping=OLD, which is needed because configs that already pin a library would otherwise end up with two mappings covering the same source, which FuseSoC rejects. Repeated mappings are dropped for the same reason: configs build these lists by appending, so the same mapping can legitimately appear twice before rewriting.feat: set proj_root from the dvsim config fileadds theproj_rootkey and the resolution rules described under Usage, so thatdvsimcan be invoked from outside the repository. Two supporting changes come with it:proj_rootrather than from the working directory, which has no repository to read whendvsimis invoked from outside the project. Previously that printedfatal: not a git repositoryand fell back to a branch ofdefault, silently dropping the branch from every scratch path.main(), rather than separately by each feature that reads a section of it. That is what makes it possible to log its path exactly once, and it is also where unknown top-level keys are rejected.Resolving these paths to absolute ones is not cosmetic: the value is handed on to flows, and from there to tools such as FuseSoC, which run with a different working directory. A relative
--proj-rootpreviously reached FuseSoC unresolved and producedFailed to register libraryfollowed by an unresolvable core.Testing
tests/test_config.pycovers discovery order, the upward walk, relative-path resolution for bothproj_rootand section values, the rejection of malformed input, and the rejection of unknown top-level keys.tests/test_fusesoc.pycovers mapping parsing, replacement, appending, argument placement, de-duplication, inertness for non-FuseSoC commands, config/CLI precedence, and the_expand()ordering described above. 392 tests pass, andruff format --check,ruff check --config ruff-ci.tomland the license header check are clean.Beyond the unit tests, this was exercised end to end against OpenTitan, in all three invocation styles: from inside the repository, from the workspace directory above it with
proj_roottaken from the config file, and with a relative--proj-rootoverriding the config file. A full 41-job block-level lint regression and a chip-level simulation build, both against an out-of-tree technology library, produced results identical to the in-tree baseline.Checklist
git commit -s), indicating acceptance of the CLA<type>[(<scope>)][!]: <description>)!or aBREAKING CHANGE:footer