Re: [PATCH RESEND v5 1/5] iommu: Refactor iommu_group_store_type()

[Leizhen (ThunderTown)]

Reviewed-by: Zhen Lei <thunder.leizhen@xxxxxxxxxx>

On 2022/4/4 19:27, John Garry wrote:
> Function iommu_group_store_type() supports changing the default domain
> of an IOMMU group.
> 
> Many conditions need to be satisfied and steps taken for this action to be
> successful.
> 
> Satisfying these conditions and steps will be required for setting other
> IOMMU group attributes, so factor into a common part and a part specific
> to update the IOMMU group attribute.
> 
> No functional change intended.
> 
> Some code comments are tidied up also.
> 
> Signed-off-by: John Garry <john.garry@xxxxxxxxxx>
> ---
>  drivers/iommu/iommu.c | 96 ++++++++++++++++++++++++++++---------------
>  1 file changed, 62 insertions(+), 34 deletions(-)
> 
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index f2c45b85b9fc..0dd766030baf 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -3000,21 +3000,57 @@ static int iommu_change_dev_def_domain(struct iommu_group *group,
>  	return ret;
>  }
>  
> +enum iommu_group_op {
> +	CHANGE_GROUP_TYPE,
> +};
> +
> +static int __iommu_group_store_type(const char *buf, struct iommu_group *group,
> +				    struct device *dev)
> +{
> +	int type;
> +
> +	if (sysfs_streq(buf, "identity"))
> +		type = IOMMU_DOMAIN_IDENTITY;
> +	else if (sysfs_streq(buf, "DMA"))
> +		type = IOMMU_DOMAIN_DMA;
> +	else if (sysfs_streq(buf, "DMA-FQ"))
> +		type = IOMMU_DOMAIN_DMA_FQ;
> +	else if (sysfs_streq(buf, "auto"))
> +		type = 0;
> +	else
> +		return -EINVAL;
> +
> +	/*
> +	 * Check if the only device in the group still has a driver bound or
> +	 * we're transistioning from DMA -> DMA-FQ
> +	 */
> +	if (device_is_bound(dev) && !(type == IOMMU_DOMAIN_DMA_FQ &&
> +	    group->default_domain->type == IOMMU_DOMAIN_DMA)) {
> +		pr_err_ratelimited("Device is still bound to driver\n");
> +		return -EINVAL;
> +	}
> +
> +	return iommu_change_dev_def_domain(group, dev, type);
> +}
> +
>  /*
>   * Changing the default domain through sysfs requires the users to unbind the
>   * drivers from the devices in the iommu group, except for a DMA -> DMA-FQ
> - * transition. Return failure if this isn't met.
> + * transition. Changing or any other IOMMU group attribute still requires the
> + * user to unbind the drivers from the devices in the iommu group. Return
> + * failure if these conditions are not met.
>   *
>   * We need to consider the race between this and the device release path.
>   * device_lock(dev) is used here to guarantee that the device release path
>   * will not be entered at the same time.
>   */
> -static ssize_t iommu_group_store_type(struct iommu_group *group,
> -				      const char *buf, size_t count)
> +static ssize_t iommu_group_store_common(struct iommu_group *group,
> +					enum iommu_group_op op,
> +					const char *buf, size_t count)
>  {
>  	struct group_device *grp_dev;
>  	struct device *dev;
> -	int ret, req_type;
> +	int ret;
>  
>  	if (!capable(CAP_SYS_ADMIN) || !capable(CAP_SYS_RAWIO))
>  		return -EACCES;
> @@ -3022,27 +3058,16 @@ static ssize_t iommu_group_store_type(struct iommu_group *group,
>  	if (WARN_ON(!group))
>  		return -EINVAL;
>  
> -	if (sysfs_streq(buf, "identity"))
> -		req_type = IOMMU_DOMAIN_IDENTITY;
> -	else if (sysfs_streq(buf, "DMA"))
> -		req_type = IOMMU_DOMAIN_DMA;
> -	else if (sysfs_streq(buf, "DMA-FQ"))
> -		req_type = IOMMU_DOMAIN_DMA_FQ;
> -	else if (sysfs_streq(buf, "auto"))
> -		req_type = 0;
> -	else
> -		return -EINVAL;
> -
>  	/*
>  	 * Lock/Unlock the group mutex here before device lock to
> -	 * 1. Make sure that the iommu group has only one device (this is a
> +	 * 1. Make sure that the IOMMU group has only one device (this is a
>  	 *    prerequisite for step 2)
>  	 * 2. Get struct *dev which is needed to lock device
>  	 */
>  	mutex_lock(&group->mutex);
>  	if (iommu_group_device_count(group) != 1) {
>  		mutex_unlock(&group->mutex);
> -		pr_err_ratelimited("Cannot change default domain: Group has more than one device\n");
> +		pr_err_ratelimited("Cannot change IOMMU group default domain attribute: Group has more than one device\n");
>  		return -EINVAL;
>  	}
>  
> @@ -3054,16 +3079,16 @@ static ssize_t iommu_group_store_type(struct iommu_group *group,
>  	/*
>  	 * Don't hold the group mutex because taking group mutex first and then
>  	 * the device lock could potentially cause a deadlock as below. Assume
> -	 * two threads T1 and T2. T1 is trying to change default domain of an
> -	 * iommu group and T2 is trying to hot unplug a device or release [1] VF
> -	 * of a PCIe device which is in the same iommu group. T1 takes group
> -	 * mutex and before it could take device lock assume T2 has taken device
> -	 * lock and is yet to take group mutex. Now, both the threads will be
> -	 * waiting for the other thread to release lock. Below, lock order was
> -	 * suggested.
> +	 * two threads, T1 and T2. T1 is trying to change default domain
> +	 * attribute of an IOMMU group and T2 is trying to hot unplug a device
> +	 * or release [1] VF of a PCIe device which is in the same IOMMU group.
> +	 * T1 takes the group mutex and before it could take device lock T2 may
> +	 * have taken device lock and is yet to take group mutex. Now, both the
> +	 * threads will be waiting for the other thread to release lock. Below,
> +	 * lock order was suggested.
>  	 * device_lock(dev);
>  	 *	mutex_lock(&group->mutex);
> -	 *		iommu_change_dev_def_domain();
> +	 *		cb->iommu_change_dev_def_domain(); [example cb]
>  	 *	mutex_unlock(&group->mutex);
>  	 * device_unlock(dev);
>  	 *
> @@ -3077,21 +3102,24 @@ static ssize_t iommu_group_store_type(struct iommu_group *group,
>  	 */
>  	mutex_unlock(&group->mutex);
>  
> -	/* Check if the device in the group still has a driver bound to it */
>  	device_lock(dev);
> -	if (device_is_bound(dev) && !(req_type == IOMMU_DOMAIN_DMA_FQ &&
> -	    group->default_domain->type == IOMMU_DOMAIN_DMA)) {
> -		pr_err_ratelimited("Device is still bound to driver\n");
> -		ret = -EBUSY;
> -		goto out;
> +	switch (op) {
> +	case CHANGE_GROUP_TYPE:
> +		ret = __iommu_group_store_type(buf, group, dev);
> +		break;
> +	default:
> +		ret = -EINVAL;
>  	}
> -
> -	ret = iommu_change_dev_def_domain(group, dev, req_type);
>  	ret = ret ?: count;
>  
> -out:
>  	device_unlock(dev);
>  	put_device(dev);
>  
>  	return ret;
>  }
> +
> +static ssize_t iommu_group_store_type(struct iommu_group *group,
> +				      const char *buf, size_t count)
> +{
> +	return iommu_group_store_common(group, CHANGE_GROUP_TYPE, buf, count);
> +}
> 

-- 
Regards,
  Zhen Lei