ARM System MMU v3.2 support.
Hardware overview is provided in the header of smmu.c.
Documentation is available here:
https://static.docs.arm.com/ihi0070/ca/IHI_0070_C_a_System_Memory_Management_Unit_Arm_Architecture_Specification.pdf
Details
Details
Tested on ARM Neoverse N1 SDP board.
Diff Detail
Diff Detail
- Lint
Lint Skipped - Unit
Tests Skipped
Event Timeline
There are a very large number of changes, so older changes are hidden. Show Older Changes
| sys/arm64/iommu/iommu.c | ||
|---|---|---|
| 344–345 | You appear to be mapping a fixed address. This won't work on non-N1SDP hardware as the GIC will be at a different physical address. You'll need to update the pci driver to ask the interrupt controller to map the memory when creating the iommu context, and free it later. | |
| sys/conf/files.arm64 | ||
| 214 | I think we want an iommu option to enable this. The iommu should be independent of both acpi and pci, e.g. we could use it in a system with FDT on a memory mapped bus. | |
| sys/dev/iommu/busdma_iommu.c | ||
| 881 | Why not #if !defined(__i386__) | |
| sys/dev/iommu/iommu_gas.c | ||
| 72 ↗ | (On Diff #75303) | It looks like you need these for IOMMU_PAGE_SIZE and IOMMU_PAGE_MASK. Could you create machine/iommu.h for these so we don't need #if defined(...) whenever we need these. |
| sys/arm64/iommu/iommu.c | ||
|---|---|---|
| 344–345 | Thanks, indeed. | |
Comment Actions
I’m slightly confused by code partitioning.
With that in mind that we have multiple types of IOMMUs (rockchip, tegra, smmu v2, allwinner …, and one SoC can have multiple different IOMMUs, I’m not sure how this patch fit in this situation.
Is iommu.c intended as base for future IOMMU independent part of framework? If yes, why it contain functions prefixed by smmu_? If not, why it’s named by generic name?
Do you have any roadmap, ideas how final (supporting multiple kinds of IOMMUs in one generic kernel) should be structured?
| sys/arm64/arm64/pmap.c | ||
|---|---|---|
| 3493 | Why? Do you expect that SMMU pmap can be used on PE? | |
| sys/arm64/iommu/iommu.c | ||
| 283 | Is GICv3 necessary precondition for SMMU v3? | |
| sys/dev/pci/pci_host_generic_acpi.c | ||
| 412 ↗ | (On Diff #75670) | this should be called by using common IOMMU interface - you may have different IOMMUs in one system. |
Comment Actions
iommu.c is a glue between generic iommu framework (dev/iommu) and smmu driver (we have multiple instances of SMMU in the system).
I agree it is confusing, so I renamed it to iommu_smmu.c.
We could probably reuse the glue for other arm64 IOMMU systems (at least smmuv2), but not sure about others.
(sorry for long reply I missed your comments)
| sys/arm64/arm64/pmap.c | ||
|---|---|---|
| 3493 | No, but we are doing SMMU TLB invalidation shortly after pmap_senter(). | |
| sys/arm64/iommu/iommu.c | ||
| 283 | No, I just have renamed variables | |
| sys/dev/pci/pci_host_generic_acpi.c | ||
| 412 ↗ | (On Diff #75670) | yes we are currently reworking this part |
Comment Actions
a change by andrew@:
Manage MSI IOMMU pages when allocating their irq This allows the interrupt controller driver only need a small change to create a map for the page the device will write to to raise an interrupt.
Comment Actions
I’m looking at this from perspective of presence of multiple different implementations of IOMMUs in ARM(64) world. From this point of view, I still have some problems with code partitioning, mainly in meaning of smmu_if.m.
I think that smmu_if.m should be taken as interface between system and individual IOMMU implementation. That’s mean that it should be decontaminated from using smmu (in names and/or as argument) and renamed to iommu_if.m.
The system wide function in smmu_iommu.c (like iommu_get_ctx(), iommu_find(),iommu_registe() or so) should be moved to own independent file (say iommu.c) and all HW dependent functionality should be passed by using iommu_if.m to given implementation.
I think that this not that hard change, 95% of code is already done. I, of course, don’t want to block you, I only think that some system interface should be prepared in forward.
Comment Actions
Rework the arm64 iommu interface so it could be used on other IOMMU engines in arm64 world, not only SMMU.
(Requested by mmel)
Comment Actions
In any case, with these minor issues fixed, LGTM
| sys/arm64/iommu/iommu.c | ||
|---|---|---|
| 209 | iommu can be used also for not-pci based devices. This should be reimplemented as new newbus method with pci_get_rid() as default for pci device class. | |
| 356 | should be locked by IOMMU_LIST_LOCK | |
| 377 | should be locked by IOMMU_LIST_LOCK | |
| sys/arm64/iommu/iommu.h | ||
| 42 | These constants are already defined in sys/dev/iommu/iommu_gas.h. | |
| sys/arm64/iommu/smmu.c | ||
| 742 | ste[1] is reinitialized few lines bellow. | |
| 752 | style, why '|=' ? | |
| 770 | imho, this should be documented in comment (or removed). | |
| 866 | this is not macro so why "\" at end of line ? | |
| 1231 | Style | |
Comment Actions
More style fixes:
o Rename header protection variables
o Make variables consistent:
struct smmu_unit is unit struct smmu_domain is domain struct smmu_ctx is ctx struct iommu_unit is iommu struct iommu_domain is iodom struct iommu_ctx is ioctx
o Add locking around smmu_ctx_lookup() method
o ctx_free and domain_free methods return void
o Call to iommu_gas_fini_domain() and iommu_domain_fini() on domain deinitialization (iommu_domain_free())