This is an automated email from Gerrit. Eric Hoffman ([email protected]) just uploaded a new patch set to Gerrit, which you can find at http://openocd.zylin.com/4223
-- gerrit commit 6f71e0abef718cbfa22b144cd719b0857f617b18 Author: Eric Hoffman <[email protected]> Date: Mon Sep 11 09:53:58 2017 -0400 Invalidate working areas when detecting target reset. When the target reset due to uncontrolled reason (i.e. other than due to a 'reset' command, or other openocd initiated reset), the working areas were not invalidated. This resulted in the target-specific modules to think that some allocated working areas were still valid, and if some critical code were in those areas, such as handler code in RAM, those areas could be accessed after the target came out of reset, resulting in unpredictable results. For example, on mips32, there is a 'fast data' handler downloaded in working area, to make for faster data copy to MIPS memory. This handler is installed the first time it is invoked (when the working area pointer is NULL). Then when invoked again, the function just check if the handler is still present, and do not download the code itself if it's already in working area (it just call it again). There is code in place in the MIPS32 target module to free the working area when the target is resumed (restore backup and invalidate), and the main target.c file also free the working areas when a 'reset' command is issued (invalidate them). But there was no provision for detecting uncontrolled reset and free the areas. The fix apply a check in the target_poll() command, and if the target-specific module return the target state 'TARGET_RESET', and the target_poll() function detect a transition to that state, then the target_poll() function call the working area invalidation function (which will properly clear/invalidate the working area pointers, without restoring backup). Also made a fix in mips32 target module to properly return at least once with the state TARGET_RESET when a reset is detected. Note that the target-specific modules can not call the working area free functions themselves to only invalidate the working areas, without restoring the working area backups. The target.c exposed function target_free_all_working_areas() is a stub which call the startc function target_free_all_working_areas_restore(), which itself accept a parameter that specify if the backups should be restored. When calling the exposed target_free_all_working_areas() function, target_free_all_working_areas_restore() is called with 'do restore backup'. When the target reset function is called (reset command), the target_free_all_working_areas_restore() is called with 'do NOT restore backup'. So, outside target.c, the working areas free function is too generic, so that's why I added a check in the target_poll() function, which should be the proper place to report a target reset (for the modules to propagate back the target state to target.c). Without a way to specify not to restore backup, this create a serious unresolvable issue if the target do reset spuriously, as the working areas WILL be restored from backup (provided the option is specified in config file, which is 'off' by default), with no way to prevent it when the working area is later freed. Whatever will be restored will be leftover from before the CPU was reset, and will overwrite the new content at those memory locations. In turn, when detecting target reset, the modules should report at least once the state to target.c (and not transition to another state while in the .poll function before returning). This, not only for the working area free function, but also for better detection and handling of spurious target reset by higher level functions. Change-Id: I144d691c0acf8d1d797304849a1a1cb0b54a73bc Signed-off-by: Eric Hoffman <[email protected]> diff --git a/src/target/mips_m4k.c b/src/target/mips_m4k.c index 7d1c06c..f7736b9 100644 --- a/src/target/mips_m4k.c +++ b/src/target/mips_m4k.c @@ -212,6 +212,15 @@ static int mips_m4k_poll(struct target *target) if (retval != ERROR_OK) return retval; LOG_DEBUG("Reset Detected"); + /* Target is (or was) in reset. */ + target->state = TARGET_RESET; + /* Return now, to report reset. Next time we'll check if we've come + out of reset. */ + return ERROR_OK; + } else if (target->state == TARGET_RESET) { + /* Target was in reset, and no longer is. Assume running, we may + change that bellow... */ + target->state = TARGET_RUNNING; } /* check for processor halted */ diff --git a/src/target/target.c b/src/target/target.c index 8f97666..f8e2d6b 100644 --- a/src/target/target.c +++ b/src/target/target.c @@ -58,6 +58,7 @@ /* default halt wait timeout (ms) */ #define DEFAULT_HALT_TIMEOUT 5000 +static void target_free_all_working_areas_restore(struct target *target, int restore); static int target_read_buffer_default(struct target *target, target_addr_t address, uint32_t count, uint8_t *buffer); static int target_write_buffer_default(struct target *target, target_addr_t address, @@ -525,6 +526,7 @@ struct target *get_current_target(struct command_context *cmd_ctx) int target_poll(struct target *target) { int retval; + int previous_state = target->state; /* We can't poll until after examine */ if (!target_was_examined(target)) { @@ -536,6 +538,12 @@ int target_poll(struct target *target) if (retval != ERROR_OK) return retval; + if ((previous_state != TARGET_RESET) && + (target->state == TARGET_RESET)) { + /* When this happens - all workareas are invalid. */ + target_free_all_working_areas_restore(target, 0); + } + if (target->halt_issued) { if (target->state == TARGET_HALTED) target->halt_issued = false; -- ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot _______________________________________________ OpenOCD-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/openocd-devel
