[RACL] Implement config parsing, racl_ctrl, and reggen checks #25664
Conversation
Razer6
requested review from
andreaskurth,
rswarbrick and
matutem
and removed request for
msfschaffner
Razer6
force-pushed
the
racl-package
branch
7 times, most recently
from
d3da1a9 to
fecd82b
Compare
Razer6
force-pushed
the
racl-package
branch
2 times, most recently
from
7d1df47 to
dfcda08
Compare
|
@andreaskurth Thanks for the initial feedback. I changed the docs or created issues for it. Can you take another look? |
Razer6
changed the title
Razer6
force-pushed
the
racl-package
branch
4 times, most recently
from
ade3d52 to
465f2af
Compare
util/raclgen/lib.py
Outdated
| # TODO further error handling | ||
| error = check_keys(racl_config, racl_required, [], [], 'RACL Config') | ||
| if error: | ||
| raise SystemExit(f"Error occured while validating {config_file}") |
There was a problem hiding this comment.
I'd strongly suggest putting a tiny bit more type checking in here. Something like extracting roles and policies as a list / dictionary, respectively (doing a basic type check) and maybe checking the item and key/value types for them respectively.
Extracting that properly will get rid of the dictionary lookups lower down in the function.
There was a problem hiding this comment.
hehe, I put a todo above exactly for that ;-)
There was a problem hiding this comment.
Created #25690 to track this.
| def generate_racl(topcfg: Dict[str, object], out_path: Path) -> None: | ||
| # Not all tops use RACL | ||
| if 'racl_config' not in topcfg: | ||
| return |
There was a problem hiding this comment.
Wouldn't it make sense to set topcfg['racl'] to None in this situation?
There was a problem hiding this comment.
The reason this was not added is to make it easier to detect if no RACL is used, i.e., down below: completecfg.get('racl', DEFAULT_RACL_CONFIG)
| @@ -115,7 +117,13 @@ | |||
| %> | |||
| `include "prim_assert.sv" | |||
|
|
|||
| module ${mod_name} ( | |||
| module ${mod_name}${' (' if not racl_support else ''} | |||
There was a problem hiding this comment.
To avoid the isolated # stuff, maybe it makes sense to use something like ${' # (' if racl_support else ' ('} ?
There was a problem hiding this comment.
The only reason for that was to keep the diff 0 when not enabling RACL on one IP. Otherwise we would generate a small whitespace diff for all regtops.
Signed-off-by: Robert Schilling <[email protected]>
Signed-off-by: Robert Schilling <[email protected]>
Signed-off-by: Robert Schilling <[email protected]>
| } | ||
|
|
||
|
|
||
| def parse_racl_config(config_path: str): |
There was a problem hiding this comment.
Coud you add a return type, probably ...str) -> Dict[str, object]: or some more specific type for the value in the Dict?
| except OSError: | ||
| raise SystemExit(sys.exc_info()[1]) | ||
|
|
||
| # TODO further error handling |
There was a problem hiding this comment.
Some hint about the missing checks?
|
|
||
| for racl_group, policies in racl_config['policies'].items(): | ||
| for policy in policies: | ||
| def compute_policy_value(permission: str): |
There was a problem hiding this comment.
Seems like the return value type delaration could be ... str) -> int:
This is the initial PR implementing RACL and consists of three commits:
Parse the RACL configuration file that is passed int the top-level HJSON file and render the
top_racl_pkg.svfrom it. If no RACL is used in the top, i.e., there is no RACL config passed, a defaulttop_racl_pkg.svis rendered to provide the minimal type information for all RACL usages in IPs. This package is a virtual core, and different Tops can generate their own version.Implement the ipgen'ed
racl_ctrlpolicy distributing IP. This IP renders all policies of a given policy group that is defined in theracl.hjsonfile. Currently, topgen only supports rendering the first RACL group'sracl_ctrlIP due to some tooling limitations. Note,racl_ctrl's registers themselves are not yet protected with RACL.Implement reggen support for RACL checks. On IP.hjson level, you can enable RACL on a per-bus granularity.