Skip to content

Conversation

@sudo-apt-abdullah
Copy link
Contributor

Closes #563
Depends on #891
#910 has sieh.yaml that needs to serve as alias for sie which is in this PR!

reset_value(): |
return COUNTINHIBIT_EN[0] ? UNDEFINED_LEGAL : 0;
sw_write(csr_value): |
if (implemented_csr?(CsrName::menvcfg) && CSR[menvcfg].CDE == 1'b0) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checking if menvcfg is implemented is correct, but it strays from how we have been handling these types of cases. Usually, we check for the extension that defines what is needed (in this case, S ~> 1.11). In IDL, this would be implemented_version?(ExtensionName::S, "~> 1.11")

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've updated the condition (implemented_version?(ExtensionName::Sm, ">=1.12")) to first check if menvcfg is there, then raise exceptions based on menvcfg.CDE.

return COUNTINHIBIT_EN[0] ? UNDEFINED_LEGAL : 0;
sw_write(csr_value): |
if (implemented_csr?(CsrName::menvcfg) && CSR[menvcfg].CDE == 1'b0) {
if (mode() == PrivilegeMode::M) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The spec says:

When menvcfg.CDE=0, attempts to access scountinhibit raise an illegal instruction exception.

So an Illegal Instruction should raise even in M mode.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The condition is already there for M mode. I think you're talking about S mode which was missing, right? Let me know if I'm overlooking something!

if (mode() == PrivilegeMode::M) {
raise(ExceptionCode::IllegalInstruction, mode(), $encoding);
}
} else if (implemented_csr?(CsrName::menvcfg) && CSR[menvcfg].CDE == 1'b1) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This check is too restrictive. It won't raise a Virtual Instruction if menvcfg is not implemented, but it should.

To fix, I think you just need to make this an "else".

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've updated the conditions. Please have a look and suggest any further improvement if needed.

reset_value(): |
return COUNTINHIBIT_EN[2] ? UNDEFINED_LEGAL : 0;
sw_write(csr_value): |
if (implemented_csr?(CsrName::menvcfg) && CSR[menvcfg].CDE == 1'b0) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comments as above

definedBy: S
description: |
sie.STIE is the interrupt-enable bit for supervisor-level timer interrupts.
sw_write(csr_value): |
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This (and all other bits in sie) needs to account for mideleg to determine if the bit has any effect

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I placed a check of respective bits from mideleg.

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.

Add missing S CSRs

3 participants