[RFC PATCH] iommu: Handle domain allocation failure gracefully

[RFC PATCH] iommu: Handle domain allocation failure gracefully

[Vasant Hegde]

Current code does not validate domain allocation failures. This will
result in random failure at later stage. Hence handle domain allocation
failures.

Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
---
Hi Joerg,
  With current code whenever amd driver failes to allocate domain we hit
  failure at late stage and it became difficult to debug. Hence I
  thought of adding this fix.

  I see that in commit 6e1aa2049154 you have intentionally didn't check
  the return value. Looking into drivers/iommu/* it looks like all
  drivers has domain_alloc() callback handler. But some drivers (like msm
  - msm_iommu_domain_alloc()) allocates default domain for certain type
  only. Hence I'm not sure whether its ok to handle domain allocation
  failures or not. Hence I have marked this patch as RFC.

  Let me know whether this patch is fine -OR- is there any other way we can
  handle domain allocation failures?

-Vasant

 drivers/iommu/iommu.c | 24 +++++++++++++-----------
 1 file changed, 13 insertions(+), 11 deletions(-)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 780fb7071577..e8a1e7fbffb9 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -289,18 +289,21 @@ int iommu_probe_device(struct device *dev)
 
 	/*
 	 * Try to allocate a default domain - needs support from the
-	 * IOMMU driver. There are still some drivers which don't
-	 * support default domains, so the return value is not yet
-	 * checked.
+	 * IOMMU driver.
 	 */
 	mutex_lock(&group->mutex);
-	iommu_alloc_default_domain(group, dev);
+	ret = iommu_alloc_default_domain(group, dev);
+	if (ret) {
+		mutex_unlock(&group->mutex);
+		iommu_group_put(group);
+		goto err_release;
+	}
 
 	/*
 	 * If device joined an existing group which has been claimed, don't
 	 * attach the default domain.
 	 */
