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

[integrated,sw] Switch logging over to dbg_print #23329

Merged
merged 4 commits into from
May 30, 2024

Conversation

sboeuf
Copy link

@sboeuf sboeuf commented May 27, 2024

Log traces with dbg_print instead of rom_print.
dbg_print comes from OpenTitan master branch and rom_print has been
deprecated.

@sboeuf sboeuf requested a review from a team as a code owner May 27, 2024 10:04
@sboeuf sboeuf requested review from jadephilipoom, sameo and loiclefort and removed request for a team and jadephilipoom May 27, 2024 10:04
Porting the dbg_print implementation from OpenTitan master branch.

Signed-off-by: Sebastien Boeuf <[email protected]>
sboeuf added 3 commits May 27, 2024 15:07
Operate the switch for generic parts (different from base_rom,
second_rom and rom_ext).

Log traces with dbg_print instead of rom_print.
dbg_print comes from OpenTitan master branch and rom_print has been
deprecated.

Signed-off-by: Sebastien Boeuf <[email protected]>
Operate the switch for ROM specific parts (base_rom, second_rom and
rom_ext).

Log traces with dbg_print instead of rom_print.
dbg_print comes from OpenTitan master branch and rom_print has been
deprecated.

Signed-off-by: Sebastien Boeuf <[email protected]>
It has now been replaced with dbg_print implementation, which is why we
can remove it.

Signed-off-by: Sebastien Boeuf <[email protected]>
Copy link
Contributor

@rswarbrick rswarbrick left a comment

Choose a reason for hiding this comment

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

This looks reasonable to me, but I think it would probably make sense for a SW person to take a look. I'm adding Chris Frantz as a reviewer, because I think he wrote dbg_print.c in the first place.

@rswarbrick rswarbrick requested a review from cfrantz May 29, 2024 13:39
@cfrantz
Copy link
Contributor

cfrantz commented May 29, 2024

I have no objection to you making this change, but please consider the following caveats:

  1. This printf implementation is not hardened. With the exception of %s, it will never follow a pointer, but there is more that can be done to harden the implementation. There will be a hardening exercise done on this implementation at some point in the future.
  2. The Earlgrey ROM is careful to exclude any of the functions from the dbg_print module. There is an exception for FPGA bitstreams (ie: printing ROM:%x), but because of (1), Earlgrey does not want this function in the ROM.
  3. dbg_printf is around 500 bytes. If you are size constrained and you don't need a general printing function, you're better off crafting your output functions from uart_write and/or uart_putchar.

@sameo
Copy link
Contributor

sameo commented May 30, 2024

I have no objection to you making this change, but please consider the following caveats:

1. This printf implementation is not hardened.  With the exception of `%s`, it will never follow a pointer, but there is more that can be done to harden the implementation.

Right, and I believe this is not a regression compared to the rom_print version.

There will be a hardening exercise done on this implementation at some point in the future.

Good, we will port that part as well.

3. `dbg_printf` is around 500 bytes.  If you are size constrained and you don't need a general printing function, you're better off crafting your output functions from `uart_write` and/or `uart_putchar`.

Thanks for the heads up. We are not that constrained. Yet...

@andreaskurth andreaskurth changed the title sw: Switch logging over to dbg_print [integrated,sw] Switch logging over to dbg_print May 30, 2024
Copy link
Contributor

@andreaskurth andreaskurth left a comment

Choose a reason for hiding this comment

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

Approving based on comments above. Thx!

@andreaskurth andreaskurth merged commit eb6a0f1 into lowRISC:integrated_dev May 30, 2024
20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants