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

Implement generic abstract command support cache #1232

Open
en-sc opened this issue Feb 24, 2025 · 1 comment · May be fixed by #1235
Open

Implement generic abstract command support cache #1232

en-sc opened this issue Feb 24, 2025 · 1 comment · May be fixed by #1235

Comments

@en-sc
Copy link
Collaborator

en-sc commented Feb 24, 2025

The idea is similar to #928.

The issue

Currently, there are many target properties derived from abstract command being supported or not:

bool abstract_read_csr_supported;
bool abstract_write_csr_supported;
bool abstract_read_fpr_supported;
bool abstract_write_fpr_supported;
yes_no_maybe_t has_aampostincrement;

There are issues with the current approach, e.g.:

  • Extensibility: Take abstract_..._supported for example. Current implementation assumes the support is the same across all the registers in a group.
  • Correctness: We should make sure abstractcs.cmderr == not_supported, not abstractcs.cmderr != none before asserting that the support is missing. This is not currently done in some places, e.g.
    /* TODO: we need to modify error handling here. */
    /* NOTE: in case of timeout cmderr is set to CMDERR_NONE */
    if (info->has_aampostincrement == YNM_MAYBE) {
    if (result == ERROR_OK) {
    /* Safety: double-check that the address was really auto-incremented */
    riscv_reg_t new_address;
    result = read_abstract_arg(target, &new_address, 1, riscv_xlen(target));
    if (result != ERROR_OK)
    return MEM_ACCESS_FAILED_DM_ACCESS_FAILED;
    if (new_address == args.address + args.size) {
    LOG_TARGET_DEBUG(target, "aampostincrement is supported on this target.");
    info->has_aampostincrement = YNM_YES;
    } else {
    LOG_TARGET_WARNING(target, "Buggy aampostincrement! Address not incremented correctly.");
    info->has_aampostincrement = YNM_NO;
    }

Proposed solution

  • Store all non-supported commands in a cache (a sorted array should suffice).
  • Query if the command is known to be not supported before executing a command (bsearch() in the array).
  • Add the command after execution into the array iff abstractcs.cmderr is not_supported.
@JanMatCodasip
Copy link
Collaborator

JanMatCodasip commented Feb 27, 2025

In my opinion, this is a very good proposal. I agree with both the general idea and the proposed approach for implementation.

fk-sc added a commit to fk-sc/riscv-openocd that referenced this issue Mar 4, 2025
Implemented cache of unsupported abstract commands.
It's purpose is to replace set of caching variables to one.
So this commit provides one ac_not_supported_cache
instead of abstract_read_csr_supported,
abstract_write_csr_supported, abstract_read_fpr_supported,
abstract_write_fpr_supported, has_aampostincrement.

Dropped check for buggy aampostincrement

Fixes riscv-collab#1232

Change-Id: I75cae1481841f2cd0393d6cc80f0d913fbe34294
Signed-off-by: Farid Khaydari <[email protected]>
@fk-sc fk-sc linked a pull request Mar 4, 2025 that will close this issue
fk-sc added a commit to fk-sc/riscv-openocd that referenced this issue Mar 4, 2025
Implemented cache of unsupported abstract commands.
It's purpose is to replace set of caching variables to one.
So this commit provides one ac_not_supported_cache
instead of abstract_read_csr_supported,
abstract_write_csr_supported, abstract_read_fpr_supported,
abstract_write_fpr_supported, has_aampostincrement.

Dropped check for buggy aampostincrement

Fixes riscv-collab#1232

Change-Id: I75cae1481841f2cd0393d6cc80f0d913fbe34294
Signed-off-by: Farid Khaydari <[email protected]>
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 a pull request may close this issue.

2 participants