LinuxLists.cc - [PATCH] iommu: Add Kconfig help text for IOMMU

2023-05-06 13:29:21

Subject: [PATCH] iommu: Add Kconfig help text for IOMMU_SVA

Shared Virtual Addressing (SVA) is not immediately intuitive to people
who work outside IOMMU subsystem, add some explanation to make it less
opaque.
Link: https://lore.kernel.org/lkml/[email protected]/t/#meaa938c91857665b376db59e16b532ba9d4ea013
Signed-off-by: Jacob Pan <[email protected]>
---
drivers/iommu/Kconfig | 11 +++++++++--
1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
index db98c3f86e8c..0f7181007dc1 100644
--- a/drivers/iommu/Kconfig
+++ b/drivers/iommu/Kconfig
@@ -1,4 +1,4 @@
-# SPDX-License-Identifier: GPL-2.0-only
+# PDX-License-Identifier: GPL-2.0-only
# The IOVA library may also be used by non-IOMMU_API users
config IOMMU_IOVA
tristate
@@ -155,7 +155,14 @@ config IOMMU_DMA

# Shared Virtual Addressing
config IOMMU_SVA
- bool
+ bool "Shared Virtual Addressing"
+ help
+ IOMMU and CPU share page tables for user processes thus DMA can use
+ CPU virtual address. Under intel architecture, it is also called
+ Shared Virtual Memory (SVM).
+ With SVA, DMA requests are tagged with PCIe Process Address Space ID
+ (PASID) or Substream ID (ARM SMMU term) with which page tables are
+ associated.

config FSL_PAMU
bool "Freescale IOMMU support"
--
2.25.1

2023-05-06 15:40:56

by kernel test robot

[permalink] [raw]

Subject: Re: [PATCH] iommu: Add Kconfig help text for IOMMU_SVA

Hi Jacob,

kernel test robot noticed the following build errors:

