Re: [PATCH v3 2/2] iommu/sva: Remove mm parameter from SVA bind API
Hi Jacob,
I love your patch! Yet something to improve:
[auto build test ERROR on e49d033bddf5b565044e2abe4241353959bc9120]
url:
https://github.com/0day-ci/linux/commits/Jacob-Pan/Simplify-and-restrict-IOMMU-SVA-APIs/20210417-052451
base: e49d033bddf5b565044e2abe4241353959bc9120
config: arm64-randconfig-r034-20210416 (attached as .config)
compiler: clang version 13.0.0 (https://github.com/llvm/llvm-project
f549176ad976caa3e19edd036df9a7e12770af7c)
reproduce (this is a W=1 build):
wget
https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O
~/bin/make.cross
chmod +x ~/bin/make.cross
# install arm64 cross compiling tool for clang build
# apt-get install binutils-aarch64-linux-gnu
#
https://github.com/0day-ci/linux/commit/6d85fee95bdcd7e53f10442ddc71d0c310d43367
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review
Jacob-Pan/Simplify-and-restrict-IOMMU-SVA-APIs/20210417-052451
git checkout 6d85fee95bdcd7e53f10442ddc71d0c310d43367
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1
ARCH=arm64
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot
All errors (new ones prefixed by >>):
>> drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c:2631:15: error: incompatible
>> function pointer types initializing 'struct iommu_sva *(*)(struct device *,
>> unsigned int)' with an expression of type 'struct iommu_sva *(struct device
>> *, struct mm_struct *, unsigned int)'
>> [-Werror,-Wincompatible-function-pointer-types]
.sva_bind = arm_smmu_sva_bind,
^
1 error generated.
vim +2631 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
f534d98b9d2705 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c Jean-Philippe
Brucker 2020-09-18 2608
48ec83bcbcf509 drivers/iommu/arm-smmu-v3.c Will Deacon
2015-05-27 2609 static struct iommu_ops arm_smmu_ops = {
48ec83bcbcf509 drivers/iommu/arm-smmu-v3.c Will Deacon
2015-05-27 2610 .capable= arm_smmu_capable,
48ec83bcbcf509 drivers/iommu/arm-smmu-v3.c Will Deacon
2015-05-27 2611 .domain_alloc = arm_smmu_domain_alloc,
48ec83bcbcf509 drivers/iommu/arm-smmu-v3.c Will Deacon
2015-05-27 2612 .domain_free= arm_smmu_domain_free,
48ec83bcbcf509 drivers/iommu/arm-smmu-v3.c Will Deacon
2015-05-27 2613 .attach_dev = arm_smmu_attach_dev,
48ec83bcbcf509 drivers/iommu/arm-smmu-v3.c Will Deacon
2015-05-27 2614 .map= arm_smmu_map,
48ec83bcbcf509 drivers/iommu/arm-smmu-v3.c Will Deacon
2015-05-27 2615 .unmap = arm_smmu_unmap,
07fdef34d2be68 drivers/iommu/arm-smmu-v3.c Zhen Lei
2018-09-20 2616 .flush_iotlb_all= arm_smmu_flush_iotlb_all,
32b124492bdf97 drivers/iommu/arm-smmu-v3.c Robin Murphy
2017-09-28 2617 .iotlb_sync = arm_smmu_iotlb_sync,
48ec83bcbcf509 drivers/iommu/arm-smmu-v3.c Will Deacon
2015-05-27 2618 .iova_to_phys = arm_smmu_iova_to_phys,
cefa0d55da3753 drivers/iommu/arm-smmu-v3.c Joerg Roedel
2020-04-29 2619 .probe_device = arm_smmu_probe_device,
cefa0d55da3753 drivers/iommu/arm-smmu-v3.c Joerg Roedel
2020-04-29 2620 .release_device = arm_smmu_release_device,
08d4ca2a672bab drivers/iommu/arm-smmu-v3.c Robin Murphy
2016-09-12 2621 .device_group = arm_smmu_device_group,
48ec83bcbcf509 drivers/iommu/arm-smmu-v3.c Will Deacon
2015-05-27 2622 .domain_get_attr= arm_smmu_domain_get_attr,
48ec83bcbcf509 drivers/iommu/arm-smmu-v3.c Will Deacon
2015-05-27 2623 .domain_set_attr= arm_smmu_domain_set_attr,
8f78515425daea drivers/iommu/arm-smmu-v3.c Robin Murphy
2016-09-12 2624 .of_xlate = arm_smmu_of_xlate,
50019f09a4baa0 drivers/iommu/arm-smmu-v3.c Eric Auger
2017-01-19 2625 .get_resv_regions = arm_smmu_get_resv_regions,
a66c5dc549d1e1 drivers/iommu/arm-smmu-v3.c Thierry Reding
2019-12-18 2626 .put_resv_regions =
generic_iommu_put_resv_regions,
f534d98b9d2705 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c Jean-Philippe
Brucker 2020-09-18 2627 .dev_has_feat =
arm_smmu_dev_has_feature,
f534d98b9d2705 drivers/iommu/arm/arm-smmu-
Re: [PATCH v3 2/2] iommu/sva: Remove mm parameter from SVA bind API
Hi Jacob,
I love your patch! Yet something to improve:
[auto build test ERROR on e49d033bddf5b565044e2abe4241353959bc9120]
url:
https://github.com/0day-ci/linux/commits/Jacob-Pan/Simplify-and-restrict-IOMMU-SVA-APIs/20210417-052451
base: e49d033bddf5b565044e2abe4241353959bc9120
config: arm64-randconfig-r022-20210416 (attached as .config)
compiler: aarch64-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
wget
https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O
~/bin/make.cross
chmod +x ~/bin/make.cross
#
https://github.com/0day-ci/linux/commit/6d85fee95bdcd7e53f10442ddc71d0c310d43367
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review
Jacob-Pan/Simplify-and-restrict-IOMMU-SVA-APIs/20210417-052451
git checkout 6d85fee95bdcd7e53f10442ddc71d0c310d43367
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross W=1
ARCH=arm64
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot
All errors (new ones prefixed by >>):
>> drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c:2631:15: error: initialization
>> of 'struct iommu_sva * (*)(struct device *, unsigned int)' from incompatible
>> pointer type 'struct iommu_sva * (*)(struct device *, struct mm_struct *,
>> unsigned int)' [-Werror=incompatible-pointer-types]
2631 | .sva_bind = arm_smmu_sva_bind,
| ^
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c:2631:15: note: (near
initialization for 'arm_smmu_ops.sva_bind')
cc1: some warnings being treated as errors
vim +2631 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
f534d98b9d2705 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c Jean-Philippe
Brucker 2020-09-18 2608
48ec83bcbcf509 drivers/iommu/arm-smmu-v3.c Will Deacon
2015-05-27 2609 static struct iommu_ops arm_smmu_ops = {
48ec83bcbcf509 drivers/iommu/arm-smmu-v3.c Will Deacon
2015-05-27 2610 .capable= arm_smmu_capable,
48ec83bcbcf509 drivers/iommu/arm-smmu-v3.c Will Deacon
2015-05-27 2611 .domain_alloc = arm_smmu_domain_alloc,
48ec83bcbcf509 drivers/iommu/arm-smmu-v3.c Will Deacon
2015-05-27 2612 .domain_free= arm_smmu_domain_free,
48ec83bcbcf509 drivers/iommu/arm-smmu-v3.c Will Deacon
2015-05-27 2613 .attach_dev = arm_smmu_attach_dev,
48ec83bcbcf509 drivers/iommu/arm-smmu-v3.c Will Deacon
2015-05-27 2614 .map= arm_smmu_map,
48ec83bcbcf509 drivers/iommu/arm-smmu-v3.c Will Deacon
2015-05-27 2615 .unmap = arm_smmu_unmap,
07fdef34d2be68 drivers/iommu/arm-smmu-v3.c Zhen Lei
2018-09-20 2616 .flush_iotlb_all= arm_smmu_flush_iotlb_all,
32b124492bdf97 drivers/iommu/arm-smmu-v3.c Robin Murphy
2017-09-28 2617 .iotlb_sync = arm_smmu_iotlb_sync,
48ec83bcbcf509 drivers/iommu/arm-smmu-v3.c Will Deacon
2015-05-27 2618 .iova_to_phys = arm_smmu_iova_to_phys,
cefa0d55da3753 drivers/iommu/arm-smmu-v3.c Joerg Roedel
2020-04-29 2619 .probe_device = arm_smmu_probe_device,
cefa0d55da3753 drivers/iommu/arm-smmu-v3.c Joerg Roedel
2020-04-29 2620 .release_device = arm_smmu_release_device,
08d4ca2a672bab drivers/iommu/arm-smmu-v3.c Robin Murphy
2016-09-12 2621 .device_group = arm_smmu_device_group,
48ec83bcbcf509 drivers/iommu/arm-smmu-v3.c Will Deacon
2015-05-27 2622 .domain_get_attr= arm_smmu_domain_get_attr,
48ec83bcbcf509 drivers/iommu/arm-smmu-v3.c Will Deacon
2015-05-27 2623 .domain_set_attr= arm_smmu_domain_set_attr,
8f78515425daea drivers/iommu/arm-smmu-v3.c Robin Murphy
2016-09-12 2624 .of_xlate = arm_smmu_of_xlate,
50019f09a4baa0 drivers/iommu/arm-smmu-v3.c Eric Auger
2017-01-19 2625 .get_resv_regions = arm_smmu_get_resv_regions,
a66c5dc549d1e1 drivers/iommu/arm-smmu-v3.c Thierry Reding
2019-12-18 2626 .put_resv_regions =
generic_iommu_put_resv_regions,
f534d98b9d2705 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c Jean-Philippe
Brucker 2020-09-18 2627 .dev_has_feat =
arm_smmu_dev_has_feature,
f534d98b9d2705 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c Jean-Philippe
Brucker 2020-09-18 2628 .dev_feat_enabled =
arm_smmu_dev_feature_enabl
Re: [PATCH 0/8] iommu: fix a couple of spelling mistakes detected by codespell tool
On 2021/4/16 23:24, Joerg Roedel wrote: > On Fri, Mar 26, 2021 at 02:24:04PM +0800, Zhen Lei wrote: >> This detection and correction covers the entire driver/iommu directory. >> >> Zhen Lei (8): >> iommu/pamu: fix a couple of spelling mistakes >> iommu/omap: Fix spelling mistake "alignement" -> "alignment" >> iommu/mediatek: Fix spelling mistake "phyiscal" -> "physical" >> iommu/sun50i: Fix spelling mistake "consits" -> "consists" >> iommu: fix a couple of spelling mistakes >> iommu/amd: fix a couple of spelling mistakes >> iommu/arm-smmu: Fix spelling mistake "initally" -> "initially" >> iommu/vt-d: fix a couple of spelling mistakes > > This patch-set doesn't apply. Please re-send it as a single patch when > v5.13-rc1 is released. OK > > Thanks, > > Joerg > > . > ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 5/8] iommu: fix a couple of spelling mistakes
On 2021/4/16 23:55, John Garry wrote: > On 26/03/2021 06:24, Zhen Lei wrote: >> There are several spelling mistakes, as follows: >> funcions ==> functions >> distiguish ==> distinguish >> detroyed ==> destroyed >> >> Signed-off-by: Zhen Lei > > I think that there should be a /s/appropriatley/appropriately/ in iommu.c OK, I will fix it in v2. > > Thanks, > john > > . > ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH v3 1/2] iommu/sva: Tighten SVA bind API with explicit flags
The void* drvdata parameter isn't really used in iommu_sva_bind_device()
API, the current IDXD code "borrows" the drvdata for a VT-d private flag
for supervisor SVA usage.
Supervisor/Privileged mode request is a generic feature. It should be
promoted from the VT-d vendor driver to the generic code.
This patch replaces void* drvdata with a unsigned int flags parameter
and adjusts callers accordingly.
Link: https://lore.kernel.org/linux-iommu/YFhiMLR35WWMW%2FHu@myrica/
Suggested-by: Jean-Philippe Brucker
Signed-off-by: Jacob Pan
---
drivers/dma/idxd/cdev.c | 2 +-
drivers/dma/idxd/init.c | 7 ++-
.../iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c | 5 -
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h | 4 ++--
drivers/iommu/intel/svm.c | 14 --
drivers/iommu/iommu.c | 9 ++---
drivers/misc/uacce/uacce.c| 2 +-
include/linux/intel-iommu.h | 2 +-
include/linux/intel-svm.h | 17 ++---
include/linux/iommu.h | 19 ---
10 files changed, 39 insertions(+), 42 deletions(-)
diff --git a/drivers/dma/idxd/cdev.c b/drivers/dma/idxd/cdev.c
index 0db9b82ed8cf..21ec82bc47b6 100644
--- a/drivers/dma/idxd/cdev.c
+++ b/drivers/dma/idxd/cdev.c
@@ -103,7 +103,7 @@ static int idxd_cdev_open(struct inode *inode, struct file
*filp)
filp->private_data = ctx;
if (device_pasid_enabled(idxd)) {
- sva = iommu_sva_bind_device(dev, current->mm, NULL);
+ sva = iommu_sva_bind_device(dev, current->mm, 0);
if (IS_ERR(sva)) {
rc = PTR_ERR(sva);
dev_err(dev, "pasid allocation failed: %d\n", rc);
diff --git a/drivers/dma/idxd/init.c b/drivers/dma/idxd/init.c
index 085a0c3b62c6..7b2290b19787 100644
--- a/drivers/dma/idxd/init.c
+++ b/drivers/dma/idxd/init.c
@@ -14,7 +14,6 @@
#include
#include
#include
-#include
#include
#include
#include
@@ -300,13 +299,11 @@ static struct idxd_device *idxd_alloc(struct pci_dev
*pdev)
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);
+ sva = iommu_sva_bind_device(&idxd->pdev->dev, NULL,
+ IOMMU_SVA_BIND_SUPERVISOR);
if (IS_ERR(sva)) {
dev_warn(&idxd->pdev->dev,
"iommu sva bind failed: %ld\n", PTR_ERR(sva));
diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
index bb251cab61f3..145ceb2fc5da 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
@@ -354,12 +354,15 @@ __arm_smmu_sva_bind(struct device *dev, struct mm_struct
*mm)
}
struct iommu_sva *
-arm_smmu_sva_bind(struct device *dev, struct mm_struct *mm, void *drvdata)
+arm_smmu_sva_bind(struct device *dev, struct mm_struct *mm, unsigned int flags)
{
struct iommu_sva *handle;
struct iommu_domain *domain = iommu_get_domain_for_dev(dev);
struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
+ if (flags)
+ return ERR_PTR(-EINVAL);
+
if (smmu_domain->stage != ARM_SMMU_DOMAIN_S1)
return ERR_PTR(-EINVAL);
diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
index f985817c967a..b971d4dcf090 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
@@ -711,7 +711,7 @@ bool arm_smmu_master_sva_enabled(struct arm_smmu_master
*master);
int arm_smmu_master_enable_sva(struct arm_smmu_master *master);
int arm_smmu_master_disable_sva(struct arm_smmu_master *master);
struct iommu_sva *arm_smmu_sva_bind(struct device *dev, struct mm_struct *mm,
- void *drvdata);
+ unsigned int flags);
void arm_smmu_sva_unbind(struct iommu_sva *handle);
u32 arm_smmu_sva_get_pasid(struct iommu_sva *handle);
void arm_smmu_sva_notifier_synchronize(void);
@@ -742,7 +742,7 @@ static inline int arm_smmu_master_disable_sva(struct
arm_smmu_master *master)
}
static inline struct iommu_sva *
-arm_smmu_sva_bind(struct device *dev, struct mm_struct *mm, void *drvdata)
+arm_smmu_sva_bind(struct device *dev, struct mm_struct *mm, unsigned int flags)
{
return ERR_PTR(-ENODEV);
}
diff --git a/drivers/iommu/intel/svm.c b/drivers/iommu/intel/svm.c
index 574a7e657a9a..d4840821f7b5 100644
--- a/drivers/iommu/intel/svm.c
+++ b/drivers/iommu/intel/svm.c
@@ -486,12 +486,9 @@ intel_svm_bind_mm(struct device *dev, unsigned int flags,
} else
pasid_max = 1 << 20;
-
[PATCH v3 2/2] iommu/sva: Remove mm parameter from SVA bind API
The mm parameter in iommu_sva_bind_device() is intended for privileged
process perform bind() on behalf of other processes. This use case has
yet to be materialized, let alone potential security implications of
adding kernel hooks without explicit user consent.
In addition, with the agreement that IOASID allocation shall be subject
cgroup limit. It will be inline with misc cgroup proposal if IOASID
allocation as part of the SVA bind is limited to the current task.
Link: https://lore.kernel.org/linux-iommu/20210303160205.151d114e@jacob-builder/
Link: https://lore.kernel.org/linux-iommu/YFhiMLR35WWMW%2FHu@myrica/
Signed-off-by: Jacob Pan
---
drivers/dma/idxd/cdev.c | 2 +-
drivers/dma/idxd/init.c | 2 +-
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c | 9 +
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h | 3 +--
drivers/iommu/intel/svm.c | 17 +++--
drivers/iommu/iommu-sva-lib.c | 11 ++-
drivers/iommu/iommu-sva-lib.h | 2 +-
drivers/iommu/iommu.c | 14 +-
drivers/misc/uacce/uacce.c | 2 +-
include/linux/intel-iommu.h | 3 +--
include/linux/iommu.h | 8 +++-
11 files changed, 36 insertions(+), 37 deletions(-)
diff --git a/drivers/dma/idxd/cdev.c b/drivers/dma/idxd/cdev.c
index 21ec82bc47b6..8c3347c8930c 100644
--- a/drivers/dma/idxd/cdev.c
+++ b/drivers/dma/idxd/cdev.c
@@ -103,7 +103,7 @@ static int idxd_cdev_open(struct inode *inode, struct file
*filp)
filp->private_data = ctx;
if (device_pasid_enabled(idxd)) {
- sva = iommu_sva_bind_device(dev, current->mm, 0);
+ sva = iommu_sva_bind_device(dev, 0);
if (IS_ERR(sva)) {
rc = PTR_ERR(sva);
dev_err(dev, "pasid allocation failed: %d\n", rc);
diff --git a/drivers/dma/idxd/init.c b/drivers/dma/idxd/init.c
index 7b2290b19787..f64a19b5e513 100644
--- a/drivers/dma/idxd/init.c
+++ b/drivers/dma/idxd/init.c
@@ -302,7 +302,7 @@ static int idxd_enable_system_pasid(struct idxd_device
*idxd)
unsigned int pasid;
struct iommu_sva *sva;
- sva = iommu_sva_bind_device(&idxd->pdev->dev, NULL,
+ sva = iommu_sva_bind_device(&idxd->pdev->dev,
IOMMU_SVA_BIND_SUPERVISOR);
if (IS_ERR(sva)) {
dev_warn(&idxd->pdev->dev,
diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
index 145ceb2fc5da..0c3014e64c77 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
@@ -305,10 +305,11 @@ static void arm_smmu_mmu_notifier_put(struct
arm_smmu_mmu_notifier *smmu_mn)
}
static struct iommu_sva *
-__arm_smmu_sva_bind(struct device *dev, struct mm_struct *mm)
+__arm_smmu_sva_bind(struct device *dev)
{
int ret;
struct arm_smmu_bond *bond;
+ struct mm_struct *mm = current->mm;
struct arm_smmu_master *master = dev_iommu_priv_get(dev);
struct iommu_domain *domain = iommu_get_domain_for_dev(dev);
struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
@@ -329,7 +330,7 @@ __arm_smmu_sva_bind(struct device *dev, struct mm_struct
*mm)
return ERR_PTR(-ENOMEM);
/* Allocate a PASID for this mm if necessary */
- ret = iommu_sva_alloc_pasid(mm, 1, (1U << master->ssid_bits) - 1);
+ ret = iommu_sva_alloc_pasid(1, (1U << master->ssid_bits) - 1);
if (ret)
goto err_free_bond;
@@ -354,7 +355,7 @@ __arm_smmu_sva_bind(struct device *dev, struct mm_struct
*mm)
}
struct iommu_sva *
-arm_smmu_sva_bind(struct device *dev, struct mm_struct *mm, unsigned int flags)
+arm_smmu_sva_bind(struct device *dev, unsigned int flags)
{
struct iommu_sva *handle;
struct iommu_domain *domain = iommu_get_domain_for_dev(dev);
@@ -367,7 +368,7 @@ arm_smmu_sva_bind(struct device *dev, struct mm_struct *mm,
unsigned int flags)
return ERR_PTR(-EINVAL);
mutex_lock(&sva_lock);
- handle = __arm_smmu_sva_bind(dev, mm);
+ handle = __arm_smmu_sva_bind(dev);
mutex_unlock(&sva_lock);
return handle;
}
diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
index b971d4dcf090..306fa59a9db5 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
@@ -710,8 +710,7 @@ bool arm_smmu_master_sva_supported(struct arm_smmu_master
*master);
bool arm_smmu_master_sva_enabled(struct arm_smmu_master *master);
int arm_smmu_master_enable_sva(struct arm_smmu_master *master);
int arm_smmu_master_disable_sva(struct arm_smmu_master *master);
-struct iommu_sva *arm_smmu_sva_bind(struct device *dev, struct mm_st
[PATCH v3 0/2] Simplify and restrict IOMMU SVA APIs
A couple of small changes to simplify and restrict SVA APIs. The motivation is to make PASID allocation palatable for cgroup consumptions. Misc cgroup is merged for v5.13, it can be extended for IOASID as another scalar resource. I have not tested on ARM platforms due to availability. Would appreciate if someone could help with the testing on uacce based SVA usages. Thanks, Jacob ChangeLog: V3 - stop passing mm to sva_bind IOMMU ops, no need to take mm refcount in the common SVA code. - deleted flag variable in idxd driver V2 - retained mm argument in iommu_sva_alloc_pasid() - keep generic supervisor flag separated from vt-d's SRE - move flag declaration out of CONFIG_IOMMU_API Jacob Pan (2): iommu/sva: Tighten SVA bind API with explicit flags iommu/sva: Remove mm parameter from SVA bind API drivers/dma/idxd/cdev.c | 2 +- drivers/dma/idxd/init.c | 7 ++ .../iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c | 12 ++ drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h | 5 ++-- drivers/iommu/intel/svm.c | 19 --- drivers/iommu/iommu-sva-lib.c | 11 + drivers/iommu/iommu-sva-lib.h | 2 +- drivers/iommu/iommu.c | 13 +-- drivers/misc/uacce/uacce.c| 2 +- include/linux/intel-iommu.h | 3 +-- include/linux/intel-svm.h | 17 ++ include/linux/iommu.h | 23 ++- 12 files changed, 56 insertions(+), 60 deletions(-) base-commit: e49d033bddf5b565044e2abe4241353959bc9120 -- 2.25.1 ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH V4 05/18] iommu/ioasid: Redefine IOASID set and allocation APIs
On Fri, Apr 16, 2021 at 10:23:32AM -0700, Jacob Pan wrote: > Perhaps similar to cgroup v1 vs v2, it took a long time and with known > limitations in v1. cgroup v2 is still having transition problems, if anything it is a cautionary tale to think really hard about uAPI because transitioning can be really hard. It might be very wise to make /dev/ioasid and /dev/vfio ioctl compatible in some way so existing software has a smoother upgrade path. For instance by defining a default IOASID Jason ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH V4 05/18] iommu/ioasid: Redefine IOASID set and allocation APIs
Hi Alex, On Fri, 16 Apr 2021 09:45:47 -0600, Alex Williamson wrote: > On Fri, 16 Apr 2021 06:12:58 -0700 > Jacob Pan wrote: > > > Hi Jason, > > > > On Thu, 15 Apr 2021 20:07:32 -0300, Jason Gunthorpe > > wrote: > > > On Thu, Apr 15, 2021 at 03:11:19PM +0200, Auger Eric wrote: > > > > Hi Jason, > > > > > > > > On 4/1/21 6:03 PM, Jason Gunthorpe wrote: > > > > > On Thu, Apr 01, 2021 at 02:08:17PM +, Liu, Yi L wrote: > > > > > > > > > >> DMA page faults are delivered to root-complex via page request > > > > >> message and it is per-device according to PCIe spec. Page request > > > > >> handling flow is: > > > > >> > > > > >> 1) iommu driver receives a page request from device > > > > >> 2) iommu driver parses the page request message. Get the > > > > >> RID,PASID, faulted page and requested permissions etc. > > > > >> 3) iommu driver triggers fault handler registered by device > > > > >> driver with iommu_report_device_fault() > > > > > > > > > > This seems confused. > > > > > > > > > > The PASID should define how to handle the page fault, not the > > > > > driver. > > > > > > > > In my series I don't use PASID at all. I am just enabling nested > > > > stage and the guest uses a single context. I don't allocate any > > > > user PASID at any point. > > > > > > > > When there is a fault at physical level (a stage 1 fault that > > > > concerns the guest), this latter needs to be reported and injected > > > > into the guest. The vfio pci driver registers a fault handler to > > > > the iommu layer and in that fault handler it fills a circ bugger > > > > and triggers an eventfd that is listened to by the VFIO-PCI QEMU > > > > device. this latter retrives the faault from the mmapped circ > > > > buffer, it knowns which vIOMMU it is attached to, and passes the > > > > fault to the vIOMMU. Then the vIOMMU triggers and IRQ in the guest. > > > > > > > > We are reusing the existing concepts from VFIO, region, IRQ to do > > > > that. > > > > > > > > For that use case, would you also use /dev/ioasid? > > > > > > /dev/ioasid could do all the things you described vfio-pci as doing, > > > it can even do them the same way you just described. > > > > > > Stated another way, do you plan to duplicate all of this code someday > > > for vfio-cxl? What about for vfio-platform? ARM SMMU can be hooked to > > > platform devices, right? > > > > > > I feel what you guys are struggling with is some choice in the iommu > > > kernel APIs that cause the events to be delivered to the pci_device > > > owner, not the PASID owner. > > > > > > That feels solvable. > > > > > Perhaps more of a philosophical question for you and Alex. There is no > > doubt that the direction you guided for /dev/ioasid is a much cleaner > > one, especially after VDPA emerged as another IOMMU backed framework. > > I think this statement answers all your remaining questions ;) > > > The question is what do we do with the nested translation features that > > have been targeting the existing VFIO-IOMMU for the last three years? > > That predates VDPA. Shall we put a stop marker *after* nested support > > and say no more extensions for VFIO-IOMMU, new features must be built > > on this new interface? > > > > If we were to close a checkout line for some unforeseen reasons, should > > we honor the customers already in line for a long time? > > > > This is not a tactic or excuse for not working on the new /dev/ioasid > > interface. In fact, I believe we can benefit from the lessons learned > > while completing the existing. This will give confidence to the new > > interface. Thoughts? > > I understand a big part of Jason's argument is that we shouldn't be in > the habit of creating duplicate interfaces, we should create one, well > designed interfaces to share among multiple subsystems. As new users > have emerged, our solution needs to change to a common one rather than > a VFIO specific one. The IOMMU uAPI provides an abstraction, but at > the wrong level, requiring userspace interfaces for each subsystem. > > Luckily the IOMMU uAPI is not really exposed as an actual uAPI, but > that changes if we proceed to enable the interfaces to tunnel it > through VFIO. > > The logical answer would therefore be that we don't make that > commitment to the IOMMU uAPI if we believe now that it's fundamentally > flawed. > I agree the uAPI data tunneling is definitely flawed in terms of scalability. I was just thinking it is still a small part of the overall picture. Considering there are other parts such as fault reporting, user space deployment, performance, and security. By completing the support on the existing VFIO framework, it would at least offer a clear landscape where the new /dev/ioasid can improve upon. Perhaps similar to cgroup v1 vs v2, it took a long time and with known limitations in v1. Anyway, I am glad we have a clear direction now. Thanks, Jacob > Ideally this new /dev/ioasid interface, and mak
[PATCH] iommu: remove redundant assignment to variable agaw
From: Colin Ian King
The variable agaw is initialized with a value that is never
read and it is being updated later with a new value as a
counter in a for-loop. The initialization is redundant and
can be removed.
Addresses-Coverity: ("Unused value")
Signed-off-by: Colin Ian King
---
drivers/iommu/intel/iommu.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index 0e04d450c38a..171dd4844ab2 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -564,7 +564,7 @@ static inline int domain_pfn_supported(struct dmar_domain
*domain,
static int __iommu_calculate_agaw(struct intel_iommu *iommu, int max_gaw)
{
unsigned long sagaw;
- int agaw = -1;
+ int agaw;
sagaw = cap_sagaw(iommu->cap);
for (agaw = width_to_agaw(max_gaw);
--
2.30.2
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 5/8] iommu: fix a couple of spelling mistakes
On 26/03/2021 06:24, Zhen Lei wrote: There are several spelling mistakes, as follows: funcions ==> functions distiguish ==> distinguish detroyed ==> destroyed Signed-off-by: Zhen Lei I think that there should be a /s/appropriatley/appropriately/ in iommu.c Thanks, john ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH V4 05/18] iommu/ioasid: Redefine IOASID set and allocation APIs
On Fri, 16 Apr 2021 06:12:58 -0700 Jacob Pan wrote: > Hi Jason, > > On Thu, 15 Apr 2021 20:07:32 -0300, Jason Gunthorpe wrote: > > > On Thu, Apr 15, 2021 at 03:11:19PM +0200, Auger Eric wrote: > > > Hi Jason, > > > > > > On 4/1/21 6:03 PM, Jason Gunthorpe wrote: > > > > On Thu, Apr 01, 2021 at 02:08:17PM +, Liu, Yi L wrote: > > > > > > > >> DMA page faults are delivered to root-complex via page request > > > >> message and it is per-device according to PCIe spec. Page request > > > >> handling flow is: > > > >> > > > >> 1) iommu driver receives a page request from device > > > >> 2) iommu driver parses the page request message. Get the RID,PASID, > > > >> faulted page and requested permissions etc. > > > >> 3) iommu driver triggers fault handler registered by device driver > > > >> with iommu_report_device_fault() > > > > > > > > This seems confused. > > > > > > > > The PASID should define how to handle the page fault, not the driver. > > > > > > > > > > In my series I don't use PASID at all. I am just enabling nested stage > > > and the guest uses a single context. I don't allocate any user PASID at > > > any point. > > > > > > When there is a fault at physical level (a stage 1 fault that concerns > > > the guest), this latter needs to be reported and injected into the > > > guest. The vfio pci driver registers a fault handler to the iommu layer > > > and in that fault handler it fills a circ bugger and triggers an eventfd > > > that is listened to by the VFIO-PCI QEMU device. this latter retrives > > > the faault from the mmapped circ buffer, it knowns which vIOMMU it is > > > attached to, and passes the fault to the vIOMMU. > > > Then the vIOMMU triggers and IRQ in the guest. > > > > > > We are reusing the existing concepts from VFIO, region, IRQ to do that. > > > > > > For that use case, would you also use /dev/ioasid? > > > > /dev/ioasid could do all the things you described vfio-pci as doing, > > it can even do them the same way you just described. > > > > Stated another way, do you plan to duplicate all of this code someday > > for vfio-cxl? What about for vfio-platform? ARM SMMU can be hooked to > > platform devices, right? > > > > I feel what you guys are struggling with is some choice in the iommu > > kernel APIs that cause the events to be delivered to the pci_device > > owner, not the PASID owner. > > > > That feels solvable. > > > Perhaps more of a philosophical question for you and Alex. There is no > doubt that the direction you guided for /dev/ioasid is a much cleaner one, > especially after VDPA emerged as another IOMMU backed framework. I think this statement answers all your remaining questions ;) > The question is what do we do with the nested translation features that have > been targeting the existing VFIO-IOMMU for the last three years? That > predates VDPA. Shall we put a stop marker *after* nested support and say no > more extensions for VFIO-IOMMU, new features must be built on this new > interface? > > If we were to close a checkout line for some unforeseen reasons, should we > honor the customers already in line for a long time? > > This is not a tactic or excuse for not working on the new /dev/ioasid > interface. In fact, I believe we can benefit from the lessons learned while > completing the existing. This will give confidence to the new > interface. Thoughts? I understand a big part of Jason's argument is that we shouldn't be in the habit of creating duplicate interfaces, we should create one, well designed interfaces to share among multiple subsystems. As new users have emerged, our solution needs to change to a common one rather than a VFIO specific one. The IOMMU uAPI provides an abstraction, but at the wrong level, requiring userspace interfaces for each subsystem. Luckily the IOMMU uAPI is not really exposed as an actual uAPI, but that changes if we proceed to enable the interfaces to tunnel it through VFIO. The logical answer would therefore be that we don't make that commitment to the IOMMU uAPI if we believe now that it's fundamentally flawed. Ideally this new /dev/ioasid interface, and making use of it as a VFIO IOMMU backend, should replace type1. Type1 will live on until that interface gets to parity, at which point we may deprecate type1, but it wouldn't make sense to continue to expand type1 in the same direction as we intend /dev/ioasid to take over in the meantime, especially if it means maintaining an otherwise dead uAPI. Thanks, Alex ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 0/8] iommu: fix a couple of spelling mistakes detected by codespell tool
On Fri, Mar 26, 2021 at 02:24:04PM +0800, Zhen Lei wrote: > This detection and correction covers the entire driver/iommu directory. > > Zhen Lei (8): > iommu/pamu: fix a couple of spelling mistakes > iommu/omap: Fix spelling mistake "alignement" -> "alignment" > iommu/mediatek: Fix spelling mistake "phyiscal" -> "physical" > iommu/sun50i: Fix spelling mistake "consits" -> "consists" > iommu: fix a couple of spelling mistakes > iommu/amd: fix a couple of spelling mistakes > iommu/arm-smmu: Fix spelling mistake "initally" -> "initially" > iommu/vt-d: fix a couple of spelling mistakes This patch-set doesn't apply. Please re-send it as a single patch when v5.13-rc1 is released. Thanks, Joerg ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 1/2] iommu: Statically set module owner
On Thu, Apr 01, 2021 at 02:56:25PM +0100, Robin Murphy wrote: > It happens that the 3 drivers which first supported being modular are > also ones which play games with their pgsize_bitmap, so have non-const > iommu_ops where dynamically setting the owner manages to work out OK. > However, it's less than ideal to force that upon all drivers which want > to be modular - like the new sprd-iommu driver which now has a potential > bug in that regard - so let's just statically set the module owner and > let ops remain const wherever possible. > > Reviewed-by: Christoph Hellwig > Acked-by: Will Deacon > Signed-off-by: Robin Murphy > --- > drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 1 + > drivers/iommu/arm/arm-smmu/arm-smmu.c | 1 + > drivers/iommu/sprd-iommu.c | 1 + > drivers/iommu/virtio-iommu.c| 1 + > include/linux/iommu.h | 9 + > 5 files changed, 5 insertions(+), 8 deletions(-) Applied these two directly on-top of my next branch. This essentially means that all topic branches are frozen now until after the merge window. Regards, Joerg ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH V4 05/18] iommu/ioasid: Redefine IOASID set and allocation APIs
Hi Jason, On 4/16/21 4:34 PM, Jason Gunthorpe wrote: > On Fri, Apr 16, 2021 at 04:26:19PM +0200, Auger Eric wrote: > >> This was largely done during several confs including plumber, KVM forum, >> for several years. Also API docs were shared on the ML. I don't remember >> any voice was raised at those moments. > > I don't think anyone objects to the high level ideas, but > implementation does matter. I don't think anyone presented "hey we > will tunnel an uAPI through VFIO to the IOMMU subsystem" - did they? At minimum https://events19.linuxfoundation.cn/wp-content/uploads/2017/11/Shared-Virtual-Memory-in-KVM_Yi-Liu.pdf But most obviously everything is documented in Documentation/userspace-api/iommu.rst where the VFIO tunneling is clearly stated ;-) But well let's work together to design a better and more elegant solution then. Thanks Eric > > Look at the fairly simple IMS situation, for example. This was > presented at plumbers too, and the slides were great - but the > implementation was too hacky. It required a major rework of the x86 > interrupt handling before it was OK. > > Jason > ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH V4 05/18] iommu/ioasid: Redefine IOASID set and allocation APIs
On Fri, Apr 16, 2021 at 04:26:19PM +0200, Auger Eric wrote: > This was largely done during several confs including plumber, KVM forum, > for several years. Also API docs were shared on the ML. I don't remember > any voice was raised at those moments. I don't think anyone objects to the high level ideas, but implementation does matter. I don't think anyone presented "hey we will tunnel an uAPI through VFIO to the IOMMU subsystem" - did they? Look at the fairly simple IMS situation, for example. This was presented at plumbers too, and the slides were great - but the implementation was too hacky. It required a major rework of the x86 interrupt handling before it was OK. Jason ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH V4 05/18] iommu/ioasid: Redefine IOASID set and allocation APIs
Hi, On 4/16/21 4:05 PM, Jason Gunthorpe wrote: > On Fri, Apr 16, 2021 at 03:38:02PM +0200, Auger Eric wrote: > >> The redesign requirement came pretty late in the development process. >> The iommu user API is upstream for a while, the VFIO interfaces have >> been submitted a long time ago and under review for a bunch of time. >> Redesigning everything with a different API, undefined at this point, is >> a major setback for our work and will have a large impact on the >> introduction of features companies are looking forward, hence our >> frustration. > > I will answer both you and Jacob at once. > > This is uAPI, once it is set it can never be changed. > > The kernel process and philosophy is to invest heavily in uAPI > development and review to converge on the best uAPI possible. > > Many past submissions have take a long time to get this right, there > are several high profile uAPI examples. > > Do you think this case is so special, or the concerns so minor, that it > should get to bypass all of the normal process? That's not my intent to bypass any process. I am just trying to understand what needs to be re-designed and for what use case. > > Ask yourself, is anyone advocating for the current direction on > technical merits alone? > > Certainly the patches I last saw where completely disgusting from a > uAPI design perspective. > > It was against the development process to organize this work the way > it was done. Merging a wack of dead code to the kernel to support a > uAPI vision that was never clearly articulated was a big mistake. > > Start from the beginning. Invest heavily in defining a high quality > uAPI. Clearly describe the uAPI to all stake holders. This was largely done during several confs including plumber, KVM forum, for several years. Also API docs were shared on the ML. I don't remember any voice was raised at those moments. Break up the > implementation into patch series without dead code. Make the > patches. Remove the dead code this group has already added. > > None of this should be a surprise. The VDPA discussion and related > "what is a mdev" over a year ago made it pretty clear VFIO is not the > exclusive user of "IOMMU in userspace" and that places limits on what > kind of uAPIs expansion it should experience going forward. Maybe clear for you but most probably not for many other stakeholders. Anyway I do not intend to further argue and I will be happy to learn from you and work with you, Jacob, Liu and all other stakeholders to define a better integration. Thanks Eric > > Jason > ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH V4 05/18] iommu/ioasid: Redefine IOASID set and allocation APIs
On Fri, Apr 16, 2021 at 03:38:02PM +0200, Auger Eric wrote: > The redesign requirement came pretty late in the development process. > The iommu user API is upstream for a while, the VFIO interfaces have > been submitted a long time ago and under review for a bunch of time. > Redesigning everything with a different API, undefined at this point, is > a major setback for our work and will have a large impact on the > introduction of features companies are looking forward, hence our > frustration. I will answer both you and Jacob at once. This is uAPI, once it is set it can never be changed. The kernel process and philosophy is to invest heavily in uAPI development and review to converge on the best uAPI possible. Many past submissions have take a long time to get this right, there are several high profile uAPI examples. Do you think this case is so special, or the concerns so minor, that it should get to bypass all of the normal process? Ask yourself, is anyone advocating for the current direction on technical merits alone? Certainly the patches I last saw where completely disgusting from a uAPI design perspective. It was against the development process to organize this work the way it was done. Merging a wack of dead code to the kernel to support a uAPI vision that was never clearly articulated was a big mistake. Start from the beginning. Invest heavily in defining a high quality uAPI. Clearly describe the uAPI to all stake holders. Break up the implementation into patch series without dead code. Make the patches. Remove the dead code this group has already added. None of this should be a surprise. The VDPA discussion and related "what is a mdev" over a year ago made it pretty clear VFIO is not the exclusive user of "IOMMU in userspace" and that places limits on what kind of uAPIs expansion it should experience going forward. Jason ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH V4 05/18] iommu/ioasid: Redefine IOASID set and allocation APIs
Hi Jason, On 4/16/21 1:07 AM, Jason Gunthorpe wrote: > On Thu, Apr 15, 2021 at 03:11:19PM +0200, Auger Eric wrote: >> Hi Jason, >> >> On 4/1/21 6:03 PM, Jason Gunthorpe wrote: >>> On Thu, Apr 01, 2021 at 02:08:17PM +, Liu, Yi L wrote: >>> DMA page faults are delivered to root-complex via page request message and it is per-device according to PCIe spec. Page request handling flow is: 1) iommu driver receives a page request from device 2) iommu driver parses the page request message. Get the RID,PASID, faulted page and requested permissions etc. 3) iommu driver triggers fault handler registered by device driver with iommu_report_device_fault() >>> >>> This seems confused. >>> >>> The PASID should define how to handle the page fault, not the driver. >> >> In my series I don't use PASID at all. I am just enabling nested stage >> and the guest uses a single context. I don't allocate any user PASID at >> any point. >> >> When there is a fault at physical level (a stage 1 fault that concerns >> the guest), this latter needs to be reported and injected into the >> guest. The vfio pci driver registers a fault handler to the iommu layer >> and in that fault handler it fills a circ bugger and triggers an eventfd >> that is listened to by the VFIO-PCI QEMU device. this latter retrives >> the faault from the mmapped circ buffer, it knowns which vIOMMU it is >> attached to, and passes the fault to the vIOMMU. >> Then the vIOMMU triggers and IRQ in the guest. >> >> We are reusing the existing concepts from VFIO, region, IRQ to do that. >> >> For that use case, would you also use /dev/ioasid? > > /dev/ioasid could do all the things you described vfio-pci as doing, > it can even do them the same way you just described. > > Stated another way, do you plan to duplicate all of this code someday > for vfio-cxl? What about for vfio-platform? ARM SMMU can be hooked to > platform devices, right? vfio regions and IRQ related APIs are common user interfaces exposed by all vfio drivers, including platform. Then the actual circular buffer implementation details can be put in a common lib. as for the thin vfio iommu wrappers, the ones you don't like, they are implemented in type1 code. Maybe the need for /dev/ioasid is more crying for PASID management but for the nested use case, that's not obvious to me and in your different replies, it was not crystal clear where the use case belongs to. The redesign requirement came pretty late in the development process. The iommu user API is upstream for a while, the VFIO interfaces have been submitted a long time ago and under review for a bunch of time. Redesigning everything with a different API, undefined at this point, is a major setback for our work and will have a large impact on the introduction of features companies are looking forward, hence our frustration. Thanks Eric > > I feel what you guys are struggling with is some choice in the iommu > kernel APIs that cause the events to be delivered to the pci_device > owner, not the PASID owner. > > That feels solvable. > > Jason > ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH V4 05/18] iommu/ioasid: Redefine IOASID set and allocation APIs
Hi Jason, On Thu, 15 Apr 2021 20:07:32 -0300, Jason Gunthorpe wrote: > On Thu, Apr 15, 2021 at 03:11:19PM +0200, Auger Eric wrote: > > Hi Jason, > > > > On 4/1/21 6:03 PM, Jason Gunthorpe wrote: > > > On Thu, Apr 01, 2021 at 02:08:17PM +, Liu, Yi L wrote: > > > > > >> DMA page faults are delivered to root-complex via page request > > >> message and it is per-device according to PCIe spec. Page request > > >> handling flow is: > > >> > > >> 1) iommu driver receives a page request from device > > >> 2) iommu driver parses the page request message. Get the RID,PASID, > > >> faulted page and requested permissions etc. > > >> 3) iommu driver triggers fault handler registered by device driver > > >> with iommu_report_device_fault() > > > > > > This seems confused. > > > > > > The PASID should define how to handle the page fault, not the driver. > > > > > > > In my series I don't use PASID at all. I am just enabling nested stage > > and the guest uses a single context. I don't allocate any user PASID at > > any point. > > > > When there is a fault at physical level (a stage 1 fault that concerns > > the guest), this latter needs to be reported and injected into the > > guest. The vfio pci driver registers a fault handler to the iommu layer > > and in that fault handler it fills a circ bugger and triggers an eventfd > > that is listened to by the VFIO-PCI QEMU device. this latter retrives > > the faault from the mmapped circ buffer, it knowns which vIOMMU it is > > attached to, and passes the fault to the vIOMMU. > > Then the vIOMMU triggers and IRQ in the guest. > > > > We are reusing the existing concepts from VFIO, region, IRQ to do that. > > > > For that use case, would you also use /dev/ioasid? > > /dev/ioasid could do all the things you described vfio-pci as doing, > it can even do them the same way you just described. > > Stated another way, do you plan to duplicate all of this code someday > for vfio-cxl? What about for vfio-platform? ARM SMMU can be hooked to > platform devices, right? > > I feel what you guys are struggling with is some choice in the iommu > kernel APIs that cause the events to be delivered to the pci_device > owner, not the PASID owner. > > That feels solvable. > Perhaps more of a philosophical question for you and Alex. There is no doubt that the direction you guided for /dev/ioasid is a much cleaner one, especially after VDPA emerged as another IOMMU backed framework. The question is what do we do with the nested translation features that have been targeting the existing VFIO-IOMMU for the last three years? That predates VDPA. Shall we put a stop marker *after* nested support and say no more extensions for VFIO-IOMMU, new features must be built on this new interface? If we were to close a checkout line for some unforeseen reasons, should we honor the customers already in line for a long time? This is not a tactic or excuse for not working on the new /dev/ioasid interface. In fact, I believe we can benefit from the lessons learned while completing the existing. This will give confidence to the new interface. Thoughts? > Jason Thanks, Jacob ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2] iommu/iova: put free_iova_mem() outside of spinlock iova_rbtree_lock
在 2021/4/16 16:53, John Garry 写道:
On 16/04/2021 04:30, chenxiang (M) wrote:
you need to make this:
if (iova)
free_iova_mem(iova);
as free_iova_mem(iova) dereferences iova:
if (iova->pfn_lo != IOVA_ANCHOR)
kmem_cache_free(iova_cache, iova)
So if iova were NULL, we crash.
Or you can make free_iova_mem() NULL safe.
Right, it has the issue. What about changing it as below?
@@ -472,10 +472,13 @@ free_iova(struct iova_domain *iovad, unsigned
long pfn)
spin_lock_irqsave(&iovad->iova_rbtree_lock, flags);
iova = private_find_iova(iovad, pfn);
- if (iova)
- private_free_iova(iovad, iova);
+ if (!iova) {
+ spin_unlock_irqrestore(&iovad->iova_rbtree_lock, flags);
+ return;
+ }
+ remove_iova(iovad, iova);
spin_unlock_irqrestore(&iovad->iova_rbtree_lock, flags);
-
+ free_iova_mem(iova);
}
EXPORT_SYMBOL_GPL(free_iova);
I don't feel too strongly about how it's done.
Please note that kmem_cache_free() is not NULL safe. So the NULL check
could be added in free_iova_mem(), but we prob don't want silent
free_iova_mem(NULL) calls, so I would stick with changing free_iova().
so I would stick with changing free_iova()
- Do you mean free_iova_mem()?
But i think there is a check (judge iova is NULL) before free_iova_mem()
for following scenarios, and it is not necessary to change in funciton
free_iova_mem():
1) iova_magazine_free_pfns()
2) alloc_iova()
3) free_iova()
4) __free_iova(): Those functions which call __free_iova() have a check
Only for function put_iova_doamin(), it doesn't check iova, but i think
iova in rbtree should not be NULL.
Thanks,
John
.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v3 01/12] iommu: Introduce dirty log tracking framework
Hi Baolu, On 2021/4/15 18:21, Lu Baolu wrote: > Hi, > > On 2021/4/15 15:43, Keqian Zhu wrote: design it as not switchable. I will modify the commit message of patch#12, thanks! >>> I am not sure that I fully get your point. But I can't see any gaps of >>> using iommu_dev_enable/disable_feature() to switch dirty log on and off. >>> Probably I missed anything. >> IOMMU_DEV_FEAT_HWDBM just tells user whether underlying IOMMU driver supports >> dirty tracking, it is not used to management the status of dirty log >> tracking. >> >> The feature reporting is per device, but the status management is per >> iommu_domain. >> Only when all devices in a domain support HWDBM, we can start dirty log for >> the domain. > > So why not > > for_each_device_attached_domain() > iommu_dev_enable_feature(IOMMU_DEV_FEAT_HWDBM) Looks reasonable, but the problem is that we just need to enable dirty log once per domain. > > ? >> >> And I think we'd better not mix the feature reporting and status management. >> Thoughts? >> > > I am worrying about having two sets of APIs for single purpose. From > vendor iommu driver's point of view, this feature is per device. Hence, > it still needs to do the same thing. Yes, we can unify the granule of feature reporting and status management. The basic granule of dirty tracking is iommu_domain, I think it's very reasonable. We need an interface to report the feature of iommu_domain, then the logic is much more clear. Every time we add new device or remove device from the domain, we should update the feature (e.g., maintain a counter of unsupported devices). What do you think about this idea? Thanks, Keqian ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2] iommu/iova: put free_iova_mem() outside of spinlock iova_rbtree_lock
On 16/04/2021 04:30, chenxiang (M) wrote:
you need to make this:
if (iova)
free_iova_mem(iova);
as free_iova_mem(iova) dereferences iova:
if (iova->pfn_lo != IOVA_ANCHOR)
kmem_cache_free(iova_cache, iova)
So if iova were NULL, we crash.
Or you can make free_iova_mem() NULL safe.
Right, it has the issue. What about changing it as below?
@@ -472,10 +472,13 @@ free_iova(struct iova_domain *iovad, unsigned long
pfn)
spin_lock_irqsave(&iovad->iova_rbtree_lock, flags);
iova = private_find_iova(iovad, pfn);
- if (iova)
- private_free_iova(iovad, iova);
+ if (!iova) {
+ spin_unlock_irqrestore(&iovad->iova_rbtree_lock, flags);
+ return;
+ }
+ remove_iova(iovad, iova);
spin_unlock_irqrestore(&iovad->iova_rbtree_lock, flags);
-
+ free_iova_mem(iova);
}
EXPORT_SYMBOL_GPL(free_iova);
I don't feel too strongly about how it's done.
Please note that kmem_cache_free() is not NULL safe. So the NULL check
could be added in free_iova_mem(), but we prob don't want silent
free_iova_mem(NULL) calls, so I would stick with changing free_iova().
Thanks,
John
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
