8000 Enable RISC-V bitmanip extensions for OpenTitan by jwnrt · Pull Request #4145 · tock/tock · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Enable RISC-V bitmanip extensions for OpenTitan #4145

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

Merged
merged 2 commits into from
Nov 18, 2024

Conversation

jwnrt
Copy link
Contributor
@jwnrt jwnrt commented Aug 22, 2024

Pull Request Overview

This pull request enables RISC-V bitmanip extension support when compiling OpenTitan.

Ibex in OpenTitan supports all the bitmanip extensions in addition to the IMC already enabled. The ratified subset of these extensions is supported upstream in LLVM but not exposed in any of the default Rust targets. We can manually enable them via this flag.

I'm not sure how important these extensions are for Tock but the .text size did reduce by 1024 bytes.

Testing Strategy

This pull request was tested by compiling for OpenTitan. I haven't run any proper tests.

Documentation Updated

  • Updated the relevant files in /docs, or no updates are required.

Formatting

  • Ran make prepush.

lschuermann
lschuermann previously approved these changes Aug 22, 2024
@lschuermann
Copy link
Member

This looks good, but the QEMU CI tests and invocations in the board's Makefile should be adjusted to enable those extensions (I'm assuming they're supported?).

@alistair23
Copy link
Contributor

The OpenTitan machine in QEMU uses a vendor CPU, so we can't just enable the extensions from the command line.

I have sent a patch to QEMU to enable the extensions. We just have to wait until the QEMU merge window opens again.

@lschuermann
Copy link
Member

I think that sounds like a good plan!

I do want to mention that -- should this be critical to OpenTitan or take a very long time to merge in QEMU -- we should probably not delay such changes that work on real hardware, but not in simulation. However, this seems like an easy enough patch to simply either way a bit and maintain downstream, or work around in CI.

alevy
alevy previously approved these changes Aug 23, 2024
@lschuermann lschuermann added the blocked-upstream Waiting on something from an upstream project label Aug 23, 2024
@alistair23
Copy link
Contributor

Considering that this was merged into OT in 2021 and we have only just been notified, I think we can wait a few more weeks :)

@jwnrt
Copy link
Contributor Author
jwnrt commented Aug 27, 2024

FYI we have a fork of Tock QEMU with support for the bitmanip extensions (plus a bunch of other things OpenTitan uses) in case that's useful to you

https://github.com/lowRISC/qemu/tree/ot-earlgrey-8.0.2

@alevy
Copy link
Member
alevy commented Aug 27, 2024

@jwnrt did you mean a fork of qemu?

@jwnrt
Copy link
Contributor Author
jwnrt commented Aug 27, 2024

Sorry, yes :)

@lschuermann
Copy link
Member

FYI we have a fork of Tock QEMU with support for the bitmanip extensions (plus a bunch of other things OpenTitan uses) in case that's useful to you

I think that we'd generally be happy to accept an additional CI workflow that runs on that fork, if that is of interest. In particular, if upstream QEMU doesn't support features or is less faithful of emulating a given HW revision, I'd say that this would be an acceptable vehicle to run tests on, in addition to the couple we run on upstream QEMU.

@alevy
Copy link
Member
alevy commented Oct 11, 2024

It looks like this was accepted upstream: https://gitlab.com/qemu-project/qemu/-/commit/06fb3bda6aadeb190be09a1513f1f0d31d119d16 but didn't make it into 9.1.0.

Are we waiting for this to be released in 9.1.1? (presumably in a few days)

@jwnrt jwnrt force-pushed the opentitan-bitmanip branch from 19aaf3e to 9fccd60 Compare October 23, 2024 13:36
@jwnrt
Copy link
Contributor Author
jwnrt commented Oct 23, 2024

Rebased on top of #4214 to include @alistair23's patch.

@lschuermann
Copy link
Member
lschuermann commented Oct 23, 2024

This doesn't seem to work (yet) unfortunately:

earlgrey_cw310 job failed with Timeout Error: Expected "OpenTitan initialisation complete. Entering main loop" but got "make[1]: Entering directory '/home/runner/work/tock/tock/boards/opentitan/earlgrey-cw310'`\r``\n`
[...]

Can you check whether this works locally for you?

@jwnrt
Copy link
Contributor Author
jwnrt commented Oct 23, 2024

It doesn't work locally (which I should have checked first). I'll try to debug.

@alevy
Copy link
Member
alevy commented Nov 6, 2024

@jwnrt What's the status of this?

@jwnrt
Copy link
Contributor Author
jwnrt commented Nov 7, 2024

Had a look at this today, QEMU is faulting on a cpop instruction which comes from Zbb.
I haven't yet worked out why QEMU doesn't like it. Maybe I messed up the QEMU upgrade and Zbb wasn't enabled after all.

@lschuermann lschuermann added waiting-on-author and removed blocked-upstream Waiting on something from an upstream project labels Nov 10, 2024
@alistair23
Copy link
Contributor

The fix isn't in QEMU 9.1.1, which is why it isn't working.

It's probably worth just updating to the current master branch

@jwnrt jwnrt marked this pull request as draft November 18, 2024 13:55
Ibex in OpenTitan supports the bitmanip extensions in addition to IMC.
The ratified set of these are supported upstream in LLVM, but not
exposed in any of the default Rust targets. We can manually enable them
by adding this flag.
@jwnrt jwnrt dismissed stale reviews from alevy and lschuermann via 23013e1 November 18, 2024 13:55
@jwnrt jwnrt force-pushed the opentitan-bitmanip branch from 9fccd60 to 23013e1 Compare November 18, 2024 13:55
@jwnrt
Copy link
Contributor Author
jwnrt commented Nov 18, 2024

I couldn't seem to get it working locally on the latest master of QEMU either

EDIT: it's working in CI so I must have forgotten to re-clone QEMU. Thanks @alistair23.

Sorry for the great big delay on this, it's very busy here at the moment.

@jwnrt jwnrt marked this pull request as ready for review November 18, 2024 14:07
@jwnrt jwnrt requested a review from lschuermann November 18, 2024 14:07
@lschuermann lschuermann added the last-call Final review period for a pull request. label Nov 18, 2024
@lschuermann lschuermann assigned lschuermann and unassigned jrvanwhy Nov 18, 2024
@lschuermann
Copy link
Member

@jwnrt all good, thanks for pushing this forward!

@alevy alevy added this pull request to the merge queue Nov 18, 2024
Merged via the queue into tock:master with commit 0dfc4ff Nov 18, 2024
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
last-call Final review period for a pull request. waiting-on-author
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants
3082
0