From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jean-Philippe Brucker Subject: Re: [RFCv2 PATCH 01/36] iommu: Keep track of processes and PASIDs Date: Thu, 9 Nov 2017 12:13:13 +0000 Message-ID: <67e7ad60-3f71-26d8-6ff6-59c95275f0e3@gmail.com> References: <20171006133203.22803-1-jean-philippe.brucker@arm.com> <20171006133203.22803-2-jean-philippe.brucker@arm.com> Mime-Version: 1.0 Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Return-path: Received: from mail-wm0-f66.google.com ([74.125.82.66]:51842 "EHLO mail-wm0-f66.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752667AbdKIMMY (ORCPT ); Thu, 9 Nov 2017 07:12:24 -0500 In-Reply-To: Content-Language: en-US Sender: linux-acpi-owner@vger.kernel.org List-Id: linux-acpi@vger.kernel.org To: Bharat Kumar Gogada , "linux-arm-kernel@lists.infradead.org" , "linux-pci@vger.kernel.org" , "linux-acpi@vger.kernel.org" , "devicetree@vger.kernel.org" , "iommu@lists.linux-foundation.org" Cc: "joro@8bytes.org" , "robh+dt@kernel.org" , Mark Rutland , Catalin Marinas , Will Deacon , Lorenzo Pieralisi , "hanjun.guo@linaro.org" , Sudeep Holla , "rjw@rjwysocki.net" , "lenb@kernel.org" , Robin Murphy , "bhelgaas@google.com" , "alex.williamson@redhat.com" , "tn@semihalf.com" , "liubo95@huawei.com" , "thunder.leizhen@huawei.com" , "xieyisheng1@huawei.com" , gabriele.paoloni@huawe 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 From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Return-Path: From: Jean-Philippe Brucker Subject: Re: [RFCv2 PATCH 01/36] iommu: Keep track of processes and PASIDs To: Bharat Kumar Gogada , "linux-arm-kernel@lists.infradead.org" , "linux-pci@vger.kernel.org" , "linux-acpi@vger.kernel.org" , "devicetree@vger.kernel.org" , "iommu@lists.linux-foundation.org" References: <20171006133203.22803-1-jean-philippe.brucker@arm.com> <20171006133203.22803-2-jean-philippe.brucker@arm.com> Message-ID: <67e7ad60-3f71-26d8-6ff6-59c95275f0e3@gmail.com> Date: Thu, 9 Nov 2017 12:13:13 +0000 MIME-Version: 1.0 In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Mark Rutland , "xieyisheng1@huawei.com" , "gabriele.paoloni@huawei.com" , Catalin Marinas , Will Deacon , "okaya@codeaurora.org" , "yi.l.liu@intel.com" , Lorenzo Pieralisi , "ashok.raj@intel.com" , "tn@semihalf.com" , "joro@8bytes.org" , "rfranz@cavium.com" , "lenb@kernel.org" , "jacob.jun.pan@linux.intel.com" , "alex.williamson@redhat.com" , "robh+dt@kernel.org" , "thunder.leizhen@huawei.com" , "bhelgaas@google.com" , "dwmw2@infradead.org" , "liubo95@huawei.com" , "rjw@rjwysocki.net" , "robdclark@gmail.com" , "hanjun.guo@linaro.org" , Sudeep Holla , Robin Murphy , "nwatters@codeaurora.org" Content-Type: text/plain; charset="us-ascii" Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+bjorn=helgaas.com@lists.infradead.org List-ID: 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 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel From mboxrd@z Thu Jan 1 00:00:00 1970 From: jphilippe.brucker@gmail.com (Jean-Philippe Brucker) Date: Thu, 9 Nov 2017 12:13:13 +0000 Subject: [RFCv2 PATCH 01/36] iommu: Keep track of processes and PASIDs In-Reply-To: References: <20171006133203.22803-1-jean-philippe.brucker@arm.com> <20171006133203.22803-2-jean-philippe.brucker@arm.com> Message-ID: <67e7ad60-3f71-26d8-6ff6-59c95275f0e3@gmail.com> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org 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