IOMMU: Add IOMMU Support - #358
Conversation
5770590 to
91d501c
Compare
91d501c to
db76418
Compare
| for entry in obj.slots.iter() { | ||
| let entry_ioaddr = ioaddr + (usize::from(entry.slot) << shift); |
There was a problem hiding this comment.
Please follow init_vspace if you can; I suspect this should be for (i, entry) in obj.entries().
There was a problem hiding this comment.
Equivalent, but will update.
| ); | ||
| } | ||
|
|
||
| let runtime_level = iopt_level + prefix_depth; |
There was a problem hiding this comment.
I need to think about this prefix a bit more, FWIW.
It's not immediately obvious from the code how this works, so it would be a good idea to include your motivation from the PR description as a comment here.
I do have a question though: how are the levels encoded in the spec, as who defines "Level = 0 => top of the page table"?
Obviously there's a mismatch here somewhere, but I'm not clear on precisely where it is.
// Level is zero indexed in the spec.
This comment to me seems to imply that the spec always starts the levels from 0.
And how is this not an issue for the VSpace? seL4 has similar behaviour; for AARch64-hyp with PASize=40 it's 3-level, but for AArch64-hyp with PASize>=44 it's 4-level, so I don't see why IOMMU would be special in this case (where special = extra behaviour not needed for vspace).
There was a problem hiding this comment.
@nspin How does your VSpace code handle PASize=40 (num_levels=3) on AArch64? (AARCH64_VSPACE_S2_START_L1 in seL4 and this relevant discussion here).
There was a problem hiding this comment.
Yep, the difference is the IODevice (which is the IOSpace) object. For a normal address space it would be the vspace that consumes 4KiB and acts as the root of the translation table. But for IOMMU the IOSpace is meagrely a capability i.e. it's a fake kernel object. The root of translation is the actually the first IOPT that gets mapped in.
There was a problem hiding this comment.
I don't know if this is the same as the hypervisor, I would be surprised...
There was a problem hiding this comment.
I think I simplified it:)
072aa65 to
474be57
Compare
midnightveil
left a comment
There was a problem hiding this comment.
I'm happy with this now, until the kernel changes (seL4/seL4#1703) go through.
Waiting on @nspin review.
| } | ||
|
|
||
| #[sel4::sel4_cfg(all(ARCH_X86_64, IOMMU))] | ||
| fn init_iopt_tree( |
There was a problem hiding this comment.
I think the *_tree postfix is a bit out of place compared to similar functions like init_vspace_x86_ept() and init_vspace(). I feel like this should be called init_iospace().
| pub struct IODevice { | ||
| pub slots: Vec<CapTableEntry>, | ||
| pub domain_id: Word, | ||
| pub pci_device: (Word, Word, Word), |
There was a problem hiding this comment.
I know that it is obvious but:
| pub pci_device: (Word, Word, Word), | |
| pub pci_device: (Word, Word, Word), // bus:device.function |
for folks who aren't familliar
There was a problem hiding this comment.
Or make it into a struct
There was a problem hiding this comment.
I think this matches the capDL spec, I don't know to what extend it can differ. (might be wrong here)
There was a problem hiding this comment.
seL4/capdl#90 is the part it would have to match, and that new module is (a) not yet upstreamed and (b) doesn't have IOMMU support yet anyways. So this PR can define what that part of the spec should look like.
d237332 to
d48b9ba
Compare
| // since this is runtime information. The second is the spec assumes that all domain ids are | ||
| // valid, which is also not true since the kernel may reserve an unknown number of domain | ||
| // ids during boot. | ||
| let [cap_data] = sel4::sys::seL4_X86_IOSpace_CapData::new( |
There was a problem hiding this comment.
I suspect you might want to make this a bit nicer. Have a look at what Nick did for 'CNodeCapData' and where it is used with syscalls.
There was a problem hiding this comment.
I think the change I made with to_sel4 could easily generalise, so a macro would be cool... I didn't implement it here though.
7a6d083 to
ac6a168
Compare
nspin
left a comment
There was a problem hiding this comment.
Thanks for this. Just a few comments, mostly nits.
| pub struct IODevice { | ||
| pub slots: Vec<CapTableEntry>, | ||
| pub domain_id: Word, | ||
| pub pci_device: (Word, Word, Word), // bus:device.function |
There was a problem hiding this comment.
Could you change this to be a struct?
That would also eliminate the need for trait ToSel4.
There was a problem hiding this comment.
yep, so we don't have to match the C/haskell spec identically, which represents this as a tuple?
There was a problem hiding this comment.
@nspin, apologies it looks like you might have missed this one, we just wanted to confirm whether its necessary to match how the C/haskell implementation defined these types. (I doesn't look like there is a full implementation in the C/haskell yet...)
I agree having this as a structure would be way better if its allowed :)
There was a problem hiding this comment.
Ty for the clarification below :)
| PageTable(object::PageTable), | ||
| AsidPool(object::AsidPool), | ||
| IODevice(object::IODevice), | ||
| IOPT(object::IOPT), |
There was a problem hiding this comment.
Could you expand IOPT to IOPageTable? Just to match the convention of the surrounding code.
There was a problem hiding this comment.
Just to confirm, so rust sel4 naming doesn't need to correspond to the c/haskell capdl?
There was a problem hiding this comment.
Not the main part of the Haskell CapDL tool. Just this part:
which doesn't have IOMMU support yet. So no need to worry about matching, this PR will determine with the JSON spec looks like.
| )) | ||
| } | ||
|
|
||
| pub fn new_from_pci_tuple(domain_id: Word, pci_device: (Word, Word, Word)) -> Self { |
There was a problem hiding this comment.
Is a tuple much better than three arguments? This could be a structured type, and could be the target of a .to_sel4() method of a corresponding structured type in sel4-capdl-initializer-types.
There was a problem hiding this comment.
Yeah fair, I simplified it to the interface to just the three arg new :)
| } | ||
|
|
||
| #[sel4_cfg(IOMMU)] | ||
| pub const fn from_level_iopt(_level: usize) -> Option<Self> { |
There was a problem hiding this comment.
Could you expand this to from_level_io_page_table?
| Ok(()) | ||
| } | ||
|
|
||
| fn init_iospaces(&mut self) -> Result<()> { |
There was a problem hiding this comment.
I think it might be cleaner to #[cfg] this method and then sel4_cfg_if its invocation above, like init_sched_contexts does.
There was a problem hiding this comment.
yep, will change it for now, but in future if SMMU support, (the ARM version), is added some form of Arch ifdef will be needed...
| match dst.mint(&src, CapRights::all(), cap_data.into()) { | ||
| Ok(_) => Ok(self.orig_cap::<cap_type::IOSpace>(obj_id)), | ||
| Err(err) => { | ||
| panic!("Error: {err} when minting an x86 IOSpace capability.") | ||
| } | ||
| } |
There was a problem hiding this comment.
This might read more cleanly with unwrap_or_else(|err| ...) rather than a match.
ac6a168 to
e9d1990
Compare
e9d1990 to
f38b17a
Compare
| PageTable(object::PageTable), | ||
| AsidPool(object::AsidPool), | ||
| IODevice(object::IODevice), | ||
| IOPT(object::IOPT), |
There was a problem hiding this comment.
Not the main part of the Haskell CapDL tool. Just this part:
which doesn't have IOMMU support yet. So no need to worry about matching, this PR will determine with the JSON spec looks like.
| pub struct IODevice { | ||
| pub slots: Vec<CapTableEntry>, | ||
| pub domain_id: Word, | ||
| pub pci_device: (Word, Word, Word), // bus:device.function |
f38b17a to
9132d3e
Compare
This commit adds support for the creation of IOMMU address spaces on x86. The capdl spec for defining an IO Address Space on x86 is a root IOSpace object, which contains in slot 0 the root IOPageTable object and in slots 1.. spare IOPageTable objects. These IOPageTable objects are similar to the normal x86 page table structures. At runtime, the initialiser will calculate how many of the spare page tables must be mapped. It does this by taking the difference of what seL4 reports to be the number of page table levels and the configured number of levels we define in the x86_io_address_space module. This means that capdl enforces a fixed upper limit on the maximum IO virtual address and will fail if seL4 reports that the IOMMU supports less than this maximum. If seL4 reports support for a larger maximum IO virtual address, this design works by mapping in the spare page tables into the highest levels of the hierarchy in slot 0, until we reach the page table responsible for translating the first valid bits of the maximum IO virtual address. Signed-off-by: Callum <c.berry@student.unsw.edu.au>
9132d3e to
718c6fa
Compare
This commit adds support for the creation of IOMMU address spaces on x86.
The capdl spec for defining an IO Address Space on x86 is a root IODevice object, which contains in slot 0 the root IOPageTable object and in slots 1.. spare IOPageTable objects. These IOPageTable objects are similar to the normal x86 page table structures.
At runtime, the initialiser will calculate how many of the spare page tables must be mapped. It does this by taking the difference of what seL4 reports to be the number of page table levels and the configured number of levels we define in the x86_io_address_space module.
This means that capdl enforces a fixed upper limit on the maximum IO virtual address and will fail if seL4 reports that the IOMMU supports less than this maximum. If seL4 reports support for a larger maximum IO virtual address, this design works by mapping in the spare page tables into the highest levels of the hierarchy in slot 0, until we reach the page table responsible for translating the first valid bits of the maximum IO virtual address.