[PATCH v2 0/7] iommu/vt-d: Several fixes for intel iommu driver

[PATCH v2 0/7] iommu/vt-d: Several fixes for intel iommu driver

[Tina Zhang]

v2:
- Remove two patches which are not necessary.
- Split the "Replace BUG()/BUG_ON() with WARN_ON/WARN_ON_ONCE()" patch
  into several small patches with each one addressing one issue.
- Use BIT_ULL in the "Fix operand size in bitwise operation" patch.
v1:
https://lore.kernel.org/linux-iommu/20230329124654.2698853-1-tina.zhang@intel.com/

Tina Zhang (7):
  iommu/vt-d: Fix operand size in bitwise operation
  iommu/vt-d: Remove BUG_ON on checking valid pfn range
  iommu/vt-d: Remove BUG_ON in handling iotlb cache invalidation
  iommu/vt-d: Remove BUG_ON when domain->pgd is NULL
  iommu/vt-d: Remove BUG_ON in map/unmap()
  iommu/vt-d: Remove a useless BUG_ON(dev->is_virtfn)
  iommu/vt-d: Remove BUG_ON in dmar_insert_dev_scope()

 drivers/iommu/intel/dmar.c  |  7 +++---
 drivers/iommu/intel/iommu.c | 45 ++++++++++++++++++++-----------------
 2 files changed, 28 insertions(+), 24 deletions(-)

-- 
2.34.1

[PATCH v2 1/7] iommu/vt-d: Fix operand size in bitwise operation

[Tina Zhang]

The patch fixes the klocwork issues that operands in a bitwise operation
have different size at line 1692 of dmar.c, line 1898 and line 1907 of
iommu.c.

Reported-by: Yongwei Ma <yongwei.ma@intel.com>
Signed-off-by: Tina Zhang <tina.zhang@intel.com>
---
 drivers/iommu/intel/dmar.c  | 2 +-
 drivers/iommu/intel/iommu.c | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/iommu/intel/dmar.c b/drivers/iommu/intel/dmar.c
index 6acfe879589c..01d0ca0019f2 100644
--- a/drivers/iommu/intel/dmar.c
+++ b/drivers/iommu/intel/dmar.c
@@ -1689,7 +1689,7 @@ static void __dmar_enable_qi(struct intel_iommu *iommu)
 	 * is present.
 	 */
 	if (ecap_smts(iommu->ecap))
-		val |= (1 << 11) | 1;
+		val |= BIT_ULL(11) | BIT_ULL(0);
 
 	raw_spin_lock_irqsave(&iommu->register_lock, flags);
 
diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index 7c2f4bd33582..5d6018b81959 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -1895,7 +1895,7 @@ context_set_sm_rid2pasid(struct context_entry *context, unsigned long pasid)
  */
 static inline void context_set_sm_dte(struct context_entry *context)
 {
-	context->lo |= (1 << 2);
+	context->lo |= BIT_ULL(2);
 }
 
 /*
@@ -1904,7 +1904,7 @@ static inline void context_set_sm_dte(struct context_entry *context)
  */
 static inline void context_set_sm_pre(struct context_entry *context)
 {
-	context->lo |= (1 << 4);
+	context->lo |= BIT_ULL(4);
 }
 
 /* Convert value to context PASID directory size field coding. */
-- 
2.34.1

[PATCH v2 2/7] iommu/vt-d: Remove BUG_ON on checking valid pfn range

[Tina Zhang]

When encountering an unexpected invalid pfn range, the kernel should
attempt recovery and proceed with execution. Therefore, using WARN_ON to
replace BUG_ON to avoid halting the machine.

Besides, one redundant checking is reduced.

