8
33
Fork
You've already forked blackmagic
23

riscv: add flashstub framework and helper functions (riscv32 only) #1764

Open
mean00 wants to merge 4 commits from mean00/riscv_flashstub into main
pull from: mean00/riscv_flashstub
merge into: blackmagic-debug:main
blackmagic-debug:main
blackmagic-debug:fix/pre-bmp-v3-cross-file-cleanup
blackmagic-debug:feature/bmda-remote-comms
blackmagic-debug:ALTracer/feature/aarch64-ident
blackmagic-debug:feature/better-meson-optimisation-handling
blackmagic-debug:feature/am335x-support
blackmagic-debug:feature/esp32-c3-support
blackmagic-debug:feature/cortex-ar-software-breakpoints
blackmagic-debug:feature/unit-testing
blackmagic-debug:feature/windows-usb-serial-interface-naming
blackmagic-debug:fix/bmp-external-spi
blackmagic-debug:ALTracer/feature/bluepillplus-platform
blackmagic-debug:ALTracer/feature/at32f43x-unrdp
blackmagic-debug:feature/const-correctness
blackmagic-debug:ALTracer/feature/fault_handlers
blackmagic-debug:ALTracer/feature/hazard3-ice40-support
blackmagic-debug:fix/ci-cleanup
blackmagic-debug:ALTracer/fix/gdb-10-12-thread
blackmagic-debug:ALTracer/feature/blackpill-f4-adc
blackmagic-debug:ALTracer/fix/cortex-desc-allocfail-report
blackmagic-debug:ALTracer/feature/spi-perf
blackmagic-debug:ALTracer/feature/calibrate_swd
blackmagic-debug:ALTracer/feature/blank-check
blackmagic-debug:feature/avr
blackmagic-debug:v2.0
blackmagic-debug:v1.9
blackmagic-debug:v1.10
blackmagic-debug:v1.8
mean00 commented 2024年02月08日 20:10:33 +01:00 (Migrated from github.com)
Copy link

Detailed description

Hi
This is a temptative patch to add some flashstub helper functions for the rv32 targets.
It tries to be very similar to the cortexm version. i.e.
Call riscv32_run_stub( target, loadaddr, param1, param2, param3, param4)
and the flashstub must end up with riscv_stub_exit(errorcode)

The main difference in behavior is the code saves and restore PC & MIE to avoid letting things dangling in ram.
Not completely sure this is needed, just to be on the safe side.

I've tested it with a CH32V307 board using the upcoming board & rvswd protocol support with a ~2x speedup.

As a sidenote and for the record, my original version was providing a temporary stack for the stub to avoid strict compiler rules about not using stack. Dont know if it is a valid idea. Nonetheless, this MR is assuming the stack is not used at all by the stub.

Thank you.

Your checklist for this pull request

  • I've read the Code of Conduct
  • I've read the guidelines for contributing to this repository
  • [Builds till linking then out of flash ] It builds for hardware native (see Building the firmware)
  • It builds as BMDA (see Building the BMDA)
  • I've tested it to the best of my ability
    Tested with a BMP derivative using CH32V3xx chips. Cant test (yet!) with vanilla bmp
  • My commit messages provide a useful short description of what the commits do
## Detailed description Hi This is a temptative patch to add some flashstub helper functions for the rv32 targets. It tries to be very similar to the cortexm version. i.e. Call _riscv32_run_stub( target, loadaddr, param1, param2, param3, param4)_ and the flashstub must end up with _riscv_stub_exit(errorcode)_ The main difference in behavior is the code saves and restore PC & MIE to avoid letting things dangling in ram. Not completely sure this is needed, just to be on the safe side. I've tested it with a CH32V307 board using the upcoming board & rvswd protocol support with a ~2x speedup. As a sidenote and for the record, my original version was providing a temporary stack for the stub to avoid strict compiler rules about not using stack. Dont know if it is a valid idea. Nonetheless, this MR is assuming the stack is not used at all by the stub. Thank you. ## Your checklist for this pull request * [X] I've read the [Code of Conduct](https://github.com/blackmagic-debug/blackmagic/blob/main/CODE_OF_CONDUCT.md) * [X] I've read the [guidelines for contributing](https://github.com/blackmagic-debug/blackmagic/blob/main/CONTRIBUTING.md) to this repository * [Builds till linking then out of flash ] It builds for hardware native (see [Building the firmware](https://github.com/blackmagic-debug/blackmagic?tab=readme-ov-file#building-black-magic-debug-firmware)) * [X] It builds as BMDA (see [Building the BMDA](https://github.com/blackmagic-debug/blackmagic?tab=readme-ov-file#building-black-magic-debug-app)) * [ ] I've tested it to the best of my ability Tested with a BMP derivative using CH32V3xx chips. Cant test (yet!) with vanilla bmp * [x] My commit messages provide a useful short description of what the commits do

We'll dive in and review this tomorrow, but as for the stack thing - the stub can use stack, it just has to set one up itself - see the RP2040 stub in #1609 which does this to improve code density and size. It's not really viable for the stub runner to do this itself, and with the lui instruction in RISC-V, it would be trivial to load a 4KiB-aligned stack pointer in the stub.