[auto build test ERROR on next-20230505]
[also build test ERROR on linus/master]
[cannot apply to joro-iommu/next v6.3 v6.3-rc7 v6.3-rc6 v6.3]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url: https://github.com/intel-lab-lkp/linux/commits/Jacob-Pan/iommu-Add-Kconfig-help-text-for-IOMMU_SVA/20230506-212836
base: next-20230505
patch link: https://lore.kernel.org/r/20230506133134.1492395-1-jacob.jun.pan%40linux.intel.com
patch subject: [PATCH] iommu: Add Kconfig help text for IOMMU_SVA
config: alpha-defconfig (https://download.01.org/0day-ci/archive/20230506/[email protected]/config)
compiler: alpha-linux-gcc (GCC) 12.1.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/intel-lab-lkp/linux/commit/7a9fdfc3792c64278f1950f3880278b989749944
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Jacob-Pan/iommu-Add-Kconfig-help-text-for-IOMMU_SVA/20230506-212836
git checkout 7a9fdfc3792c64278f1950f3880278b989749944
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=alpha olddefconfig
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=alpha SHELL=/bin/bash drivers/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <[email protected]>
| Link: https://lore.kernel.org/oe-kbuild-all/[email protected]/

All errors (new ones prefixed by >>):

drivers/iommu/iommu-sva.c: In function 'iommu_sva_bind_device':
>> drivers/iommu/iommu-sva.c:69:32: error: invalid use of undefined type 'struct dev_iommu'
69 | max_pasids = dev->iommu->max_pasids;
| ^~
>> drivers/iommu/iommu-sva.c:78:32: error: invalid application of 'sizeof' to incomplete type 'struct iommu_sva'
78 | handle = kzalloc(sizeof(*handle), GFP_KERNEL);
| ^
>> drivers/iommu/iommu-sva.c:109:15: error: invalid use of undefined type 'struct iommu_sva'
109 | handle->dev = dev;
| ^~
drivers/iommu/iommu-sva.c:110:15: error: invalid use of undefined type 'struct iommu_sva'
110 | handle->domain = domain;
| ^~
drivers/iommu/iommu-sva.c: In function 'iommu_sva_unbind_device':
drivers/iommu/iommu-sva.c:134:45: error: invalid use of undefined type 'struct iommu_sva'
134 | struct iommu_domain *domain = handle->domain;
| ^~
drivers/iommu/iommu-sva.c:136:36: error: invalid use of undefined type 'struct iommu_sva'
136 | struct device *dev = handle->dev;
| ^~
drivers/iommu/iommu-sva.c: In function 'iommu_sva_get_pasid':
drivers/iommu/iommu-sva.c:150:45: error: invalid use of undefined type 'struct iommu_sva'
150 | struct iommu_domain *domain = handle->domain;
| ^~
--
In file included from include/linux/spinlock.h:63,
from include/linux/mmzone.h:8,
from include/linux/gfp.h:7,
from include/linux/mm.h:7,
from include/linux/scatterlist.h:8,
from include/linux/iommu.h:10,
from drivers/iommu/io-pgfault.c:8:
drivers/iommu/io-pgfault.c: In function 'iommu_queue_iopf':
>> drivers/iommu/io-pgfault.c:153:35: error: invalid use of undefined type 'struct dev_iommu'
153 | lockdep_assert_held(&param->lock);
| ^~
include/linux/lockdep.h:430:61: note: in definition of macro 'lockdep_assert_held'
430 | #define lockdep_assert_held(l) do { (void)(l); } while (0)
| ^
drivers/iommu/io-pgfault.c:163:27: error: invalid use of undefined type 'struct dev_iommu'
163 | iopf_param = param->iopf_param;
| ^~
drivers/iommu/io-pgfault.c: In function 'iopf_queue_flush_dev':
drivers/iommu/io-pgfault.c:239:26: error: invalid use of undefined type 'struct dev_iommu'
239 | mutex_lock(&param->lock);
| ^~
drivers/iommu/io-pgfault.c:240:27: error: invalid use of undefined type 'struct dev_iommu'
240 | iopf_param = param->iopf_param;
| ^~
drivers/iommu/io-pgfault.c:245:28: error: invalid use of undefined type 'struct dev_iommu'
245 | mutex_unlock(&param->lock);
| ^~
drivers/iommu/io-pgfault.c: In function 'iopf_queue_add_device':
drivers/iommu/io-pgfault.c:307:26: error: invalid use of undefined type 'struct dev_iommu'
307 | mutex_lock(&param->lock);
| ^~
drivers/iommu/io-pgfault.c:308:19: error: invalid use of undefined type 'struct dev_iommu'
308 | if (!param->iopf_param) {
| ^~
drivers/iommu/io-pgfault.c:310:22: error: invalid use of undefined type 'struct dev_iommu'
310 | param->iopf_param = iopf_param;
| ^~
drivers/iommu/io-pgfault.c:313:28: error: invalid use of undefined type 'struct dev_iommu'
313 | mutex_unlock(&param->lock);
| ^~
drivers/iommu/io-pgfault.c: In function 'iopf_queue_remove_device':
drivers/iommu/io-pgfault.c:343:26: error: invalid use of undefined type 'struct dev_iommu'
343 | mutex_lock(&param->lock);
| ^~
drivers/iommu/io-pgfault.c:344:27: error: invalid use of undefined type 'struct dev_iommu'
344 | iopf_param = param->iopf_param;
| ^~
drivers/iommu/io-pgfault.c:347:22: error: invalid use of undefined type 'struct dev_iommu'
347 | param->iopf_param = NULL;
| ^~
drivers/iommu/io-pgfault.c:350:28: error: invalid use of undefined type 'struct dev_iommu'
350 | mutex_unlock(&param->lock);
| ^~

vim +69 drivers/iommu/iommu-sva.c

be51b1d6bbff48 drivers/iommu/iommu-sva-lib.c Lu Baolu 2022-10-31 45
be51b1d6bbff48 drivers/iommu/iommu-sva-lib.c Lu Baolu 2022-10-31 46 /**
be51b1d6bbff48 drivers/iommu/iommu-sva-lib.c Lu Baolu 2022-10-31 47 * iommu_sva_bind_device() - Bind a process address space to a device
be51b1d6bbff48 drivers/iommu/iommu-sva-lib.c Lu Baolu 2022-10-31 48 * @dev: the device
be51b1d6bbff48 drivers/iommu/iommu-sva-lib.c Lu Baolu 2022-10-31 49 * @mm: the mm to bind, caller must hold a reference to mm_users
be51b1d6bbff48 drivers/iommu/iommu-sva-lib.c Lu Baolu 2022-10-31 50 *
be51b1d6bbff48 drivers/iommu/iommu-sva-lib.c Lu Baolu 2022-10-31 51 * Create a bond between device and address space, allowing the device to
be51b1d6bbff48 drivers/iommu/iommu-sva-lib.c Lu Baolu 2022-10-31 52 * access the mm using the PASID returned by iommu_sva_get_pasid(). If a
be51b1d6bbff48 drivers/iommu/iommu-sva-lib.c Lu Baolu 2022-10-31 53 * bond already exists between @device and @mm, an additional internal
be51b1d6bbff48 drivers/iommu/iommu-sva-lib.c Lu Baolu 2022-10-31 54 * reference is taken. Caller must call iommu_sva_unbind_device()
be51b1d6bbff48 drivers/iommu/iommu-sva-lib.c Lu Baolu 2022-10-31 55 * to release each reference.
be51b1d6bbff48 drivers/iommu/iommu-sva-lib.c Lu Baolu 2022-10-31 56 *
be51b1d6bbff48 drivers/iommu/iommu-sva-lib.c Lu Baolu 2022-10-31 57 * iommu_dev_enable_feature(dev, IOMMU_DEV_FEAT_SVA) must be called first, to
be51b1d6bbff48 drivers/iommu/iommu-sva-lib.c Lu Baolu 2022-10-31 58 * initialize the required SVA features.
be51b1d6bbff48 drivers/iommu/iommu-sva-lib.c Lu Baolu 2022-10-31 59 *
be51b1d6bbff48 drivers/iommu/iommu-sva-lib.c Lu Baolu 2022-10-31 60 * On error, returns an ERR_PTR value.
be51b1d6bbff48 drivers/iommu/iommu-sva-lib.c Lu Baolu 2022-10-31 61 */
be51b1d6bbff48 drivers/iommu/iommu-sva-lib.c Lu Baolu 2022-10-31 62 struct iommu_sva *iommu_sva_bind_device(struct device *dev, struct mm_struct *mm)
be51b1d6bbff48 drivers/iommu/iommu-sva-lib.c Lu Baolu 2022-10-31 63 {
be51b1d6bbff48 drivers/iommu/iommu-sva-lib.c Lu Baolu 2022-10-31 64 struct iommu_domain *domain;
be51b1d6bbff48 drivers/iommu/iommu-sva-lib.c Lu Baolu 2022-10-31 65 struct iommu_sva *handle;
be51b1d6bbff48 drivers/iommu/iommu-sva-lib.c Lu Baolu 2022-10-31 66 ioasid_t max_pasids;
be51b1d6bbff48 drivers/iommu/iommu-sva-lib.c Lu Baolu 2022-10-31 67 int ret;
be51b1d6bbff48 drivers/iommu/iommu-sva-lib.c Lu Baolu 2022-10-31 68
be51b1d6bbff48 drivers/iommu/iommu-sva-lib.c Lu Baolu 2022-10-31 @69 max_pasids = dev->iommu->max_pasids;
be51b1d6bbff48 drivers/iommu/iommu-sva-lib.c Lu Baolu 2022-10-31 70 if (!max_pasids)
be51b1d6bbff48 drivers/iommu/iommu-sva-lib.c Lu Baolu 2022-10-31 71 return ERR_PTR(-EOPNOTSUPP);
be51b1d6bbff48 drivers/iommu/iommu-sva-lib.c Lu Baolu 2022-10-31 72
be51b1d6bbff48 drivers/iommu/iommu-sva-lib.c Lu Baolu 2022-10-31 73 /* Allocate mm->pasid if necessary. */
be51b1d6bbff48 drivers/iommu/iommu-sva-lib.c Lu Baolu 2022-10-31 74 ret = iommu_sva_alloc_pasid(mm, 1, max_pasids - 1);
be51b1d6bbff48 drivers/iommu/iommu-sva-lib.c Lu Baolu 2022-10-31 75 if (ret)
be51b1d6bbff48 drivers/iommu/iommu-sva-lib.c Lu Baolu 2022-10-31 76 return ERR_PTR(ret);
be51b1d6bbff48 drivers/iommu/iommu-sva-lib.c Lu Baolu 2022-10-31 77
be51b1d6bbff48 drivers/iommu/iommu-sva-lib.c Lu Baolu 2022-10-31 @78 handle = kzalloc(sizeof(*handle), GFP_KERNEL);
be51b1d6bbff48 drivers/iommu/iommu-sva-lib.c Lu Baolu 2022-10-31 79 if (!handle)
be51b1d6bbff48 drivers/iommu/iommu-sva-lib.c Lu Baolu 2022-10-31 80 return ERR_PTR(-ENOMEM);
be51b1d6bbff48 drivers/iommu/iommu-sva-lib.c Lu Baolu 2022-10-31 81
be51b1d6bbff48 drivers/iommu/iommu-sva-lib.c Lu Baolu 2022-10-31 82 mutex_lock(&iommu_sva_lock);
be51b1d6bbff48 drivers/iommu/iommu-sva-lib.c Lu Baolu 2022-10-31 83 /* Search for an existing domain. */
be51b1d6bbff48 drivers/iommu/iommu-sva-lib.c Lu Baolu 2022-10-31 84 domain = iommu_get_domain_for_dev_pasid(dev, mm->pasid,
be51b1d6bbff48 drivers/iommu/iommu-sva-lib.c Lu Baolu 2022-10-31 85 IOMMU_DOMAIN_SVA);
be51b1d6bbff48 drivers/iommu/iommu-sva-lib.c Lu Baolu 2022-10-31 86 if (IS_ERR(domain)) {
be51b1d6bbff48 drivers/iommu/iommu-sva-lib.c Lu Baolu 2022-10-31 87 ret = PTR_ERR(domain);
be51b1d6bbff48 drivers/iommu/iommu-sva-lib.c Lu Baolu 2022-10-31 88 goto out_unlock;
be51b1d6bbff48 drivers/iommu/iommu-sva-lib.c Lu Baolu 2022-10-31 89 }
be51b1d6bbff48 drivers/iommu/iommu-sva-lib.c Lu Baolu 2022-10-31 90
be51b1d6bbff48 drivers/iommu/iommu-sva-lib.c Lu Baolu 2022-10-31 91 if (domain) {
be51b1d6bbff48 drivers/iommu/iommu-sva-lib.c Lu Baolu 2022-10-31 92 domain->users++;
be51b1d6bbff48 drivers/iommu/iommu-sva-lib.c Lu Baolu 2022-10-31 93 goto out;
be51b1d6bbff48 drivers/iommu/iommu-sva-lib.c Lu Baolu 2022-10-31 94 }
be51b1d6bbff48 drivers/iommu/iommu-sva-lib.c Lu Baolu 2022-10-31 95
be51b1d6bbff48 drivers/iommu/iommu-sva-lib.c Lu Baolu 2022-10-31 96 /* Allocate a new domain and set it on device pasid. */
be51b1d6bbff48 drivers/iommu/iommu-sva-lib.c Lu Baolu 2022-10-31 97 domain = iommu_sva_domain_alloc(dev, mm);
be51b1d6bbff48 drivers/iommu/iommu-sva-lib.c Lu Baolu 2022-10-31 98 if (!domain) {
be51b1d6bbff48 drivers/iommu/iommu-sva-lib.c Lu Baolu 2022-10-31 99 ret = -ENOMEM;
be51b1d6bbff48 drivers/iommu/iommu-sva-lib.c Lu Baolu 2022-10-31 100 goto out_unlock;
be51b1d6bbff48 drivers/iommu/iommu-sva-lib.c Lu Baolu 2022-10-31 101 }
be51b1d6bbff48 drivers/iommu/iommu-sva-lib.c Lu Baolu 2022-10-31 102
be51b1d6bbff48 drivers/iommu/iommu-sva-lib.c Lu Baolu 2022-10-31 103 ret = iommu_attach_device_pasid(domain, dev, mm->pasid);
be51b1d6bbff48 drivers/iommu/iommu-sva-lib.c Lu Baolu 2022-10-31 104 if (ret)
be51b1d6bbff48 drivers/iommu/iommu-sva-lib.c Lu Baolu 2022-10-31 105 goto out_free_domain;
be51b1d6bbff48 drivers/iommu/iommu-sva-lib.c Lu Baolu 2022-10-31 106 domain->users = 1;
be51b1d6bbff48 drivers/iommu/iommu-sva-lib.c Lu Baolu 2022-10-31 107 out:
be51b1d6bbff48 drivers/iommu/iommu-sva-lib.c Lu Baolu 2022-10-31 108 mutex_unlock(&iommu_sva_lock);
be51b1d6bbff48 drivers/iommu/iommu-sva-lib.c Lu Baolu 2022-10-31 @109 handle->dev = dev;
be51b1d6bbff48 drivers/iommu/iommu-sva-lib.c Lu Baolu 2022-10-31 110 handle->domain = domain;
be51b1d6bbff48 drivers/iommu/iommu-sva-lib.c Lu Baolu 2022-10-31 111
be51b1d6bbff48 drivers/iommu/iommu-sva-lib.c Lu Baolu 2022-10-31 112 return handle;
be51b1d6bbff48 drivers/iommu/iommu-sva-lib.c Lu Baolu 2022-10-31 113
be51b1d6bbff48 drivers/iommu/iommu-sva-lib.c Lu Baolu 2022-10-31 114 out_free_domain:
be51b1d6bbff48 drivers/iommu/iommu-sva-lib.c Lu Baolu 2022-10-31 115 iommu_domain_free(domain);
be51b1d6bbff48 drivers/iommu/iommu-sva-lib.c Lu Baolu 2022-10-31 116 out_unlock:
be51b1d6bbff48 drivers/iommu/iommu-sva-lib.c Lu Baolu 2022-10-31 117 mutex_unlock(&iommu_sva_lock);
be51b1d6bbff48 drivers/iommu/iommu-sva-lib.c Lu Baolu 2022-10-31 118 kfree(handle);
be51b1d6bbff48 drivers/iommu/iommu-sva-lib.c Lu Baolu 2022-10-31 119
be51b1d6bbff48 drivers/iommu/iommu-sva-lib.c Lu Baolu 2022-10-31 120 return ERR_PTR(ret);
be51b1d6bbff48 drivers/iommu/iommu-sva-lib.c Lu Baolu 2022-10-31 121 }
be51b1d6bbff48 drivers/iommu/iommu-sva-lib.c Lu Baolu 2022-10-31 122 EXPORT_SYMBOL_GPL(iommu_sva_bind_device);
be51b1d6bbff48 drivers/iommu/iommu-sva-lib.c Lu Baolu 2022-10-31 123

--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests

2023-05-06 15:46:32

by Linus Torvalds

[permalink] [raw]

Subject: Re: [PATCH] iommu: Add Kconfig help text for IOMMU_SVA

On Sat, May 6, 2023 at 6:27 AM Jacob Pan <[email protected]> wrote:
>
> Shared Virtual Addressing (SVA) is not immediately intuitive to people
> who work outside IOMMU subsystem, add some explanation to make it less
> opaque.

So the patch has at least two problems.

The first is that you corrupted the SPDX line.

But the second is that the change from

- bool
+ bool "Shared Virtual Addressing"

means that now Kconfig *asks* about this thing, which was never the
intent. The other Kconfig options that need it all do a

select IOMMU_SVA

to get it picked.

I suspect that's why it then causes errors - because now the test
robot can enable the option even in situations where it makes no sense
and doesn't work.

So no, it's not that the option needs a help entry that can be queried
at Kconfig time. It's that the option needs a name that makes sense
and isn't some random jumble of letters that is only understandable to
people who already know exactly what it is.

Linus

2023-05-06 15:46:52

by kernel test robot

[permalink] [raw]

Subject: Re: [PATCH] iommu: Add Kconfig help text for IOMMU_SVA

Hi Jacob,

kernel test robot noticed the following build warnings:

[auto build test WARNING on next-20230505]
[also build test WARNING on linus/master]
[cannot apply to joro-iommu/next v6.3 v6.3-rc7 v6.3-rc6 v6.3]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url: https://github.com/intel-lab-lkp/linux/commits/Jacob-Pan/iommu-Add-Kconfig-help-text-for-IOMMU_SVA/20230506-212836
base: next-20230505
patch link: https://lore.kernel.org/r/20230506133134.1492395-1-jacob.jun.pan%40linux.intel.com
patch subject: [PATCH] iommu: Add Kconfig help text for IOMMU_SVA
config: sparc-defconfig (https://download.01.org/0day-ci/archive/20230506/[email protected]/config)
compiler: sparc-linux-gcc (GCC) 12.1.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/intel-lab-lkp/linux/commit/7a9fdfc3792c64278f1950f3880278b989749944
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Jacob-Pan/iommu-Add-Kconfig-help-text-for-IOMMU_SVA/20230506-212836
git checkout 7a9fdfc3792c64278f1950f3880278b989749944
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=sparc olddefconfig
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=sparc SHELL=/bin/bash drivers/iommu/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <[email protected]>
| Link: https://lore.kernel.org/oe-kbuild-all/[email protected]/

All warnings (new ones prefixed by >>):

In file included from arch/sparc/include/asm/mmu_context_32.h:7,
from arch/sparc/include/asm/mmu_context.h:7,
from include/linux/mmu_context.h:5,
from drivers/iommu/iommu-sva.c:5:
include/asm-generic/mm_hooks.h:10:40: warning: 'struct mm_struct' declared inside parameter list will not be visible outside of this definition or declaration
10 | static inline int arch_dup_mmap(struct mm_struct *oldmm,
| ^~~~~~~~~
include/asm-generic/mm_hooks.h:16:42: warning: 'struct mm_struct' declared inside parameter list will not be visible outside of this definition or declaration
16 | static inline void arch_exit_mmap(struct mm_struct *mm)
| ^~~~~~~~~
include/asm-generic/mm_hooks.h:20:38: warning: 'struct mm_struct' declared inside parameter list will not be visible outside of this definition or declaration
20 | static inline void arch_unmap(struct mm_struct *mm,
| ^~~~~~~~~
include/asm-generic/mm_hooks.h:25:15: error: unknown type name 'bool'
25 | static inline bool arch_vma_access_permitted(struct vm_area_struct *vma,
| ^~~~
include/asm-generic/mm_hooks.h:26:17: error: unknown type name 'bool'
26 | bool write, bool execute, bool foreign)
| ^~~~
include/asm-generic/mm_hooks.h:1:1: note: 'bool' is defined in header '<stdbool.h>'; did you forget to '#include <stdbool.h>'?
+++ |+#include <stdbool.h>
1 | /* SPDX-License-Identifier: GPL-2.0 */
include/asm-generic/mm_hooks.h:26:29: error: unknown type name 'bool'
26 | bool write, bool execute, bool foreign)
| ^~~~
include/asm-generic/mm_hooks.h:26:29: note: 'bool' is defined in header '<stdbool.h>'; did you forget to '#include <stdbool.h>'?
include/asm-generic/mm_hooks.h:26:43: error: unknown type name 'bool'
26 | bool write, bool execute, bool foreign)
| ^~~~
include/asm-generic/mm_hooks.h:26:43: note: 'bool' is defined in header '<stdbool.h>'; did you forget to '#include <stdbool.h>'?
>> arch/sparc/include/asm/mmu_context_32.h:13:54: warning: 'struct mm_struct' declared inside parameter list will not be visible outside of this definition or declaration
13 | int init_new_context(struct task_struct *tsk, struct mm_struct *mm);
| ^~~~~~~~~
>> arch/sparc/include/asm/mmu_context_32.h:13:29: warning: 'struct task_struct' declared inside parameter list will not be visible outside of this definition or declaration
13 | int init_new_context(struct task_struct *tsk, struct mm_struct *mm);
| ^~~~~~~~~~~
arch/sparc/include/asm/mmu_context_32.h:21:29: warning: 'struct mm_struct' declared inside parameter list will not be visible outside of this definition or declaration
21 | void destroy_context(struct mm_struct *mm);
| ^~~~~~~~~
arch/sparc/include/asm/mmu_context_32.h:25:23: warning: 'struct task_struct' declared inside parameter list will not be visible outside of this definition or declaration
25 | struct task_struct *tsk);
| ^~~~~~~~~~~
arch/sparc/include/asm/mmu_context_32.h:24:23: warning: 'struct mm_struct' declared inside parameter list will not be visible outside of this definition or declaration
24 | void switch_mm(struct mm_struct *old_mm, struct mm_struct *mm,
| ^~~~~~~~~
In file included from arch/sparc/include/asm/mmu.h:7,
from include/linux/mmu_context.h:6:
arch/sparc/include/asm/mmu_32.h:9:8: error: unknown type name 'ctxd_t'
9 | extern ctxd_t *srmmu_ctx_table_phys;
| ^~~~~~
include/linux/mmu_context.h:39:15: error: unknown type name 'bool'
39 | static inline bool arch_pgtable_dma_compat(struct mm_struct *mm)
| ^~~~
include/linux/mmu_context.h: In function 'arch_pgtable_dma_compat':
include/linux/mmu_context.h:41:16: error: 'true' undeclared (first use in this function)
41 | return true;
| ^~~~
include/linux/mmu_context.h:7:1: note: 'true' is defined in header '<stdbool.h>'; did you forget to '#include <stdbool.h>'?
6 | #include <asm/mmu.h>
+++ |+#include <stdbool.h>
7 |
include/linux/mmu_context.h:41:16: note: each undeclared identifier is reported only once for each function it appears in
41 | return true;
| ^~~~
drivers/iommu/iommu-sva.c: In function 'iommu_sva_bind_device':
drivers/iommu/iommu-sva.c:69:32: error: invalid use of undefined type 'struct dev_iommu'
69 | max_pasids = dev->iommu->max_pasids;
| ^~
drivers/iommu/iommu-sva.c:78:32: error: invalid application of 'sizeof' to incomplete type 'struct iommu_sva'
78 | handle = kzalloc(sizeof(*handle), GFP_KERNEL);
| ^
drivers/iommu/iommu-sva.c:109:15: error: invalid use of undefined type 'struct iommu_sva'
109 | handle->dev = dev;
| ^~
drivers/iommu/iommu-sva.c:110:15: error: invalid use of undefined type 'struct iommu_sva'
110 | handle->domain = domain;
| ^~
drivers/iommu/iommu-sva.c: In function 'iommu_sva_unbind_device':
drivers/iommu/iommu-sva.c:134:45: error: invalid use of undefined type 'struct iommu_sva'
134 | struct iommu_domain *domain = handle->domain;
| ^~
drivers/iommu/iommu-sva.c:136:36: error: invalid use of undefined type 'struct iommu_sva'
136 | struct device *dev = handle->dev;
| ^~
drivers/iommu/iommu-sva.c: In function 'iommu_sva_get_pasid':
drivers/iommu/iommu-sva.c:150:45: error: invalid use of undefined type 'struct iommu_sva'
150 | struct iommu_domain *domain = handle->domain;
| ^~
include/linux/mmu_context.h: In function 'arch_pgtable_dma_compat':
include/linux/mmu_context.h:42:1: error: control reaches end of non-void function [-Werror=return-type]
42 | }
| ^
cc1: some warnings being treated as errors

vim +13 arch/sparc/include/asm/mmu_context_32.h

f5e706ad886b6a include/asm-sparc/mmu_context_32.h Sam Ravnborg 2008-07-17 8
b585e8551b352c arch/sparc/include/asm/mmu_context_32.h Sam Ravnborg 2012-07-26 9 /* Initialize a new mmu context. This is invoked when a new
f5e706ad886b6a include/asm-sparc/mmu_context_32.h Sam Ravnborg 2008-07-17 10 * address space instance (unique or shared) is instantiated.
f5e706ad886b6a include/asm-sparc/mmu_context_32.h Sam Ravnborg 2008-07-17 11 */
ca0f34b575ade0 arch/sparc/include/asm/mmu_context_32.h Nicholas Piggin 2020-09-02 12 #define init_new_context init_new_context
b585e8551b352c arch/sparc/include/asm/mmu_context_32.h Sam Ravnborg 2012-07-26 @13 int init_new_context(struct task_struct *tsk, struct mm_struct *mm);
f5e706ad886b6a include/asm-sparc/mmu_context_32.h Sam Ravnborg 2008-07-17 14

--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests

2023-05-06 15:47:18

by kernel test robot

[permalink] [raw]

Subject: Re: [PATCH] iommu: Add Kconfig help text for IOMMU_SVA

2023-05-06 23:03:00

by Jacob Pan

[permalink] [raw]

Subject: Re: [PATCH] iommu: Add Kconfig help text for IOMMU_SVA

Hi Linus,

On Sat, 6 May 2023 08:43:08 -0700, Linus Torvalds
<[email protected]> wrote:

> On Sat, May 6, 2023 at 6:27 AM Jacob Pan <[email protected]>
> wrote:
> >
> > Shared Virtual Addressing (SVA) is not immediately intuitive to people
> > who work outside IOMMU subsystem, add some explanation to make it less
> > opaque.
>
> So the patch has at least two problems.
>
> The first is that you corrupted the SPDX line.
>
Sorry, my mistake. Will not try sending patches before fully waking up.

> But the second is that the change from
>
> - bool
> + bool "Shared Virtual Addressing"
>
> means that now Kconfig *asks* about this thing, which was never the
> intent. The other Kconfig options that need it all do a
>
> select IOMMU_SVA
>
> to get it picked.
>
> I suspect that's why it then causes errors - because now the test
> robot can enable the option even in situations where it makes no sense
> and doesn't work.
>
> So no, it's not that the option needs a help entry that can be queried
> at Kconfig time. It's that the option needs a name that makes sense
> and isn't some random jumble of letters that is only understandable to
> people who already know exactly what it is.
Right, how about IOMMU_SHARING_CPU_PGTABLE?

Since this is more of a one-sided relationship where IOMMU walks CPU page
tables.

Thanks,

Jacob

2023-05-07 19:15:29

by Linus Torvalds

[permalink] [raw]

Subject: Re: [PATCH] iommu: Add Kconfig help text for IOMMU_SVA

On Sat, May 6, 2023 at 3:03 PM Jacob Pan <[email protected]> wrote:
>
> Right, how about IOMMU_SHARING_CPU_PGTABLE?

I think from a VM / process angle, I'd actually prefer calling the
"pasid" part just that: IOMMU_PASID.

The VM code certainly understands about address space IDs, even if
people have called them different things: normal people called them
ASID's long ago, then Intel at some pointed decided that "PCID" made
sense as a name (narrator: "no it didn't"), and then you got that
combined "PASID" thing.

Now, it may be that this then goes hand-in-hand with other IOMMU code
that isn't *about* PASID itself, but that depends on PASID's being
present, and so I'd just expect IOMMU_PASID to be one of those options
that are selected by other options.

So maybe there is some part of IOMMU_SVA that is not about PASID
itself, but I really think that the PASID code itself should just have
that CONFIG_PASID around it.

End result: from a legibility standpoint, I think it could be as
simple as having that

config IOMMU_SVA

option have a "select IOMMU_PASID".

Then make the VM/process PASID code depend on that. Maybe the "struct
device *" stuff makes more sense under CONFIG_IOMMU_SVA, ie things
like iopf_queue_add_device() and friends.

How does that sound? Maybe those two options then always end up going
together, but even if that is the case, I think from a VM/process
standpoint it makes a lot more sense to simply have a "PASID enabled"
option. It's much more understandable in that context, while something
like "IOMMU_SVA" really is just a random jumble of letters to a VM
person.

And while the individual words in IOMMU_SHARING_CPU_PGTABLE all make
sense, it's not clear what the combination means, and why it should
have anything to do with then having an address space identifier for
it.

Linus

2023-05-08 16:39:04

by Jacob Pan

[permalink] [raw]

Subject: Re: [PATCH] iommu: Add Kconfig help text for IOMMU_SVA

Hi Linus,

On Sun, 7 May 2023 11:52:18 -0700, Linus Torvalds
<[email protected]> wrote:

> On Sat, May 6, 2023 at 3:03 PM Jacob Pan <[email protected]>
> wrote:
> >
> > Right, how about IOMMU_SHARING_CPU_PGTABLE?
>
> I think from a VM / process angle, I'd actually prefer calling the
> "pasid" part just that: IOMMU_PASID.
>
> The VM code certainly understands about address space IDs, even if
> people have called them different things: normal people called them
> ASID's long ago, then Intel at some pointed decided that "PCID" made
> sense as a name (narrator: "no it didn't"), and then you got that
> combined "PASID" thing.
>
> Now, it may be that this then goes hand-in-hand with other IOMMU code
> that isn't *about* PASID itself, but that depends on PASID's being
> present, and so I'd just expect IOMMU_PASID to be one of those options
> that are selected by other options.
>
> So maybe there is some part of IOMMU_SVA that is not about PASID
> itself, but I really think that the PASID code itself should just have
> that CONFIG_PASID around it.
Conversely, we could also have some part of PASID that is not about SVA.
e.g. Today, on PASID enabled IOMMUs, DMA request w/o PASID (legacy) uses a
special PASID 0. This has nothing to do with mm->pasid.

> End result: from a legibility standpoint, I think it could be as
> simple as having that
>
> config IOMMU_SVA
>
> option have a "select IOMMU_PASID".
> Then make the VM/process PASID code depend on that. Maybe the "struct
> device *" stuff makes more sense under CONFIG_IOMMU_SVA, ie things
> like iopf_queue_add_device() and friends.
right, we don;t support non-PASID IOPF.

> How does that sound? Maybe those two options then always end up going
> together, but even if that is the case, I think from a VM/process
> standpoint it makes a lot more sense to simply have a "PASID enabled"
> option. It's much more understandable in that context, while something
> like "IOMMU_SVA" really is just a random jumble of letters to a VM
> person.
My only concern is the case above where DMA API uses a PASID for legacy DMA
requests w/o PASID. I am also trying to add non-zero PASID for
Intel's ENQCMDS.
https://lore.kernel.org/linux-iommu/[email protected]/
The PASIDs used in this case uses IOVA page tables, not shared with any
mm_struct.

From this use case, we need to select IOMMU_PASID, but not necessarily for
mm->pasid which IMHO is only meaningful when IOMMU shares page tables with
the CPU.

> And while the individual words in IOMMU_SHARING_CPU_PGTABLE all make
> sense, it's not clear what the combination means, and why it should
> have anything to do with then having an address space identifier for
> it.
>
> Linus
>

Thanks,

Jacob

2023-05-08 16:47:19

by Linus Torvalds

[permalink] [raw]

Subject: Re: [PATCH] iommu: Add Kconfig help text for IOMMU_SVA

On Mon, May 8, 2023 at 9:35 AM Jacob Pan <[email protected]> wrote:
>
> Conversely, we could also have some part of PASID that is not about SVA.

If that's the case, then we should *definitely* have that IOMMU_PASID
kind of config option.

Then IOMMU_SVA - that needs PASID - can "select" it, but any other use
- that doesn't bother about SVA but might want PASID for its own
nefarious purposes - can also select it.

This is why we have config options. Not just for legibility, but also
for "there are different scenarios".

Linus

2023-05-08 17:05:08

by Jason Gunthorpe

[permalink] [raw]

Subject: Re: [PATCH] iommu: Add Kconfig help text for IOMMU_SVA

On Mon, May 08, 2023 at 09:42:18AM -0700, Linus Torvalds wrote:
> On Mon, May 8, 2023 at 9:35 AM Jacob Pan <[email protected]> wrote:
> >
> > Conversely, we could also have some part of PASID that is not about SVA.
>
> If that's the case, then we should *definitely* have that IOMMU_PASID
> kind of config option.
>
> Then IOMMU_SVA - that needs PASID - can "select" it, but any other use
> - that doesn't bother about SVA but might want PASID for its own
> nefarious purposes - can also select it.

Yes, this has been unwinding slowly, but this mm part is about SVA,
and specifically ENQCMD, and not about PASID, other than SVA and
ENCQCMD are both using PASID.

CONFIG_IOMMU_MM_PASID perhaps? Says what it does without a clue about
why it does it. x86 arch code could select it ideally?

Jason

2023-05-08 17:30:01

by Linus Torvalds

[permalink] [raw]

Subject: Re: [PATCH] iommu: Add Kconfig help text for IOMMU_SVA

On Mon, May 8, 2023 at 9:55 AM Jason Gunthorpe <[email protected]> wrote:
>
> CONFIG_IOMMU_MM_PASID perhaps? Says what it does without a clue about
> why it does it. x86 arch code could select it ideally?

Maybe we don't even need the "IOMMU" part. It's a core x86
architecture feature. Maybe it usually (always?) gets used within the
framework of some IOMMU work, but I guess ENCQCMD could be used in
some hardwired way even without that (ie is it possible to have just
some "basic PASID set up by VMM environment" thing?)

I don't actually know who uses it and how, so I'm probably not the
right person to name it, but just looking at the x86 code that sets
it, the PASID code technically has no connection to any iommu code,
it's literally a core CPU feature with an MSR and some magic faulting
thing, and seems to be possibly usable as-is.

That existing

#ifdef CONFIG_IOMMU_SVA

in the x86 traps.c code just looks odd, and making it be
CONFIG_IOMMU_MM_PASID sounds better to me. I'm just not sure about the
"IOMMU" part either. Just "MM_PASID"?

That said, the arm-smmu-v3-sva.c code clearly *also* uses pasid,
except it seems to really want to call it "ssid".

So "having a per-mm ASID for IO" is clearly a common feature. But
naming seems hard, with x86 calling it PASID, arm64 seemingly calling
it "SSID".

Right now the only user *does* seem to be through the IOMMU SVA code,
but that may or may not be fundamental.

Now, "SSID" is a completely horrible name, as I immediately realized
when I tried to google for it. So arm64 is just wrong, and we're most
definitely continuing to call it PASID.

I'd lean towards just "CONFIG_MM_PASID" or something, but at some
point this is bikeshedding, and I don't know about any possible
non-iommu direct uses?

Linus

2023-05-08 20:31:18

by Jacob Pan

[permalink] [raw]

Subject: Re: [PATCH] iommu: Add Kconfig help text for IOMMU_SVA

Hi Linus,

On Mon, 8 May 2023 10:17:53 -0700, Linus Torvalds
<[email protected]> wrote:

> On Mon, May 8, 2023 at 9:55 AM Jason Gunthorpe <[email protected]> wrote:
> >
> > CONFIG_IOMMU_MM_PASID perhaps? Says what it does without a clue about
> > why it does it. x86 arch code could select it ideally?
>
> Maybe we don't even need the "IOMMU" part. It's a core x86
> architecture feature. Maybe it usually (always?) gets used within the
> framework of some IOMMU work, but I guess ENCQCMD could be used in
> some hardwired way even without that (ie is it possible to have just
> some "basic PASID set up by VMM environment" thing?)
>
> I don't actually know who uses it and how, so I'm probably not the
> right person to name it, but just looking at the x86 code that sets
> it, the PASID code technically has no connection to any iommu code,
> it's literally a core CPU feature with an MSR and some magic faulting
> thing, and seems to be possibly usable as-is.
>
> That existing
>
> #ifdef CONFIG_IOMMU_SVA
>
> in the x86 traps.c code just looks odd, and making it be
> CONFIG_IOMMU_MM_PASID sounds better to me. I'm just not sure about the
> "IOMMU" part either. Just "MM_PASID"?
>
> That said, the arm-smmu-v3-sva.c code clearly *also* uses pasid,
> except it seems to really want to call it "ssid".
>
> So "having a per-mm ASID for IO" is clearly a common feature. But
> naming seems hard, with x86 calling it PASID, arm64 seemingly calling
> it "SSID".
>
> Right now the only user *does* seem to be through the IOMMU SVA code,
> but that may or may not be fundamental.
>
> Now, "SSID" is a completely horrible name, as I immediately realized
> when I tried to google for it. So arm64 is just wrong, and we're most
> definitely continuing to call it PASID.
>
> I'd lean towards just "CONFIG_MM_PASID" or something, but at some
> point this is bikeshedding, and I don't know about any possible
> non-iommu direct uses?
+Jean who has mentioned potential use of PASID without IOMMU. But I don't
think there is anything in the current kernel.
Leave the name MM_PASID aside, I am trying to capture the discussion with a
patch below. I am struggling to get a clean solution since SVA code is
common as you said "having a per-mm ASID for IO". Having x86 Kconfig select
MM_PASID would be redundant if it is already selected by IOMMU_SVA. Perhaps
I am totally missing the point.

diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
index 58b1f208eff5..d69acc69bbbb 100644
--- a/arch/x86/kernel/traps.c
+++ b/arch/x86/kernel/traps.c
@@ -652,7 +652,7 @@ static bool fixup_iopl_exception(struct pt_regs *regs)
*/
static bool try_fixup_enqcmd_gp(void)
{
-#ifdef CONFIG_IOMMU_SVA
+#ifdef CONFIG_IOMMU_MM_PASID
u32 pasid;

/*
diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
index db98c3f86e8c..7106f3af74ee 100644
--- a/drivers/iommu/Kconfig
+++ b/drivers/iommu/Kconfig
@@ -153,9 +153,13 @@ config IOMMU_DMA
select IRQ_MSI_IOMMU
select NEED_SG_DMA_LENGTH

+config IOMMU_MM_PASID
+ bool
+
# Shared Virtual Addressing
config IOMMU_SVA
bool
+ select IOMMU_MM_PASID

config FSL_PAMU
bool "Freescale IOMMU support"
diff --git a/drivers/iommu/intel/Kconfig b/drivers/iommu/intel/Kconfig
index 2e56bd79f589..b4d7bd68a911 100644
--- a/drivers/iommu/intel/Kconfig
+++ b/drivers/iommu/intel/Kconfig
@@ -50,6 +50,7 @@ config INTEL_IOMMU_SVM
depends on X86_64
select MMU_NOTIFIER
select IOMMU_SVA
+ select IOMMU_MM_PASID
help
Shared Virtual Memory (SVM) provides a facility for devices
to access DMA resources through process address space by
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index e8c9a7da1060..bdd7f4c1b9ad 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -1166,22 +1166,12 @@ static inline bool tegra_dev_iommu_get_stream_id(struct device *dev, u32 *stream
}

#ifdef CONFIG_IOMMU_SVA
-static inline void mm_pasid_init(struct mm_struct *mm)
-{
- mm->pasid = IOMMU_PASID_INVALID;
-}
-static inline bool mm_valid_pasid(struct mm_struct *mm)
-{
- return mm->pasid != IOMMU_PASID_INVALID;
-}
-void mm_pasid_drop(struct mm_struct *mm);
struct iommu_sva *iommu_sva_bind_device(struct device *dev,
struct mm_struct *mm);
void iommu_sva_unbind_device(struct iommu_sva *handle);
u32 iommu_sva_get_pasid(struct iommu_sva *handle);
#else
-static inline struct iommu_sva *
-iommu_sva_bind_device(struct device *dev, struct mm_struct *mm)
+static inline struct iommu_sva *iommu_sva_bind_device(struct device *dev, struct mm_struct *mm)
{
return NULL;
}
@@ -1194,9 +1184,22 @@ static inline u32 iommu_sva_get_pasid(struct iommu_sva *handle)
{
return IOMMU_PASID_INVALID;
}
+#endif /* CONFIG_IOMMU_SVA */
+
+#ifdef CONFIG_IOMMU_MM_PASID
+static inline void mm_pasid_init(struct mm_struct *mm)
+{
+ mm->pasid = IOMMU_PASID_INVALID;
+}
+static inline bool mm_valid_pasid(struct mm_struct *mm)
+{
+ return mm->pasid != IOMMU_PASID_INVALID;
+}
+void mm_pasid_drop(struct mm_struct *mm);
+#else
static inline void mm_pasid_init(struct mm_struct *mm) {}
static inline bool mm_valid_pasid(struct mm_struct *mm) { return false; }
static inline void mm_pasid_drop(struct mm_struct *mm) {}
-#endif /* CONFIG_IOMMU_SVA */
+#endif /* CONFIG_IOMMU_MM_PASID */

#endif /* __LINUX_IOMMU_H */
diff --git a/include/linux/mm_types.h b/include/linux/mm_types.h
index 306a3d1a0fa6..70740a4ab58a 100644
--- a/include/linux/mm_types.h
+++ b/include/linux/mm_types.h
@@ -771,7 +771,7 @@ struct mm_struct {
#endif
struct work_struct async_put_work;

-#ifdef CONFIG_IOMMU_SVA
+#ifdef CONFIG_IOMMU_MM_PASID
u32 pasid;
#endif
#ifdef CONFIG_KSM
diff --git a/include/linux/sched.h b/include/linux/sched.h
index eed5d65b8d1f..0b6498eafb0c 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -945,7 +945,7 @@ struct task_struct {
/* Recursion prevention for eventfd_signal() */
unsigned in_eventfd:1;
#endif
-#ifdef CONFIG_IOMMU_SVA
+#ifdef CONFIG_IOMMU_MM_PASID
unsigned pasid_activated:1;
#endif
#ifdef CONFIG_CPU_SUP_INTEL
diff --git a/kernel/fork.c b/kernel/fork.c
index ed4e01daccaa..cb02ddadd6fb 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -1177,7 +1177,7 @@ static struct task_struct *dup_task_struct(struct task_struct *orig, int node)
tsk->use_memdelay = 0;
#endif

-#ifdef CONFIG_IOMMU_SVA
+#ifdef CONFIG_IOMMU_MM_PASID
tsk->pasid_activated = 0;
#endif

diff --git a/mm/init-mm.c b/mm/init-mm.c
index efa97b57acfd..b97414c2b2f7 100644
--- a/mm/init-mm.c
+++ b/mm/init-mm.c
@@ -42,7 +42,7 @@ struct mm_struct init_mm = {
#endif
.user_ns = &init_user_ns,
.cpu_bitmap = CPU_BITS_NONE,
-#ifdef CONFIG_IOMMU_SVA
+#ifdef CONFIG_IOMMU_MM_PASID
.pasid = IOMMU_PASID_INVALID,
#endif
INIT_MM_CONTEXT(init_mm)

Thanks,

Jacob

2023-05-09 00:23:08

by Tian, Kevin

[permalink] [raw]

Subject: RE: [PATCH] iommu: Add Kconfig help text for IOMMU_SVA

> From: Linus Torvalds <[email protected]>
> Sent: Tuesday, May 9, 2023 1:18 AM
>
> On Mon, May 8, 2023 at 9:55 AM Jason Gunthorpe <[email protected]> wrote:
> >
> > CONFIG_IOMMU_MM_PASID perhaps? Says what it does without a clue
> about
> > why it does it. x86 arch code could select it ideally?
>
> Maybe we don't even need the "IOMMU" part. It's a core x86
> architecture feature. Maybe it usually (always?) gets used within the
> framework of some IOMMU work, but I guess ENCQCMD could be used in
> some hardwired way even without that (ie is it possible to have just
> some "basic PASID set up by VMM environment" thing?)

This makes sense to me. At least that is one possible way in concept,
though we don't have such example now.

>
> I don't actually know who uses it and how, so I'm probably not the
> right person to name it, but just looking at the x86 code that sets
> it, the PASID code technically has no connection to any iommu code,
> it's literally a core CPU feature with an MSR and some magic faulting
> thing, and seems to be possibly usable as-is.
>
> That existing
>
> #ifdef CONFIG_IOMMU_SVA
>
> in the x86 traps.c code just looks odd, and making it be
> CONFIG_IOMMU_MM_PASID sounds better to me. I'm just not sure about
> the
> "IOMMU" part either. Just "MM_PASID"?
>
> That said, the arm-smmu-v3-sva.c code clearly *also* uses pasid,
> except it seems to really want to call it "ssid".
>
> So "having a per-mm ASID for IO" is clearly a common feature. But

Yes. Currently PASID for mm is globally allocated so each mm is
associated with a single unique pasid.

> naming seems hard, with x86 calling it PASID, arm64 seemingly calling
> it "SSID".
>
> Right now the only user *does* seem to be through the IOMMU SVA code,
> but that may or may not be fundamental.
>
> Now, "SSID" is a completely horrible name, as I immediately realized
> when I tried to google for it. So arm64 is just wrong, and we're most
> definitely continuing to call it PASID.

We have undergone this naming game when enabling SVA before.

At that point IOASID was considered as a generic term to cover both
PCI PASID and ARM specific SSID. But in the end PASID/IOASID are
still mixed in many common code.

IMHO sticking with PASID should be fine.

>
> I'd lean towards just "CONFIG_MM_PASID" or something, but at some
> point this is bikeshedding, and I don't know about any possible
> non-iommu direct uses?
>

I agree that CONFIG_MM_PASID is more accurate in definition.

2023-05-09 00:23:33

by Tian, Kevin

[permalink] [raw]

Subject: RE: [PATCH] iommu: Add Kconfig help text for IOMMU_SVA

> From: Jacob Pan <[email protected]>
> Sent: Tuesday, May 9, 2023 4:22 AM
>
> Leave the name MM_PASID aside, I am trying to capture the discussion with
> a
> patch below. I am struggling to get a clean solution since SVA code is
> common as you said "having a per-mm ASID for IO". Having x86 Kconfig
> select
> MM_PASID would be redundant if it is already selected by IOMMU_SVA.

Currently SVA relies on mm->pasid, w/ or w/o ENQCMD.

From this angle having IOMMU_SVA select MM_PASID looks sufficient to me.

If one day there is a reason to constrain mm->pasid only for ENQCMD, then
we can move to let arch config select MM_PASID.

2023-05-09 02:15:06

by Lu Baolu

[permalink] [raw]

Subject: Re: [PATCH] iommu: Add Kconfig help text for IOMMU_SVA

On 5/9/23 4:21 AM, Jacob Pan wrote:
> diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
> index db98c3f86e8c..7106f3af74ee 100644
> --- a/drivers/iommu/Kconfig
> +++ b/drivers/iommu/Kconfig
> @@ -153,9 +153,13 @@ config IOMMU_DMA
> select IRQ_MSI_IOMMU
> select NEED_SG_DMA_LENGTH
>
> +config IOMMU_MM_PASID
> + bool
> +
> # Shared Virtual Addressing
> config IOMMU_SVA
> bool
> + select IOMMU_MM_PASID
>
> config FSL_PAMU
> bool "Freescale IOMMU support"
> diff --git a/drivers/iommu/intel/Kconfig b/drivers/iommu/intel/Kconfig
> index 2e56bd79f589..b4d7bd68a911 100644
> --- a/drivers/iommu/intel/Kconfig
> +++ b/drivers/iommu/intel/Kconfig
> @@ -50,6 +50,7 @@ config INTEL_IOMMU_SVM
> depends on X86_64
> select MMU_NOTIFIER
> select IOMMU_SVA
> + select IOMMU_MM_PASID

IOMMU_SVA has already selected IOMMU_MM_PASID, so there's no need to
select it again here?

IOMMU_SVA selects IOMMU_MM_PASID, the vt-d driver itself has no need to
know about this.

> help
> Shared Virtual Memory (SVM) provides a facility for devices
> to access DMA resources through process address space by

Best regards,
baolu

2023-05-09 23:42:12

by Jason Gunthorpe

[permalink] [raw]

Subject: Re: [PATCH] iommu: Add Kconfig help text for IOMMU_SVA

On Mon, May 08, 2023 at 10:17:53AM -0700, Linus Torvalds wrote:

> That said, the arm-smmu-v3-sva.c code clearly *also* uses pasid,
> except it seems to really want to call it "ssid".

This code is incorrect.. It is using the mm->pasid to try to deliver
an invalidation, but our new design does not restrict the userspace to
a single PASID, thus it will fail to properly synchronize the ATC in
some degenerate cases. In general the driver needs to be able to go
over a list of PASIDs that the domains are linked to and replicate
invalidations (of any sort) to the whole list.

It is still a WIP to make this driver implement the driver contract
for PASID logic properly.

> Now, "SSID" is a completely horrible name, as I immediately realized
> when I tried to google for it. So arm64 is just wrong, and we're most
> definitely continuing to call it PASID.

Yes, PASID comes from PCIe so we are using that.

> I'd lean towards just "CONFIG_MM_PASID" or something, but at some
> point this is bikeshedding, and I don't know about any possible
> non-iommu direct uses?

I don't think there is any other use, IOMMU should be the only
consumer of PASID, so it is a fine name as well.

Thanks,
Jason