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

target/riscv: use cacheable read/write function to handle DCSR #920

Merged
merged 1 commit into from
Oct 10, 2023

Conversation

lz-bro
Copy link
Contributor

@lz-bro lz-bro commented Sep 21, 2023

Avoid cache and memory inconsistencies on DCSR
Change-Id: c99efe3

Copy link
Collaborator

@JanMatCodasip JanMatCodasip left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @lz-bro, thank you for catching the bug.

I recommend a different way of fixing it:

The functions riscv013_set_register and riscv013_get_register should remain low-level and not deal with the register cache. That means they should only call register_read_direct() and register_write_direct(), not the higher-level cache aware functions riscv_get_register() and riscv_set_register().

My proposal is to move the rid checks to riscv_get_register():

int riscv_get_register(struct target *target, riscv_reg_t *value,
		enum gdb_regno regid)
{
	RISCV_INFO(r);
	assert(r->get_register);

	keep_alive();

	/* Handle virtual registers - moved code */

	if (rid == GDB_REGNO_PC) {
		return riscv_get_register(target, value, GDB_REGNO_DPC);
	} else if (rid == GDB_REGNO_PRIV) {
		uint64_t dcsr;
		if (riscv_get_register(target, &dcsr, GDB_REGNO_DCSR) != ERROR_OK)
			return ERROR_FAIL;
		/*...*/
	}
	
	/* Handle read from the actual register - existing code */
	
	if (!target->reg_cache) {
		assert(!target_was_examined(target));
		LOG_TARGET_DEBUG(target, "No cache, reading %s from target",
				gdb_regno_name(regid));
		return r->get_register(target, value, regid);
	}

	struct reg *reg = get_reg_cache_entry(target, regid);
	/* ... */
}

Analogously for riscv_set_register().

@timsifive Does this make sense to you, too? Or am I overreaching on this one?

@lz-bro
Copy link
Contributor Author

lz-bro commented Sep 25, 2023

@JanMatCodasip Thank you for your suggestion, I will modify it later

Copy link
Collaborator

@timsifive timsifive left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not clear to me what bug this is fixing. Was there a discussion somewhere else?

Also, it's failing several of the automated build actions (which I just enabled for you). Please take a look.

src/target/riscv/riscv.c Outdated Show resolved Hide resolved
timsifive
timsifive previously approved these changes Sep 27, 2023
Copy link
Collaborator

@timsifive timsifive left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Cleans up the code and fixes a bug. :-)

JanMatCodasip
JanMatCodasip previously approved these changes Oct 5, 2023
Copy link
Collaborator

@JanMatCodasip JanMatCodasip left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thank you.

@JanMatCodasip
Copy link
Collaborator

@lz-bro Please note that there is a conflict that should be resolved prior to the merge.

Thanks.

Signed-off-by: liangzhen <zhen.liang@spacemit.com>
@lz-bro lz-bro dismissed stale reviews from JanMatCodasip and timsifive via 3f1339f October 7, 2023 01:31
@lz-bro
Copy link
Contributor Author

lz-bro commented Oct 10, 2023

@lz-bro Please note that there is a conflict that should be resolved prior to the merge.

Thanks.
I have resolved the conflict,thank you.

@timsifive timsifive merged commit 781a626 into riscv-collab:riscv Oct 10, 2023
5 checks passed
@en-sc
Copy link
Collaborator

en-sc commented Jan 17, 2024

The move of priv read/write from version-specific part causes priv.v to be read incorrectly for 0.11 targets.
The issue is: dcsr[5] is dcsr.v for the current spec, but it is dcsr.debugint for 0.11
Will prepare a patch with a fix.

@en-sc
Copy link
Collaborator

en-sc commented Jan 17, 2024

Will prepare a patch with a fix.

#997

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 this pull request may close these issues.

4 participants