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
