target/riscv: access registers via reg->type
#1187
Conversation
en-sc
force-pushed
the
en-sc/ref-reg-get-set
branch
from
02e2dae to
6387dd4
Compare
|
Please note: this PR cherry-picks https://review.openocd.org/c/openocd/+/8070 |
en-sc
force-pushed
the
en-sc/ref-reg-get-set
branch
from
6387dd4 to
a54d86f
Compare
There was a problem hiding this comment.
@en-sc Thank you for preparing this change.
I have not yet managed a proper review, but I have revived and rebased the underlying upstream patch.
My preference would be to have that upstream patch merged first, then import the change to riscv-openocd via a periodic merge from upstream, and then finally merge this PR. However, it is difficult to estimate when/if the upstream patch gets a review.
To anyone from the riscv-openocd audience: If you could help with the review in OpenOCD upstream, that would be very appreciated!
There was a problem hiding this comment.
Note: In the review I focused on the second commit only: target/riscv: access registers via reg->type. Any comments to the other underlying upstream commit should be posted directly to the upstream review.
Overall, I like this change very much. I have some concerns over get_pc() and set_pc(), and then trivial suggestions about comments and naming. Other than that, the code looks all right - thank you.
|
@en-sc minor comment regarding commit description: alvais -> always :) |
en-sc
force-pushed
the
en-sc/ref-reg-get-set
branch
3 times, most recently
from
d45da3c to
2b852d6
Compare
en-sc
force-pushed
the
en-sc/ref-reg-get-set
branch
2 times, most recently
from
ba03990 to
8ca5b54
Compare
…pability Cherrry-picked form https://review.openocd.org/c/openocd/+/8070/21 1) OpenOCD has the capability to 'force' a register read from the target. This functionality however silently breaks the register cache: During 'get_reg force' or 'reg <name> force', reg->type->get() is called which will silently overwrite dirty items in the register cache, causing a loss of unwritten register values. This patch fixes that by adding a flush callback for registers, and by using it when it is needed. 2) The register write commands did not have the 'force' flag; this was present for register read commands only. This patch adds it. 3) This patch also introduces the flush_reg_cache command. It flushes all registers and can optionally invalidate the register cache after the flush. For targets which implement the register cache, the flush() callback in struct reg_arch_type should be implemented (in separate patches, by the maintainers of each of the target type). This functionality is also useful for test purposes. Example: - In RISC-V, some registers are WARL (write any read legal) and this command allows to check this behavior. We plan to implement the corresponding callback in the RISC-V target. Change-Id: I9537a5f05b46330f70aad17f77b2b80dedad068a Signed-off-by: Marek Vrbka <[email protected]> Signed-off-by: Jan Matyas <[email protected]>
* `int riscv_reg_get()` and `int riscv_reg_set()` are implemented in
terms of `reg->type->get/set` instead of the other way around. This
makes it easier to support custom behavior for some registers.
* Cacheability is determined by `reg->type` instead of
`riscv_reg_impl_gdb_regno_cacheable()`.
* Issues with redirection of `priv` -> `dcsr` and `pc` -> `dpc` are
addressed at the "topmost" level.
- `priv` and `pc` are always invalid.
- Fixed some issues, e.g. the first `pc` write printed-out an
uninitialized value:
```
> reg pc 0
pc (/64): 0x000075da6b33db20
```
Change-Id: I514547f455d62b289fb5dee62753bf5d9aa3b8ae
Signed-off-by: Evgeniy Naydanov <[email protected]>
en-sc
force-pushed
the
en-sc/ref-reg-get-set
branch
from
8ca5b54 to
85654c1
Compare
int riscv_reg_get()andint riscv_reg_set()are implemented interms of
reg->type->get/setinstead of the other way around. Thismakes it easier to support custom behavior for some registers.
reg->typeinstead ofriscv_reg_impl_gdb_regno_cacheable().priv->dcsrandpc->dpcareaddressed at the "topmost" level.
privandpcare always invalid.pcwrite printed-out anuninitialized value: