From mboxrd@z Thu Jan 1 00:00:00 1970 From: okaya@codeaurora.org Subject: Re: [RFCv2 PATCH 05/36] iommu/process: Bind and unbind process to and from devices Date: Fri, 19 Jan 2018 08:07:21 -0500 Message-ID: References: <20171006133203.22803-1-jean-philippe.brucker@arm.com> <20171006133203.22803-6-jean-philippe.brucker@arm.com> <0772e71e-4861-1e7b-f248-88aaba8bf2fc@codeaurora.org> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII; format=flowed Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: Sender: linux-pci-owner@vger.kernel.org To: Jean-Philippe Brucker Cc: 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, 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@huawei.com, nwatters@codeaurora.org, rfranz@cavium.com, dwmw2@infradead.org, jacob.jun.pan@linux.intel.com, yi.l.liu@intel.com, ashok. List-Id: linux-acpi@vger.kernel.org On 2018-01-19 05:27, Jean-Philippe Brucker wrote: > Hi Sinan, > > On 19/01/18 04:52, Sinan Kaya wrote: >> Hi Jean-Philippe, >> >> On 10/6/2017 9:31 AM, Jean-Philippe Brucker wrote: >>> /** >>> + * iommu_process_bind_device - Bind a process address space to a >>> device >>> + * @dev: the device >>> + * @task: the process to bind >>> + * @pasid: valid address where the PASID will be stored >>> + * @flags: bond properties (IOMMU_PROCESS_BIND_*) >>> + * >>> + * Create a bond between device and task, allowing the device to >>> access the >>> + * process address space using the returned PASID. >>> + * >>> + * On success, 0 is returned and @pasid contains a valid ID. >>> Otherwise, an error >>> + * is returned. >>> + */ >>> +int iommu_process_bind_device(struct device *dev, struct task_struct >>> *task, >>> + int *pasid, int flags) >> >> This API doesn't play nice with endpoint device drivers that have >> PASID limitations. >> >> The AMD driver seems to have PASID limitations per product that are >> not being >> advertised in the PCI capability. >> >> device_iommu_pasid_init() >> { >> pasid_limit = min_t(unsigned int, >> (unsigned int)(1 << >> kfd->device_info->max_pasid_bits), >> iommu_info.max_pasids); >> /* >> * last pasid is used for kernel queues doorbells >> * in the future the last pasid might be used for a kernel >> thread. >> */ >> pasid_limit = min_t(unsigned int, >> pasid_limit, >> kfd->doorbell_process_limit - 1); >> } >> >> kfd->device_info->max_pasid_bits seems to contain per device >> limitations. >> >> Would you be willing to extend the API so that the requester can >> impose some limit >> on the PASID value that is getting allocated. > > Good point. Following the feedback for this series, next version adds > another public function: > > int iommu_sva_device_init(struct device *dev, int features); > > that has to be called by the device driver before any bind(). The > intent > is to let some IOMMU drivers initialize PASID tables and other features > lazily, only if the device driver actually intends to use them. Maybe I > could change this function to: > > int iommu_sva_device_init(struct device *dev, int features, unsigned > int > max_pasid); > > @features is a bitmask telling what the device driver needs (PASID > and/or > page faults). If features has IOMMU_SVA_FEAT_PASID set, then device > driver > can set a max_pasid limit, that we'd store in our private device-iommu > data. If max_pasid is 0, then we'd use the PCI limit. Yes, this should work. > > Thanks, > Jean From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Return-Path: MIME-Version: 1.0 Date: Fri, 19 Jan 2018 08:07:21 -0500 From: okaya@codeaurora.org To: Jean-Philippe Brucker Subject: Re: [RFCv2 PATCH 05/36] iommu/process: Bind and unbind process to and from devices In-Reply-To: References: <20171006133203.22803-1-jean-philippe.brucker@arm.com> <20171006133203.22803-6-jean-philippe.brucker@arm.com> <0772e71e-4861-1e7b-f248-88aaba8bf2fc@codeaurora.org> Message-ID: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Mark Rutland , xieyisheng1@huawei.com, gabriele.paoloni@huawei.com, linux-pci@vger.kernel.org, Will Deacon , yi.l.liu@intel.com, Lorenzo Pieralisi , ashok.raj@intel.com, tn@semihalf.com, joro@8bytes.org, robdclark@gmail.com, linux-acpi@vger.kernel.org, Catalin Marinas , rfranz@cavium.com, lenb@kernel.org, devicetree@vger.kernel.org, jacob.jun.pan@linux.intel.com, alex.williamson@redhat.com, robh+dt@kernel.org, thunder.leizhen@huawei.com, bhelgaas@google.com, linux-arm-kernel@lists.infradead.org, dwmw2@infradead.org, liubo95@huawei.com, rjw@rjwysocki.net, iommu@lists.linux-foundation.org, hanjun.guo@linaro.org, Sudeep Holla , Robin Murphy , linux-pci-owner@vger.kernel.org, nwatters@codeaurora.org Content-Type: text/plain; charset="us-ascii"; Format="flowed" Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+bjorn=helgaas.com@lists.infradead.org List-ID: On 2018-01-19 05:27, Jean-Philippe Brucker wrote: > Hi Sinan, > > On 19/01/18 04:52, Sinan Kaya wrote: >> Hi Jean-Philippe, >> >> On 10/6/2017 9:31 AM, Jean-Philippe Brucker wrote: >>> /** >>> + * iommu_process_bind_device - Bind a process address space to a >>> device >>> + * @dev: the device >>> + * @task: the process to bind >>> + * @pasid: valid address where the PASID will be stored >>> + * @flags: bond properties (IOMMU_PROCESS_BIND_*) >>> + * >>> + * Create a bond between device and task, allowing the device to >>> access the >>> + * process address space using the returned PASID. >>> + * >>> + * On success, 0 is returned and @pasid contains a valid ID. >>> Otherwise, an error >>> + * is returned. >>> + */ >>> +int iommu_process_bind_device(struct device *dev, struct task_struct >>> *task, >>> + int *pasid, int flags) >> >> This API doesn't play nice with endpoint device drivers that have >> PASID limitations. >> >> The AMD driver seems to have PASID limitations per product that are >> not being >> advertised in the PCI capability. >> >> device_iommu_pasid_init() >> { >> pasid_limit = min_t(unsigned int, >> (unsigned int)(1 << >> kfd->device_info->max_pasid_bits), >> iommu_info.max_pasids); >> /* >> * last pasid is used for kernel queues doorbells >> * in the future the last pasid might be used for a kernel >> thread. >> */ >> pasid_limit = min_t(unsigned int, >> pasid_limit, >> kfd->doorbell_process_limit - 1); >> } >> >> kfd->device_info->max_pasid_bits seems to contain per device >> limitations. >> >> Would you be willing to extend the API so that the requester can >> impose some limit >> on the PASID value that is getting allocated. > > Good point. Following the feedback for this series, next version adds > another public function: > > int iommu_sva_device_init(struct device *dev, int features); > > that has to be called by the device driver before any bind(). The > intent > is to let some IOMMU drivers initialize PASID tables and other features > lazily, only if the device driver actually intends to use them. Maybe I > could change this function to: > > int iommu_sva_device_init(struct device *dev, int features, unsigned > int > max_pasid); > > @features is a bitmask telling what the device driver needs (PASID > and/or > page faults). If features has IOMMU_SVA_FEAT_PASID set, then device > driver > can set a max_pasid limit, that we'd store in our private device-iommu > data. If max_pasid is 0, then we'd use the PCI limit. Yes, this should work. > > 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: okaya@codeaurora.org (okaya at codeaurora.org) Date: Fri, 19 Jan 2018 08:07:21 -0500 Subject: [RFCv2 PATCH 05/36] iommu/process: Bind and unbind process to and from devices In-Reply-To: References: <20171006133203.22803-1-jean-philippe.brucker@arm.com> <20171006133203.22803-6-jean-philippe.brucker@arm.com> <0772e71e-4861-1e7b-f248-88aaba8bf2fc@codeaurora.org> Message-ID: To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On 2018-01-19 05:27, Jean-Philippe Brucker wrote: > Hi Sinan, > > On 19/01/18 04:52, Sinan Kaya wrote: >> Hi Jean-Philippe, >> >> On 10/6/2017 9:31 AM, Jean-Philippe Brucker wrote: >>> /** >>> + * iommu_process_bind_device - Bind a process address space to a >>> device >>> + * @dev: the device >>> + * @task: the process to bind >>> + * @pasid: valid address where the PASID will be stored >>> + * @flags: bond properties (IOMMU_PROCESS_BIND_*) >>> + * >>> + * Create a bond between device and task, allowing the device to >>> access the >>> + * process address space using the returned PASID. >>> + * >>> + * On success, 0 is returned and @pasid contains a valid ID. >>> Otherwise, an error >>> + * is returned. >>> + */ >>> +int iommu_process_bind_device(struct device *dev, struct task_struct >>> *task, >>> + int *pasid, int flags) >> >> This API doesn't play nice with endpoint device drivers that have >> PASID limitations. >> >> The AMD driver seems to have PASID limitations per product that are >> not being >> advertised in the PCI capability. >> >> device_iommu_pasid_init() >> { >> pasid_limit = min_t(unsigned int, >> (unsigned int)(1 << >> kfd->device_info->max_pasid_bits), >> iommu_info.max_pasids); >> /* >> * last pasid is used for kernel queues doorbells >> * in the future the last pasid might be used for a kernel >> thread. >> */ >> pasid_limit = min_t(unsigned int, >> pasid_limit, >> kfd->doorbell_process_limit - 1); >> } >> >> kfd->device_info->max_pasid_bits seems to contain per device >> limitations. >> >> Would you be willing to extend the API so that the requester can >> impose some limit >> on the PASID value that is getting allocated. > > Good point. Following the feedback for this series, next version adds > another public function: > > int iommu_sva_device_init(struct device *dev, int features); > > that has to be called by the device driver before any bind(). The > intent > is to let some IOMMU drivers initialize PASID tables and other features > lazily, only if the device driver actually intends to use them. Maybe I > could change this function to: > > int iommu_sva_device_init(struct device *dev, int features, unsigned > int > max_pasid); > > @features is a bitmask telling what the device driver needs (PASID > and/or > page faults). If features has IOMMU_SVA_FEAT_PASID set, then device > driver > can set a max_pasid limit, that we'd store in our private device-iommu > data. If max_pasid is 0, then we'd use the PCI limit. Yes, this should work. > > Thanks, > Jean