Signed-off-by: Tina Zhang <tina.zhang@intel.com>
---
 drivers/iommu/intel/iommu.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index 5d6018b81959..1426df19e760 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -1005,9 +1005,9 @@ static void dma_pte_clear_range(struct dmar_domain *domain,
 	unsigned int large_page;
 	struct dma_pte *first_pte, *pte;
 
-	BUG_ON(!domain_pfn_supported(domain, start_pfn));
-	BUG_ON(!domain_pfn_supported(domain, last_pfn));
-	BUG_ON(start_pfn > last_pfn);
+	if (WARN_ON(!domain_pfn_supported(domain, last_pfn)) ||
+	    WARN_ON(start_pfn > last_pfn))
+		return;
 
 	/* we don't need lock here; nobody else touches the iova range */
 	do {
@@ -1166,9 +1166,9 @@ static void dma_pte_clear_level(struct dmar_domain *domain, int level,
 static void domain_unmap(struct dmar_domain *domain, unsigned long start_pfn,
 			 unsigned long last_pfn, struct list_head *freelist)
 {
-	BUG_ON(!domain_pfn_supported(domain, start_pfn));
-	BUG_ON(!domain_pfn_supported(domain, last_pfn));
-	BUG_ON(start_pfn > last_pfn);
+	if (WARN_ON(!domain_pfn_supported(domain, last_pfn)) ||
+	    WARN_ON(start_pfn > last_pfn))
+		return;
 
 	/* we don't need lock here; nobody else touches the iova range */
 	dma_pte_clear_level(domain, agaw_to_level(domain->agaw),
-- 
2.34.1

[PATCH v2 3/7] iommu/vt-d: Remove BUG_ON in handling iotlb cache invalidation

[Tina Zhang]

VT-d iotlb cache invalidation request with unexpected type is considered
as a bug to developers, which can be fixed. So, when such kind of issue
comes out, it needs to be reported through the kernel log, instead of
halting the system. Replacing BUG_ON with warning reporting.

Signed-off-by: Tina Zhang <tina.zhang@intel.com>
---
 drivers/iommu/intel/iommu.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index 1426df19e760..de7fe7dcbc5c 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -1272,7 +1272,9 @@ static void __iommu_flush_context(struct intel_iommu *iommu,
 			| DMA_CCMD_SID(source_id) | DMA_CCMD_FM(function_mask);
 		break;
 	default:
-		BUG();
+		pr_warn("%s: Unexpected context-cache invalidation type 0x%llx\n",
+			iommu->name, type);
+		return;
 	}
 	val |= DMA_CCMD_ICC;
 
@@ -1308,7 +1310,9 @@ static void __iommu_flush_iotlb(struct intel_iommu *iommu, u16 did,
 		val_iva = size_order | addr;
 		break;
 	default:
-		BUG();
+		pr_warn("%s: Unexpected iotlb invalidation type 0x%llx\n",
+			iommu->name, type);
+		return;
 	}
 	/* Note: set drain read/write */
 #if 0
@@ -1508,7 +1512,8 @@ static void iommu_flush_iotlb_psi(struct intel_iommu *iommu,
 	uint64_t addr = (uint64_t)pfn << VTD_PAGE_SHIFT;
 	u16 did = domain_id_iommu(domain, iommu);
 
-	BUG_ON(pages == 0);
+	if (WARN_ON(!pages))
+		return;
 
 	if (ih)
 		ih = 1 << 6;
-- 
2.34.1

[PATCH v2 4/7] iommu/vt-d: Remove BUG_ON when domain->pgd is NULL

[Tina Zhang]

When performing domain_context_mapping or searching dma-pte of a pfn,
with invalid domain page table directory address, which is unexpected,
shouldn't crash kernel. Therefore, using if() instead of BUG_ON.

Signed-off-by: Tina Zhang <tina.zhang@intel.com>
---
 drivers/iommu/intel/iommu.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index de7fe7dcbc5c..e1881d48c2aa 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -915,10 +915,7 @@ static struct dma_pte *pfn_to_dma_pte(struct dmar_domain *domain,
 	int level = agaw_to_level(domain->agaw);
 	int offset;
 
-	BUG_ON(!domain->pgd);
-
-	if (!domain_pfn_supported(domain, pfn))
-		/* Address beyond IOMMU's addressing capabilities. */
+	if (unlikely(!domain->pgd || !domain_pfn_supported(domain, pfn)))
 		return NULL;
 
 	parent = domain->pgd;
@@ -1927,6 +1924,9 @@ static int domain_context_mapping_one(struct dmar_domain *domain,
 	struct context_entry *context;
 	int ret;
 
+	if (unlikely(!domain->pgd))
+		return -EINVAL;
+
 	WARN_ON(did == 0);
 
 	if (hw_pass_through && domain_type_is_si(domain))
@@ -1935,8 +1935,6 @@ static int domain_context_mapping_one(struct dmar_domain *domain,
 	pr_debug("Set context mapping for %02x:%02x.%d\n",
 		bus, PCI_SLOT(devfn), PCI_FUNC(devfn));
 
-	BUG_ON(!domain->pgd);
-
 	spin_lock(&iommu->lock);
 	ret = -ENOMEM;
 	context = iommu_context_addr(iommu, bus, devfn, 1);
-- 
2.34.1

[PATCH v2 5/7] iommu/vt-d: Remove BUG_ON in map/unmap()

[Tina Zhang]

Domain map/unmap with invalid parameters shouldn't crash the kernel.
Therefore, using if() replaces the BUG_ON.

Signed-off-by: Tina Zhang <tina.zhang@intel.com>
---
 drivers/iommu/intel/iommu.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index e1881d48c2aa..61166b6b992d 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -2186,7 +2186,8 @@ __domain_mapping(struct dmar_domain *domain, unsigned long iov_pfn,
 	phys_addr_t pteval;
 	u64 attr;
 
-	BUG_ON(!domain_pfn_supported(domain, iov_pfn + nr_pages - 1));
+	if (unlikely(!domain_pfn_supported(domain, iov_pfn + nr_pages - 1)))
+		return -EINVAL;
 
 	if ((prot & (DMA_PTE_READ|DMA_PTE_WRITE)) == 0)
 		return -EINVAL;
@@ -4343,8 +4344,9 @@ static size_t intel_iommu_unmap(struct iommu_domain *domain,
 
 	/* Cope with horrid API which requires us to unmap more than the
 	   size argument if it happens to be a large-page mapping. */
-	BUG_ON(!pfn_to_dma_pte(dmar_domain, iova >> VTD_PAGE_SHIFT, &level,
-			       GFP_ATOMIC));
+	if (unlikely(!pfn_to_dma_pte(dmar_domain, iova >> VTD_PAGE_SHIFT,
+					 &level, GFP_ATOMIC)))
+		return 0;
 
 	if (size < VTD_PAGE_SIZE << level_to_offset_bits(level))
 		size = VTD_PAGE_SIZE << level_to_offset_bits(level);
-- 
2.34.1

[PATCH v2 6/7] iommu/vt-d: Remove a useless BUG_ON(dev->is_virtfn)

[Tina Zhang]

When dmar_alloc_pci_notify_info() is being invoked, the invoker has
ensured the dev->is_virtfn is false. So, remove the useless BUG_ON in
dmar_alloc_pci_notify_info().

Signed-off-by: Tina Zhang <tina.zhang@intel.com>
---
 drivers/iommu/intel/dmar.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/iommu/intel/dmar.c b/drivers/iommu/intel/dmar.c
index 01d0ca0019f2..9684c96247f8 100644
--- a/drivers/iommu/intel/dmar.c
+++ b/drivers/iommu/intel/dmar.c
@@ -127,8 +127,6 @@ dmar_alloc_pci_notify_info(struct pci_dev *dev, unsigned long event)
 	struct pci_dev *tmp;
 	struct dmar_pci_notify_info *info;
 
-	BUG_ON(dev->is_virtfn);
-
 	/*
 	 * Ignore devices that have a domain number higher than what can
 	 * be looked up in DMAR, e.g. VMD subdevices with domain 0x10000
-- 
2.34.1

[PATCH v2 7/7] iommu/vt-d: Remove BUG_ON in dmar_insert_dev_scope()

[Tina Zhang]

The dmar_insert_dev_scope() could fail if any unexpected condition is
encountered. However, in this situation, the kernel should attempt
recovery and proceed with execution. Remove BUG_ON with WARN_ON, so that
kernel can avoid being crashed when an unexpected condition occurs.

Signed-off-by: Tina Zhang <tina.zhang@intel.com>
---
 drivers/iommu/intel/dmar.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/iommu/intel/dmar.c b/drivers/iommu/intel/dmar.c
index 9684c96247f8..7eceee081235 100644
--- a/drivers/iommu/intel/dmar.c
+++ b/drivers/iommu/intel/dmar.c
@@ -262,7 +262,8 @@ int dmar_insert_dev_scope(struct dmar_pci_notify_info *info,
 						   get_device(dev));
 				return 1;
 			}
-		BUG_ON(i >= devices_cnt);
+		if (WARN_ON(i >= devices_cnt))
+			return -EINVAL;
 	}
 
 	return 0;
-- 
2.34.1

Re: [PATCH v2 4/7] iommu/vt-d: Remove BUG_ON when domain->pgd is NULL

[Baolu Lu]

On 3/31/23 10:42 AM, Tina Zhang wrote:
> When performing domain_context_mapping or searching dma-pte of a pfn,
> with invalid domain page table directory address, which is unexpected,
> shouldn't crash kernel. Therefore, using if() instead of BUG_ON.
> 
> Signed-off-by: Tina Zhang <tina.zhang@intel.com>
> ---
>   drivers/iommu/intel/iommu.c | 10 ++++------
>   1 file changed, 4 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
> index de7fe7dcbc5c..e1881d48c2aa 100644
> --- a/drivers/iommu/intel/iommu.c
> +++ b/drivers/iommu/intel/iommu.c
> @@ -915,10 +915,7 @@ static struct dma_pte *pfn_to_dma_pte(struct dmar_domain *domain,
>   	int level = agaw_to_level(domain->agaw);
>   	int offset;
>   
> -	BUG_ON(!domain->pgd);
> -
> -	if (!domain_pfn_supported(domain, pfn))
> -		/* Address beyond IOMMU's addressing capabilities. */
> +	if (unlikely(!domain->pgd || !domain_pfn_supported(domain, pfn)))
>   		return NULL;
>   
>   	parent = domain->pgd;
> @@ -1927,6 +1924,9 @@ static int domain_context_mapping_one(struct dmar_domain *domain,
>   	struct context_entry *context;
>   	int ret;
>   
> +	if (unlikely(!domain->pgd))
> +		return -EINVAL;
> +
>   	WARN_ON(did == 0);
>   
>   	if (hw_pass_through && domain_type_is_si(domain))
> @@ -1935,8 +1935,6 @@ static int domain_context_mapping_one(struct dmar_domain *domain,
>   	pr_debug("Set context mapping for %02x:%02x.%d\n",
>   		bus, PCI_SLOT(devfn), PCI_FUNC(devfn));
>   
> -	BUG_ON(!domain->pgd);
> -
>   	spin_lock(&iommu->lock);
>   	ret = -ENOMEM;
>   	context = iommu_context_addr(iommu, bus, devfn, 1);

I'd suggest to remove all checks for domain->pgd.

If a domain has map/unmap ops, it should always have its own page table
(a.k.a. domain->pgd). Hence the domain->pgd check is unnecessary.

Best regards,
baolu

Re: [PATCH v2 7/7] iommu/vt-d: Remove BUG_ON in dmar_insert_dev_scope()

[Baolu Lu]

On 3/31/23 10:42 AM, Tina Zhang wrote:
> The dmar_insert_dev_scope() could fail if any unexpected condition is
> encountered. However, in this situation, the kernel should attempt
> recovery and proceed with execution. Remove BUG_ON with WARN_ON, so that
> kernel can avoid being crashed when an unexpected condition occurs.
> 
> Signed-off-by: Tina Zhang <tina.zhang@intel.com>
> ---
>   drivers/iommu/intel/dmar.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iommu/intel/dmar.c b/drivers/iommu/intel/dmar.c
> index 9684c96247f8..7eceee081235 100644
> --- a/drivers/iommu/intel/dmar.c
> +++ b/drivers/iommu/intel/dmar.c
> @@ -262,7 +262,8 @@ int dmar_insert_dev_scope(struct dmar_pci_notify_info *info,
>   						   get_device(dev));
>   				return 1;
>   			}
> -		BUG_ON(i >= devices_cnt);
> +		if (WARN_ON(i >= devices_cnt))
> +			return -EINVAL;

You have returned an error number. Do you still need a WARN_ON()?

>   	}
>   
>   	return 0;

Best regards,
baolu

Re: [PATCH v2 7/7] iommu/vt-d: Remove BUG_ON in dmar_insert_dev_scope()

[tina.zhang]

Hi,

On 3/31/23 11:27, Baolu Lu wrote:
> On 3/31/23 10:42 AM, Tina Zhang wrote:
>> The dmar_insert_dev_scope() could fail if any unexpected condition is
>> encountered. However, in this situation, the kernel should attempt
>> recovery and proceed with execution. Remove BUG_ON with WARN_ON, so that
>> kernel can avoid being crashed when an unexpected condition occurs.
>>
>> Signed-off-by: Tina Zhang <tina.zhang@intel.com>
>> ---
>>   drivers/iommu/intel/dmar.c | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/iommu/intel/dmar.c b/drivers/iommu/intel/dmar.c
>> index 9684c96247f8..7eceee081235 100644
>> --- a/drivers/iommu/intel/dmar.c
>> +++ b/drivers/iommu/intel/dmar.c
>> @@ -262,7 +262,8 @@ int dmar_insert_dev_scope(struct 
>> dmar_pci_notify_info *info,
>>                              get_device(dev));
>>                   return 1;
>>               }
>> -        BUG_ON(i >= devices_cnt);
>> +        if (WARN_ON(i >= devices_cnt))
>> +            return -EINVAL;
> 
> You have returned an error number. Do you still need a WARN_ON()?
I think we'd better keep WARN_ON() here. Because this situation isn't 
expected (i.e. this-should-never-happen) by the introduced mechanism[1].

[1]: 
https://github.com/torvalds/linux/commit/59ce0515cdaf3b7d47893d12f61e51d691863788

Regards,
-Tina

> 
>>       }
>>       return 0;
> 
> Best regards,
> baolu

Re: [PATCH v2 4/7] iommu/vt-d: Remove BUG_ON when domain->pgd is NULL

[tina.zhang]

Hi,

On 3/31/23 11:25, Baolu Lu wrote:
> On 3/31/23 10:42 AM, Tina Zhang wrote:
>> When performing domain_context_mapping or searching dma-pte of a pfn,
>> with invalid domain page table directory address, which is unexpected,
>> shouldn't crash kernel. Therefore, using if() instead of BUG_ON.
>>
>> Signed-off-by: Tina Zhang <tina.zhang@intel.com>
>> ---
>>   drivers/iommu/intel/iommu.c | 10 ++++------
>>   1 file changed, 4 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
>> index de7fe7dcbc5c..e1881d48c2aa 100644
>> --- a/drivers/iommu/intel/iommu.c
>> +++ b/drivers/iommu/intel/iommu.c
>> @@ -915,10 +915,7 @@ static struct dma_pte *pfn_to_dma_pte(struct 
>> dmar_domain *domain,
>>       int level = agaw_to_level(domain->agaw);
>>       int offset;
>> -    BUG_ON(!domain->pgd);
>> -
>> -    if (!domain_pfn_supported(domain, pfn))
>> -        /* Address beyond IOMMU's addressing capabilities. */
>> +    if (unlikely(!domain->pgd || !domain_pfn_supported(domain, pfn)))
>>           return NULL;
>>       parent = domain->pgd;
>> @@ -1927,6 +1924,9 @@ static int domain_context_mapping_one(struct 
>> dmar_domain *domain,
>>       struct context_entry *context;
>>       int ret;
>> +    if (unlikely(!domain->pgd))
>> +        return -EINVAL;
>> +
>>       WARN_ON(did == 0);
>>       if (hw_pass_through && domain_type_is_si(domain))
>> @@ -1935,8 +1935,6 @@ static int domain_context_mapping_one(struct 
>> dmar_domain *domain,
>>       pr_debug("Set context mapping for %02x:%02x.%d\n",
>>           bus, PCI_SLOT(devfn), PCI_FUNC(devfn));
>> -    BUG_ON(!domain->pgd);
>> -
>>       spin_lock(&iommu->lock);
>>       ret = -ENOMEM;
>>       context = iommu_context_addr(iommu, bus, devfn, 1);
> 
> I'd suggest to remove all checks for domain->pgd.
> 
> If a domain has map/unmap ops, it should always have its own page table
> (a.k.a. domain->pgd). Hence the domain->pgd check is unnecessary.
Agree.

Regards,
-Tina
> 
> Best regards,
> baolu