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: move the dcsr modification out of program buffer #1190

Open
wants to merge 1 commit into
base: riscv
Choose a base branch
from

Conversation

lz-bro
Copy link
Contributor

@lz-bro lz-bro commented Dec 24, 2024

When riscv virt2phys_mode is hw, reduce the use of unnecessary program buffers. See #1188 for the original issue.

@lz-bro lz-bro force-pushed the enable-hardware-translation branch from 83bd140 to 46e3fe5 Compare December 24, 2024 07:29
Copy link
Collaborator

@en-sc en-sc left a comment

Choose a reason for hiding this comment

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

@lz-bro, thank you for the patch!

Please take a look at my suggestions.

src/target/riscv/riscv-013.c Outdated Show resolved Hide resolved
src/target/riscv/riscv-013.c Outdated Show resolved Hide resolved
src/target/riscv/riscv-013.c Outdated Show resolved Hide resolved
@en-sc en-sc linked an issue Dec 25, 2024 that may be closed by this pull request
@lz-bro lz-bro force-pushed the enable-hardware-translation branch 2 times, most recently from 67516c3 to af1b33a Compare December 25, 2024 12:59
en-sc
en-sc previously approved these changes Dec 25, 2024
Copy link
Collaborator

@en-sc en-sc left a comment

Choose a reason for hiding this comment

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

LGTM.

Only a minor nitpick.

src/target/riscv/riscv-013.c Outdated Show resolved Hide resolved
en-sc
en-sc previously approved these changes Dec 25, 2024
en-sc
en-sc previously approved these changes Dec 27, 2024
Copy link
Collaborator

@en-sc en-sc left a comment

Choose a reason for hiding this comment

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

Oh, great catch!

LGTM

MarekVCodasip
MarekVCodasip previously approved these changes Jan 22, 2025
Copy link
Collaborator

@MarekVCodasip MarekVCodasip left a comment

Choose a reason for hiding this comment

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

LGTM, I do have a suggestion, up to you.

src/target/riscv/riscv-013.c Outdated Show resolved Hide resolved
return ERROR_OK;
}

static int restore_privilege(struct target *target, riscv_reg_t mstatus, riscv_reg_t mstatus_old,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
static int restore_privilege(struct target *target, riscv_reg_t mstatus, riscv_reg_t mstatus_old,
static int restore_privilege_from_virt2phys_mode(struct target *target, riscv_reg_t mstatus, riscv_reg_t mstatus_old,

If you change the name of modify_privilege(), then change it here as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed

Copy link
Collaborator

Choose a reason for hiding this comment

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

I feel like restore_privilege_for_virt2phys_mode() sounds wrong as we are restoring it from it. I would either remove the for word or replace it with from.

Thanks

when riscv virt2phys_mode is hw, reduce the use of
unnecessary program buffers.
@lz-bro lz-bro dismissed stale reviews from MarekVCodasip and en-sc via 83b79a8 January 23, 2025 01:38
@lz-bro lz-bro force-pushed the enable-hardware-translation branch from 1390357 to 83b79a8 Compare January 23, 2025 01:38
Copy link
Collaborator

@MarekVCodasip MarekVCodasip left a comment

Choose a reason for hiding this comment

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

I still have one more thing left.

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.

How to enable hardware translation when progbufsize too small
4 participants