Implement Zicbom, Zicboz (cbo.flush, cbo.inval, cbo.zero)

[Timmmm]

Note that Zicbop (prefetch hints) does not need to be implemented because all it does is label some existing base instructions as prefetch hints.

Also I have not wired up the enable flags to the emulators because it is rather tedious (and will hopefully be replaced by riscv-config at some point).

[github-actions]

Test Results

712 tests  ±0   712 ✅ ±0   0s ⏱️ ±0s
  6 suites ±0     0 💤 ±0 
  1 files   ±0     0 ❌ ±0 

Results for commit fcd31da. ± Comparison against base commit 05b845c.

♻️ This comment has been updated with latest results.

[bacam]

Note that Zicbop (prefetch hints) does not need to be implemented because all it does is label some existing base instructions as prefetch hints.

Shouldn't there be encoding and assembly information at least? It might also be useful to calculate the address and pass it to a built-in function so that it could be logged (e.g., to check that some code's prefetch instruction refers to the correct address), along with a comment noting that no exceptions are taken if something goes wrong.

[Timmmm]

Shouldn't there be encoding and assembly information at least? It might also be useful to calculate the address and pass it to a built-in function so that it could be logged (e.g., to check that some code's prefetch instruction refers to the correct address), along with a comment noting that no exceptions are taken if something goes wrong.

That's a good point... I think it can be a separate PR though if anyone wants to tackle it.

[animesh211]

HI,
Just wanted to check, Is this the main PR for cbo.zero support in sail? and Can anyone please let me know, if there's an ETA to close this.

[Timmmm]

Yes it is - it's based on the older one (#137) with some fixes. As far as I am concerned it is ready to merge; it's just waiting on review.

No ETA I'm afraid - not many people are actively reviewing PRs and unfortunately nobody is paid to do it currently. (IMO RVI should hire someone to do this but that's an ongoing discussion.)

[billmcspadden-riscv]

I'll review it. However, I see that there are some conflicts that need to be resolved. Bill Mc.

…

On Tue, Jul 16, 2024 at 6:18 AM Tim Hutt ***@***.***> wrote: Yes it is - it's based on the older one (#137 <#137>) with some fixes. As far as I am concerned it is ready to merge; it's just waiting on review. No ETA I'm afraid - not many people are actively reviewing PRs and unfortunately nobody is paid to do it currently. (IMO RVI should hire someone to do this but that's an ongoing discussion.) — Reply to this email directly, view it on GitHub <#455 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AXROLOETFGKT3KKCYQHYNCLZMT6PZAVCNFSM6AAAAABGTDUZUKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDEMZQGY2DENRQGY> . You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>

-- Bill McSpadden Formal Verification Engineer RISC-V International mobile: 503-807-9309

[billmcspadden-riscv]

@tim Hutt ***@***.***> , how did you test your implementation? Bill Mc.

…

On Tue, Jul 16, 2024 at 7:45 AM Bill McSpadden ***@***.***> wrote: I'll review it. However, I see that there are some conflicts that need to be resolved. Bill Mc. On Tue, Jul 16, 2024 at 6:18 AM Tim Hutt ***@***.***> wrote: > Yes it is - it's based on the older one (#137 > <#137>) with some fixes. As far > as I am concerned it is ready to merge; it's just waiting on review. > > No ETA I'm afraid - not many people are actively reviewing PRs and > unfortunately nobody is paid to do it currently. (IMO RVI should hire > someone to do this but that's an ongoing discussion.) > > — > Reply to this email directly, view it on GitHub > <#455 (comment)>, > or unsubscribe > <https://github.com/notifications/unsubscribe-auth/AXROLOETFGKT3KKCYQHYNCLZMT6PZAVCNFSM6AAAAABGTDUZUKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDEMZQGY2DENRQGY> > . > You are receiving this because you are subscribed to this thread.Message > ID: ***@***.***> > -- Bill McSpadden Formal Verification Engineer RISC-V International mobile: 503-807-9309

-- Bill McSpadden Formal Verification Engineer RISC-V International mobile: 503-807-9309

[Timmmm]

Thanks, I've rebased and fixed the conflicts.

It's been tested by running against our A730 chip with the following tests:

• cbo.zero-01.S from riscv-arch-test
• zero.S from riscv-tests
• A very basic directed test we wrote that does some stores and loads combined with the CBO instructions and CSR settings.
• STING - it's not really clear exactly what they test (the descriptions are unhelpfully short like "Test for cmo instruction - CBO.FLUSH") but they're generally pretty good.

The first two only test cbo.zero; I'm not sure why they didn't add tests for the other instructions.

[billmcspadden-riscv]

Great! I'm glad to see that you found the zero.S test in riscv-tests. I was going to point you to it, but as usual, you are ahead of the game. Bill Mc.

…

On Tue, Jul 16, 2024 at 9:06 AM Tim Hutt ***@***.***> wrote: Thanks, I've rebased and fixed the conflicts. It's been tested by running against our A730 chip with the following tests: - cbo.zero-01.S from riscv-arch-test - zero.S from riscv-tests - A very basic directed test we wrote that does some stores and loads combined with the CBO instructions and CSR settings. - STING - it's not really clear exactly what they test (the descriptions are unhelpfully short like "Test for cmo instruction - CBO.FLUSH") but they're generally pretty good. The first two only test cbo.zero; I'm not sure why they didn't add tests for the other instructions. — Reply to this email directly, view it on GitHub <#455 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AXROLOECMBET5BYWFU7MI53ZMUSHJAVCNFSM6AAAAABGTDUZUKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDEMZQHE4DMMZXGE> . You are receiving this because your review was requested.Message ID: ***@***.***>

-- Bill McSpadden Formal Verification Engineer RISC-V International mobile: 503-807-9309

[Timmmm]

Thanks for the review @jrtc27 !

[animesh211]

HI, It'll be great if this can be reviewed and closed.

[aprnath]

Adding a gentle nudge to animesh211's request for this to be merged and closed.

[Timmmm]

I rebased this and updated it to use the new extensionEnabled() function. Arguably this:

function clause extensionEnabled(Ext_Zicbom) = sys_enable_zicbom()

should be this

function clause extensionEnabled(Ext_Zicbom) = sys_enable_zicbom() & cbo_zero_enabled(cur_privilege)

however I left it as it was just so we can get this in.

@billmcspadden-riscv any objection to merging this? I think it has been ready with no objections for a long time.

[billmcspadden-riscv]

LGTM

[Timmmm]

Thanks!