-
Notifications
You must be signed in to change notification settings - Fork 78
read-only-zero case when CSR disabled - misa.yaml file #918
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
base: main
Are you sure you want to change the base?
Conversation
|
@ThinkOpenly @dhower-qc Please take a look |
dhower-qc
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the contribution! Just a few small nits
spec/std/isa/csr/misa.yaml
Outdated
| type: RO | ||
| reset_value(): | | ||
| return (MXLEN == 32) ? 2'b01 : 2'b10; | ||
| return (!MISA_CSR_IMPLEMENTED) ? 0 : (MXLEN == 32) ? 2'b01 : 2'b10; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would suggest either adding parenthesis or converting this to if (MISA_CSR_IMPLEMENTED) { .. } else { ... } for clarity.
(applies many times in this PR)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we also convert those existing ternary conditions to if-else blocks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we also convert those existing ternary conditions to if-else blocks.
leave the existing ternaries, please.
spec/std/isa/csr/misa.yaml
Outdated
| .. Turn off each present bit invidually and try affected behaviors | ||
| . Check | ||
| .. Fail unless turning off bit disables extension as expected | ||
| .. Fail unless turning off bit disables extension as expected |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You'll need a newline at the end for regressions to pass
|
Partially addresses #576 ; still leaves checks of CSR[misa].extension without checking for read-only-0 status. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #918 +/- ##
=======================================
Coverage 43.30% 43.30%
=======================================
Files 10 10
Lines 4787 4787
Branches 1298 1298
=======================================
Hits 2073 2073
Misses 2714 2714
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
@dhower-qc @ThinkOpenly Shall I go ahead with misa field update, so this can complete the remaining work |
@jevinjojo, you mean you don't want to change all 558 occurences where @dhower-qc, I presume the tedious fix would be to change all of them from: to something like: Basically, let everything "pass" if Do we want a more maintainable implementation? I can't think of a great one offhand. |
|
@ThinkOpenly @dhower-qc No 😄, I was referring the phase 3 from our issue breakdown, updating the CSR files |
Phase 3 is updating the 558 references. :-) I suggest you address the existing comments in the content you've already submitted first, then decide if you wish to proceed with phase 3 or just open an issue to have that addressed later. (If you do wish to proceed, we should discuss the best approach first.) |
e57c2d6 to
a176914
Compare
|
@ThinkOpenly @dhower-qc added the guards to CSR[misa] reference in the core specs and CSR yaml files, also updated the ruby code-gen template. take a look |
ThinkOpenly
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is very close to complete. Thank you for your efforts.
I suggest a few ways to make the changes simpler.
spec/std/isa/csr/cycle.yaml
Outdated
| } | ||
| } else if (mode() == PrivilegeMode::U) { | ||
| if (CSR[misa].S == 1'b1) { | ||
| if ((!MISA_CSR_IMPLEMENTED) || ((!MISA_CSR_IMPLEMENTED) || (CSR[misa].S == 1'b1))) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems redundant. Can it be simplified to:
| if ((!MISA_CSR_IMPLEMENTED) || ((!MISA_CSR_IMPLEMENTED) || (CSR[misa].S == 1'b1))) { | |
| if (!MISA_CSR_IMPLEMENTED || CSR[misa].S == 1'b1) { |
Here and elsewhere, of course.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Almost. We can't just give a 'pass' when misa is read-only-0. In that case, we have to check 'implemented?(ExtensionName::NAME)'. When misa is not read-only-0, having the bit set implies the extension is implemented (and has not been dynamically disabled). When misa is read-only-0, the misa bit does not give any information on the extension.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah. Understood. If misa is not implemented, don't just avoid it, grab the setting from the configuration. So, for this instance:
| if ((!MISA_CSR_IMPLEMENTED) || ((!MISA_CSR_IMPLEMENTED) || (CSR[misa].S == 1'b1))) { | |
| if ((!MISA_CSR_IMPLEMENTED && implemented?(ExtensionName::S) || ((!MISA_CSR_IMPLEMENTED) || (CSR[misa].S == 1'b1))) { |
spec/std/isa/csr/misa.yaml
Outdated
| if (MISA_CSR_IMPLEMENTED) { | ||
| return (implemented?(ExtensionName::A) && MUTABLE_MISA_A) ? CsrFieldType::RW : CsrFieldType::RO; | ||
| } else { | ||
| return CsrFieldType::RO; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you use the slightly simpler implementation below, here and elsewhere?
| if (MISA_CSR_IMPLEMENTED) { | |
| return (implemented?(ExtensionName::A) && MUTABLE_MISA_A) ? CsrFieldType::RW : CsrFieldType::RO; | |
| } else { | |
| return CsrFieldType::RO; | |
| } | |
| if (MISA_CSR_IMPLEMENTED && implemented?(ExtensionName::A) && MUTABLE_MISA_A) { | |
| return CsrFieldType::RW; | |
| } else { | |
| return CsrFieldType::RO; | |
| } |
spec/std/isa/csr/misa.yaml
Outdated
| if (MISA_CSR_IMPLEMENTED) { | ||
| return 1; | ||
| } else { | ||
| return 0; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is reasonably replaced with a ternary.
| if (MISA_CSR_IMPLEMENTED) { | |
| return 1; | |
| } else { | |
| return 0; | |
| } | |
| return (MISA_CSR_IMPLEMENTED) ? 1 : 0; |
spec/std/isa/csr/misa.yaml
Outdated
| if (MISA_CSR_IMPLEMENTED) { | ||
| return ( | ||
| (CSR[misa].MXL << (xlen() - 2)) | | ||
| (CSR[misa].V << 21) | | ||
| (CSR[misa].U << 20) | | ||
| (CSR[misa].S << 18) | | ||
| (CSR[misa].M << 12) | | ||
| (CSR[misa].I << 8) | | ||
| (CSR[misa].H << 7) | | ||
| ((CSR[misa].A & CSR[misa].M & CSR[misa].F & CSR[misa].D) << 6) | # 'G' | ||
| (CSR[misa].F << 5) | | ||
| (CSR[misa].D << 3) | | ||
| (CSR[misa].C << 2) | | ||
| (CSR[misa].B << 1) | | ||
| CSR[misa].A); | ||
| } else { | ||
| return 0; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's no need to change the indentation here if you insert a test/return at the beginning:
| if (MISA_CSR_IMPLEMENTED) { | |
| return ( | |
| (CSR[misa].MXL << (xlen() - 2)) | | |
| (CSR[misa].V << 21) | | |
| (CSR[misa].U << 20) | | |
| (CSR[misa].S << 18) | | |
| (CSR[misa].M << 12) | | |
| (CSR[misa].I << 8) | | |
| (CSR[misa].H << 7) | | |
| ((CSR[misa].A & CSR[misa].M & CSR[misa].F & CSR[misa].D) << 6) | # 'G' | |
| (CSR[misa].F << 5) | | |
| (CSR[misa].D << 3) | | |
| (CSR[misa].C << 2) | | |
| (CSR[misa].B << 1) | | |
| CSR[misa].A); | |
| } else { | |
| return 0; | |
| } | |
| if (!MISA_CSR_IMPLEMENTED) { | |
| return 0; | |
| } | |
| return ( | |
| (CSR[misa].MXL << (xlen() - 2)) | | |
| (CSR[misa].V << 21) | | |
| (CSR[misa].U << 20) | | |
| (CSR[misa].S << 18) | | |
| (CSR[misa].M << 12) | | |
| (CSR[misa].I << 8) | | |
| (CSR[misa].H << 7) | | |
| ((CSR[misa].A & CSR[misa].M & CSR[misa].F & CSR[misa].D) << 6) | # 'G' | |
| (CSR[misa].F << 5) | | |
| (CSR[misa].D << 3) | | |
| (CSR[misa].C << 2) | | |
| (CSR[misa].B << 1) | | |
| CSR[misa].A); |
spec/std/isa/ext/Sm.yaml
Outdated
| MISA_READ_ONLY_ZERO: | ||
| description: | | ||
| When true, indicates the `misa` CSR is hardwired to read-only zero. | ||
| When false, the `misa` CSR behaves as a standard WARL register. | ||
| Only applicable when MISA_CSR_IMPLEMENTED == false. | ||
| schema: | ||
| type: boolean | ||
| default: false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is needed, as it is redundant with MISA_CSR_IMPLEMENTED.
| case @data["length"] | ||
| when "MXLEN" | ||
| "CSR[misa].MXL == 0" | ||
| "(!MISA_CSR_IMPLEMENTED) || (CSR[misa].MXL == 0)" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If misa is not implemented, it is read-only-0. Since this is just testing against zero, you need not test to see if it's implemented.
Same comment applies anywhere the test is with zero (equal or not equal).
aa4ab1f to
7ae01d6
Compare
|
@ThinkOpenly @dhower-qc I think it's almost done :) |
spec/std/isa/csr/misa.yaml
Outdated
| if (csr_value.F == 0 && csr_value.D == 1) { | ||
| return UNDEFINED_LEGAL_DETERMINISTIC; | ||
| } | ||
| if (MISA_CSR_IMPLEMENTED) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dhower-qc, is this test needed? In the !MISA_CSR_IMPLEMENTED case, all fields are marked read-only, so that means we never call the sw_write() method, is that correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need a little flow chart as to how "writable" is checked. I suspect something like:
- "writable" attribute for the CSR
- "type"/"type()" attribute/method for the field
- "legal?()" method
- "sw_write()" method logic
If that's reasonable accurate, then we won't get to the "sw_write()" method in the read-only-0 case, because the write is prevented by the "type()" method. So, no special code is needed here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dhower-qc, is this test needed? In the
!MISA_CSR_IMPLEMENTEDcase, all fields are marked read-only, so that means we never call thesw_write()method, is that correct?
correct
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need a little flow chart as to how "writable" is checked. I suspect something like:
- "writable" attribute for the CSR
- "type"/"type()" attribute/method for the field
- "legal?()" method
- "sw_write()" method logic
If that's reasonable accurate, then we won't get to the "sw_write()" method in the read-only-0 case, because the write is prevented by the "type()" method. So, no special code is needed here.
- isn't actually used anywhere. we should remove it. Its purpose is handled through sw_write/sw_read, which together try to define what happens if the write is illegal.
Otherwise, the order is correct (1-2-4)
spec/std/isa/csr/misa.yaml
Outdated
| } | ||
| legal?(csr_value): | | ||
| return !(csr_value.Q == 1 && csr_value.D == 0); | ||
| if (MISA_CSR_IMPLEMENTED) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar question to above, is this test needed in the !MISA_CSR_IMPLEMENTED case? (Do we ever need to test for legal values of a CSR if that CSR is read-only?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When MISA_CSR_IMPLEMENTED is false, the CSR is read-only-0, so no writes can actually occur. any write value is legal?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I want to assert that in the read-only-0 case, we don't need to test for legal values because no values will ever be accepted/written. I don't think you need any new code here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I said above, let's just remove the legal? field
spec/std/isa/csr/misa.yaml
Outdated
| Indicates support for the `V` (vector) extension. | ||
| [when,"MUTABLE_MISA_V == true"] | ||
| [when,"MUTABLE_MISA_V == true && MISA_CSR_IMPLEMENTED == true"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I imagine we don't need this, since if any bits of the CSR are mutable, the CSR must be implemented. I'm not sure if we enforce that, though, which we should. To do that, we probably need some "EXTRA_VALIDATION" in Sm.yaml somewhere to check if any MUTABLE parameters are true when IMPLEMENTED is false. Is that something you want to include here, or shall we open a new issue?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in the Sm.yaml file an extra_validation check under MISA_CSR_IMPLEMENTED and validation verifies that if any MISA bit (A-Z) is also need to marked as mutable. do we need to add error message if the condition violated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in the
Sm.yamlfile an extra_validation check under MISA_CSR_IMPLEMENTED and validation verifies that if any MISA bit (A-Z) is also need to marked as mutable.
I'm leaning toward adding the extra_validation in the extensions where the mutability parameter is defined. For example, in spec/std/isa/ext/A.yaml, under MUTABLE_MISA_A add something like:
extra_validation: assert MISA_CSR_IMPLEMENTED == true
(untested). In that way, "everybody" only needs to know that one "implemented" parameter, rather than Sm.yaml having to know every other "mutability" parameter.
do we need to add error message if the condition violated
No, that will come automatically when the architecture is resolved.
spec/std/isa/ext/Sm.yaml
Outdated
| MTVEC_MODES: | ||
| default: true | ||
| MTVEC_MODES: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this related? I think this should be removed from this PR.
spec/std/isa/isa/fp.idl
Outdated
| } | ||
| body { | ||
| if (MUTABLE_MISA_F && CSR[misa].F == 0) { | ||
| if (MUTABLE_MISA_F && ((!MISA_CSR_IMPLEMENTED && !implemented?(ExtensionName::F)) || (CSR[misa].F == 0))) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change is unnecessary. If the F bit is mutable, then the CSR is implemented.
spec/std/isa/isa/globals.isa
Outdated
| CSR[mcause].INT = 1'b0; | ||
| CSR[mcause].CODE = $bits(exception_code); | ||
| if (CSR[misa].H == 1) { | ||
| if ((!MISA_CSR_IMPLEMENTED) || (CSR[misa].H == 1)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs the longer test:
| if ((!MISA_CSR_IMPLEMENTED) || (CSR[misa].H == 1)) { | |
| if ((!MISA_CSR_IMPLEMENTED && implemented?(ExtensionName::H)) || (CSR[misa].H == 1)) { |
spec/std/isa/isa/globals.isa
Outdated
| } | ||
| CSR[mstatus].MPP = $bits(from_mode); | ||
| } else if (CSR[misa].S == 1 && (handling_mode == PrivilegeMode::S)) { | ||
| } else if ((!MISA_CSR_IMPLEMENTED) || (CSR[misa].S == 1 )&& (handling_mode == PrivilegeMode::S)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs the longer test, too.
spec/std/isa/isa/globals.isa
Outdated
| CSR[scause].CODE = $bits(exception_code); | ||
| CSR[mstatus].SPP = $bits(from_mode)[0]; | ||
| if (CSR[misa].H == 1) { | ||
| if ((!MISA_CSR_IMPLEMENTED) || (CSR[misa].H == 1)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs the longer test, too.
spec/std/isa/isa/globals.isa
Outdated
| } | ||
| } | ||
| } else if (CSR[misa].H == 1 && (handling_mode == PrivilegeMode::VS)) { | ||
| } else if ((!MISA_CSR_IMPLEMENTED) || (CSR[misa].H == 1 )&& (handling_mode == PrivilegeMode::VS)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
longer test
spec/std/isa/isa/globals.isa
Outdated
| return 32; | ||
| } else { | ||
| if (mode() == PrivilegeMode::M) { | ||
| if (CSR[misa].MXL == $bits(XRegWidth::XLEN32)) { | ||
| if ((!MISA_CSR_IMPLEMENTED) || (CSR[misa].MXL == $bits(XRegWidth::XLEN32))) { | ||
| return 32; | ||
| } else if (CSR[misa].MXL == $bits(XRegWidth::XLEN64)) { | ||
| } else if ((!MISA_CSR_IMPLEMENTED) || (CSR[misa].MXL == $bits(XRegWidth::XLEN64))) { | ||
| return 64; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not quite sure here, but pretty sure this needs to change. This always returns 32 if !MISA_CSR_IMPLEMENTED. @dhower-qc ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yea, this can just check that the MXLEN parameter is 32/64. even when misa is implemented, MXL is read-only (this was a backward-looking, incompatible change made to the spec)
spec/std/isa/isa/globals.isa
Outdated
| # with the use of mstatus.MPRV | ||
| if (mode() == PrivilegeMode::M) { | ||
| if (CSR[misa].U == 1 && CSR[mstatus].MPRV == 1) { | ||
| if ((!MISA_CSR_IMPLEMENTED) || (CSR[misa].U == 1 )&& CSR[mstatus].MPRV == 1) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this needs a longer test, and be careful with how the logic is grouped. Using A || B && C could be confusing (and seems to be incorrect here).
spec/std/isa/isa/globals.isa
Outdated
| if ((!MISA_CSR_IMPLEMENTED) || (CSR[misa].U == 1 )&& CSR[mstatus].MPRV == 1) { | ||
| if (CSR[mstatus].MPP == 0b00) { | ||
| if (CSR[misa].H == 1 && mpv() == 0b1) { | ||
| if ((!MISA_CSR_IMPLEMENTED) || (CSR[misa].H == 1 )&& mpv() == 0b1) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same
spec/std/isa/isa/globals.isa
Outdated
| } else if ((!MISA_CSR_IMPLEMENTED) || (CSR[misa].S == 1 )&& CSR[mstatus].MPP == 0b01) { | ||
| if ((!MISA_CSR_IMPLEMENTED) || (CSR[misa].H == 1 )&& mpv() == 0b1) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same... there are more cases below where you are just checking for MISA_CSR_IMPLEMENTED, but you also need to check implemented?..., OK?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
okay
ThinkOpenly
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I submitted a bunch of "single comments", above, rather than a single review. Hopefully it all makes sense.
|
Keep up the good work! |
fa3c6e5 to
15c0c7a
Compare
|
@ThinkOpenly @dhower-qc did the changes, can you verify if I missed anything. |
ThinkOpenly
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some minor, but important changes.
spec/std/isa/isa/interrupts.idl
Outdated
| CSR[mstatush].MPV = $bits(mode())[2]; | ||
| } | ||
| if ((!MISA_CSR_IMPLEMENTED && implemented?(ExtensionName::H)) || (CSR[misa].H == 1'b1)) { | ||
| CSR[mstatus].MPV = $bits(mode())[2]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The original code can't be simplified here. Notice that the "then" block is using mstatus and the else block is using mstatush, so you'll need to revert that part of this change, but keep the "MISA_CSR_IMPLEMENTED" change.
9fed2ef to
1fbc3e7
Compare
Signed-off-by: Jevin Jojo <[email protected]>
|
@ThinkOpenly @dhower-qc Any changes required. |
dhower-qc
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work. Still a bit more to do :) Check for references in spec/**/inst/*.yaml for misa; there should be a lot.
| Reports the XLEN and "major" extensions supported by the ISA. | ||
| [when,"MISA_CSR_IMPLEMENTED == false"] | ||
| This CSR is read-only-0 (all bits hardwired to zero) and cannot be written. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We've update how this is handled:
| Reports the XLEN and "major" extensions supported by the ISA. | |
| [when,"MISA_CSR_IMPLEMENTED == false"] | |
| This CSR is read-only-0 (all bits hardwired to zero) and cannot be written. | |
| - id: csr-misa-purpose | |
| text: | | |
| Reports the XLEN and "major" extensions supported by the ISA. | |
| when: | |
| param: | |
| name: MISA_CSR_IMPLEMENTED | |
| value: true | |
| - id: csr-misa-unimplemented | |
| text: | | |
| This CSR is read-only-0 (all bits hardwired to zero) and cannot be written. | |
| when: | |
| param: | |
| name: MISA_CSR_IMPLEMENTED | |
| value: false |
Note that the schema will reject this until #891 ships
| the extension can be disabled in the `misa.A` bit. | ||
| schema: | ||
| type: boolean | ||
| extra_validation: assert MISA_CSR_IMPLEMENTED == true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MISA_CSR_IMPLEMENTED only needs to be true if MUTABLE_MISA_A is true:
| extra_validation: assert MISA_CSR_IMPLEMENTED == true | |
| extra_validation: assert MISA_CSR_IMPLEMENTED == true if MUTABLE_MISA_A |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
still here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I checked with git diff to see if there line ending/permission changes, also tried resetting it. but didn't work. I'm not sure can you tell me what might be causing this?
| } else { | ||
| if (mode() == PrivilegeMode::M) { | ||
| if (CSR[misa].MXL == $bits(XRegWidth::XLEN32)) { | ||
| return 32; | ||
| } else if (CSR[misa].MXL == $bits(XRegWidth::XLEN64)) { | ||
| return 64; | ||
| } | ||
| } else if (implemented?(ExtensionName::S) && mode() == PrivilegeMode::S) { | ||
| if (CSR[mstatus].SXL == $bits(XRegWidth::XLEN32)) { | ||
| return 32; | ||
| } else if (CSR[mstatus].SXL == $bits(XRegWidth::XLEN64)) { | ||
| return 64; | ||
| } | ||
| } else if (implemented?(ExtensionName::U) && mode() == PrivilegeMode::U) { | ||
| if (CSR[mstatus].UXL == $bits(XRegWidth::XLEN32)) { | ||
| return 32; | ||
| } else if (CSR[mstatus].UXL == $bits(XRegWidth::XLEN64)) { | ||
| return 64; | ||
| } | ||
| } else if (implemented?(ExtensionName::H) && mode() == PrivilegeMode::VS) { | ||
| if (CSR[hstatus].VSXL == $bits(XRegWidth::XLEN32)) { | ||
| return 32; | ||
| } else if (CSR[hstatus].VSXL == $bits(XRegWidth::XLEN64)) { | ||
| return 64; | ||
| } | ||
| } else if (implemented?(ExtensionName::H) && mode() == PrivilegeMode::VU) { | ||
| if (CSR[vsstatus].UXL == $bits(XRegWidth::XLEN32)) { | ||
| return 32; | ||
| } else if (CSR[vsstatus].UXL == $bits(XRegWidth::XLEN64)) { | ||
| return 64; | ||
| } | ||
| } else if (mode() == PrivilegeMode::M) { | ||
| return MXLEN; | ||
| } else if (implemented?(ExtensionName::S) && mode() == PrivilegeMode::S) { | ||
| if (CSR[mstatus].SXL == $bits(XRegWidth::XLEN32)) { | ||
| return 32; | ||
| } else if (CSR[mstatus].SXL == $bits(XRegWidth::XLEN64)) { | ||
| return 64; | ||
| } | ||
| } else if (implemented?(ExtensionName::U) && mode() == PrivilegeMode::U) { | ||
| if (CSR[mstatus].UXL == $bits(XRegWidth::XLEN32)) { | ||
| return 32; | ||
| } else if (CSR[mstatus].UXL == $bits(XRegWidth::XLEN64)) { | ||
| return 64; | ||
| } | ||
| } else if (implemented?(ExtensionName::H) && mode() == PrivilegeMode::VS) { | ||
| if (CSR[hstatus].VSXL == $bits(XRegWidth::XLEN32)) { | ||
| return 32; | ||
| } else if (CSR[hstatus].VSXL == $bits(XRegWidth::XLEN64)) { | ||
| return 64; | ||
| } | ||
| } else if (implemented?(ExtensionName::H) && mode() == PrivilegeMode::VU) { | ||
| if (CSR[vsstatus].UXL == $bits(XRegWidth::XLEN32)) { | ||
| return 32; | ||
| } else if (CSR[vsstatus].UXL == $bits(XRegWidth::XLEN64)) { | ||
| return 64; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this diff here?
| return 64; | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove
| case @data["length"] | ||
| when "MXLEN" | ||
| "CSR[misa].MXL == 1" | ||
| "(!MISA_CSR_IMPLEMENTED) || (CSR[misa].MXL == 1)" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| "(!MISA_CSR_IMPLEMENTED) || (CSR[misa].MXL == 1)" | |
| "MXLEN == 64" |
What you wrote isn't quite right, since it doesn't tell you what MXLEN is when misa isn't implemented. MXLEN can never change, so we don't need to consult misa at all here
|
@dhower-qc @ThinkOpenly Will do the changes in a few days. |
Signed-off-by: Jevin Jojo <[email protected]>
|
Should we expect additional changes here? |
Issue
Proposed Changes