Page MenuHomeFreeBSD

ARM SMMU v3.2 support
ClosedPublic

Authored by br on Apr 29 2020, 4:05 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sat, Mar 16, 4:09 PM
Unknown Object (File)
Sat, Mar 16, 2:20 PM
Unknown Object (File)
Sat, Mar 16, 2:20 PM
Unknown Object (File)
Sat, Mar 16, 2:20 PM
Unknown Object (File)
Sat, Mar 16, 2:20 PM
Unknown Object (File)
Sat, Mar 16, 2:20 PM
Unknown Object (File)
Sat, Mar 16, 2:20 PM
Unknown Object (File)
Sat, Mar 16, 2:19 PM

Details

Summary

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

Test Plan

Tested on ARM Neoverse N1 SDP board.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
br marked an inline comment as done.

Regenerate with acpi code committed

br edited the summary of this revision. (Show Details)
br edited the test plan for this revision. (Show Details)

Regenerate with latest changes to dev/iommu

br edited the summary of this revision. (Show Details)

remove empty line

sys/arm64/iommu/iommu.c
343–344 ↗(On Diff #75303)

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
213 ↗(On Diff #75303)

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
889 ↗(On Diff #75303)

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.

Query the GIC page from the ITS driver using MSI_MAP, instead of hardcoding

sys/arm64/iommu/iommu.c
343–344 ↗(On Diff #75303)

Thanks, indeed.
We create an iommu context not in the PCI driver, but later a bit, when a PCI peripheral requests bus_dma tag, so I have slightly different solution using intr_map_msi().
please check

br marked an inline comment as done.

allow irq == 0 in map_msi()

br marked an inline comment as done.

device iommu added

br marked an inline comment as done.Aug 6 2020, 1:38 PM
br added inline comments.
sys/conf/files.arm64
213 ↗(On Diff #75303)

Ok

sys/dev/iommu/busdma_iommu.c
889 ↗(On Diff #75303)

Thanks. Fixed.

sys/dev/iommu/iommu_gas.c
72 ↗(On Diff #75303)

Done.

br marked an inline comment as done.

smmu_map_msi() added

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 ↗(On Diff #75670)

Why? Do you expect that SMMU pmap can be used on PE?

sys/arm64/iommu/iommu.c
282 ↗(On Diff #75670)

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.

Add quirk: iommu set to buswide for Marvell AHCI

In D24618#583893, @mmel wrote:

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?

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 ↗(On Diff #75670)

No, but we are doing SMMU TLB invalidation shortly after pmap_senter().

sys/arm64/iommu/iommu.c
282 ↗(On Diff #75670)

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

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.

The final patch to review

Rename iommu_smmu.c to smmu_iommu.c

Set domain.end address (max guest addr + 1) to the VM_MAXUSER_ADDR

update copyright for smmu_quircks.c

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.

Rework the arm64 iommu interface so it could be used on other IOMMU engines in arm64 world, not only SMMU.
(Requested by mmel)

In any case, with these minor issues fixed, LGTM

sys/arm64/iommu/iommu.c
208 ↗(On Diff #79434)

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.
However, this can be done as a subsequent step, so please only add a comment to document this fact.

355 ↗(On Diff #79434)

should be locked by IOMMU_LIST_LOCK

376 ↗(On Diff #79434)

should be locked by IOMMU_LIST_LOCK

sys/arm64/iommu/iommu.h
41 ↗(On Diff #79434)

These constants are already defined in sys/dev/iommu/iommu_gas.h.
You should include it in this header.

sys/arm64/iommu/smmu.c
741 ↗(On Diff #79434)

ste[1] is reinitialized few lines bellow.

751 ↗(On Diff #79434)

style, why '|=' ?

769 ↗(On Diff #79434)

imho, this should be documented in comment (or removed).

865 ↗(On Diff #79434)

this is not macro so why "\" at end of line ?

1230 ↗(On Diff #79434)

Style

This revision is now accepted and ready to land.Nov 15 2020, 10:22 AM

Address mmel's comments.

This revision now requires review to proceed.Nov 16 2020, 12:03 AM

Perfect. Many thanks for cooperation.

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

This revision was not accepted when it landed; it landed in state Needs Review.Nov 16 2020, 9:56 PM
This revision was automatically updated to reflect the committed changes.