Hi Bharat,

On 11/08/2017 05:50 PM, Bharat Kumar Gogada wrote:

Hi Jean,

+static struct iommu_process *
+iommu_process_alloc(struct iommu_domain *domain, struct task_struct
+*task) {
+       int err;
+       int pasid;
+       struct iommu_process *process;
+
+       if (WARN_ON(!domain->ops->process_alloc ||
!domain->ops->process_free))
+               return ERR_PTR(-ENODEV);
+
+       process = domain->ops->process_alloc(task);
+       if (IS_ERR(process))
+               return process;
+       if (!process)
+               return ERR_PTR(-ENOMEM);
+
+       process->pid            = get_task_pid(task, PIDTYPE_PID);
+       process->release        = domain->ops->process_free;
+       INIT_LIST_HEAD(&process->domains);
+       kref_init(&process->kref);
+
+       if (!process->pid) {
+               err = -EINVAL;
+               goto err_free_process;
+       }
+
+       idr_preload(GFP_KERNEL);
+       spin_lock(&iommu_process_lock);
+       pasid = idr_alloc_cyclic(&iommu_process_idr, process,
domain->min_pasid,
+                                domain->max_pasid + 1, GFP_ATOMIC);
If EP supports only one pasid; domain->min_pasid=1 and domain->max_pasid=0.
When idr_alloc_cyclic is called it invokes idr_get_free_cmn function
where we have following condition. (Based on kernel 4.14-rc6)
        if (!radix_tree_tagged(root, IDR_FREE))
                start = max(start, maxindex + 1);
              if (start > max)
                return ERR_PTR(-ENOSPC);
Here max is being assigned zero by the time this function is invoked,
this value is based on domain->max_pasid.
This condition fails and ENOSPC is returned.
       
In this case even though hardware supports PASID, BIND flow fails.

It should fail, since we're reserving PASID 0 for non-PASID transactions
with S1DSS=0b10. In addition, the SMMUv3 specification does not allow
using PASID with a single entry. See the description of S1CDMax in 5.2
Stream Table Entry:

"when this field is 0, the substreams of the STE are disabled and one CD
is available. (The minimum useful number of substreams is 2.) Any
transaction with a SubstreamID will be terminated with an abort and a
C_BAD_SUBSTREAMID event recorded."

Any reason why pasid allocation moved to idr allocations rather than
bitmap allocations as in v1 patches ?

Yes, idr provides a convenient way to quickly retrieve the context
associated with a PASID, when handling a fault. v1 had the allocation in a
bitmap and storing in a rb-tree. By using an idr we combine both and rely
on a well-tested infrastructure.

Note that in the future we might need to go back to handcrafting the PASID
allocation, but it will probably still be based on idr.

Thanks,
Jean