lowRISC / opentitan

OpenTitan: Open source silicon root of trust
https://www.opentitan.org
Apache License 2.0
2.44k stars 730 forks source link

[tools/fusesoc] Realign with upstream FuseSoC #20379

Open olofk opened 8 months ago

olofk commented 8 months ago

Description

I would like to start the discussion about getting OpenTitan to use upstream FuseSoC. There are several users who gets confused by the OpenTitan fork and I believe some of the new features of FuseSoC would benefit OpenTitan as well. It's been a while since I last looked at this, but would it be possible to list the specific features currently used by OpenTitan to see if we can start this process?

jwnrt commented 6 months ago

I'm not an expert, but I think it's mainly (only?) the ability to depend on core files produced by generators. We use this for primitives generated by primgen.

It seems we tried to upstream something in the past, but wanted more design on it: https://github.com/olofk/fusesoc/pull/391

olofk commented 4 months ago

I would consider myself an expert, and you are right. The dynamic dependency in the OpenTitan fork is a bit problematic. There are different ways to solve this, but @rswarbrick and I discussed this offline a while ago and concluded that the best way forward would be to just add explicit dependencies in the affected prim packages and avoid a lot of complexity that way.

The other, smaller issue is some syntax fixes that have been fixed with https://github.com/lowRISC/opentitan/pull/21898

a-will commented 4 months ago

This is just a random aside, but unless I'm missing developments, support for generated cores also isn't complete in the OpenTitan fork. The whole tree added by the generated core's own dependencies isn't added to the requirements, so we have a very awkward graph, where some catch-all prim core tries to do the job of adding dependencies that might be needed by a dependency of a generated core.

Generating cores and inserting their subtrees to the main tree are some fundamental features for OpenTitan's use case. fusesoc's build graph management is specialized and not as mature as bazel's, and it'd be nice if we could get a better integration between them. I'm not sure what form that would take, but it'd be nice to have one source of truth / not have to manually write dependencies in two different places.

While I'm on the subject of bazel interaction, we'd also need to ensure newer fusesoc versions can operate such that they don't write directories outside what is provided in arguments (or under the current working directory). Our builds are always sandboxed for reproducibility, and more recent fusesoc versions are writing to $XDG_CACHE_HOME, which is forbidden. Can that all be removed with configuration? (With the library_root and cache_root keys, it seems like so, but golly does the config file format need documentation, hehe.)

olofk commented 4 months ago

This is just a random aside, but unless I'm missing developments, support for generated cores also isn't complete in the OpenTitan fork. The whole tree added by the generated core's own dependencies isn't added to the requirements, so we have a very awkward graph, where some catch-all prim core tries to do the job of adding dependencies that might be needed by a dependency of a generated core.

This is still the case upstream. I actually stumbled upon it recently when I was making a graphviz generator for the dependency tree. I believe it should be quite easy to fix though.

Generating cores and inserting their subtrees to the main tree are some fundamental features for OpenTitan's use case. fusesoc's build graph management is specialized and not as mature as bazel's, and it'd be nice if we could get a better integration between them. I'm not sure what form that would take, but it'd be nice to have one source of truth / not have to manually write dependencies in two different places.

I have two things to say here. First, the runtime dependency injection in the OpenTitan fork will not be part of upstream. I understand the appeal, but at some point, the added complexity far outweighs the effort to just create a meta core that contains the actual dependencies. I have an idea that I believe will cause the least disruption to OpenTitan but adds one more layer.

When it comes to Bazel integration in general though, I have been eyeing this for a while and been working towards it, primarily on the Edalize side. With the new Flow API in Edalize, the build graph generation is separated from the EDA project file and launcher creation. Today, a Makefile is generated to execute the flow graph, but the idea is to have this pluggable so that we could generate ninja files, github CI action jobs, SLURM jobs or Bazel build rules. In my view, that would be the best integration point because we can let Edalize handle all the nitty gritty EDA interfacing stuff and then hand over the build graph to the main Bazel flow. I wrote a bit about this a couple of years ago, but haven't moved forward with this yet. There are a couple of details that needs to be handled first, mostly related to env vars.

While I'm on the subject of bazel interaction, we'd also need to ensure newer fusesoc versions can operate such that they don't write directories outside what is provided in arguments (or under the current working directory). Our builds are always sandboxed for reproducibility, and more recent fusesoc versions are writing to $XDG_CACHE_HOME, which is forbidden. Can that all be removed with configuration? (With the library_root and cache_root keys, it seems like so, but golly does the config file format need documentation, hehe.)

The situation should be better. A former client had similar needs of hermetic builds and using generators, so there were some improvements in that area. You can e.g. run fusesoc config cache_root somedir to set the cache_root (or fusesoc config cache_root to check the current value). I wish I could also say that the documentation has been much improved, but I have been taught not to lie.

olofk commented 4 months ago

FYI, I just pushed a fix for the detached generated cores in the dependency tree https://github.com/olofk/fusesoc/commit/3f77403cb5edbba984e7a8482bfc5d3b1d35ab7f

a-will commented 1 month ago

I have two things to say here. First, the runtime dependency injection in the OpenTitan fork will not be part of upstream. I understand the appeal, but at some point, the added complexity far outweighs the effort to just create a meta core that contains the actual dependencies. I have an idea that I believe will cause the least disruption to OpenTitan but adds one more layer.

Are you talking about the itchy fileset_* flags that end up redefining the dependencies of a core? If so, then I'd agree that the virtual provides feature makes more sense here, and backend partners could simply supply the implementing cores for the list of required virtual ones.

Perhaps the slightly awkward bit is trying to reuse the parameters and target definitions for top-level cores that make use of "runtime" dependency injection. I think at this point, a backend partner would have to copy and modify the top-level core to bring in a different set of virtual providers (where the partner's closed-source cores are not present in the open-source repo).

This doesn't seem like a tall order if one is starting from scratch. Most likely, the partner needs a different flow for their targets anyway. However, it would be quite the change for one that wasn't set up to use virtual provides and instead, used the flags for runtime dependency injection everywhere.

a-will commented 1 month ago

Are you talking about the itchy fileset_* flags that end up redefining the dependencies of a core? If so, then I'd agree that the virtual provides feature makes more sense here, and backend partners could simply supply the implementing cores for the list of required virtual ones.

Hm... actually, the docs say that is supposed to be supported upstream.

@olofk What was the "runtime dependency injection" you found problematic?