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

Seeding open-p4studio/docs #49

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Seeding open-p4studio/docs #49

wants to merge 5 commits into from

Conversation

vgurevich
Copy link
Contributor

Added the docs directory together with the first document, explaining how to compile P4 code.

Created the docs directory for miscellaneous documentation

Signed-off-by: Vladimir Gurevich <[email protected]>
Signed-off-by: Vladimir Gurevich <[email protected]>
Added the information about the additional `-DP4C=` flag.
Also, replaced typographic quotation marks with ASCII quotes for the easier copy-paste

Signed-off-by: Vladimir Gurevich <[email protected]>

Generally speaking, the compiler produces three main artifacts that are neccessary to run a P4 program:

1. The compiled program binary, called `tofino.bin` (or `tofino2.bin`) that needs to be loaded into the ASIC or the model so that they can execute the P4 code
Copy link
Contributor

Choose a reason for hiding this comment

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

When I built open-p4studio, and added a symbolic link named bf-p4c that pointed to p4c, I could run the bf-p4c command above, but it did not create any files with a suffix of .bin. It did produce a file with a suffix of .bfa, which I think contained Tofino assembly.

Should I have linked bf-p4c to something else instead of p4c? Or could open-p4studio have problems, if it is not creating .bin files without additional command line options?

Copy link
Contributor Author

@vgurevich vgurevich Jan 27, 2025

Choose a reason for hiding this comment

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

I can confirm seeing the same thing on my system.

I can see that the compiler driver has been significantly modified, compared to Intel's SDE and specifically a new parameter, --enable-bf-asm has been introduced. By default it is not set, which is why bfas is not invoked.

@fruffy -- any specific reasons for these changes? Please, note that if you do not run the assembler, you would not even know if the program fits into the number of stages available on the device.

Copy link
Contributor

Choose a reason for hiding this comment

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

bf-asm is not supplied with the compiler here: https://github.com/p4lang/p4c/tree/main/backends/tofino

Instead it is available in the studio here:
https://github.com/p4lang/open-p4studio/tree/main/pkgsrc/p4-compilers

The option is required to be able to build the compiler in both environments. I do not know why bf-asm is split this way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, @fruffy! I suspect that this might've happened because Intel didn't want to release the register map initially, although given this project that decision makes very little sense.

I'll file an issue, because as I explained, one must be running the assembler every time they compile the program even if they do not think they might need the binary.

One way to fix it is to utilize the internal check inside the compiler driver that distinguishes between the "DEVELOPER" and "INSTALLED" environments. Another is to have an option '--disable-bf-asm' instead.

The bottom line is that once open-p4studio is installed, the compiler driver must (by default) run the assembler afterwards.

Copy link
Contributor

Choose a reason for hiding this comment

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

We could merge this back into the p4c repository. That would simplify code a bunch and make the compiler more usable out of the box.

But I am currently working on this on my spare time, so not sure when I can get around to it.

Generally speaking, the compiler produces three main artifacts that are neccessary to run a P4 program:

1. The compiled program binary, called `tofino.bin` (or `tofino2.bin`) that needs to be loaded into the ASIC or the model so that they can execute the P4 code
2. The `bf-rt.json` file that describes all the objects, defined in the P4 program, such as tables and their key fields, actions and their action data fields, parser value sets and externs
Copy link
Contributor

Choose a reason for hiding this comment

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

The file I saw created was bfrt.json, not bf-rt.json

Copy link
Contributor

Choose a reason for hiding this comment

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

When I followed the instructions using cmake below, I did see a file named bf-rt.json created.

Copy link
Contributor Author

@vgurevich vgurevich Jan 27, 2025

Choose a reason for hiding this comment

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

This is funny! I haven't noticed that previously, most probably because as explained in this section that directory that is created when running the compiler directly is never used to run the program.

As I would always say in those cases "the great artists never repeat themselves" and thus P4 Studio code is full of inconsistencies. This one can probably fixed pretty easily if we decide to do that.

For the sake of the doc I can say "bf-rt.json (sometimes also named bfrt.json)". Or vice versa. Will that help?

Copy link
Contributor

Choose a reason for hiding this comment

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

Seems reasonable to me. I was just reading through your instructions and spot-checking a few things here and there to see if they matched open-p4studio behavior or not. Accurate documentation is usually nice to have.

vgurevich and others added 2 commits January 28, 2025 00:42
Addressed the feedback regarding `bfrt.json` filename.
Fixed a couple of typos.
Added a couple of clarifications

Signed-off-by: Vladimir Gurevich <[email protected]>
Signed-off-by: Andy Fingerhut <[email protected]>
Signed-off-by: fruffy <[email protected]>
Co-authored-by: Andy Fingerhut <[email protected]>
Co-authored-by: Fabian Ruffy <[email protected]>
@jafingerhut
Copy link
Contributor

Some merge seems to have gone strangely here? Also DCO check is failing.

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