We'll dive in and review this tomorrow, but as for the stack thing - the stub *can* use stack, it just has to set one up itself - see the RP2040 stub in #1609 which does this to improve code density and size. It's not really viable for the stub runner to do this itself, and with the `lui` instruction in RISC-V, it would be trivial to load a 4KiB-aligned stack pointer in the stub.
mean00 commented 2024年02月11日 15:12:03 +01:00 (Migrated from github.com)
Copy link

Hi,
This MR exhibits an issue related to the CSR register management introduced earlier (my bad) because it manipulates some CSR directly.
The CSR registers are offset by RV_CSR_GDB_OFFSET to present them as regular register to gdb
It means that also when used internally, something like
t->reg_write(t, RV_CSR_MIE, &zero, 4);
should actually be
t->reg_write(t, RV_CSR_MIE+RV_CSR_GDB_OFFSET, &zero, 4);

This looks ugly and a bit error prone. Not sure what is the best way to deal with that.

Hi, This MR exhibits an issue related to the CSR register management introduced earlier (my bad) because it manipulates some CSR directly. The CSR registers are offset by RV_CSR_GDB_OFFSET to present them as regular register to gdb It means that also when used internally, something like _**t->reg_write(t, RV_CSR_MIE, &zero, 4);**_ should actually be **_t->reg_write(t, RV_CSR_MIE+RV_CSR_GDB_OFFSET, &zero, 4);_** This looks ugly and a bit error prone. Not sure what is the best way to deal with that.

Given that going through the target API there is just a convoluted way to access riscv_csr_read/riscv_csr_write - we would suggest that the logic use these functions directly, solving the need to deal with the GDB offset and specify the register width that way (you can use RV_CSR_FORCE_32_BIT with RV_CSR_MIE to ensure the right width access will always happen, or just assume because it's a RISC-V 32 part that it's the right width)

Given that going through the target API there is just a convoluted way to access riscv_csr_read/riscv_csr_write - we would suggest that the logic use these functions directly, solving the need to deal with the GDB offset and specify the register width that way (you can use `RV_CSR_FORCE_32_BIT` with `RV_CSR_MIE` to ensure the right width access will always happen, or just assume because it's a RISC-V 32 part that it's the right width)
mean00 commented 2024年02月11日 16:00:07 +01:00 (Migrated from github.com)
Copy link

Indeed, thank you for the pointer, that's way better.

Indeed, thank you for the pointer, that's way better.
mean00 commented 2024年02月12日 08:15:08 +01:00 (Migrated from github.com)
Copy link

Updated accordingly.
Thank you.

Updated accordingly. Thank you.
dragonmux left a comment
Copy link

This is looking good, though there are some items to address and talk about before this can be considered ready for merge. Most of the items are about formatting and code style though.

This is looking good, though there are some items to address and talk about before this can be considered ready for merge. Most of the items are about formatting and code style though.
@ -0,0 +1,34 @@
/*

Please include a proper license header and do not use #pragma once - please use guard macros like the rest of the headers.

Please include a proper license header and do not use `#pragma once` - please use guard macros like the rest of the headers.
dragonmux marked this conversation as resolved

Please use target for the target_s * parameter name, we'd like not to introduce more clang-tidy lints when we've been trying to fix these.

Perhaps use loadaddr rather than codeexec, or at least codeaddr to more clearly indicate what this parameter is?

Please use `target` for the `target_s *` parameter name, we'd like not to introduce more clang-tidy lints when we've been trying to fix these. Perhaps use `loadaddr` rather than `codeexec`, or at least `codeaddr` to more clearly indicate what this parameter is?

Please define each of these separately and with initialisation values. This is a clang-tidy lint.

Please define each of these separately and with initialisation values. This is a `clang-tidy` lint.

This condition and the next look merge-able, and more importantly, look to be able to be simplified such that it removes the gotos (we'd really like not to introduce any, they make for horrible codegen and a horrid time understanding code flow.

For example:

if (reason == TARGET_HALT_REQUEST)
 ret = true;
target->halt_request(target);

Now, we have questions about ret as well and its (re)use here as that logic doesn't make sense right now, but that should also simplify the while loop too so as to just need a break; statement from that check for timeout expiry.

Please write comments detailing your intent with this code - it's very difficult to tell from the plain text of this code what you are intending to have happen here and why, vs what this actually does.

This condition and the next look merge-able, and more importantly, look to be able to be simplified such that it removes the gotos (we'd *really* like not to introduce any, they make for horrible codegen and a horrid time understanding code flow. For example: ```c if (reason == TARGET_HALT_REQUEST) ret = true; target->halt_request(target); ``` Now, we have questions about `ret` as well and its (re)use here as that logic doesn't make sense right now, but that should also simplify the while loop too so as to just need a `break;` statement from that check for timeout expiry. Please write comments detailing your intent with this code - it's very difficult to tell from the plain text of this code what you are intending to have happen here and why, vs what this actually does.

Please remove this trailing comment.

Please remove this trailing comment.

Given how this gets used and how small a function this is, but with it not marked inline, we expect this to be contributing a significant amount to the codegen for riscv32_run_stub() - if you grab the hart at the top of that function, you can then call the underlying read and write routines directly, which should result in leaner code generation. Same with the read function.

Given how this gets used and how small a function this is, but with it not marked inline, we expect this to be contributing a significant amount to the codegen for riscv32_run_stub() - if you grab the hart at the top of that function, you can then call the underlying read and write routines directly, which should result in leaner code generation. Same with the read function.

These need suffixing with U to make them unsigned. It looks like these should really belong in the riscv_debug.h header so all RISC-V debug code can benefit from them.

These need suffixing with `U` to make them unsigned. It looks like these should really belong in the riscv_debug.h header so all RISC-V debug code can benefit from them.

The braces here can be (should be) removed. Please also run clang-format as this is not formatted properly for this code base.

The braces here can be (should be) removed. Please also run clang-format as this is not formatted properly for this code base.

Please use the code base's multi-line comment style which is

/*
 * A multi-line
 * comment
 * 
 * With empty lines
 */

