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
Open

Implement generic abstract command support cache #1232

en-sc opened this issue Feb 24, 2025 · 1 comment

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.

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

No branches or pull requests

2 participants