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

[RACL] Add documentation and example config #26198

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

davidschrammel
Copy link
Contributor

@davidschrammel davidschrammel commented Feb 10, 2025

This commit adds initial documentation for RACL and describes the racl configuration files in the form of inline examples.
Furthermore, it generates a racl_configuration.md file which documents the current RACL configuration for each module instance and interface.

I don't have a strong opinion on how to format the RACL config information in the generated tables. We can still discuss and changes this!

@davidschrammel davidschrammel marked this pull request as ready for review February 10, 2025 18:02
@davidschrammel davidschrammel removed the request for review from msfschaffner February 10, 2025 18:02
@davidschrammel davidschrammel force-pushed the racl-doc-1 branch 2 times, most recently from 8aebeae to 873e7a6 Compare February 11, 2025 16:09
util/raclgen.py Outdated Show resolved Hide resolved
util/raclgen.py Outdated Show resolved Hide resolved
@davidschrammel davidschrammel force-pushed the racl-doc-1 branch 4 times, most recently from af586d9 to 9f6cebf Compare February 11, 2025 17:14
hw/ip_templates/racl_ctrl/README.md Outdated Show resolved Hide resolved
hw/ip_templates/racl_ctrl/README.md Outdated Show resolved Hide resolved
```hjson
racl_mappings: {
soc: 'top_darjeeling/data/racl/all_rd_wr_mapping.hjson'
sram: 'top_darjeeling/data/racl/all_rd_wr_mapping.hjson'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it make sense to give an example where the different interfaces have different mappings?

hw/ip_templates/racl_ctrl/README.md Show resolved Hide resolved
parser.add_argument('--racl-config',
'-r',
required=True,
required=not is_doc,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is rather clever! But the error messages might be a bit confusing. I'd probably make these optional for argparse, but then put an extra check just after the call to parse_args, which does something like this? (Completely untested!)

    if not args.is_doc:
        if (args.racl_config is None or
            args.ip is None or
            args.mapping is None):
          raise SystemExit("--racl-config, --ip and --mapping are all required for non-doc runs.")

Copy link
Contributor Author

@davidschrammel davidschrammel Feb 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rswarbrick I pushed some changes and came up with a different solution. Let me know what you think of it!

% ./util/raclgen.py
usage: 
raclgen.py --doc DOC
    Generates markdown documentation of the RACL configuration for a given top.

raclgen.py --racl-config RACL_CONFIG --ip IP --mapping MAPPING [--if-name IF_NAME]
    Generates the RACL policy selection vector for the given IP, RACL mapping, and interface name.
              
raclgen.py: error: the following arguments are required: --racl-config/-r, --ip/-i, --mapping/-m

Arguably, this error message is still not 100% correct but I would like to avoid the if (args.racl_config ... checks.

util/raclgen.py Show resolved Hide resolved
@davidschrammel davidschrammel added the CI:Rerun Rerun failed CI jobs label Feb 13, 2025
@github-actions github-actions bot removed the CI:Rerun Rerun failed CI jobs label Feb 13, 2025
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.

3 participants