| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Tue, 29 Mar 2022 10:39:52 -0700
From: Jacob Pan <jacob.jun.pan@...ux.intel.com>
To: "Tian, Kevin" <kevin.tian@...el.com>
Cc: "iommu@...ts.linux-foundation.org" <iommu@...ts.linux-foundation.org>,
LKML <linux-kernel@...r.kernel.org>,
Joerg Roedel <joro@...tes.org>,
"Jason Gunthorpe" <jgg@...dia.com>,
Christoph Hellwig <hch@...radead.org>,
Lu Baolu <baolu.lu@...ux.intel.com>,
Jean-Philippe Brucker <jean-philippe@...aro.com>,
"Pan, Jacob jun" <jacob.jun.pan@...el.com>,
"Raj, Ashok" <ashok.raj@...el.com>,
"Kumar, Sanjay K" <sanjay.k.kumar@...el.com>,
"Jiang, Dave" <dave.jiang@...el.com>,
"Luck, Tony" <tony.luck@...el.com>,
"Zanussi, Tom" <tom.zanussi@...el.com>,
"Williams, Dan J" <dan.j.williams@...el.com>,
"Liu, Yi L" <yi.l.liu@...el.com>, jacob.jun.pan@...ux.intel.com
Subject: Re: [PATCH v2 6/8] dmaengine: idxd: Use DMA API for in-kernel DMA
with PASID
Hi Kevin,
On Fri, 18 Mar 2022 06:10:40 +0000, "Tian, Kevin" <kevin.tian@...el.com>
wrote:
> > From: Jacob Pan <jacob.jun.pan@...ux.intel.com>
> > Sent: Tuesday, March 15, 2022 1:07 PM
> >
> > The current in-kernel supervisor PASID support is based on the SVM/SVA
> > machinery in SVA lib. The binding between a kernel PASID and kernel
> > mapping has many flaws. See discussions in the link below.
> >
> > This patch enables in-kernel DMA by switching from SVA lib to the
> > standard DMA mapping APIs. Since both DMA requests with and without
> > PASIDs are mapped identically, there is no change to how DMA APIs are
> > used after the kernel PASID is enabled.
> >
> > Link: https://lore.kernel.org/linux-
> > iommu/20210511194726.GP1002214@...dia.com/
> > Signed-off-by: Jacob Pan <jacob.jun.pan@...ux.intel.com>
> > ---
> > drivers/dma/idxd/idxd.h | 1 -
> > drivers/dma/idxd/init.c | 34 +++++++++-------------------------
> > drivers/dma/idxd/sysfs.c | 7 -------
> > 3 files changed, 9 insertions(+), 33 deletions(-)
> >
> > diff --git a/drivers/dma/idxd/idxd.h b/drivers/dma/idxd/idxd.h
> > index da72eb15f610..a09ab4a6e1c1 100644
> > --- a/drivers/dma/idxd/idxd.h
> > +++ b/drivers/dma/idxd/idxd.h
> > @@ -276,7 +276,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 08a5f4310188..5d1f8dd4abf6 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"
> > @@ -466,36 +467,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)
>
> idxd_enable_pasid_dma() since system pasid is a confusing term now?
> Or just remove the idxd specific wrappers and have the caller to call
> iommu_enable_pasid_dma() directly given the simple logic here.
>
agreed, will do.
> > {
> > - int flags;
> > - unsigned int pasid;
> > - struct iommu_sva *sva;
> > + u32 pasid;
> > + int ret;
> >
> > - 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);
> > - }
> > -
> > - pasid = iommu_sva_get_pasid(sva);
> > - if (pasid == IOMMU_PASID_INVALID) {
> > - iommu_sva_unbind_device(sva);
> > - return -ENODEV;
> > + ret = iommu_enable_pasid_dma(&idxd->pdev->dev, &pasid);
> > + if (ret) {
> > + dev_err(&idxd->pdev->dev, "No DMA PASID %d\n", ret);
> > + return ret;
> > }
> > -
> > - 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);
> > }
> >
> > static int idxd_probe(struct idxd_device *idxd)
> > @@ -524,10 +511,7 @@ static int idxd_probe(struct idxd_device *idxd)
> > } 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");
>
> why removing above 2 lines? they are related to a module param thus
> not affected by the logic in this series.
>
This should be in a separate patch. I consulted with Dave, sva module param
is not needed anymore.
Thanks for pointing it out.
> > }
> > -
> > idxd_read_caps(idxd);
> > idxd_read_table_offsets(idxd);
> >
> > diff --git a/drivers/dma/idxd/sysfs.c b/drivers/dma/idxd/sysfs.c
> > index 7e19ab92b61a..fde6656695ba 100644
> > --- a/drivers/dma/idxd/sysfs.c
> > +++ b/drivers/dma/idxd/sysfs.c
> > @@ -839,13 +839,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');
> > --
> > 2.25.1
>
Thanks,
Jacob
Powered by blists - more mailing lists