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

Fix #18 - riscv64 platform build support #27

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

Conversation

eshattow
Copy link

Patch from @apcameron user. Closes #18 by adding riscv64 platform build support.

@gdt
Copy link
Contributor

gdt commented Jan 14, 2025

This looks reasonable to me. Two comments

I am unclear on LE vs BE, and how that works in this codebase. I know only that BE is somewhat rare and that some codebases do not deal with it properly. I don't see anything in the commit message describing this. I would expect that RISC-V can operate in LE or BE, but I don't know that, and I don't know if you've tested both, one, or neither. Were I someone with authority to hit merge, I 'd want to know that. But then I would be clearer on the BE/LE issues than I am :-)

As a style nit, I lean to the commit message describing the change, which is just

Add support for building on riscv64

and then putting the Fixes #18 as part of the continuation line. If someone is looking later, the change is important, and that there was an open bug report is not.

But that is totally a nit, which I wouldn't have commented on if not tagged in review.

@eshattow
Copy link
Author

This looks reasonable to me. Two comments

I am unclear on LE vs BE, and how that works in this codebase. I know only that BE is somewhat rare and that some codebases do not deal with it properly. I don't see anything in the commit message describing this. I would expect that RISC-V can operate in LE or BE, but I don't know that, and I don't know if you've tested both, one, or neither. Were I someone with authority to hit merge, I 'd want to know that. But then I would be clearer on the BE/LE issues than I am :-)

RISC-V is little-endian by default for software compatibility to the leading architectures ARM and x86. All shipping RISC-V hardware is little-endian which Linux supports. There are not yet any examples of RISC-V big-endian and no support for BE RISC-V in Linux. Further reading for context: [PATCH 1/1] linux-user: add support for big endian variants of riscv

As a style nit, I lean to the commit message describing the change, which is just

Add support for building on riscv64

and then putting the Fixes #18 as part of the continuation line. If someone is looking later, the change is important, and that there was an open bug report is not.

I agree. GitHub web UI is confusing for me, should this be a reword rebase from commandline and force push, or else how to do this so it does not break the Pull Request?

But that is totally a nit, which I wouldn't have commented on if not tagged in review.

Thanks for your review! -E

@gdt
Copy link
Contributor

gdt commented Jan 15, 2025

I suggest noting in the code block that it's for LE. (I think people in NetBSD try to be able to run agile CPUs as BE too, just to push the envelope on portability. Not saying that makes sense, but clarity is good if not really painful.)

Yes, you can amend the commit, or fixup/rebase-i and then force push. That replaces the PR contents and is very normal.

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.

RISC-V support: What's needed?
2 participants