smmuv2: Add ability to specify sid and cb numbers in a cdl file #34
Conversation
Signed-off-by: Alex Pavey <[email protected]>
lsf37
left a comment
lsf37
left a comment
There was a problem hiding this comment.
If I understand this change correctly, it will need some further work for the front ends, otherwise it will break existing specs with SMMU.
Please also add an example .cdl file in capDL-tool that use the feature and add it to the test cases in the Makefile.
| sid_number++; | ||
| ZF_LOGF_IF(sid_number > MAX_STREAM_IDS, "Stream ID numbers exhausted"); |
There was a problem hiding this comment.
This looks like a breaking change in behaviour to me. If sid numbers where previously allocated automatically and now always need to be specified, existing specs will break. I don't think that is a good option.
If we want to keep the loader simple, which I would prefer, the front-ends now need to provide this sid allocation when no sids are listed in the spec.
The change also removes a check for MAX_STREAM_IDS, which, granted, looks like a fairly arbitrary number, but provided some sanity check at least. That is probably also better done in the front end.
There was a problem hiding this comment.
I agree with the suggestion to have the allocation provided by the front-end when required. It should be relatively straightforward to do in either the Python or Haskell parts of the toolchain, although it might be a bit more complicated if we want to support a mix of manually specified sids and cbs with others that are automatically allocated.
Does anyone have a use-case that would be helped by a mix like that? If not, I think it would be simplest to not provide support for such specifications.
There was a problem hiding this comment.
This looks like a breaking change in behaviour to me. If sid numbers where previously allocated automatically and now always need to be specified, existing specs will break. I don't think that is a good option.
It doesn't make sense not specifying a SID. It would be like creating an interrupt handler and not wanting to specify the interrupt ID.
|
|
||
| data SIDExtraParam = | ||
| SIDNumber { | ||
| sidNumber :: Word } |
There was a problem hiding this comment.
nitpick: please indent + 2 (the sidNumber :: Word } line)
|
The gitlint check is complaining about a too-long title (max 50 characters). Please shorten, but instead provide context and background/reasoning for the change in the body of the commit message instead. |
corlewis
left a comment
corlewis
left a comment
There was a problem hiding this comment.
Thank you for this work @Apave24, it definitely looks like it will result in an improvement to the SMMU support! I do agree with @lsf37 though, in that one of the other parts of the toolchain should be updated as well so that we do not break existing specifications.
| sid_number++; | ||
| ZF_LOGF_IF(sid_number > MAX_STREAM_IDS, "Stream ID numbers exhausted"); |
There was a problem hiding this comment.
I agree with the suggestion to have the allocation provided by the front-end when required. It should be relatively straightforward to do in either the Python or Haskell parts of the toolchain, although it might be a bit more complicated if we want to support a mix of manually specified sids and cbs with others that are automatically allocated.
Does anyone have a use-case that would be helped by a mix like that? If not, I think it would be simplest to not provide support for such specifications.
| * When this gets extended we need to decide to add sid number -> cb number map into the haskell / python tool | ||
| * or generate the capdl spec so that the order remains correct here e.g a list a stream ids followed by the cb they | ||
| * are mapped to, the cb condition here (1***) will the reset the stream id number back to 0 for the next context bank. | ||
| */ |
There was a problem hiding this comment.
The comment on lines 641-644 needs to be slightly updated for this change. In particular, I think that the second half of the comment involving the order of stream ids and context banks in the specification is no longer relevant.
| sizeBits = fromMaybe 0 size_bits | ||
| showObjectFields _ _ (RTReply {}) _ _ _ = ".type = CDL_RTReply," | ||
| showObjectFields _ _ (ARMSID {}) _ _ _ = ".type = CDL_SID," | ||
| showObjectFields _ _ (ARMCB {}) _ _ _ = ".type = CDL_CB," |
There was a problem hiding this comment.
It's not relevant to this PR but I'm slightly confused about how this previously worked. It looks like the initialiser already required that context banks had a number specified, but nothing was being passed through by the CapDL-tool here.
| | ARMSID { sidNumber :: Maybe Word } | ||
| | ARMCB { bankNumber :: Maybe Word } |
There was a problem hiding this comment.
Depending on what the decision is about stream ids and context banks having their numbers manually specified or automatically generated, it might make sense for these fields to be Word instead of Maybe Word.
There was a problem hiding this comment.
I agree it makes sense for a sidNumber to be a word. I guess the difference is not setting the argument would be interpreted as the same as setting it to 0?
| validObjPars (Obj ARMSID_T ps []) = | ||
| subsetConstrs ps (replicate (numConstrs (Addr undefined)) (SIDExtraParam undefined)) |
There was a problem hiding this comment.
Hmm, it's pretty harmless but it looks like quite a few of the other cases in this function were incorrectly copied. I'll fix the existing ones, but this should actually be
| validObjPars (Obj ARMSID_T ps []) = | |
| subsetConstrs ps (replicate (numConstrs (Addr undefined)) (SIDExtraParam undefined)) | |
| validObjPars (Obj ARMSID_T ps []) = | |
| subsetConstrs ps (replicate (numConstrs (SIDNumber undefined)) (SIDExtraParam undefined)) |
4 participants
The main driver behind these changes are to allow the camkes tool to map specific stream id's to specific context banks for passthrough devices within a camkes vm application configuration. Related to seL4/camkes-vm#17