Skip to content
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

smmuv2: Add ability to specify sid and cb numbers in a cdl file #34

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions capDL-tool/CapDL/AST.hs
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,11 @@ data SCExtraParam =
scData :: Word }
deriving (Show, Eq, Ord, Typeable, Data)

data SIDExtraParam =
SIDNumber {
sidNumber :: Word }
Copy link
Member

@lsf37 lsf37 Feb 2, 2022

Choose a reason for hiding this comment

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

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

deriving (Eq, Show, Ord, Typeable, Data)

data CBExtraParam =
CBNumber {
cbNumber :: Word }
Expand Down Expand Up @@ -107,6 +112,8 @@ data ObjParam =
frameExtraParam :: FrameExtraParam}
| SCExtraParam {
sc_extraParam :: SCExtraParam }
| SIDExtraParam {
sid_extraParam :: SIDExtraParam }
| CBExtraParam {
cb_extraParam :: CBExtraParam }
| IOAPICIRQExtraParam {
Expand Down
9 changes: 8 additions & 1 deletion capDL-tool/CapDL/MakeModel.hs
Original file line number Diff line number Diff line change
Expand Up @@ -273,6 +273,11 @@ getSCData [] = Nothing
getSCData (SCExtraParam (SCData scData) : _) = Just scData
getSCData (_ : xs) = getSCData xs

getSIDExtraInfo :: [ObjParam] -> Maybe Word
getSIDExtraInfo [] = Nothing
getSIDExtraInfo (SIDExtraParam (SIDNumber x) : _) = Just x
getSIDExtraInfo (_ : xs) = getSIDExtraInfo xs

getCBExtraInfo :: [ObjParam] -> Maybe Word
getCBExtraInfo [] = Nothing
getCBExtraInfo (CBExtraParam (CBNumber x) : _) = Just x
Expand Down Expand Up @@ -435,6 +440,8 @@ validObjPars (Obj MSIIrqSlot_T ps []) =
subsetConstrs ps (replicate (numConstrs (Addr undefined)) (MSIIRQExtraParam undefined))
validObjPars (Obj ARMIrqSlot_T ps []) =
subsetConstrs ps (replicate (numConstrs (Addr undefined)) (ARMIRQExtraParam undefined))
validObjPars (Obj ARMSID_T ps []) =
subsetConstrs ps (replicate (numConstrs (Addr undefined)) (SIDExtraParam undefined))
Comment on lines +443 to +444
Copy link
Member

Choose a reason for hiding this comment

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

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

validObjPars (Obj ARMCB_T ps []) =
subsetConstrs ps (replicate (numConstrs (Addr undefined)) (CBExtraParam undefined))
validObjPars obj = null (params obj)
Expand Down Expand Up @@ -477,7 +484,7 @@ objectOf n obj =
Obj VCPU_T [] [] -> VCPU
Obj SC_T ps [] -> SC (getSCExtraInfo n ps) (getMaybeBitSize ps)
Obj RTReply_T [] [] -> RTReply
Obj ARMSID_T [] [] -> ARMSID
Obj ARMSID_T ps [] -> ARMSID (getSIDExtraInfo ps)
Obj ARMCB_T ps [] -> ARMCB (getCBExtraInfo ps)
Obj _ _ (_:_) ->
error $ "Only untyped caps can have objects as content: " ++
Expand Down
2 changes: 1 addition & 1 deletion capDL-tool/CapDL/Model.hs
Original file line number Diff line number Diff line change
Expand Up @@ -206,7 +206,7 @@ data KernelObject a
target :: Word }

-- fake kernel objects for smmu
| ARMSID
| ARMSID { sidNumber :: Maybe Word }
| ARMCB { bankNumber :: Maybe Word }
Comment on lines +209 to 210
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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?


-- X86 specific objects
Expand Down
13 changes: 13 additions & 0 deletions capDL-tool/CapDL/ParserUtils.hs
Original file line number Diff line number Diff line change
Expand Up @@ -278,6 +278,13 @@ sc_period = do
n <- integer64
return $ Period n

sid_number :: MapParser SIDExtraParam
sid_number = do
reserved "sid"
colon
n <- number
return $ SIDNumber n

cb_number :: MapParser CBExtraParam
cb_number = do
reserved "bank"
Expand Down Expand Up @@ -306,6 +313,11 @@ sc_extra_param = do
<|> sc_data)
return $ SCExtraParam param

sid_extra_param :: MapParser ObjParam
sid_extra_param = do
param <- sid_number
return $ SIDExtraParam param

cb_extra_param :: MapParser ObjParam
cb_extra_param = do
param <- cb_number
Expand Down Expand Up @@ -457,6 +469,7 @@ object_param =
<|> arm_irq_extra_param
<|> ports_param
<|> asid_high_param
<|> sid_extra_param
<|> cb_extra_param

object_params :: MapParser [ObjParam]
Expand Down
20 changes: 18 additions & 2 deletions capDL-tool/CapDL/PrintC.hs
Original file line number Diff line number Diff line change
Expand Up @@ -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,"
Copy link
Member

Choose a reason for hiding this comment

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

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.

showObjectFields _ _ (ARMSID sid_number) _ _ _ =
".type = CDL_SID," +++
".sid_extra = {" +++
".sid = " ++ show (sid) +++
"}"
where
sid = case sid_number of
Just num -> num
_ -> 0
showObjectFields _ _ (ARMCB cb_number) _ _ _ =
".type = CDL_CB," +++
".cb_extra = {" +++
".bank = " ++ show (bank) +++
"}"
where
bank = case cb_number of
Just num -> num
_ -> 0
showObjectFields _ _ x _ _ _ = assert False $
"UNSUPPORTED OBJECT TYPE: " ++ show x

