diff mbox series

[17/23] iommu/vt-d: Prepare for multiple DMA domain types

Message ID 11efdfa4ee223d12769d17459fcf789c626d7b82.1626888445.git.robin.murphy@arm.com (mailing list archive)
State New, archived
Headers show
Series iommu: Refactor DMA domain strictness | expand

Commit Message

Robin Murphy July 21, 2021, 6:20 p.m. UTC 
In preparation for the strict vs. non-strict decision for DMA domains to
be expressed in the domain type, make sure we expose our flush queue
awareness by accepting the new domain type, and test the specific
feature flag where we want to identify DMA domains in general. The DMA
ops setup can simply be made unconditional, since iommu-dma already
knows not to touch identity domains.

Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---
 drivers/iommu/intel/iommu.c | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

Comments

kernel test robot July 22, 2021, 4:44 p.m. UTC | #1 
Hi Robin,

I love your patch! Yet something to improve:

[auto build test ERROR on iommu/next]
[also build test ERROR on rockchip/for-next linus/master v5.14-rc2 next-20210722]
[cannot apply to sunxi/sunxi/for-next]
[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]

url:    https://github.com/0day-ci/linux/commits/Robin-Murphy/iommu-Refactor-DMA-domain-strictness/20210722-022514
base:   https://git.kernel.org/pub/scm/linux/kernel/git/joro/iommu.git next
config: ia64-allmodconfig (attached as .config)
compiler: ia64-linux-gcc (GCC) 10.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/c05e0e1856b394eff1167c00f7bbd6ac7cc9dea6
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Robin-Murphy/iommu-Refactor-DMA-domain-strictness/20210722-022514
        git checkout c05e0e1856b394eff1167c00f7bbd6ac7cc9dea6
        # save the attached .config to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-10.3.0 make.cross O=build_dir ARCH=ia64 SHELL=/bin/bash

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   In file included from arch/ia64/include/asm/bug.h:17,
                    from include/linux/bug.h:5,
                    from include/linux/thread_info.h:13,
                    from include/asm-generic/preempt.h:5,
                    from ./arch/ia64/include/generated/asm/preempt.h:1,
                    from include/linux/preempt.h:78,
                    from include/linux/spinlock.h:51,
                    from include/linux/wait.h:9,
                    from include/linux/wait_bit.h:8,
                    from include/linux/fs.h:6,
                    from include/linux/debugfs.h:15,
                    from drivers/iommu/intel/iommu.c:18:
   drivers/iommu/intel/iommu.c: In function 'domain_get_iommu':
>> drivers/iommu/intel/iommu.c:604:38: error: '__IOMMU_DOMAIN_DMA' undeclared (first use in this function); did you mean 'IOMMU_DOMAIN_DMA'?
     604 |  if (WARN_ON(!(domain->domain.type & __IOMMU_DOMAIN_DMA)))
         |                                      ^~~~~~~~~~~~~~~~~~
   include/asm-generic/bug.h:121:25: note: in definition of macro 'WARN_ON'
     121 |  int __ret_warn_on = !!(condition);    \
         |                         ^~~~~~~~~
   drivers/iommu/intel/iommu.c:604:38: note: each undeclared identifier is reported only once for each function it appears in
     604 |  if (WARN_ON(!(domain->domain.type & __IOMMU_DOMAIN_DMA)))
         |                                      ^~~~~~~~~~~~~~~~~~
   include/asm-generic/bug.h:121:25: note: in definition of macro 'WARN_ON'
     121 |  int __ret_warn_on = !!(condition);    \
         |                         ^~~~~~~~~


vim +604 drivers/iommu/intel/iommu.c

   597	
   598	/* This functionin only returns single iommu in a domain */
   599	struct intel_iommu *domain_get_iommu(struct dmar_domain *domain)
   600	{
   601		int iommu_id;
   602	
   603		/* si_domain and vm domain should not get here. */
 > 604		if (WARN_ON(!(domain->domain.type & __IOMMU_DOMAIN_DMA)))
   605			return NULL;
   606	
   607		for_each_domain_iommu(iommu_id, domain)
   608			break;
   609	
   610		if (iommu_id < 0 || iommu_id >= g_num_of_iommus)
   611			return NULL;
   612	
   613		return g_iommus[iommu_id];
   614	}
   615	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