(likewise for the two functions above - though, we've got some thoughts on those as they likely increase code size over the odds as-is)

Please use the code base's multi-line comment style which is ```c /* * A multi-line * comment * * With empty lines */ ``` (likewise for the two functions above - though, we've got some thoughts on those as they likely increase code size over the odds as-is)

Same comment here about comment style as in riscv32.c

Same comment here about comment style as in riscv32.c
mean00 commented 2024年02月18日 17:34:13 +01:00 (Migrated from github.com)
Copy link

I think i've taken all or about all of the comments into account.

I've left the two helper functions as inline to have a simpler code. I guess they might be helpful later on.
The generated codesize should be the same.

Similarily i've let the error handling a bit detailed as to allow further more detailed use cases. It should not cause binary bloat.

( On a side note, clang-tidy 17 does not like much the .clang-tidy due to varying indentation and after fixing it complains really a lot )

I think i've taken all or about all of the comments into account. I've left the two helper functions as inline to have a simpler code. I guess they might be helpful later on. The generated codesize should be the same. Similarily i've let the error handling a bit detailed as to allow further more detailed use cases. It should not cause binary bloat. ( On a side note, clang-tidy 17 does not like much the .clang-tidy due to varying indentation and after fixing it complains really a lot )
dragonmux left a comment
Copy link

This code is looking significantly better - good work! We do have a few small items found in re-reviewing, but with those fixed we'll be very happy to get this merged.

This code is looking significantly better - good work! We do have a few small items found in re-reviewing, but with those fixed we'll be very happy to get this merged.

The content of this multi-line comment (and the others below) are over-indented after the start of line *'s - we run a single space between the * and the first word in the comment in this context.

The content of this multi-line comment (and the others below) are over-indented after the start of line `*`'s - we run a single space between the `*` and the first word in the comment in this context.

tun => run? Seems like a typo.

`tun` => `run`? Seems like a typo.

The parens here can be dropped - == binds tighter than =, so there is no ambiguity without them on what this means. the 0 should be suffixed with a U to prevent a signed-unsigned conversion during comparison.

The parens here can be dropped - `==` binds tighter than `=`, so there is no ambiguity without them on what this means. the `0` should be suffixed with a `U` to prevent a signed-unsigned conversion during comparison.

These still need suffixing with U to mark them unsigned.

These still need suffixing with `U` to mark them unsigned.
This pull request has changes conflicting with the target branch.
  • src/target/riscv_debug.h
View command line instructions

Manual merge helper

Use this merge commit message when completing the merge manually.

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u origin mean00/riscv_flashstub:mean00/riscv_flashstub
git switch mean00/riscv_flashstub
Sign in to join this conversation.
No reviewers
Labels
Clear labels
BMD App
Black Magic Debug App (aka. PC hosted) (not firmware)
BMP Firmware
Black Magic Probe Firmware (not PC hosted software)
Bug
Confirmed bug
Build system
Build system
Can't reproduce
Maintainers can't reproduce this problem
CI
Continuous Integration System
Contribution wanted
User contributions welcome
Documentation
Project documentation
Draft
Work in progress draft
Duplicate
This issue or pull request already exists
Enhancement
General project improvement
Feedback wanted
Requires additional submitter feedback
Foreign Host Board
Non Native hardware to runing Black Magic firmware on
GDB
Issue/PR related to GDB
Good first issue
Good for newcommers
HwIssue Mitigation
Solving or mitigating a Hardware issue in Software
Information Needed
Maintainers need more information
NativeHardware
Official Black Magic Debug Hardware
New Host Board
New hardware to run Black Magic firmware on
New Target
New debug target
Off Topic
Something that does not involve the project in any way
Potential Bug
A potential, unconfirmed or very special circumstance bug
Regression
Bug caused by a regression
User Interest Needed
More user interest required before consideration
User Testing Needed
Looking for user testing reports
Won't fix
Outside of the project scope or works as intended
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
2 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
blackmagic-debug/blackmagic!1764
Reference in a new issue
blackmagic-debug/blackmagic
No description provided.
Delete branch "mean00/riscv_flashstub"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?