Expand Down
10 changes: 10 additions & 0 deletions capdl-loader-app/include/capdl.h
Original file line number Diff line number Diff line change
Expand Up @@ -279,6 +279,10 @@ typedef struct {
seL4_Word data;
} CDL_SCExtraInfo;

typedef struct {
uint32_t sid;
} CDL_SIDExtraInfo;

typedef struct {
uint8_t bank;
} CDL_CBExtraInfo;
Expand Down Expand Up @@ -353,6 +357,7 @@ typedef struct {
CDL_TCBExtraInfo tcb_extra;
CDL_SCExtraInfo sc_extra;
CDL_CBExtraInfo cb_extra;
CDL_SIDExtraInfo sid_extra;
CDL_IOAPICIRQExtraInfo ioapicirq_extra;
CDL_MSIIRQExtraInfo msiirq_extra;
CDL_ARMIRQExtraInfo armirq_extra;
Expand Down Expand Up @@ -526,6 +531,11 @@ static inline uint8_t CDL_TCB_Priority(CDL_Object *obj)
return obj->tcb_extra.priority;
}

static inline uint32_t CDL_SID_Sid(CDL_Object *obj)
{
return obj->sid_extra.sid;
}

static inline uint8_t CDL_CB_Bank(CDL_Object *obj)
{
return obj->cb_extra.bank;
Expand Down
8 changes: 1 addition & 7 deletions capdl-loader-app/src/main.c
Original file line number Diff line number Diff line change
Expand Up @@ -52,8 +52,6 @@

#define STACK_ALIGNMENT_BYTES 16

#define MAX_STREAM_IDS 60

static seL4_CPtr capdl_to_sel4_orig[CONFIG_CAPDL_LOADER_MAX_OBJECTS];
static seL4_CPtr capdl_to_sel4_copy[CONFIG_CAPDL_LOADER_MAX_OBJECTS];
static seL4_CPtr capdl_to_sel4_irq[CONFIG_CAPDL_LOADER_MAX_OBJECTS];
Expand Down Expand Up @@ -82,7 +80,6 @@ extern char _end[];
* This is expected to be initialised by 'init_copy_addr' on system init */
uintptr_t copy_addr;

uint32_t sid_number = 0;
/* In the case where we just want a 4K page and we cannot allocate
* a page table ourselves, we use this pre allocated region that
* is guaranteed to have a pagetable */
Expand Down Expand Up @@ -648,17 +645,14 @@ unsigned int create_object(CDL_Model *spec, CDL_Object *obj, CDL_ObjID id, seL4_
*/
Copy link
Member

Choose a reason for hiding this comment

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

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.

#ifdef CONFIG_ARM_SMMU
if (CDL_Obj_Type(obj) == CDL_SID) {
err = seL4_ARM_SIDControl_GetSID(seL4_CapSMMUSIDControl, sid_number, seL4_CapInitThreadCNode, free_slot,
err = seL4_ARM_SIDControl_GetSID(seL4_CapSMMUSIDControl, CDL_SID_Sid(obj), seL4_CapInitThreadCNode, free_slot,
CONFIG_WORD_SIZE);
ZF_LOGF_IF(err != seL4_NoError, "Failed to allocate SID cap");
sid_number++;
ZF_LOGF_IF(sid_number > MAX_STREAM_IDS, "Stream ID numbers exhausted");
Comment on lines -654 to -655
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

return seL4_NoError;
} else if (CDL_Obj_Type(obj) == CDL_CB) {
err = seL4_ARM_CBControl_GetCB(seL4_CapSMMUCBControl, CDL_CB_Bank(obj), seL4_CapInitThreadCNode, free_slot,
CONFIG_WORD_SIZE);
ZF_LOGF_IF(err != seL4_NoError, "Failed to allocate CB cap");
sid_number = 0; //(1***)
return seL4_NoError;
}
#endif
Expand Down
4 changes: 2 additions & 2 deletions python-capdl-tool/capdl/Allocator.py
Original file line number Diff line number Diff line change
Expand Up @@ -165,9 +165,9 @@ def alloc(self, type, name=None, label=None, **kwargs):
elif type == ObjectType.seL4_RTReplyObject:
o = RTReply(name)
elif type == ObjectType.seL4_ARMSID:
o = StreamID(name)
o = StreamID(name, **kwargs)
elif type == ObjectType.seL4_ARMCB:
o = ContextBank(name)
o = ContextBank(name, **kwargs)
else:
raise Exception('Invalid object type %s' % type)
self.spec.add_object(o)
Expand Down
12 changes: 6 additions & 6 deletions python-capdl-tool/capdl/Object.py
Original file line number Diff line number Diff line change
Expand Up @@ -640,26 +640,26 @@ def get_size_bits(self):


class StreamID(Object):
def __init__(self, name):
def __init__(self, name, streamid=0):
super().__init__(name)

self.sid = streamid

def __repr__(self):
return '%s = streamid' % self.name
return '%s = streamid (sid: %d)' % (self.name, self.sid)

def get_size_bits(self):
return None


class ContextBank(Object):
def __init__(self, name):
def __init__(self, name, bank=0):
super().__init__(name)

# Assignment of context bank numbers will evolve with use case
self.bank = 0
self.bank = bank

def __repr__(self):
s = '%s = contextbank (bank: %d)' % (self.name, self.bank)
self.bank += 1
return s

def get_size_bits(self):
Expand Down