Robin Murphy July 22, 2021, 5:30 p.m. UTC | #2 
On 2021-07-22 17:44, kernel test robot wrote:
> Hi Robin,
> 
> I love your patch! Yet something to improve:
> 
> [auto build test ERROR on iommu/next]
> [also build test ERROR on rockchip/for-next linus/master v5.14-rc2 next-20210722]
> [cannot apply to sunxi/sunxi/for-next]
> [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]
> 
> url:    https://github.com/0day-ci/linux/commits/Robin-Murphy/iommu-Refactor-DMA-domain-strictness/20210722-022514
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/joro/iommu.git next
> config: ia64-allmodconfig (attached as .config)
> compiler: ia64-linux-gcc (GCC) 10.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/c05e0e1856b394eff1167c00f7bbd6ac7cc9dea6
>          git remote add linux-review https://github.com/0day-ci/linux
>          git fetch --no-tags linux-review Robin-Murphy/iommu-Refactor-DMA-domain-strictness/20210722-022514
>          git checkout c05e0e1856b394eff1167c00f7bbd6ac7cc9dea6
>          # save the attached .config to linux build tree
>          mkdir build_dir
>          COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-10.3.0 make.cross O=build_dir ARCH=ia64 SHELL=/bin/bash
> 
> If you fix the issue, kindly add following tag as appropriate
> Reported-by: kernel test robot <lkp@intel.com>
> 
> All errors (new ones prefixed by >>):
> 
>     In file included from arch/ia64/include/asm/bug.h:17,
>                      from include/linux/bug.h:5,
>                      from include/linux/thread_info.h:13,
>                      from include/asm-generic/preempt.h:5,
>                      from ./arch/ia64/include/generated/asm/preempt.h:1,
>                      from include/linux/preempt.h:78,
>                      from include/linux/spinlock.h:51,
>                      from include/linux/wait.h:9,
>                      from include/linux/wait_bit.h:8,
>                      from include/linux/fs.h:6,
>                      from include/linux/debugfs.h:15,
>                      from drivers/iommu/intel/iommu.c:18:
>     drivers/iommu/intel/iommu.c: In function 'domain_get_iommu':
>>> drivers/iommu/intel/iommu.c:604:38: error: '__IOMMU_DOMAIN_DMA' undeclared (first use in this function); did you mean 'IOMMU_DOMAIN_DMA'?
>       604 |  if (WARN_ON(!(domain->domain.type & __IOMMU_DOMAIN_DMA)))
>           |                                      ^~~~~~~~~~~~~~~~~~
>     include/asm-generic/bug.h:121:25: note: in definition of macro 'WARN_ON'
>       121 |  int __ret_warn_on = !!(condition);    \
>           |                         ^~~~~~~~~
>     drivers/iommu/intel/iommu.c:604:38: note: each undeclared identifier is reported only once for each function it appears in
>       604 |  if (WARN_ON(!(domain->domain.type & __IOMMU_DOMAIN_DMA)))
>           |                                      ^~~~~~~~~~~~~~~~~~
>     include/asm-generic/bug.h:121:25: note: in definition of macro 'WARN_ON'
>       121 |  int __ret_warn_on = !!(condition);    \
>           |                         ^~~~~~~~~
> 
> 
> vim +604 drivers/iommu/intel/iommu.c
> 
>     597	
>     598	/* This functionin only returns single iommu in a domain */
>     599	struct intel_iommu *domain_get_iommu(struct dmar_domain *domain)
>     600	{
>     601		int iommu_id;
>     602	
>     603		/* si_domain and vm domain should not get here. */
>   > 604		if (WARN_ON(!(domain->domain.type & __IOMMU_DOMAIN_DMA)))

Bleh, of course that should be __IOMMU_DOMAIN_DMA_API like the other two 
instances. I'll fix this locally ready for v2.

Thanks,
Robin.

>     605			return NULL;
>     606	
>     607		for_each_domain_iommu(iommu_id, domain)
>     608			break;
>     609	
>     610		if (iommu_id < 0 || iommu_id >= g_num_of_iommus)
>     611			return NULL;
>     612	
>     613		return g_iommus[iommu_id];
>     614	}
>     615	
> 
> ---
> 0-DAY CI Kernel Test Service, Intel Corporation
> https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
> 
> 
> _______________________________________________
> iommu mailing list
> iommu@lists.linux-foundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/iommu
>
kernel test robot July 22, 2021, 6:44 p.m. UTC | #3 
Hi Robin,

I love your patch! Yet something to improve:

[auto build test ERROR on iommu/next]
[also build test ERROR on rockchip/for-next linus/master v5.14-rc2 next-20210722]
[cannot apply to sunxi/sunxi/for-next]
[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]

