From mboxrd@z Thu Jan 1 00:00:00 1970 From: =?UTF-8?Q?Christian_K=c3=b6nig?= Subject: Re: [PATCH v2 01/40] iommu: Introduce Shared Virtual Addressing API Date: Thu, 6 Sep 2018 13:12:42 +0200 Message-ID: References: <20180511190641.23008-1-jean-philippe.brucker@arm.com> <20180511190641.23008-2-jean-philippe.brucker@arm.com> <03d31ba5-1eda-ea86-8c0c-91d14c86fe83@arm.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <03d31ba5-1eda-ea86-8c0c-91d14c86fe83-5wv7dgnIgG8@public.gmane.org> Content-Language: en-US List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: iommu-bounces-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org Errors-To: iommu-bounces-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org To: Jean-Philippe Brucker , Auger Eric , linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org, linux-pci-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-acpi-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org, kvm-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-mm-Bw31MaZKKs3YtjvyW6yDsg@public.gmane.org Cc: xieyisheng1-hv44wF8Li93QT0dZR+AlfA@public.gmane.org, ilias.apalodimas-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org, will.deacon-5wv7dgnIgG8@public.gmane.org, okaya-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org, ashok.raj-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org, rfranz-YGCgFSpz5w/QT0dZR+AlfA@public.gmane.org, kevin.tian-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org, alex.williamson-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org, rgummal-gjFFaj9aHVfQT0dZR+AlfA@public.gmane.org, dwmw2-wEGCiKHe2LqWVfeAwA7xHQ@public.gmane.org, liubo95-hv44wF8Li93QT0dZR+AlfA@public.gmane.org, robin.murphy-5wv7dgnIgG8@public.gmane.org List-Id: linux-acpi@vger.kernel.org Am 06.09.2018 um 13:09 schrieb Jean-Philippe Brucker: > Hi Eric, > > Thanks for reviewing > > On 05/09/2018 12:29, Auger Eric wrote: >>> +int iommu_sva_device_init(struct device *dev, unsigned long features, >>> + unsigned int max_pasid) >> what about min_pasid? > No one asked for it... The max_pasid parameter is here for drivers that > have vendor-specific PASID size limits, such as AMD KFD (see > kfd_iommu_device_init and > https://patchwork.kernel.org/patch/9989307/#21389571). But in most cases > the PASID size will only depend on the PCI PASID capability and the > IOMMU limits, both known by the IOMMU driver, so device drivers won't > have to set max_pasid. > > IOMMU drivers need to set min_pasid in the sva_device_init callback > because it may be either 1 (e.g. Arm where PASID #0 is reserved) or 0 > (Intel Vt-d rev2), but at the moment I can't see a reason for device > drivers to override min_pasid Sorry to ruin your day, but if I'm not completely mistaken PASID zero is reserved in the AMD KFD as well. Regards, Christian. > >>> + /* >>> + * IOMMU driver updates the limits depending on the IOMMU and device >>> + * capabilities. >>> + */ >>> + ret = domain->ops->sva_device_init(dev, param); >>> + if (ret) >>> + goto err_free_param; >> So you are likely to call sva_device_init even if it was already called >> (ie. dev->iommu_param->sva_param is already set). Can't you test whether >> dev->iommu_param->sva_param is already set first? > Right, that's probably better > >>> +/** >>> + * iommu_sva_device_shutdown() - Shutdown Shared Virtual Addressing for a device >>> + * @dev: the device >>> + * >>> + * Disable SVA. Device driver should ensure that the device isn't performing any >>> + * DMA while this function is running. >>> + */ >>> +int iommu_sva_device_shutdown(struct device *dev) >> Not sure the returned value is required for a shutdown operation. > I don't know either. I like them because they help me debug, but are > otherwise rather useless if we don't describe precise semantics. The > caller cannot do anything with it. Given that the corresponding IOMMU op > is already void, I can change this function to void as well > >>> +struct iommu_sva_param { >> What are the feature values? > At the moment only IOMMU_SVA_FEAT_IOPF, introduced by patch 09 > >>> + unsigned long features; >>> + unsigned int min_pasid; >>> + unsigned int max_pasid; >>> +}; >>> + >>> /** >>> * struct iommu_ops - iommu ops and capabilities >>> * @capable: check capability >>> @@ -219,6 +225,8 @@ struct page_response_msg { >>> * @domain_free: free iommu domain >>> * @attach_dev: attach device to an iommu domain >>> * @detach_dev: detach device from an iommu domain >>> + * @sva_device_init: initialize Shared Virtual Adressing for a device >> Addressing >>> + * @sva_device_shutdown: shutdown Shared Virtual Adressing for a device >> Addressing > Nice catch > > Thanks, > Jean From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Return-Path: Subject: Re: [PATCH v2 01/40] iommu: Introduce Shared Virtual Addressing API To: Jean-Philippe Brucker , Auger Eric , 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, kvm@vger.kernel.org, linux-mm@kvack.org References: <20180511190641.23008-1-jean-philippe.brucker@arm.com> <20180511190641.23008-2-jean-philippe.brucker@arm.com> <03d31ba5-1eda-ea86-8c0c-91d14c86fe83@arm.com> From: =?UTF-8?Q?Christian_K=c3=b6nig?= Message-ID: Date: Thu, 6 Sep 2018 13:12:42 +0200 MIME-Version: 1.0 In-Reply-To: <03d31ba5-1eda-ea86-8c0c-91d14c86fe83@arm.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: xieyisheng1@huawei.com, ilias.apalodimas@linaro.org, xuzaibo@huawei.com, thunder.leizhen@huawei.com, will.deacon@arm.com, okaya@codeaurora.org, yi.l.liu@intel.com, ashok.raj@intel.com, tn@semihalf.com, joro@8bytes.org, bharatku@xilinx.com, liudongdong3@huawei.com, rfranz@cavium.com, kevin.tian@intel.com, jacob.jun.pan@linux.intel.com, alex.williamson@redhat.com, rgummal@xilinx.com, jonathan.cameron@huawei.com, shunyong.yang@hxt-semitech.com, dwmw2@infradead.org, liubo95@huawei.com, jcrouse@codeaurora.org, robdclark@gmail.com, robin.murphy@arm.com, nwatters@codeaurora.org, baolu.lu@linux.intel.com 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: Am 06.09.2018 um 13:09 schrieb Jean-Philippe Brucker: > Hi Eric, > > Thanks for reviewing > > On 05/09/2018 12:29, Auger Eric wrote: >>> +int iommu_sva_device_init(struct device *dev, unsigned long features, >>> + unsigned int max_pasid) >> what about min_pasid? > No one asked for it... The max_pasid parameter is here for drivers that > have vendor-specific PASID size limits, such as AMD KFD (see > kfd_iommu_device_init and > https://patchwork.kernel.org/patch/9989307/#21389571). But in most cases > the PASID size will only depend on the PCI PASID capability and the > IOMMU limits, both known by the IOMMU driver, so device drivers won't > have to set max_pasid. > > IOMMU drivers need to set min_pasid in the sva_device_init callback > because it may be either 1 (e.g. Arm where PASID #0 is reserved) or 0 > (Intel Vt-d rev2), but at the moment I can't see a reason for device > drivers to override min_pasid Sorry to ruin your day, but if I'm not completely mistaken PASID zero is reserved in the AMD KFD as well. Regards, Christian. > >>> + /* >>> + * IOMMU driver updates the limits depending on the IOMMU and device >>> + * capabilities. >>> + */ >>> + ret = domain->ops->sva_device_init(dev, param); >>> + if (ret) >>> + goto err_free_param; >> So you are likely to call sva_device_init even if it was already called >> (ie. dev->iommu_param->sva_param is already set). Can't you test whether >> dev->iommu_param->sva_param is already set first? > Right, that's probably better > >>> +/** >>> + * iommu_sva_device_shutdown() - Shutdown Shared Virtual Addressing for a device >>> + * @dev: the device >>> + * >>> + * Disable SVA. Device driver should ensure that the device isn't performing any >>> + * DMA while this function is running. >>> + */ >>> +int iommu_sva_device_shutdown(struct device *dev) >> Not sure the returned value is required for a shutdown operation. > I don't know either. I like them because they help me debug, but are > otherwise rather useless if we don't describe precise semantics. The > caller cannot do anything with it. Given that the corresponding IOMMU op > is already void, I can change this function to void as well > >>> +struct iommu_sva_param { >> What are the feature values? > At the moment only IOMMU_SVA_FEAT_IOPF, introduced by patch 09 > >>> + unsigned long features; >>> + unsigned int min_pasid; >>> + unsigned int max_pasid; >>> +}; >>> + >>> /** >>> * struct iommu_ops - iommu ops and capabilities >>> * @capable: check capability >>> @@ -219,6 +225,8 @@ struct page_response_msg { >>> * @domain_free: free iommu domain >>> * @attach_dev: attach device to an iommu domain >>> * @detach_dev: detach device from an iommu domain >>> + * @sva_device_init: initialize Shared Virtual Adressing for a device >> Addressing >>> + * @sva_device_shutdown: shutdown Shared Virtual Adressing for a device >> Addressing > Nice catch > > 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 Return-Path: Received: from mail-pg1-f200.google.com (mail-pg1-f200.google.com [209.85.215.200]) by kanga.kvack.org (Postfix) with ESMTP id 35D276B7872 for ; Thu, 6 Sep 2018 07:13:03 -0400 (EDT) Received: by mail-pg1-f200.google.com with SMTP id f32-v6so5390724pgm.14 for ; Thu, 06 Sep 2018 04:13:03 -0700 (PDT) Received: from NAM05-DM3-obe.outbound.protection.outlook.com (mail-eopbgr730047.outbound.protection.outlook.com. [40.107.73.47]) by mx.google.com with ESMTPS id gn12si4610240plb.147.2018.09.06.04.13.01 for (version=TLS1_2 cipher=ECDHE-RSA-AES128-SHA bits=128/128); Thu, 06 Sep 2018 04:13:02 -0700 (PDT) Subject: Re: [PATCH v2 01/40] iommu: Introduce Shared Virtual Addressing API References: <20180511190641.23008-1-jean-philippe.brucker@arm.com> <20180511190641.23008-2-jean-philippe.brucker@arm.com> <03d31ba5-1eda-ea86-8c0c-91d14c86fe83@arm.com> From: =?UTF-8?Q?Christian_K=c3=b6nig?= Message-ID: Date: Thu, 6 Sep 2018 13:12:42 +0200 MIME-Version: 1.0 In-Reply-To: <03d31ba5-1eda-ea86-8c0c-91d14c86fe83@arm.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Content-Language: en-US Sender: owner-linux-mm@kvack.org List-ID: To: Jean-Philippe Brucker , Auger Eric , 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, kvm@vger.kernel.org, linux-mm@kvack.org Cc: xieyisheng1@huawei.com, liubo95@huawei.com, xuzaibo@huawei.com, thunder.leizhen@huawei.com, will.deacon@arm.com, okaya@codeaurora.org, yi.l.liu@intel.com, ashok.raj@intel.com, tn@semihalf.com, joro@8bytes.org, bharatku@xilinx.com, liudongdong3@huawei.com, rfranz@cavium.com, kevin.tian@intel.com, jacob.jun.pan@linux.intel.com, jcrouse@codeaurora.org, rgummal@xilinx.com, jonathan.cameron@huawei.com, shunyong.yang@hxt-semitech.com, robin.murphy@arm.com, ilias.apalodimas@linaro.org, alex.williamson@redhat.com, robdclark@gmail.com, dwmw2@infradead.org, nwatters@codeaurora.org, baolu.lu@linux.intel.com Am 06.09.2018 um 13:09 schrieb Jean-Philippe Brucker: > Hi Eric, > > Thanks for reviewing > > On 05/09/2018 12:29, Auger Eric wrote: >>> +int iommu_sva_device_init(struct device *dev, unsigned long features, >>> + unsigned int max_pasid) >> what about min_pasid? > No one asked for it... The max_pasid parameter is here for drivers that > have vendor-specific PASID size limits, such as AMD KFD (see > kfd_iommu_device_init and > https://patchwork.kernel.org/patch/9989307/#21389571). But in most cases > the PASID size will only depend on the PCI PASID capability and the > IOMMU limits, both known by the IOMMU driver, so device drivers won't > have to set max_pasid. > > IOMMU drivers need to set min_pasid in the sva_device_init callback > because it may be either 1 (e.g. Arm where PASID #0 is reserved) or 0 > (Intel Vt-d rev2), but at the moment I can't see a reason for device > drivers to override min_pasid Sorry to ruin your day, but if I'm not completely mistaken PASID zero is reserved in the AMD KFD as well. Regards, Christian. > >>> + /* >>> + * IOMMU driver updates the limits depending on the IOMMU and device >>> + * capabilities. >>> + */ >>> + ret = domain->ops->sva_device_init(dev, param); >>> + if (ret) >>> + goto err_free_param; >> So you are likely to call sva_device_init even if it was already called >> (ie. dev->iommu_param->sva_param is already set). Can't you test whether >> dev->iommu_param->sva_param is already set first? > Right, that's probably better > >>> +/** >>> + * iommu_sva_device_shutdown() - Shutdown Shared Virtual Addressing for a device >>> + * @dev: the device >>> + * >>> + * Disable SVA. Device driver should ensure that the device isn't performing any >>> + * DMA while this function is running. >>> + */ >>> +int iommu_sva_device_shutdown(struct device *dev) >> Not sure the returned value is required for a shutdown operation. > I don't know either. I like them because they help me debug, but are > otherwise rather useless if we don't describe precise semantics. The > caller cannot do anything with it. Given that the corresponding IOMMU op > is already void, I can change this function to void as well > >>> +struct iommu_sva_param { >> What are the feature values? > At the moment only IOMMU_SVA_FEAT_IOPF, introduced by patch 09 > >>> + unsigned long features; >>> + unsigned int min_pasid; >>> + unsigned int max_pasid; >>> +}; >>> + >>> /** >>> * struct iommu_ops - iommu ops and capabilities >>> * @capable: check capability >>> @@ -219,6 +225,8 @@ struct page_response_msg { >>> * @domain_free: free iommu domain >>> * @attach_dev: attach device to an iommu domain >>> * @detach_dev: detach device from an iommu domain >>> + * @sva_device_init: initialize Shared Virtual Adressing for a device >> Addressing >>> + * @sva_device_shutdown: shutdown Shared Virtual Adressing for a device >> Addressing > Nice catch > > Thanks, > Jean From mboxrd@z Thu Jan 1 00:00:00 1970 From: christian.koenig@amd.com (=?UTF-8?Q?Christian_K=c3=b6nig?=) Date: Thu, 6 Sep 2018 13:12:42 +0200 Subject: [PATCH v2 01/40] iommu: Introduce Shared Virtual Addressing API In-Reply-To: <03d31ba5-1eda-ea86-8c0c-91d14c86fe83@arm.com> References: <20180511190641.23008-1-jean-philippe.brucker@arm.com> <20180511190641.23008-2-jean-philippe.brucker@arm.com> <03d31ba5-1eda-ea86-8c0c-91d14c86fe83@arm.com> Message-ID: To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org Am 06.09.2018 um 13:09 schrieb Jean-Philippe Brucker: > Hi Eric, > > Thanks for reviewing > > On 05/09/2018 12:29, Auger Eric wrote: >>> +int iommu_sva_device_init(struct device *dev, unsigned long features, >>> + unsigned int max_pasid) >> what about min_pasid? > No one asked for it... The max_pasid parameter is here for drivers that > have vendor-specific PASID size limits, such as AMD KFD (see > kfd_iommu_device_init and > https://patchwork.kernel.org/patch/9989307/#21389571). But in most cases > the PASID size will only depend on the PCI PASID capability and the > IOMMU limits, both known by the IOMMU driver, so device drivers won't > have to set max_pasid. > > IOMMU drivers need to set min_pasid in the sva_device_init callback > because it may be either 1 (e.g. Arm where PASID #0 is reserved) or 0 > (Intel Vt-d rev2), but at the moment I can't see a reason for device > drivers to override min_pasid Sorry to ruin your day, but if I'm not completely mistaken PASID zero is reserved in the AMD KFD as well. Regards, Christian. > >>> + /* >>> + * IOMMU driver updates the limits depending on the IOMMU and device >>> + * capabilities. >>> + */ >>> + ret = domain->ops->sva_device_init(dev, param); >>> + if (ret) >>> + goto err_free_param; >> So you are likely to call sva_device_init even if it was already called >> (ie. dev->iommu_param->sva_param is already set). Can't you test whether >> dev->iommu_param->sva_param is already set first? > Right, that's probably better > >>> +/** >>> + * iommu_sva_device_shutdown() - Shutdown Shared Virtual Addressing for a device >>> + * @dev: the device >>> + * >>> + * Disable SVA. Device driver should ensure that the device isn't performing any >>> + * DMA while this function is running. >>> + */ >>> +int iommu_sva_device_shutdown(struct device *dev) >> Not sure the returned value is required for a shutdown operation. > I don't know either. I like them because they help me debug, but are > otherwise rather useless if we don't describe precise semantics. The > caller cannot do anything with it. Given that the corresponding IOMMU op > is already void, I can change this function to void as well > >>> +struct iommu_sva_param { >> What are the feature values? > At the moment only IOMMU_SVA_FEAT_IOPF, introduced by patch 09 > >>> + unsigned long features; >>> + unsigned int min_pasid; >>> + unsigned int max_pasid; >>> +}; >>> + >>> /** >>> * struct iommu_ops - iommu ops and capabilities >>> * @capable: check capability >>> @@ -219,6 +225,8 @@ struct page_response_msg { >>> * @domain_free: free iommu domain >>> * @attach_dev: attach device to an iommu domain >>> * @detach_dev: detach device from an iommu domain >>> + * @sva_device_init: initialize Shared Virtual Adressing for a device >> Addressing >>> + * @sva_device_shutdown: shutdown Shared Virtual Adressing for a device >> Addressing > Nice catch > > Thanks, > Jean