-	if (group->default_domain && !group->owner) {
+	if (!group->owner) {
 		ret = __iommu_attach_device(group->default_domain, dev);
 		if (ret) {
 			mutex_unlock(&group->mutex);
@@ -1665,7 +1668,7 @@ static int probe_get_default_domain_type(struct device *dev, void *data)
 	return 0;
 }
 
-static void probe_alloc_default_domain(struct bus_type *bus,
+static int probe_alloc_default_domain(struct bus_type *bus,
 				       struct iommu_group *group)
 {
 	struct __group_domain_type gtype;
@@ -1679,7 +1682,7 @@ static void probe_alloc_default_domain(struct bus_type *bus,
 	if (!gtype.type)
 		gtype.type = iommu_def_domain_type;
 
-	iommu_group_alloc_default_domain(bus, group, gtype.type);
+	return iommu_group_alloc_default_domain(bus, group, gtype.type);
 
 }
 
@@ -1753,11 +1756,10 @@ int bus_iommu_probe(struct bus_type *bus)
 		mutex_lock(&group->mutex);
 
 		/* Try to allocate default domain */
-		probe_alloc_default_domain(bus, group);
-
-		if (!group->default_domain) {
+		ret = probe_alloc_default_domain(bus, group);
+		if (ret) {
 			mutex_unlock(&group->mutex);
-			continue;
+			break;
 		}
 
 		iommu_group_create_direct_mappings(group);
-- 
2.31.1

Re: [RFC PATCH] iommu: Handle domain allocation failure gracefully

[Baolu Lu]

On 9/14/22 10:44 PM, Vasant Hegde wrote:
> Current code does not validate domain allocation failures. This will
> result in random failure at later stage. Hence handle domain allocation
> failures.
> 
> Signed-off-by: Vasant Hegde<vasant.hegde@amd.com>
> ---
> Hi Joerg,
>    With current code whenever amd driver failes to allocate domain we hit
>    failure at late stage and it became difficult to debug. Hence I
>    thought of adding this fix.
> 
>    I see that in commit 6e1aa2049154 you have intentionally didn't check
>    the return value. Looking into drivers/iommu/* it looks like all
>    drivers has domain_alloc() callback handler. But some drivers (like msm
>    - msm_iommu_domain_alloc()) allocates default domain for certain type
>    only. Hence I'm not sure whether its ok to handle domain allocation
>    failures or not. Hence I have marked this patch as RFC.
> 
>    Let me know whether this patch is fine -OR- is there any other way we can
>    handle domain allocation failures?
> 
> -Vasant
> 
>   drivers/iommu/iommu.c | 24 +++++++++++++-----------
>   1 file changed, 13 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index 780fb7071577..e8a1e7fbffb9 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -289,18 +289,21 @@ int iommu_probe_device(struct device *dev)
>   
>   	/*
>   	 * Try to allocate a default domain - needs support from the
> -	 * IOMMU driver. There are still some drivers which don't
> -	 * support default domains, so the return value is not yet
> -	 * checked.
> +	 * IOMMU driver.
>   	 */
>   	mutex_lock(&group->mutex);
> -	iommu_alloc_default_domain(group, dev);
> +	ret = iommu_alloc_default_domain(group, dev);
> +	if (ret) {
> +		mutex_unlock(&group->mutex);
> +		iommu_group_put(group);
> +		goto err_release;
> +	}

Some IOMMU drivers (s390, gart, tegra...) don't support default domains.
iommu_alloc_default_domain() returns a ENOMEM and the probe process
continues.

Above changes this behavior. The probe process will always fails with
those IOMMU drivers.

Best regards,
baolu

Re: [RFC PATCH] iommu: Handle domain allocation failure gracefully

[Vasant Hegde]

Baolu,


On 9/15/2022 8:44 AM, Baolu Lu wrote:
> On 9/14/22 10:44 PM, Vasant Hegde wrote:
>> Current code does not validate domain allocation failures. This will
>> result in random failure at later stage. Hence handle domain allocation
>> failures.
>>
>> Signed-off-by: Vasant Hegde<vasant.hegde@amd.com>
>> ---
>> Hi Joerg,
>>    With current code whenever amd driver failes to allocate domain we hit
>>    failure at late stage and it became difficult to debug. Hence I
>>    thought of adding this fix.
>>
>>    I see that in commit 6e1aa2049154 you have intentionally didn't check
>>    the return value. Looking into drivers/iommu/* it looks like all
>>    drivers has domain_alloc() callback handler. But some drivers (like msm
>>    - msm_iommu_domain_alloc()) allocates default domain for certain type
>>    only. Hence I'm not sure whether its ok to handle domain allocation
>>    failures or not. Hence I have marked this patch as RFC.
>>
>>    Let me know whether this patch is fine -OR- is there any other way we can
>>    handle domain allocation failures?
>>
>> -Vasant
>>
>>   drivers/iommu/iommu.c | 24 +++++++++++++-----------
>>   1 file changed, 13 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
>> index 780fb7071577..e8a1e7fbffb9 100644
>> --- a/drivers/iommu/iommu.c
>> +++ b/drivers/iommu/iommu.c
>> @@ -289,18 +289,21 @@ int iommu_probe_device(struct device *dev)
>>         /*
>>        * Try to allocate a default domain - needs support from the
>> -     * IOMMU driver. There are still some drivers which don't
>> -     * support default domains, so the return value is not yet
>> -     * checked.
>> +     * IOMMU driver.
>>        */
>>       mutex_lock(&group->mutex);
>> -    iommu_alloc_default_domain(group, dev);
>> +    ret = iommu_alloc_default_domain(group, dev);
>> +    if (ret) {
>> +        mutex_unlock(&group->mutex);
>> +        iommu_group_put(group);
>> +        goto err_release;
>> +    }
> 
> Some IOMMU drivers (s390, gart, tegra...) don't support default domains.
> iommu_alloc_default_domain() returns a ENOMEM and the probe process
> continues.

Thanks for the clarification.

> 
> Above changes this behavior. The probe process will always fails with
> those IOMMU drivers.

Right. I am wondering whether we have any other way to check the return value?



-Vasant

Re: [RFC PATCH] iommu: Handle domain allocation failure gracefully

[Baolu Lu]

On 2022/9/15 14:13, Vasant Hegde wrote:
> On 9/15/2022 8:44 AM, Baolu Lu wrote:
>> On 9/14/22 10:44 PM, Vasant Hegde wrote:
>>> Current code does not validate domain allocation failures. This will
>>> result in random failure at later stage. Hence handle domain allocation
>>> failures.
>>>
>>> Signed-off-by: Vasant Hegde<vasant.hegde@amd.com>
>>> ---
>>> Hi Joerg,
>>>     With current code whenever amd driver failes to allocate domain we hit
>>>     failure at late stage and it became difficult to debug. Hence I
>>>     thought of adding this fix.
>>>
>>>     I see that in commit 6e1aa2049154 you have intentionally didn't check
>>>     the return value. Looking into drivers/iommu/* it looks like all
>>>     drivers has domain_alloc() callback handler. But some drivers (like msm
>>>     - msm_iommu_domain_alloc()) allocates default domain for certain type
>>>     only. Hence I'm not sure whether its ok to handle domain allocation
>>>     failures or not. Hence I have marked this patch as RFC.
>>>
>>>     Let me know whether this patch is fine -OR- is there any other way we can
>>>     handle domain allocation failures?
>>>
>>> -Vasant
>>>
>>>    drivers/iommu/iommu.c | 24 +++++++++++++-----------
>>>    1 file changed, 13 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
>>> index 780fb7071577..e8a1e7fbffb9 100644
>>> --- a/drivers/iommu/iommu.c
>>> +++ b/drivers/iommu/iommu.c
>>> @@ -289,18 +289,21 @@ int iommu_probe_device(struct device *dev)
>>>          /*
>>>         * Try to allocate a default domain - needs support from the
>>> -     * IOMMU driver. There are still some drivers which don't
>>> -     * support default domains, so the return value is not yet
>>> -     * checked.
>>> +     * IOMMU driver.
>>>         */
>>>        mutex_lock(&group->mutex);
>>> -    iommu_alloc_default_domain(group, dev);
>>> +    ret = iommu_alloc_default_domain(group, dev);
>>> +    if (ret) {
>>> +        mutex_unlock(&group->mutex);
>>> +        iommu_group_put(group);
>>> +        goto err_release;
>>> +    }
>> Some IOMMU drivers (s390, gart, tegra...) don't support default domains.
>> iommu_alloc_default_domain() returns a ENOMEM and the probe process
>> continues.
> Thanks for the clarification.
> 
>> Above changes this behavior. The probe process will always fails with
>> those IOMMU drivers.
> Right. I am wondering whether we have any other way to check the return value?

The __iommu_probe_device() treats ENODEV as a special return value which
actually means "the device has no IOMMU hardware, go ahead with other
devices." You can check with Joerg whether you can play the same game
here.

Best regards,
baolu

Re: [RFC PATCH] iommu: Handle domain allocation failure gracefully

[Vasant Hegde]

Joerg,


On 9/15/2022 12:58 PM, Baolu Lu wrote:
> On 2022/9/15 14:13, Vasant Hegde wrote:
>> On 9/15/2022 8:44 AM, Baolu Lu wrote:
>>> On 9/14/22 10:44 PM, Vasant Hegde wrote:
>>>> Current code does not validate domain allocation failures. This will
>>>> result in random failure at later stage. Hence handle domain allocation
>>>> failures.
>>>>
>>>> Signed-off-by: Vasant Hegde<vasant.hegde@amd.com>
>>>> ---
>>>> Hi Joerg,
>>>>     With current code whenever amd driver failes to allocate domain we hit
>>>>     failure at late stage and it became difficult to debug. Hence I
>>>>     thought of adding this fix.
>>>>
>>>>     I see that in commit 6e1aa2049154 you have intentionally didn't check
>>>>     the return value. Looking into drivers/iommu/* it looks like all
>>>>     drivers has domain_alloc() callback handler. But some drivers (like msm
>>>>     - msm_iommu_domain_alloc()) allocates default domain for certain type
>>>>     only. Hence I'm not sure whether its ok to handle domain allocation
>>>>     failures or not. Hence I have marked this patch as RFC.
>>>>
>>>>     Let me know whether this patch is fine -OR- is there any other way we can
>>>>     handle domain allocation failures?
>>>>
>>>> -Vasant
>>>>
>>>>    drivers/iommu/iommu.c | 24 +++++++++++++-----------
>>>>    1 file changed, 13 insertions(+), 11 deletions(-)
>>>>
>>>> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
>>>> index 780fb7071577..e8a1e7fbffb9 100644
>>>> --- a/drivers/iommu/iommu.c
>>>> +++ b/drivers/iommu/iommu.c
>>>> @@ -289,18 +289,21 @@ int iommu_probe_device(struct device *dev)
>>>>          /*
>>>>         * Try to allocate a default domain - needs support from the
>>>> -     * IOMMU driver. There are still some drivers which don't
>>>> -     * support default domains, so the return value is not yet
>>>> -     * checked.
>>>> +     * IOMMU driver.
>>>>         */
>>>>        mutex_lock(&group->mutex);
>>>> -    iommu_alloc_default_domain(group, dev);
>>>> +    ret = iommu_alloc_default_domain(group, dev);
>>>> +    if (ret) {
>>>> +        mutex_unlock(&group->mutex);
>>>> +        iommu_group_put(group);
>>>> +        goto err_release;
>>>> +    }
>>> Some IOMMU drivers (s390, gart, tegra...) don't support default domains.
>>> iommu_alloc_default_domain() returns a ENOMEM and the probe process
>>> continues.
>> Thanks for the clarification.
>>
>>> Above changes this behavior. The probe process will always fails with
>>> those IOMMU drivers.
>> Right. I am wondering whether we have any other way to check the return value?
> 
> The __iommu_probe_device() treats ENODEV as a special return value which
> actually means "the device has no IOMMU hardware, go ahead with other
> devices." You can check with Joerg whether you can play the same game
> here.

Any suggestion? Is there someway to handle error code?


-Vasant