On 28.08.20 10:44, Alice Guo wrote: > Enable SMMU for processors which use MMU-500 to implement the ARM SMMU > architecture v2. > > Signed-off-by: Alice Guo <alice....@nxp.com> > --- > hypervisor/arch/arm64/Kbuild | 1 + > hypervisor/arch/arm64/include/asm/sysregs.h | 7 + > hypervisor/arch/arm64/smmu.c | 1092 +++++++++++++++++++ > include/jailhouse/cell-config.h | 15 + > include/jailhouse/sizes.h | 47 + > 5 files changed, 1162 insertions(+) > create mode 100644 hypervisor/arch/arm64/smmu.c > create mode 100644 include/jailhouse/sizes.h >
I'm far from happy with this yet, unfortunately. What makes me very unsatisfied are specifically these two points: - several of my comments on v1 of this patch where not addressed - the code contains a lot of dead stuff, copied over from Linux but now effectively unused -> review of the resulting code was not done thoroughly enough I would like to see the following meta changes in v3: - drop anything that is SMMUv1 related - dead silicon, no new designs will support it (and we will only miss support on the historic AMD Seattle - no loss) - drop support for generic SMMUv2, ie. demand ARM MMU-500 - that's what's in the field, others do not exist -> JAILHOUSE_IOMMU_ARM_MMU500 - switch __arm_smmu_tlb_sync to a simple ten-million-iterations loop, then error out, no need for the (rather questionable) optimizations of the kernel; and fold it into its only caller arm_smmu_tlb_sync_global - drop unneeded global_sync_lock - only called in synchronous context - split-off the sizes.h addition into a separate patch (I think I asked that already) - split-off the cell-config.h extensions into a separate patch - and follow my guidelines in the v1 comments on its design - describe the semantic of arm_sid_mask and how to define it - important for other targets like the zynqmp - check again all my comments on v1 and make sure they are addressed - or ask if something is unclear; specifically check error handling again, there are still cases open - check for dead code paths, specifically because feature flags aren't set, just read - also make sure we do not miss anything valuable because there is no DT telling us about certain features - make sure the coding style is not violated In addition, I have one important functional question: Why don't we need any TLB flush on cell init and exit? SMMUv3 does that (CMDQ_OP_TLBI_S12_VMALL), other IOMMUs do that as well. That looks highly suspicious. If there is no need for something like arm_smmu_tlb_sync_context in the Jailhouse use case, the reason must be documented, ideally with a comment at the end of the unit update in arm_smmu_cell_init. Jan -- You received this message because you are subscribed to the Google Groups "Jailhouse" group. To unsubscribe from this group and stop receiving emails from it, send an email to jailhouse-dev+unsubscr...@googlegroups.com. To view this discussion on the web visit https://groups.google.com/d/msgid/jailhouse-dev/4ad83901-5903-422f-6efc-b8738f88dfcf%40web.de.
