On Thu, Aug 15, 2019 at 01:09:07PM +0100, Robin Murphy wrote: > On 15/08/2019 11:56, Will Deacon wrote: > >On Fri, Aug 09, 2019 at 06:07:52PM +0100, Robin Murphy wrote: > >>Allocating and initialising a context for a domain is another point > >>where certain implementations are known to want special behaviour. > >>Currently the other half of the Cavium workaround comes into play here, > >>so let's finish the job to get the whole thing right out of the way. > >> > >>Signed-off-by: Robin Murphy <[email protected]> > >>--- > >> drivers/iommu/arm-smmu-impl.c | 39 +++++++++++++++++++++++++-- > >> drivers/iommu/arm-smmu.c | 51 +++++++---------------------------- > >> drivers/iommu/arm-smmu.h | 42 +++++++++++++++++++++++++++-- > >> 3 files changed, 86 insertions(+), 46 deletions(-) > >> > >>diff --git a/drivers/iommu/arm-smmu-impl.c b/drivers/iommu/arm-smmu-impl.c > >>index c8904da08354..7a657d47b6ec 100644 > >>--- a/drivers/iommu/arm-smmu-impl.c > >>+++ b/drivers/iommu/arm-smmu-impl.c > >>@@ -48,6 +48,12 @@ const struct arm_smmu_impl calxeda_impl = { > >> }; > >>+struct cavium_smmu { > >>+ struct arm_smmu_device smmu; > >>+ u32 id_base; > >>+}; > >>+#define to_csmmu(s) container_of(s, struct cavium_smmu, smmu) > > > >To be honest with you, I'd just use container_of directly for the two > >callsites that need it. "to_csmmu" isn't a great name when we're also got > >the calxeda thing in here. > > Sure, by this point I was mostly just going for completeness in terms of > sketching out an example for subclassing arm_smmu_device. The Tegra patches > will now serve as a more complete example anyway, so indeed we can live > without it here. > > >> static int cavium_cfg_probe(struct arm_smmu_device *smmu) > >> { > >> static atomic_t context_count = ATOMIC_INIT(0); > >>@@ -56,17 +62,46 @@ static int cavium_cfg_probe(struct arm_smmu_device > >>*smmu) > >> * Ensure ASID and VMID allocation is unique across all SMMUs in > >> * the system. > >> */ > >>- smmu->cavium_id_base = atomic_fetch_add(smmu->num_context_banks, > >>+ to_csmmu(smmu)->id_base = atomic_fetch_add(smmu->num_context_banks, > >> &context_count); > >> dev_notice(smmu->dev, "\tenabling workaround for Cavium erratum > >> 27704\n"); > >> return 0; > >> } > >>+int cavium_init_context(struct arm_smmu_domain *smmu_domain) > >>+{ > >>+ u32 id_base = to_csmmu(smmu_domain->smmu)->id_base; > >>+ > >>+ if (smmu_domain->stage == ARM_SMMU_DOMAIN_S2) > >>+ smmu_domain->cfg.vmid += id_base; > >>+ else > >>+ smmu_domain->cfg.asid += id_base; > >>+ > >>+ return 0; > >>+} > >>+ > >> const struct arm_smmu_impl cavium_impl = { > >> .cfg_probe = cavium_cfg_probe, > >>+ .init_context = cavium_init_context, > >> }; > >>+struct arm_smmu_device *cavium_smmu_impl_init(struct arm_smmu_device *smmu) > >>+{ > >>+ struct cavium_smmu *csmmu; > >>+ > >>+ csmmu = devm_kzalloc(smmu->dev, sizeof(*csmmu), GFP_KERNEL); > >>+ if (!csmmu) > >>+ return ERR_PTR(-ENOMEM); > >>+ > >>+ csmmu->smmu = *smmu; > >>+ csmmu->smmu.impl = &cavium_impl; > >>+ > >>+ devm_kfree(smmu->dev, smmu); > >>+ > >>+ return &csmmu->smmu; > >>+} > >>+ > >> #define ARM_MMU500_ACTLR_CPRE (1 << 1) > >>@@ -121,7 +156,7 @@ struct arm_smmu_device *arm_smmu_impl_init(struct > >>arm_smmu_device *smmu) > >> smmu->impl = &calxeda_impl; > >> if (smmu->model == CAVIUM_SMMUV2) > >>- smmu->impl = &cavium_impl; > >>+ return cavium_smmu_impl_init(smmu); > >> if (smmu->model == ARM_MMU500) > >> smmu->impl = &arm_mmu500_impl; > > > >Maybe rework this so we do the calxeda detection first (and return if we > >match), followed by a switch on smmu->model to make it crystal clear that > >we match only one? > > As I see it, "match only one" is really only a short-term thing, though, so > I didn't want to get *too* hung up on it. Ultimately we're going to have > cases where we need to combine e.g. MMU-500 implementation quirks with > platform integration quirks - I've been mostly planning on coming back to > think about that (and potentially rework this whole logic) later, but I > guess it wouldn't hurt to plan out a bit more structure from the start.
I was going to ask something similar. I'm guessing that the intent is that we'll eventually we'll have a couple of arm-smmu-<vendor>.c files and we'll need some sort of centralized place to set up the smmu->impl pointer. I had figured that it would be table based or something, but you make a good point about mixing and matching different workarounds. I don't really have a solution, just something I'm pondering while I'm thinking about how to start merging some of the qcom stuff into this. Jordan -- The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project _______________________________________________ iommu mailing list [email protected] https://lists.linuxfoundation.org/mailman/listinfo/iommu
