smmuv2: Add ability to specify sid and cb numbers in a cdl file #34
Conversation
Signed-off-by: Alex Pavey <[email protected]>
| 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.
| @@ -58,6 +58,11 @@ data SCExtraParam = | |||
| scData :: Word } | |||
| deriving (Show, Eq, Ord, Typeable, Data) | |||
|
|
|||
| 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. |
| 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.
| @@ -648,17 +645,14 @@ unsigned int create_object(CDL_Model *spec, CDL_Object *obj, CDL_ObjID id, seL4_ | |||
| */ | |||
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.
| @@ -377,8 +377,24 @@ showObjectFields _ _ (SC info size_bits) _ _ _ = | |||
| sc_data = fromMaybe 0 (maybe Nothing scData info) | |||
| 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)) |
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