On Wed, Apr 30, 2025 at 03:39:22PM -0700, Nicolin Chen wrote:
On Wed, Apr 30, 2025 at 09:59:13PM +0000, Pranjal Shrivastava wrote:
enum iommu_viommu_type { IOMMU_VIOMMU_TYPE_DEFAULT = 0, IOMMU_VIOMMU_TYPE_ARM_SMMUV3 = 1,
- IOMMU_VIOMMU_TYPE_TEGRA241_CMDQV = 2,
+};
This is a little confusing.. I understand that we need a new viommu type to copy the new struct iommu_viommu_tegra241_cmdqv b/w the user & kernel
But, in a previous patch (Add vsmmu_alloc impl op), we add a check to fallback to the standard type SMMUv3, if the impl_ops->vsmmu_alloc returns -EOPNOTSUPP:
if (master->smmu->impl_ops && master->smmu->impl_ops->vsmmu_alloc) vsmmu = master->smmu->impl_ops->vsmmu_alloc( master->smmu, s2_parent, ictx, viommu_type, user_data); if (PTR_ERR(vsmmu) == -EOPNOTSUPP) { if (viommu_type != IOMMU_VIOMMU_TYPE_ARM_SMMUV3) return ERR_PTR(-EOPNOTSUPP); /* Fallback to standard SMMUv3 type if viommu_type matches */ vsmmu = iommufd_viommu_alloc(ictx, struct arm_vsmmu, core, &arm_vsmmu_ops);
Now, if we'll ALWAYS try to allocate an impl-specified vsmmu first, even when the viommu_type == IOMMU_VIOMMU_TYPE_ARM_SMMUV3, we are anyways going to return back from the impl_ops->vsmmu_alloc with -EOPNOTSUPP.
That's not necessarily true. An impl_ops->vsmmu_alloc can support IOMMU_VIOMMU_TYPE_ARM_SMMUV3 potentially, e.g. an impl could just toggle a few special bits in a register and return a valid vsmmu pointer.
It doesn't work like this with VCMDQ as it supports its own type, but for the long run I think we should pass in the standard type to impl_ops->vsmmu_alloc too.
Then we'll again check if the retval was -EOPNOTSUPP and re-check the viommu_type requested.. which seems a little counter intuitive.
It's just prioritizing the impl_ops->vsmmu_alloc. Similar to the probe, if VCMDQ is missing or encountering some initialization problem, give it a chance to fallback to the standard SMMU.
I changed to this and it should be clear now:
+ /* Prioritize the impl that may support IOMMU_VIOMMU_TYPE_ARM_SMMUV3 */ + if (master->smmu->impl_ops && master->smmu->impl_ops->vsmmu_alloc) + vsmmu = master->smmu->impl_ops->vsmmu_alloc( + master->smmu, s2_parent, ictx, viommu_type, user_data); + if (PTR_ERR(vsmmu) == -EOPNOTSUPP) { + /* Otherwise, allocate an IOMMU_VIOMMU_TYPE_ARM_SMMUV3 here */ + if (viommu_type == IOMMU_VIOMMU_TYPE_ARM_SMMUV3) + vsmmu = iommufd_viommu_alloc(ictx, struct arm_vsmmu, + core, &arm_vsmmu_ops);
Thanks Nicolin