url:    https://github.com/0day-ci/linux/commits/Robin-Murphy/iommu-Refactor-DMA-domain-strictness/20210722-022514
base:   https://git.kernel.org/pub/scm/linux/kernel/git/joro/iommu.git next
config: x86_64-randconfig-a015-20210722 (attached as .config)
compiler: clang version 13.0.0 (https://github.com/llvm/llvm-project 9625ca5b602616b2f5584e8a49ba93c52c141e40)
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 x86_64 cross compiling tool for clang build
        # apt-get install binutils-x86-64-linux-gnu
        # https://github.com/0day-ci/linux/commit/c05e0e1856b394eff1167c00f7bbd6ac7cc9dea6
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Robin-Murphy/iommu-Refactor-DMA-domain-strictness/20210722-022514
        git checkout c05e0e1856b394eff1167c00f7bbd6ac7cc9dea6
        # save the attached .config to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross O=build_dir ARCH=x86_64 SHELL=/bin/bash

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

>> drivers/iommu/intel/iommu.c:604:38: error: use of undeclared identifier '__IOMMU_DOMAIN_DMA'
           if (WARN_ON(!(domain->domain.type & __IOMMU_DOMAIN_DMA)))
                                               ^
   1 error generated.


vim +/__IOMMU_DOMAIN_DMA +604 drivers/iommu/intel/iommu.c

   597	
   598	/* This functionin only returns single iommu in a domain */
   599	struct intel_iommu *domain_get_iommu(struct dmar_domain *domain)
   600	{
   601		int iommu_id;
   602	
   603		/* si_domain and vm domain should not get here. */
 > 604		if (WARN_ON(!(domain->domain.type & __IOMMU_DOMAIN_DMA)))
   605			return NULL;
   606	
   607		for_each_domain_iommu(iommu_id, domain)
   608			break;
   609	
   610		if (iommu_id < 0 || iommu_id >= g_num_of_iommus)
   611			return NULL;
   612	
   613		return g_iommus[iommu_id];
   614	}
   615	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
Baolu Lu July 24, 2021, 5:23 a.m. UTC | #4 
Hi Robin,

On 2021/7/22 2:20, Robin Murphy wrote:
> In preparation for the strict vs. non-strict decision for DMA domains to
> be expressed in the domain type, make sure we expose our flush queue
> awareness by accepting the new domain type, and test the specific
> feature flag where we want to identify DMA domains in general. The DMA
> ops setup can simply be made unconditional, since iommu-dma already
> knows not to touch identity domains.
> 
> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> ---
>   drivers/iommu/intel/iommu.c | 15 ++++++---------
>   1 file changed, 6 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
> index e2add5a0caef..77d322272743 100644
> --- a/drivers/iommu/intel/iommu.c
> +++ b/drivers/iommu/intel/iommu.c
> @@ -601,7 +601,7 @@ struct intel_iommu *domain_get_iommu(struct dmar_domain *domain)
>   	int iommu_id;
>   
>   	/* si_domain and vm domain should not get here. */
> -	if (WARN_ON(domain->domain.type != IOMMU_DOMAIN_DMA))
> +	if (WARN_ON(!(domain->domain.type & __IOMMU_DOMAIN_DMA)))
>   		return NULL;
>   
>   	for_each_domain_iommu(iommu_id, domain)
> @@ -1035,7 +1035,7 @@ static struct dma_pte *pfn_to_dma_pte(struct dmar_domain *domain,
>   			pteval = ((uint64_t)virt_to_dma_pfn(tmp_page) << VTD_PAGE_SHIFT) | DMA_PTE_READ | DMA_PTE_WRITE;
>   			if (domain_use_first_level(domain)) {
>   				pteval |= DMA_FL_PTE_XD | DMA_FL_PTE_US;
> -				if (domain->domain.type == IOMMU_DOMAIN_DMA)
> +				if (domain->domain.type & __IOMMU_DOMAIN_DMA_API)
>   					pteval |= DMA_FL_PTE_ACCESS;
>   			}
>   			if (cmpxchg64(&pte->val, 0ULL, pteval))
> @@ -2346,7 +2346,7 @@ __domain_mapping(struct dmar_domain *domain, unsigned long iov_pfn,
>   	if (domain_use_first_level(domain)) {
>   		attr |= DMA_FL_PTE_XD | DMA_FL_PTE_US;
>   
> -		if (domain->domain.type == IOMMU_DOMAIN_DMA) {
> +		if (domain->domain.type & __IOMMU_DOMAIN_DMA_API) {
>   			attr |= DMA_FL_PTE_ACCESS;
>   			if (prot & DMA_PTE_WRITE)
>   				attr |= DMA_FL_PTE_DIRTY;
> @@ -4528,6 +4528,7 @@ static struct iommu_domain *intel_iommu_domain_alloc(unsigned type)
>   
>   	switch (type) {
>   	case IOMMU_DOMAIN_DMA:
> +	case IOMMU_DOMAIN_DMA_FQ:
>   	case IOMMU_DOMAIN_UNMANAGED:
>   		dmar_domain = alloc_domain(0);
>   		if (!dmar_domain) {
> @@ -5164,12 +5165,8 @@ static void intel_iommu_release_device(struct device *dev)
>   
>   static void intel_iommu_probe_finalize(struct device *dev)
>   {
> -	struct iommu_domain *domain = iommu_get_domain_for_dev(dev);
> -
> -	if (domain && domain->type == IOMMU_DOMAIN_DMA)
> -		iommu_setup_dma_ops(dev, 0, U64_MAX);
> -	else
> -		set_dma_ops(dev, NULL);
> +	set_dma_ops(dev, NULL);

Is it reasonable to remove above line? The idea is that vendor iommu
driver should not override the dma_ops if device doesn't have a DMA
domain.

> +	iommu_setup_dma_ops(dev, 0, U64_MAX);
>   }
>   
>   static void intel_iommu_get_resv_regions(struct device *device,
> 

Best regards,
baolu
Robin Murphy July 26, 2021, 8:30 a.m. UTC | #5 
On 2021-07-24 06:23, Lu Baolu wrote:
> Hi Robin,
> 
> On 2021/7/22 2:20, Robin Murphy wrote:
>> In preparation for the strict vs. non-strict decision for DMA domains to
>> be expressed in the domain type, make sure we expose our flush queue
>> awareness by accepting the new domain type, and test the specific
>> feature flag where we want to identify DMA domains in general. The DMA
>> ops setup can simply be made unconditional, since iommu-dma already
>> knows not to touch identity domains.
>>
>> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
>> ---
>>   drivers/iommu/intel/iommu.c | 15 ++++++---------
>>   1 file changed, 6 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
>> index e2add5a0caef..77d322272743 100644
>> --- a/drivers/iommu/intel/iommu.c
>> +++ b/drivers/iommu/intel/iommu.c
>> @@ -601,7 +601,7 @@ struct intel_iommu *domain_get_iommu(struct 
>> dmar_domain *domain)
>>       int iommu_id;
>>       /* si_domain and vm domain should not get here. */
>> -    if (WARN_ON(domain->domain.type != IOMMU_DOMAIN_DMA))
>> +    if (WARN_ON(!(domain->domain.type & __IOMMU_DOMAIN_DMA)))
>>           return NULL;
>>       for_each_domain_iommu(iommu_id, domain)
>> @@ -1035,7 +1035,7 @@ static struct dma_pte *pfn_to_dma_pte(struct 
>> dmar_domain *domain,
>>               pteval = ((uint64_t)virt_to_dma_pfn(tmp_page) << 
>> VTD_PAGE_SHIFT) | DMA_PTE_READ | DMA_PTE_WRITE;
>>               if (domain_use_first_level(domain)) {
>>                   pteval |= DMA_FL_PTE_XD | DMA_FL_PTE_US;
>> -                if (domain->domain.type == IOMMU_DOMAIN_DMA)
>> +                if (domain->domain.type & __IOMMU_DOMAIN_DMA_API)
>>                       pteval |= DMA_FL_PTE_ACCESS;
>>               }
>>               if (cmpxchg64(&pte->val, 0ULL, pteval))
>> @@ -2346,7 +2346,7 @@ __domain_mapping(struct dmar_domain *domain, 
>> unsigned long iov_pfn,
>>       if (domain_use_first_level(domain)) {
>>           attr |= DMA_FL_PTE_XD | DMA_FL_PTE_US;
>> -        if (domain->domain.type == IOMMU_DOMAIN_DMA) {
>> +        if (domain->domain.type & __IOMMU_DOMAIN_DMA_API) {
>>               attr |= DMA_FL_PTE_ACCESS;
>>               if (prot & DMA_PTE_WRITE)
>>                   attr |= DMA_FL_PTE_DIRTY;
>> @@ -4528,6 +4528,7 @@ static struct iommu_domain 
>> *intel_iommu_domain_alloc(unsigned type)
>>       switch (type) {
>>       case IOMMU_DOMAIN_DMA:
>> +    case IOMMU_DOMAIN_DMA_FQ:
>>       case IOMMU_DOMAIN_UNMANAGED:
>>           dmar_domain = alloc_domain(0);
>>           if (!dmar_domain) {
>> @@ -5164,12 +5165,8 @@ static void intel_iommu_release_device(struct 
>> device *dev)
>>   static void intel_iommu_probe_finalize(struct device *dev)
>>   {
>> -    struct iommu_domain *domain = iommu_get_domain_for_dev(dev);
>> -
>> -    if (domain && domain->type == IOMMU_DOMAIN_DMA)
>> -        iommu_setup_dma_ops(dev, 0, U64_MAX);
>> -    else
>> -        set_dma_ops(dev, NULL);
>> +    set_dma_ops(dev, NULL);
> 
> Is it reasonable to remove above line? The idea is that vendor iommu
> driver should not override the dma_ops if device doesn't have a DMA
> domain.

As the commit message implies, that's exactly how iommu_setup_dma_ops() 
has always behaved anyway. There should be no functional change here.

Robin.

>> +    iommu_setup_dma_ops(dev, 0, U64_MAX);
>>   }
>>   static void intel_iommu_get_resv_regions(struct device *device,
>>
> 
> Best regards,
> baolu
diff mbox series

Patch

diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index e2add5a0caef..77d322272743 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -601,7 +601,7 @@  struct intel_iommu *domain_get_iommu(struct dmar_domain *domain)
 	int iommu_id;
 
 	/* si_domain and vm domain should not get here. */
-	if (WARN_ON(domain->domain.type != IOMMU_DOMAIN_DMA))
+	if (WARN_ON(!(domain->domain.type & __IOMMU_DOMAIN_DMA)))
 		return NULL;
 
 	for_each_domain_iommu(iommu_id, domain)
@@ -1035,7 +1035,7 @@  static struct dma_pte *pfn_to_dma_pte(struct dmar_domain *domain,
 			pteval = ((uint64_t)virt_to_dma_pfn(tmp_page) << VTD_PAGE_SHIFT) | DMA_PTE_READ | DMA_PTE_WRITE;
 			if (domain_use_first_level(domain)) {
 				pteval |= DMA_FL_PTE_XD | DMA_FL_PTE_US;
-				if (domain->domain.type == IOMMU_DOMAIN_DMA)
+				if (domain->domain.type & __IOMMU_DOMAIN_DMA_API)
 					pteval |= DMA_FL_PTE_ACCESS;
 			}
 			if (cmpxchg64(&pte->val, 0ULL, pteval))
@@ -2346,7 +2346,7 @@  __domain_mapping(struct dmar_domain *domain, unsigned long iov_pfn,
 	if (domain_use_first_level(domain)) {
 		attr |= DMA_FL_PTE_XD | DMA_FL_PTE_US;
 
-		if (domain->domain.type == IOMMU_DOMAIN_DMA) {
+		if (domain->domain.type & __IOMMU_DOMAIN_DMA_API) {
 			attr |= DMA_FL_PTE_ACCESS;
 			if (prot & DMA_PTE_WRITE)
 				attr |= DMA_FL_PTE_DIRTY;
@@ -4528,6 +4528,7 @@  static struct iommu_domain *intel_iommu_domain_alloc(unsigned type)
 
 	switch (type) {
 	case IOMMU_DOMAIN_DMA:
+	case IOMMU_DOMAIN_DMA_FQ:
 	case IOMMU_DOMAIN_UNMANAGED:
 		dmar_domain = alloc_domain(0);
 		if (!dmar_domain) {
@@ -5164,12 +5165,8 @@  static void intel_iommu_release_device(struct device *dev)
 
 static void intel_iommu_probe_finalize(struct device *dev)
 {
-	struct iommu_domain *domain = iommu_get_domain_for_dev(dev);
-
-	if (domain && domain->type == IOMMU_DOMAIN_DMA)
-		iommu_setup_dma_ops(dev, 0, U64_MAX);
-	else
-		set_dma_ops(dev, NULL);
+	set_dma_ops(dev, NULL);
+	iommu_setup_dma_ops(dev, 0, U64_MAX);
 }
 
 static void intel_iommu_get_resv_regions(struct device *device,