If "C" cannot be disabled, all changes to misa must be suppressed

[ahadali-10x]

Changed

• Updated legalize_misa function in the riscv_sys_regs.sail in the model/ directory to add condition If "C" cannot be disabled because of next PC is misaligned, all changes to misa will be suppressed

Issue Fixed

• Fix for issue Writing misa should suppress entire write if C=0 illegal #123

[billmcspadden-riscv]

One more question:
Did you test this? If so, what form did the test take?

[ahadali-10x]

The original PR was #143 since I don't have permission to make changes to that. So I submitted this new PR.
Hi, @scottj97 Could you please verify that the changes are according to the spec as you asked in the comment?

[github-actions]

Unit Test Results

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

Results for commit e01da53. ± Comparison against base commit ae905fb.

♻️ This comment has been updated with latest results.

[jrtc27]

Various outstanding comments

[jrtc27]

You'll also need to have a proper commit message that documents the F/D part of this. That should probably even be a separate commit to be honest.

[ptomsich]

One more question: Did you test this? If so, what form did the test take?

Given that it doesn't even compile, we can safely assume that the answer will be "no"...

[scottj97]

You'll also need to have a proper commit message that documents the F/D part of this. That should probably even be a separate commit to be honest.

Agreed, if there are two behavioral changes here, that needs to be two separate commits, and arguably even two separate PRs, since this one's title refers only to misa.C.

I'll try to test the C for correct behavior once this PR settles down.

[ahadali-10x]

Updated the commit to add changes only related to the C extension and tested the changes through assembly tests. Changes related to F/D extension are in addition to these changes. Will submit a separate PR after these changes are merged.

[ahadali-10x]

@jrtc27 @billmcspadden-riscv Let me know if anything needs to be changed. Otherwise, this PR can be merged.

[ptomsich]

One more question: Did you test this? If so, what form did the test take?

@ahadali-10x I was about to merge, when I saw that this question still seemed unanswered... could you provide the requested information? Thx.

[ahadali-10x]

Updated the commit to add changes only related to the C extension and tested the changes through assembly tests.

Yes, it is tested with the assembly tests.

[jrtc27]

Which "assembly tests"?..

[ahadali-10x]

I wrote them manually.

[allenjbaum]

Can we assume that you used a standard ACT tests format, or did you do something different?

…

On Tue, Aug 15, 2023 at 12:54 PM Ahad Ali ***@***.***> wrote: I wrote them manually. — Reply to this email directly, view it on GitHub <#269 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AHPXVJVHL4HDMIYEGDSMU5DXVPHWTANCNFSM6AAAAAAYYOP4NY> . You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>

[ahadali-10x]

Yes I followed the ACT tests format.

[allenjbaum]

Can you put a pointer to the tests here? It sounds like a review of it is desied.

…

On Tue, Aug 15, 2023 at 1:36 PM Ahad Ali ***@***.***> wrote: Yes I followed the ACT tests format. — Reply to this email directly, view it on GitHub <#269 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AHPXVJRXQQKGBUWVZK47RTLXVPMWPANCNFSM6AAAAAAYYOP4NY> . You are receiving this because you commented.Message ID: ***@***.***>

[ahadali-10x]

Here is the link to test: https://github.com/ahadali-10x/riscv-arch-test/tree/sail_test/riscv-test-suite/rv32i_m/sail_test. These tests were run on the RISCOF by passing the required command line arguments. Checked results manually.

[ahadali-10x]

@allenjbaum Kindly let me know if anything else is needed.

[billmcspadden-riscv]

@ahadali-10x , question: I'm looking for the text in the Priv Spec that requires this change. I'm not finding anything. Would you point me to the place in the spec that requires this functionality?

Based on the title of this PR, the logic of the code looks correct to me.

Bill Mc.

[ahadali-10x]

Writing misa may increase IALIGN, e.g., by disabling the “C” extension. If an instruction that
would write misa increases IALIGN, and the subsequent instruction’s address is not IALIGN-bit
aligned, the write to misa is suppressed, leaving misa unchanged.

This is the second last paragraph of section 3.1.1 of the privilege spec.

[billmcspadden-riscv]

I approve.

[ahadali-10x]

@ptomsich Let me know if you need any more information. Otherwise, this PR can be merged.