* Re: [PATCH 4/4] dmaengine: idxd: Use DMA API for in-kernel DMA with PASID
[not found] ` <1638884834-83028-5-git-send-email-jacob.jun.pan@linux.intel.com>
@ 2021-12-07 23:27 ` Dave Jiang
2021-12-08 4:56 ` Vinod Koul
0 siblings, 1 reply; 3+ messages in thread
From: Dave Jiang @ 2021-12-07 23:27 UTC (permalink / raw)
To: Jacob Pan, iommu, LKML, Joerg Roedel, Jason Gunthorpe,
Christoph Hellwig, Greg Kroah-Hartman, Jean-Philippe Brucker
Cc: Jacob Pan, Lu Baolu, Raj Ashok, Kumar, Sanjay K, Tony Luck,
Yi Liu, Tian, Kevin, Barry Song, Zanussi, Tom, Dan Williams,
Vinod Koul, dmaengine@vger.kernel.org
On 12/7/2021 6:47 AM, Jacob Pan wrote:
> In-kernel DMA should be managed by DMA mapping API. The existing kernel
> PASID support is based on the SVA machinery in SVA lib that is intended
> for user process SVA. The binding between a kernel PASID and kernel
> mapping has many flaws. See discussions in the link below.
>
> This patch utilizes iommu_enable_pasid_dma() to enable DSA to perform DMA
> requests with PASID under the same mapping managed by DMA mapping API.
> In addition, SVA-related bits for kernel DMA are removed. As a result,
> DSA users shall use DMA mapping API to obtain DMA handles instead of
> using kernel virtual addresses.
>
> Link: https://lore.kernel.org/linux-iommu/20210511194726.GP1002214@nvidia.com/
> Signed-off-by: Jacob Pan <jacob.jun.pan@linux.intel.com>
Acked-by: Dave Jiang <dave.jiang@intel.com>
Also cc Vinod and dmaengine@vger
> ---
> .../admin-guide/kernel-parameters.txt | 6 --
> drivers/dma/Kconfig | 10 ----
> drivers/dma/idxd/idxd.h | 1 -
> drivers/dma/idxd/init.c | 59 ++++++-------------
> drivers/dma/idxd/sysfs.c | 7 ---
> 5 files changed, 19 insertions(+), 64 deletions(-)
>
> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index 9725c546a0d4..fe73d02c62f3 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -1751,12 +1751,6 @@
> In such case C2/C3 won't be used again.
> idle=nomwait: Disable mwait for CPU C-states
>
> - idxd.sva= [HW]
> - Format: <bool>
> - Allow force disabling of Shared Virtual Memory (SVA)
> - support for the idxd driver. By default it is set to
> - true (1).
> -
> idxd.tc_override= [HW]
> Format: <bool>
> Allow override of default traffic class configuration
> diff --git a/drivers/dma/Kconfig b/drivers/dma/Kconfig
> index 6bcdb4e6a0d1..3b28bd720e7d 100644
> --- a/drivers/dma/Kconfig
> +++ b/drivers/dma/Kconfig
> @@ -313,16 +313,6 @@ config INTEL_IDXD_COMPAT
>
> If unsure, say N.
>
> -# Config symbol that collects all the dependencies that's necessary to
> -# support shared virtual memory for the devices supported by idxd.
> -config INTEL_IDXD_SVM
> - bool "Accelerator Shared Virtual Memory Support"
> - depends on INTEL_IDXD
> - depends on INTEL_IOMMU_SVM
> - depends on PCI_PRI
> - depends on PCI_PASID
> - depends on PCI_IOV
> -
> config INTEL_IDXD_PERFMON
> bool "Intel Data Accelerators performance monitor support"
> depends on INTEL_IDXD
> diff --git a/drivers/dma/idxd/idxd.h b/drivers/dma/idxd/idxd.h
> index 0cf8d3145870..3155e3a2d3ae 100644
> --- a/drivers/dma/idxd/idxd.h
> +++ b/drivers/dma/idxd/idxd.h
> @@ -262,7 +262,6 @@ struct idxd_device {
> struct idxd_wq **wqs;
> struct idxd_engine **engines;
>
> - struct iommu_sva *sva;
> unsigned int pasid;
>
> int num_groups;
> diff --git a/drivers/dma/idxd/init.c b/drivers/dma/idxd/init.c
> index 7bf03f371ce1..44633f8113e2 100644
> --- a/drivers/dma/idxd/init.c
> +++ b/drivers/dma/idxd/init.c
> @@ -16,6 +16,7 @@
> #include <linux/idr.h>
> #include <linux/intel-svm.h>
> #include <linux/iommu.h>
> +#include <linux/dma-iommu.h>
> #include <uapi/linux/idxd.h>
> #include <linux/dmaengine.h>
> #include "../dmaengine.h"
> @@ -28,10 +29,6 @@ MODULE_LICENSE("GPL v2");
> MODULE_AUTHOR("Intel Corporation");
> MODULE_IMPORT_NS(IDXD);
>
> -static bool sva = true;
> -module_param(sva, bool, 0644);
> -MODULE_PARM_DESC(sva, "Toggle SVA support on/off");
> -
> bool tc_override;
> module_param(tc_override, bool, 0644);
> MODULE_PARM_DESC(tc_override, "Override traffic class defaults");
> @@ -530,36 +527,22 @@ static struct idxd_device *idxd_alloc(struct pci_dev *pdev, struct idxd_driver_d
>
> static int idxd_enable_system_pasid(struct idxd_device *idxd)
> {
> - int flags;
> - unsigned int pasid;
> - struct iommu_sva *sva;
> -
> - flags = SVM_FLAG_SUPERVISOR_MODE;
> -
> - sva = iommu_sva_bind_device(&idxd->pdev->dev, NULL, &flags);
> - if (IS_ERR(sva)) {
> - dev_warn(&idxd->pdev->dev,
> - "iommu sva bind failed: %ld\n", PTR_ERR(sva));
> - return PTR_ERR(sva);
> - }
> + u32 pasid;
>
> - pasid = iommu_sva_get_pasid(sva);
> - if (pasid == IOMMU_PASID_INVALID) {
> - iommu_sva_unbind_device(sva);
> + pasid = iommu_enable_pasid_dma(&idxd->pdev->dev);
> + if (pasid == INVALID_IOASID) {
> + dev_err(&idxd->pdev->dev, "No kernel DMA PASID\n");
> return -ENODEV;
> }
> -
> - idxd->sva = sva;
> idxd->pasid = pasid;
> - dev_dbg(&idxd->pdev->dev, "system pasid: %u\n", pasid);
> +
> return 0;
> }
>
> static void idxd_disable_system_pasid(struct idxd_device *idxd)
> {
> -
> - iommu_sva_unbind_device(idxd->sva);
> - idxd->sva = NULL;
> + iommu_disable_pasid_dma(&idxd->pdev->dev);
> + idxd->pasid = 0;
> }
>
> static int idxd_probe(struct idxd_device *idxd)
> @@ -575,21 +558,17 @@ static int idxd_probe(struct idxd_device *idxd)
>
> dev_dbg(dev, "IDXD reset complete\n");
>
> - if (IS_ENABLED(CONFIG_INTEL_IDXD_SVM) && sva) {
> - rc = iommu_dev_enable_feature(dev, IOMMU_DEV_FEAT_SVA);
> - if (rc == 0) {
> - rc = idxd_enable_system_pasid(idxd);
> - if (rc < 0) {
> - iommu_dev_disable_feature(dev, IOMMU_DEV_FEAT_SVA);
> - dev_warn(dev, "Failed to enable PASID. No SVA support: %d\n", rc);
> - } else {
> - set_bit(IDXD_FLAG_PASID_ENABLED, &idxd->flags);
> - }
> - } else {
> - dev_warn(dev, "Unable to turn on SVA feature.\n");
> - }
> - } else if (!sva) {
> - dev_warn(dev, "User forced SVA off via module param.\n");
> + /*
> + * Try to enable both in-kernel and user DMA request with PASID.
> + * PASID is supported unless both user and kernel PASID are
> + * supported. Do not fail probe here in that idxd can still be
> + * used w/o PASID or IOMMU.
> + */
> + if (iommu_dev_enable_feature(dev, IOMMU_DEV_FEAT_SVA) ||
> + idxd_enable_system_pasid(idxd)) {
> + dev_warn(dev, "Failed to enable PASID\n");
> + } else {
> + set_bit(IDXD_FLAG_PASID_ENABLED, &idxd->flags);
> }
>
> idxd_read_caps(idxd);
> diff --git a/drivers/dma/idxd/sysfs.c b/drivers/dma/idxd/sysfs.c
> index a9025be940db..35737299c355 100644
> --- a/drivers/dma/idxd/sysfs.c
> +++ b/drivers/dma/idxd/sysfs.c
> @@ -776,13 +776,6 @@ static ssize_t wq_name_store(struct device *dev,
> if (strlen(buf) > WQ_NAME_SIZE || strlen(buf) == 0)
> return -EINVAL;
>
> - /*
> - * This is temporarily placed here until we have SVM support for
> - * dmaengine.
> - */
> - if (wq->type == IDXD_WQT_KERNEL && device_pasid_enabled(wq->idxd))
> - return -EOPNOTSUPP;
> -
> memset(wq->name, 0, WQ_NAME_SIZE + 1);
> strncpy(wq->name, buf, WQ_NAME_SIZE);
> strreplace(wq->name, '\n', '\0');
^ permalink raw reply [flat|nested] 3+ messages in thread