smmuv2: Add ability to specify sid and cb numbers in a cdl file by alexpavey · Pull Request #34 · seL4/capdl

alexpavey · 2022-02-02T19:27:55Z

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

lsf37

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.

lsf37 · 2022-02-02T22:02:55Z

-        sid_number++;
-        ZF_LOGF_IF(sid_number > MAX_STREAM_IDS, "Stream ID numbers exhausted");


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.

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.

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.

Kent is right, that it doesn't really make sense to not specify an SID.

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.

Is that needed at all? That is entirely platform dependent and checked by the microkernel, so it will just crash at runtime during this call if it is too high.

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

I am not that familiar with this repository, so I am assuming the front-ends mean the tools that generate a CapdDL spec? The breaking case only seems to be if you have an old spec file that you are using with a new loader? Is this something we should address in this change?

I think it probably includes things like the CAmkES generation of capDL specs, if they support SMMU, so that our other repositories which (maybe?) use the SMMU don't break.

Based off of @Furao's and @kent-mcleod's comments, the latest push chooses to require SIDs and missing SIDs are caught at build time. An example .cdl file has also been added.

lsf37 · 2022-02-02T22:03:23Z


+data SIDExtraParam =
+    SIDNumber {
+    sidNumber :: Word }


nitpick: please indent + 2 (the sidNumber :: Word } line)

This has been fixed in the latest push.

lsf37 · 2022-02-02T22:13:40Z

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

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.

corlewis · 2022-02-03T04:00:57Z

-        sid_number++;
-        ZF_LOGF_IF(sid_number > MAX_STREAM_IDS, "Stream ID numbers exhausted");


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.

corlewis · 2022-02-03T04:01:03Z

@@ -648,17 +645,14 @@ unsigned int create_object(CDL_Model *spec, CDL_Object *obj, CDL_ObjID id, seL4_
     */


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.

The comments have been updated in the latest push.

corlewis · 2022-02-03T04:03:37Z

    sizeBits  = fromMaybe 0 size_bits
 showObjectFields _ _ (RTReply {}) _ _ _ = ".type = CDL_RTReply,"
-showObjectFields _ _ (ARMSID {}) _ _ _ = ".type = CDL_SID,"
-showObjectFields _ _ (ARMCB {}) _ _ _ = ".type = CDL_CB,"


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.

I guess if it defaults to zero (which it should, I think) then it would work for at least one context bank... any more would fail because it's already in use.

corlewis · 2022-02-03T04:05:03Z

+    | ARMSID { sidNumber :: Maybe Word }
    | ARMCB { bankNumber :: Maybe Word }


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.

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?

We will update this to just Word

I have made this update in the latest commit.

corlewis · 2022-02-03T04:19:39Z

+validObjPars (Obj ARMSID_T ps []) =
+  subsetConstrs ps (replicate (numConstrs (Addr undefined)) (SIDExtraParam undefined))


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

Suggested change

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))

midnightveil · 2026-10-19T02:03:29Z


 class StreamID(Object):
-    def __init__(self, name):
+    def __init__(self, name, streamid=0):


Should this have a default? Assigning everything 0 will not work past one instance, and it probably makes sense to fail during build then later at boot.

Agreed, this should not get a default value.

This change has been made in the latest push.

doironty · 2026-10-26T13:22:01Z

Bumping for visibility. Please let me know if any additional changes are required to get this PR merged. Thank you for your time! :)

Add a mandatory `sid:` parameter to `streamid` objects. python-capdl-tool now takes the stream ID and bank number as allocation arguments. `streamid` objects without a `sid:` parameter are now rejected. An example CDL file is provided for reference. Signed-off-by: Alex Pavey <Alex.Pavey@dornerworks.com> (cherry picked from commit 0c0871c) Signed-off-by: Tyler Doiron <Tyler.Doiron@dornerworks.com>

doironty · 2026-10-29T13:17:43Z

Latest push made to fix the License Check.

alexpavey mentioned this pull request Feb 2, 2022

vm_arm: Add ability to specify smmu v2 sid to cb mappings seL4/camkes-vm#17

Open

lsf37 requested changes Feb 2, 2022

View reviewed changes

lsf37 requested a review from corlewis February 2, 2022 22:09

corlewis requested changes Feb 3, 2022

View reviewed changes

corlewis reviewed Feb 3, 2022

View reviewed changes

kent-mcleod mentioned this pull request Feb 8, 2022

capDL: updates for smmuv2 functionality #8

Closed

kent-mcleod approved these changes Nov 16, 2022

View reviewed changes

doironty force-pushed the smmu_v2_fixes_improvements branch 2 times, most recently from 9728f45 to 4b20465 Compare October 14, 2026 16:51

midnightveil reviewed Oct 19, 2026

View reviewed changes

doironty force-pushed the smmu_v2_fixes_improvements branch 3 times, most recently from c8e9442 to 4c06791 Compare October 20, 2026 14:26

doironty force-pushed the smmu_v2_fixes_improvements branch from 4c06791 to 17e82be Compare October 29, 2026 13:16

		sid_number++;
		ZF_LOGF_IF(sid_number > MAX_STREAM_IDS, "Stream ID numbers exhausted");

		@@ -648,17 +645,14 @@ unsigned int create_object(CDL_Model spec, CDL_Object obj, CDL_ObjID id, seL4_
		*/

		\| ARMSID { sidNumber :: Maybe Word }
		\| ARMCB { bankNumber :: Maybe Word }

		validObjPars (Obj ARMSID_T ps []) =
		subsetConstrs ps (replicate (numConstrs (Addr undefined)) (SIDExtraParam undefined))

Conversation

alexpavey commented Feb 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lsf37 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lsf37 Feb 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lsf37 commented Feb 2, 2022

Uh oh!

corlewis left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

midnightveil Oct 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

doironty commented Oct 26, 2026

Uh oh!

doironty commented Oct 29, 2026

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

alexpavey commented Feb 2, 2022 •

edited

Loading

lsf37 Feb 2, 2022 •

edited

Loading

midnightveil Oct 19, 2026 •

edited

Loading