Add cnode tag#539
Conversation
This adds a smaller CNode as PD's cspace and moves the original Microkit resource CNode to the root's first slot, as this makes the CSpace management more flexible. The changes are: - size of root CNode: 64 - size of Microkit CNode: 1024 -> 512 - guard size of root CNode: 0 - guard size of Microkit CNode: seL4_WordBits - 6 - 9 This also fixes the capability creation for shared VSpace and CSpace. Signed-off-by: Terry Bai <tianyi.bai@unsw.edu.au>
This adds rules for QEMU tests on x86 and enables the 'capdl_spec.json' file generation for analysis. Signed-off-by: Terry Bai <tianyi.bai@unsw.edu.au>
This checks if people try to place extra caps in slot 0, which is reserved for Microkit CNode, and adds a test case for it. Signed-off-by: Terry Bai <tianyi.bai@unsw.edu.au>
This adds 'cnode' tag in SDF, providing a way for users to define custom-sized CNodes to be shared among PDs. One typical use case is to pass capabilities from one PD to another. Custom CNodes can be mapped into PDs via 'cap_cspace' with target 'cnode_name' specified, and other CapMapType fails with 'cnode_name' specified. Signed-off-by: Terry Bai <tianyi.bai@unsw.edu.au>
This adds a few test cases for checking if invalid cnode tags or cspace mapping fails as expected. Signed-off-by: Terry Bai <tianyi.bai@unsw.edu.au>
| named_object.name.as_ref().unwrap() | ||
| ); | ||
| }; | ||
| if let Some(src_pd) = cap_map.pd { |
There was a problem hiding this comment.
I think this is assignment within if-check, that's extremely confusing.
I would either make temp vars outside the if-checks, or don't bother with them. cap_map.pd and cap_map.cnode are short and clear enough.
There was a problem hiding this comment.
It's if-let destructuring, which is the canonical way to pattern match this in Rust.
However, I would actually agree with you regarding "source" on the CapMap type here internally: https://github.com/seL4/microkit/pull/539/changes#r3491145821
we can use a rust tagged enum and make this clearer.
The way I structured this code I was intended that the source-kind checks would be actually located with the CapMapType.
i.e. we could represent this data structure as
CapMapKind {
Tcb { source: enum {
PdSource(id),
CNodeSource(id)
}
}
or something like that, and then you just do this and you can't construct a CapMap data type without representing our internal invariants.
You could also do the separate source field but then you gather an additional _ => unreachable!("not valid here") case in your matching.
There was a problem hiding this comment.
right, this looks more rust-y.
| capdl_util_make_cnode_cap(pd_src_shadow_cspace.cspace, 0, guard_size) | ||
| } | ||
| }; | ||
| } else if let Some(src_cnode) = cap_map.cnode { |
There was a problem hiding this comment.
It makes no logical sense that this would be an else-if instead of an if, as you're checking different fields. From a higher level it of course makes perfect sense, as they are mutually exclusive.
However, as you already wrap them in an Option, would it make more sense in Rust to have one shared src field of different types and do a switch on type instead?
| let pd_guard_size = kernel_config.cap_address_bits - cnode.size_bits as u64 - PD_ROOT_CAP_BITS as u64; | ||
| let cnode_cap_self_ref = capdl_util_make_cnode_cap(cnode_obj_id, 0, pd_guard_size.try_into().unwrap()); |
There was a problem hiding this comment.
Below it does it without the weird try_into/unwrap combo. Maybe use as u8 above like is done below?
Perhaps add something like an capdl_util_make_sub_cnode_cap() function and add that to capdl util? Then you can call that everywhere and keep this code in one place.
Edit: I realised that capdl util probably doesn't know about kernel_config.cap_address_bits and PD_ROOT_CAP_BITS, so it can't be there and must be a less generic, more local make_sub_cnode_cap().
| let pd_guard_size = kernel_config.cap_address_bits - *size_bits as u64 - PD_ROOT_CAP_BITS as u64; | ||
| let cnode_cap = capdl_util_make_cnode_cap(*cnode_obj_id, 0, pd_guard_size.try_into().unwrap()); |
There was a problem hiding this comment.
Maybe replace with a capdl_util_make_sub_cnode_cap() call here too.
However, I'm not sure you want to do this unconditionally, because the user may want to manage a 2-level sub-CSpace themselves. Gobbling all remaining bits up as guard would make this impossible.
I agree that this should be the default behaviour, but I think that it should also be possible for users to specify the guard size (and perhaps value) manually.
So the the guard size would be cap_address_bits - PD_ROOT_CAP_BITS - size_bits - user_guard_size, where the latter is default 0 and the result must be >= 0.
If adding capdl_util_make_sub_cnode_cap(), you would call it with size_bits + user_guard_size instead of just size_bits.
There was a problem hiding this comment.
The consideration here was for CNode cap resolution, which strictly needs to consume all the cap_address_bits. That's also why I placed a self-ref cap at slot 0 for every sub-CNode.
Do you have better ideas about this?
There was a problem hiding this comment.
I think that it should also be possible for users to specify the guard size (and perhaps value) manually.
I remember my original proposal now: Ideally the user can specify the CSpace size in bits directly, instead of specifying the guard bits. Then the user managed those bits and they always have a fixed, known size. The default would then be cspace size bits equal to the cnode size bits. Specifying the guard size would result in a subspace that depends on PD_ROOT_CAP_BITS, and the user would still have no clue what space they are managing.
The consideration here was for CNode cap resolution, which strictly needs to consume all the cap_address_bits. That's also why I placed a self-ref cap at slot 0 for every sub-CNode.
Okay, as you know we have two different caps CNode caps here: The self-ref cap and the sub-CSpace cap.
By placing the self-ref cap in slot 0, you force full cap_address_bits alignment of the sub-CSpace, meaning it's forced to a 1-level sub-CSpace. Maybe you can work around this by adding a guard size that consumes all remaining bits for the self-ref cap, but I don't think so, I think the kernel check happens at the wrong moment for that. Worth a try though.
So the solution is to not reserve slot 0 for the self-ref cap, but to put that one in Microkit managed space.
This means you have two values to convey to the user: The self-ref cap address and the start CPtr of the user sub-CSpace.
This is fair bit more complicated than what you have now though, so leaving this for a future implementation is fine.
| return Err(value_error( | ||
| xml_sdf, | ||
| node, | ||
| format!("invalid paramter 'cnode_name' for target CapMapType"), |
| return Err(value_error( | ||
| xml_sdf, | ||
| node, | ||
| format!("Either 'pd' and 'cnode_name' should be specified"), |
There was a problem hiding this comment.
See, that's why I prefer a generic src instead of one attribute per source type. What you're doing here doesn't scale very well, and it has unclear semantics by design, making things harder for the user instead of easier.
There was a problem hiding this comment.
The issue with a generic "source" is that you run into namespace issues, and have to add meaning to stting prefixes or as we do here, new attributes.
If you have better ideas how to handle this I'm all ears but scaling is one rea why we had the syntax be "cap_cmode" as there must eventually be a limit on where cnodes are made. (same for other cap types).
That this all currently goes through the same code path right now for every cap type was a simplification.
There was a problem hiding this comment.
What namespace issues? Could you give an example?
For cap management, you basically need to gather the information you need to make the right seL4_CNode_Mint call. The seL4 API is inconsistent and weird, so it has parameters that don't apply to all cap types. That's just how seL4 is, that isn't a namespace issue.
If you work at this level, it's fine to match the seL4 API more, inconsistencies and all. It's fine if you make it easier for the user and provide not just "data", but more explicit "badge", "guard_size" and "guard_value" attributes. Yes, that means you have an attribute "badge" that's usually unused. And you can still add an error or warning if it's used on a cap type that doesn't use it if you want. But as using those on caps that don't use them doesn't make much sense, the added value of doing this is minimal, especially for an advanced feature like this.
There was a problem hiding this comment.
What namespace issues? Could you give an example?
Naming a source as a CPtr isn't very helpful. One wishes to name an appropriate microkit object; for instance: the CSpace/VSpace/Tcb/SC of a PD, or here, the name of a CNode (this is what I meant by namespace, as that would require a global lookup of object name). Or maybe a PD's frames. Or page tables.
There was a problem hiding this comment.
If you have cross-references of any kind, you want unique names, because then you can use those as unique ids in your XML. So yes, you want a global lookup of object names, I don't see why that would be a problem.
There was a problem hiding this comment.
Ah, I forgot: A PD has only one name and multiple caps. I understand what you mean now.
|
|
||
| impl CNode { | ||
| fn from_xml(xml_sdf: &XmlSystemDescription, node: &roxmltree::Node) -> Result<CNode, String> { | ||
| check_attributes(xml_sdf, node, &["name", "type", "size_bits"])?; |
There was a problem hiding this comment.
I'm wondering if it makes more sense to specify this as "slot count" instead of "size bits", because then the declaration wouldn't depend on e.g. how big a cnode slot is. (which tbf is consistent across all 64 bit arches but).
Unless that's how this value is already used, in which case "slot_count_bits" is more precise.
(Is it worth using bits where we can't represent illegal non-pow-2 values or should we just use pow-of-2 numbers and have to catch the non-powrr-of-2 error case. I don't know).
There was a problem hiding this comment.
For Microkit tool, bits makes the effort less to do the checks and calculation, e.g., guard size.
For users, I don't know.
There was a problem hiding this comment.
I assumed that size_bits was the slot count and not the memory size of the CNode. I agree that slot_count_bits would make this explicit.
For users I think both would be fine, but size in slots instead of bits is easier to understand. But if size in bits is used elsewhere already, then it's better to be consistent.
| return Err(value_error( | ||
| xml_sdf, | ||
| node, | ||
| format!("Either 'pd' and 'cnode_name' should be specified"), |
There was a problem hiding this comment.
"or" not and here btw
This PR adds CNode tag to SDF, providing a way for users to define custom CNodes and share among PDs. One of the use cases is to pass capabilities from one PD to another, e.g., ACPI driver passes CNode containing memory window untypeds to PCIe driver so it can dynamically allocate and map memory regions for device drivers.
This PR is based on switch-to-two-level-cnodes PR as it should be getting merged soon.