Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Add ssqosid extension and csr yaml #653
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?
Add ssqosid extension and csr yaml #653
Changes from 7 commits
6fd7f88
08084a3
091c477
ffcce03
cdadf98
bed59a8
cefa26a
10cc9f0
2693eec
9af87be
b22aea6
ee25208
084c743
8ff0e7e
9e9d9f2
04e166e
f47a908
9185363
9484dc9
abcd342
12a4d9b
73e1a92
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
"WARL fields" means that, technically, an implementation can decide to accept any set of valid identifiers (including nothing) and reject the rest. We can't model that, so I suggest we don't try.
Practically, this usually means that an implementation is going to select some number of bits to implement (>= the size of the fields) for each. We can model that.
To reflect the number of implemented bits, we need a parameter in Ssqosid for each of RCID and MCID widths, and sw_write() functions to enforce it.
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.
@syedowaisalishah, have you had a chance to try this yet?
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 I haven’t had a chance to try this yet—I just finished the earlier part and will be starting on this next.
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.
ok, thanks for the update
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, I’ve implemented the validation logic in the sw_write() function to enforce the WARL constraints for RCID and MCID. Since the YAML schema doesn't allow top-level parameters:, I hardcoded the masks directly in the IDL code like this:
This effectively limits both fields to their respective implemented widths (12 bits here).
Is this the correct way to handle it, given the schema limitation?
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 means you need to implement
sw_read()
andsw_write()
to validate this restriction.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 also needs to be similarly validated.
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.
@ThinkOpenly , I've implemented both sw_read() and sw_write() to enforce access restrictions based on Smstateen and the SRMCFG bit in mstateen0, as specified in the normative text. Here's what the logic looks like:
However, I'm currently getting the following IDL validation error:
CSR 'mstateen0' is not defined
This is expected since mstateen0.yaml is introduced in PR #592, which hasn't been merged yet.
Could you please advise me on what I should do?
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 #592 is getting close. I don't have much experience with this, but you can apparently rebase your branch on top of the branch for #592 and pick up those changes for your testing. Once #592 gets merged, your branch will automatically get rebased to main, and will continue working.
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.
@ThinkOpenly.
I struggled a bit with rebasing onto the branch from PR #592, so for testing purposes, I manually copied over the mstateen* CSR definitions and the Smstateen extension from that PR. I also added srmcfg as a field in mstateen0 to resolve the validation error.
With those changes, I was able to work through and resolve all the issues I faced during IDL writing—except for one that I’m still stuck on. Specifically, I get the following error:
A type error occurred no symbol named 'value' on line 146
idl code:
Here’s the relevant snippet:
return value;
I’m not sure what I’m doing wrong here. Do you have any guidance on how to resolve this?
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 wonder if this should be
0
, given the text:But, given the "suggested" language, I'm not sure, or if that implies a configuration parameter is required? @dhower-qc