diff mbox series

[v9,05/10] uacce: Enable IOMMU_DEV_FEAT_IOPF

Message ID 20210108145217.2254447-6-jean-philippe@linaro.org
State New
Headers show
Series iommu: I/O page faults for SMMUv3 | expand

Commit Message

Jean-Philippe Brucker Jan. 8, 2021, 2:52 p.m. UTC
The IOPF (I/O Page Fault) feature is now enabled independently from the
SVA feature, because some IOPF implementations are device-specific and
do not require IOMMU support for PCIe PRI or Arm SMMU stall.

Enable IOPF unconditionally when enabling SVA for now. In the future, if
a device driver implementing a uacce interface doesn't need IOPF
support, it will need to tell the uacce module, for example with a new
flag.

Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
---
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Zhangfei Gao <zhangfei.gao@linaro.org>
Cc: Zhou Wang <wangzhou1@hisilicon.com>
---
 drivers/misc/uacce/uacce.c | 32 +++++++++++++++++++++++++-------
 1 file changed, 25 insertions(+), 7 deletions(-)

Comments

Zhangfei Gao Jan. 11, 2021, 3:29 a.m. UTC | #1
On 2021/1/8 下午10:52, Jean-Philippe Brucker wrote:
> The IOPF (I/O Page Fault) feature is now enabled independently from the

> SVA feature, because some IOPF implementations are device-specific and

> do not require IOMMU support for PCIe PRI or Arm SMMU stall.

>

> Enable IOPF unconditionally when enabling SVA for now. In the future, if

> a device driver implementing a uacce interface doesn't need IOPF

> support, it will need to tell the uacce module, for example with a new

> flag.

>

> Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>

> ---

> Cc: Arnd Bergmann <arnd@arndb.de>

> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

> Cc: Zhangfei Gao <zhangfei.gao@linaro.org>

> Cc: Zhou Wang <wangzhou1@hisilicon.com>

Thanks Jean
Acked-by: Zhangfei Gao <zhangfei.gao@linaro.org>
Jonathan Cameron Jan. 19, 2021, 12:27 p.m. UTC | #2
On Fri, 8 Jan 2021 15:52:13 +0100
Jean-Philippe Brucker <jean-philippe@linaro.org> wrote:

> The IOPF (I/O Page Fault) feature is now enabled independently from the
> SVA feature, because some IOPF implementations are device-specific and
> do not require IOMMU support for PCIe PRI or Arm SMMU stall.
> 
> Enable IOPF unconditionally when enabling SVA for now. In the future, if
> a device driver implementing a uacce interface doesn't need IOPF
> support, it will need to tell the uacce module, for example with a new
> flag.
> 
> Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
Hi Jean-Philippe,

A minor suggestion inline but I'm not that bothered so either way
looks good to me.

> ---
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Zhangfei Gao <zhangfei.gao@linaro.org>
> Cc: Zhou Wang <wangzhou1@hisilicon.com>
> ---
>  drivers/misc/uacce/uacce.c | 32 +++++++++++++++++++++++++-------
>  1 file changed, 25 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/misc/uacce/uacce.c b/drivers/misc/uacce/uacce.c
> index d07af4edfcac..41ef1eb62a14 100644
> --- a/drivers/misc/uacce/uacce.c
> +++ b/drivers/misc/uacce/uacce.c
> @@ -385,6 +385,24 @@ static void uacce_release(struct device *dev)
>  	kfree(uacce);
>  }
>  
> +static unsigned int uacce_enable_sva(struct device *parent, unsigned int flags)
> +{
> +	if (!(flags & UACCE_DEV_SVA))
> +		return flags;
> +
> +	flags &= ~UACCE_DEV_SVA;
> +
> +	if (iommu_dev_enable_feature(parent, IOMMU_DEV_FEAT_IOPF))
> +		return flags;
> +
> +	if (iommu_dev_enable_feature(parent, IOMMU_DEV_FEAT_SVA)) {
> +		iommu_dev_disable_feature(parent, IOMMU_DEV_FEAT_IOPF);
> +		return flags;
> +	}
> +
> +	return flags | UACCE_DEV_SVA;
> +}

I'm a great fan of paired enable / disable functions.
Whilst it would be trivial, maybe it is worth introducing

uacce_disable_sva()?
Also make that do the flags check internally to make it match
up with the enable path.


> +
>  /**
>   * uacce_alloc() - alloc an accelerator
>   * @parent: pointer of uacce parent device
> @@ -404,11 +422,7 @@ struct uacce_device *uacce_alloc(struct device *parent,
>  	if (!uacce)
>  		return ERR_PTR(-ENOMEM);
>  
> -	if (flags & UACCE_DEV_SVA) {
> -		ret = iommu_dev_enable_feature(parent, IOMMU_DEV_FEAT_SVA);
> -		if (ret)
> -			flags &= ~UACCE_DEV_SVA;
> -	}
> +	flags = uacce_enable_sva(parent, flags);
>  
>  	uacce->parent = parent;
>  	uacce->flags = flags;
> @@ -432,8 +446,10 @@ struct uacce_device *uacce_alloc(struct device *parent,
>  	return uacce;
>  
>  err_with_uacce:
> -	if (flags & UACCE_DEV_SVA)
> +	if (flags & UACCE_DEV_SVA) {
>  		iommu_dev_disable_feature(uacce->parent, IOMMU_DEV_FEAT_SVA);
> +		iommu_dev_disable_feature(uacce->parent, IOMMU_DEV_FEAT_IOPF);
> +	}
>  	kfree(uacce);
>  	return ERR_PTR(ret);
>  }
> @@ -487,8 +503,10 @@ void uacce_remove(struct uacce_device *uacce)
>  	mutex_unlock(&uacce->queues_lock);
>  
>  	/* disable sva now since no opened queues */
> -	if (uacce->flags & UACCE_DEV_SVA)
> +	if (uacce->flags & UACCE_DEV_SVA) {
>  		iommu_dev_disable_feature(uacce->parent, IOMMU_DEV_FEAT_SVA);
> +		iommu_dev_disable_feature(uacce->parent, IOMMU_DEV_FEAT_IOPF);
> +	}
>  
>  	if (uacce->cdev)
>  		cdev_device_del(uacce->cdev, &uacce->dev);
Jean-Philippe Brucker Jan. 20, 2021, 5:42 p.m. UTC | #3
On Tue, Jan 19, 2021 at 12:27:59PM +0000, Jonathan Cameron wrote:
> On Fri, 8 Jan 2021 15:52:13 +0100

> Jean-Philippe Brucker <jean-philippe@linaro.org> wrote:

> 

> > The IOPF (I/O Page Fault) feature is now enabled independently from the

> > SVA feature, because some IOPF implementations are device-specific and

> > do not require IOMMU support for PCIe PRI or Arm SMMU stall.

> > 

> > Enable IOPF unconditionally when enabling SVA for now. In the future, if

> > a device driver implementing a uacce interface doesn't need IOPF

> > support, it will need to tell the uacce module, for example with a new

> > flag.

> > 

> > Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>

> Hi Jean-Philippe,

> 

> A minor suggestion inline but I'm not that bothered so either way

> looks good to me.


No problem, I'll add the disable function

Thanks,
Jean
Dave Jiang Jan. 20, 2021, 8:47 p.m. UTC | #4
On 1/8/2021 7:52 AM, Jean-Philippe Brucker wrote:
> The IOPF (I/O Page Fault) feature is now enabled independently from the
> SVA feature, because some IOPF implementations are device-specific and
> do not require IOMMU support for PCIe PRI or Arm SMMU stall.
>
> Enable IOPF unconditionally when enabling SVA for now. In the future, if
> a device driver implementing a uacce interface doesn't need IOPF
> support, it will need to tell the uacce module, for example with a new
> flag.
>
> Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
> ---
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Zhangfei Gao <zhangfei.gao@linaro.org>
> Cc: Zhou Wang <wangzhou1@hisilicon.com>
> ---
>   drivers/misc/uacce/uacce.c | 32 +++++++++++++++++++++++++-------
>   1 file changed, 25 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/misc/uacce/uacce.c b/drivers/misc/uacce/uacce.c
> index d07af4edfcac..41ef1eb62a14 100644
> --- a/drivers/misc/uacce/uacce.c
> +++ b/drivers/misc/uacce/uacce.c
> @@ -385,6 +385,24 @@ static void uacce_release(struct device *dev)
>   	kfree(uacce);
>   }
>   
> +static unsigned int uacce_enable_sva(struct device *parent, unsigned int flags)
> +{
> +	if (!(flags & UACCE_DEV_SVA))
> +		return flags;
> +
> +	flags &= ~UACCE_DEV_SVA;
> +
> +	if (iommu_dev_enable_feature(parent, IOMMU_DEV_FEAT_IOPF))
> +		return flags;
> +
> +	if (iommu_dev_enable_feature(parent, IOMMU_DEV_FEAT_SVA)) {
> +		iommu_dev_disable_feature(parent, IOMMU_DEV_FEAT_IOPF);
> +		return flags;
> +	}

Sorry to jump in a bit late on this and not specifically towards the 
intent of this patch. But I'd like to start a discussion on if we want 
to push the iommu dev feature enabling to the device driver itself 
rather than having UACCE control this? Maybe allow the device driver to 
manage the feature bits and UACCE only verify that they are enabled?

 1. The device driver knows what platform it's on and what specific
    feature bits its devices supports. Maybe in the future if there are
    feature bits that's needed on one platform and not on another?
 2. This allows the possibility of multiple uacce device registered to 1
    pci dev, which for a device with asymmetric queues (Intel DSA/idxd
    driver) that is desirable feature. The current setup forces a single
    uacce device per pdev. If additional uacce devs are registered, the
    first removal of uacce device will disable the feature bit for the
    rest of the registered devices. With uacce managing the feature bit,
    it would need to add device context to the parent pdev and ref
    counting. It may be cleaner to just allow device driver to manage
    the feature bits and the driver should have all the information on
    when the feature needs to be turned on and off.

- DaveJ


> +
> +	return flags | UACCE_DEV_SVA;
> +}
> +
>   /**
>    * uacce_alloc() - alloc an accelerator
>    * @parent: pointer of uacce parent device
> @@ -404,11 +422,7 @@ struct uacce_device *uacce_alloc(struct device *parent,
>   	if (!uacce)
>   		return ERR_PTR(-ENOMEM);
>   
> -	if (flags & UACCE_DEV_SVA) {
> -		ret = iommu_dev_enable_feature(parent, IOMMU_DEV_FEAT_SVA);
> -		if (ret)
> -			flags &= ~UACCE_DEV_SVA;
> -	}
> +	flags = uacce_enable_sva(parent, flags);
>   
>   	uacce->parent = parent;
>   	uacce->flags = flags;
> @@ -432,8 +446,10 @@ struct uacce_device *uacce_alloc(struct device *parent,
>   	return uacce;
>   
>   err_with_uacce:
> -	if (flags & UACCE_DEV_SVA)
> +	if (flags & UACCE_DEV_SVA) {
>   		iommu_dev_disable_feature(uacce->parent, IOMMU_DEV_FEAT_SVA);
> +		iommu_dev_disable_feature(uacce->parent, IOMMU_DEV_FEAT_IOPF);
> +	}
>   	kfree(uacce);
>   	return ERR_PTR(ret);
>   }
> @@ -487,8 +503,10 @@ void uacce_remove(struct uacce_device *uacce)
>   	mutex_unlock(&uacce->queues_lock);
>   
>   	/* disable sva now since no opened queues */
> -	if (uacce->flags & UACCE_DEV_SVA)
> +	if (uacce->flags & UACCE_DEV_SVA) {
>   		iommu_dev_disable_feature(uacce->parent, IOMMU_DEV_FEAT_SVA);
> +		iommu_dev_disable_feature(uacce->parent, IOMMU_DEV_FEAT_IOPF);
> +	}
>   
>   	if (uacce->cdev)
>   		cdev_device_del(uacce->cdev, &uacce->dev);
Zhou Wang Jan. 22, 2021, 11:53 a.m. UTC | #5
On 2021/1/21 4:47, Dave Jiang wrote:
> 
> On 1/8/2021 7:52 AM, Jean-Philippe Brucker wrote:
>> The IOPF (I/O Page Fault) feature is now enabled independently from the
>> SVA feature, because some IOPF implementations are device-specific and
>> do not require IOMMU support for PCIe PRI or Arm SMMU stall.
>>
>> Enable IOPF unconditionally when enabling SVA for now. In the future, if
>> a device driver implementing a uacce interface doesn't need IOPF
>> support, it will need to tell the uacce module, for example with a new
>> flag.
>>
>> Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
>> ---
>> Cc: Arnd Bergmann <arnd@arndb.de>
>> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>> Cc: Zhangfei Gao <zhangfei.gao@linaro.org>
>> Cc: Zhou Wang <wangzhou1@hisilicon.com>
>> ---
>>   drivers/misc/uacce/uacce.c | 32 +++++++++++++++++++++++++-------
>>   1 file changed, 25 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/misc/uacce/uacce.c b/drivers/misc/uacce/uacce.c
>> index d07af4edfcac..41ef1eb62a14 100644
>> --- a/drivers/misc/uacce/uacce.c
>> +++ b/drivers/misc/uacce/uacce.c
>> @@ -385,6 +385,24 @@ static void uacce_release(struct device *dev)
>>       kfree(uacce);
>>   }
>>   +static unsigned int uacce_enable_sva(struct device *parent, unsigned int flags)
>> +{
>> +    if (!(flags & UACCE_DEV_SVA))
>> +        return flags;
>> +
>> +    flags &= ~UACCE_DEV_SVA;
>> +
>> +    if (iommu_dev_enable_feature(parent, IOMMU_DEV_FEAT_IOPF))
>> +        return flags;
>> +
>> +    if (iommu_dev_enable_feature(parent, IOMMU_DEV_FEAT_SVA)) {
>> +        iommu_dev_disable_feature(parent, IOMMU_DEV_FEAT_IOPF);
>> +        return flags;
>> +    }
> 
> Sorry to jump in a bit late on this and not specifically towards the
> intent of this patch. But I'd like to start a discussion on if we want
> to push the iommu dev feature enabling to the device driver itself rather
> than having UACCE control this? Maybe allow the device driver to manage
> the feature bits and UACCE only verify that they are enabled?
> 
> 1. The device driver knows what platform it's on and what specific
>    feature bits its devices supports. Maybe in the future if there are
>    feature bits that's needed on one platform and not on another?

Hi Dave,
Dave Jiang Jan. 22, 2021, 3:43 p.m. UTC | #6
On 1/22/2021 4:53 AM, Zhou Wang wrote:
> On 2021/1/21 4:47, Dave Jiang wrote:

>> On 1/8/2021 7:52 AM, Jean-Philippe Brucker wrote:

>>> The IOPF (I/O Page Fault) feature is now enabled independently from the

>>> SVA feature, because some IOPF implementations are device-specific and

>>> do not require IOMMU support for PCIe PRI or Arm SMMU stall.

>>>

>>> Enable IOPF unconditionally when enabling SVA for now. In the future, if

>>> a device driver implementing a uacce interface doesn't need IOPF

>>> support, it will need to tell the uacce module, for example with a new

>>> flag.

>>>

>>> Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>

>>> ---

>>> Cc: Arnd Bergmann <arnd@arndb.de>

>>> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

>>> Cc: Zhangfei Gao <zhangfei.gao@linaro.org>

>>> Cc: Zhou Wang <wangzhou1@hisilicon.com>

>>> ---

>>>    drivers/misc/uacce/uacce.c | 32 +++++++++++++++++++++++++-------

>>>    1 file changed, 25 insertions(+), 7 deletions(-)

>>>

>>> diff --git a/drivers/misc/uacce/uacce.c b/drivers/misc/uacce/uacce.c

>>> index d07af4edfcac..41ef1eb62a14 100644

>>> --- a/drivers/misc/uacce/uacce.c

>>> +++ b/drivers/misc/uacce/uacce.c

>>> @@ -385,6 +385,24 @@ static void uacce_release(struct device *dev)

>>>        kfree(uacce);

>>>    }

>>>    +static unsigned int uacce_enable_sva(struct device *parent, unsigned int flags)

>>> +{

>>> +    if (!(flags & UACCE_DEV_SVA))

>>> +        return flags;

>>> +

>>> +    flags &= ~UACCE_DEV_SVA;

>>> +

>>> +    if (iommu_dev_enable_feature(parent, IOMMU_DEV_FEAT_IOPF))

>>> +        return flags;

>>> +

>>> +    if (iommu_dev_enable_feature(parent, IOMMU_DEV_FEAT_SVA)) {

>>> +        iommu_dev_disable_feature(parent, IOMMU_DEV_FEAT_IOPF);

>>> +        return flags;

>>> +    }

>> Sorry to jump in a bit late on this and not specifically towards the

>> intent of this patch. But I'd like to start a discussion on if we want

>> to push the iommu dev feature enabling to the device driver itself rather

>> than having UACCE control this? Maybe allow the device driver to manage

>> the feature bits and UACCE only verify that they are enabled?

>>

>> 1. The device driver knows what platform it's on and what specific

>>     feature bits its devices supports. Maybe in the future if there are

>>     feature bits that's needed on one platform and not on another?

> Hi Dave,

>

>  From the discussion in this series, the meaning of IOMMU_DEV_FEAT_IOPF here

> is the IOPF capability of iommu device itself. So I think check it in UACCE

> will be fine.

>

>> 2. This allows the possibility of multiple uacce device registered to 1

>>     pci dev, which for a device with asymmetric queues (Intel DSA/idxd

>>     driver) that is desirable feature. The current setup forces a single

>>     uacce device per pdev. If additional uacce devs are registered, the

>>     first removal of uacce device will disable the feature bit for the

>>     rest of the registered devices. With uacce managing the feature bit,

>>     it would need to add device context to the parent pdev and ref

>>     counting. It may be cleaner to just allow device driver to manage

>>     the feature bits and the driver should have all the information on

>>     when the feature needs to be turned on and off.

> Yes, we have this problem, however, this problem exists for IOMMU_DEV_FEAT_SVA

> too. How about to fix it in another patch?


Hi Zhou,

Right that's what I'm implying. I'm not pushing back on the IOPF feature 
set. Just trying to survey  the opinions from people on moving the 
feature settings to the actual drivers rather than having it in UACCE. I 
will create some patches to show what I mean for comments.


>

> Best,

> Zhou

>

>> - DaveJ

>>

>>

>>> +

>>> +    return flags | UACCE_DEV_SVA;

>>> +}

>>> +

>>>    /**

>>>     * uacce_alloc() - alloc an accelerator

>>>     * @parent: pointer of uacce parent device

>>> @@ -404,11 +422,7 @@ struct uacce_device *uacce_alloc(struct device *parent,

>>>        if (!uacce)

>>>            return ERR_PTR(-ENOMEM);

>>>    -    if (flags & UACCE_DEV_SVA) {

>>> -        ret = iommu_dev_enable_feature(parent, IOMMU_DEV_FEAT_SVA);

>>> -        if (ret)

>>> -            flags &= ~UACCE_DEV_SVA;

>>> -    }

>>> +    flags = uacce_enable_sva(parent, flags);

>>>          uacce->parent = parent;

>>>        uacce->flags = flags;

>>> @@ -432,8 +446,10 @@ struct uacce_device *uacce_alloc(struct device *parent,

>>>        return uacce;

>>>      err_with_uacce:

>>> -    if (flags & UACCE_DEV_SVA)

>>> +    if (flags & UACCE_DEV_SVA) {

>>>            iommu_dev_disable_feature(uacce->parent, IOMMU_DEV_FEAT_SVA);

>>> +        iommu_dev_disable_feature(uacce->parent, IOMMU_DEV_FEAT_IOPF);

>>> +    }

>>>        kfree(uacce);

>>>        return ERR_PTR(ret);

>>>    }

>>> @@ -487,8 +503,10 @@ void uacce_remove(struct uacce_device *uacce)

>>>        mutex_unlock(&uacce->queues_lock);

>>>          /* disable sva now since no opened queues */

>>> -    if (uacce->flags & UACCE_DEV_SVA)

>>> +    if (uacce->flags & UACCE_DEV_SVA) {

>>>            iommu_dev_disable_feature(uacce->parent, IOMMU_DEV_FEAT_SVA);

>>> +        iommu_dev_disable_feature(uacce->parent, IOMMU_DEV_FEAT_IOPF);

>>> +    }

>>>          if (uacce->cdev)

>>>            cdev_device_del(uacce->cdev, &uacce->dev);

>> .

>>
diff mbox series

Patch

diff --git a/drivers/misc/uacce/uacce.c b/drivers/misc/uacce/uacce.c
index d07af4edfcac..41ef1eb62a14 100644
--- a/drivers/misc/uacce/uacce.c
+++ b/drivers/misc/uacce/uacce.c
@@ -385,6 +385,24 @@  static void uacce_release(struct device *dev)
 	kfree(uacce);
 }
 
+static unsigned int uacce_enable_sva(struct device *parent, unsigned int flags)
+{
+	if (!(flags & UACCE_DEV_SVA))
+		return flags;
+
+	flags &= ~UACCE_DEV_SVA;
+
+	if (iommu_dev_enable_feature(parent, IOMMU_DEV_FEAT_IOPF))
+		return flags;
+
+	if (iommu_dev_enable_feature(parent, IOMMU_DEV_FEAT_SVA)) {
+		iommu_dev_disable_feature(parent, IOMMU_DEV_FEAT_IOPF);
+		return flags;
+	}
+
+	return flags | UACCE_DEV_SVA;
+}
+
 /**
  * uacce_alloc() - alloc an accelerator
  * @parent: pointer of uacce parent device
@@ -404,11 +422,7 @@  struct uacce_device *uacce_alloc(struct device *parent,
 	if (!uacce)
 		return ERR_PTR(-ENOMEM);
 
-	if (flags & UACCE_DEV_SVA) {
-		ret = iommu_dev_enable_feature(parent, IOMMU_DEV_FEAT_SVA);
-		if (ret)
-			flags &= ~UACCE_DEV_SVA;
-	}
+	flags = uacce_enable_sva(parent, flags);
 
 	uacce->parent = parent;
 	uacce->flags = flags;
@@ -432,8 +446,10 @@  struct uacce_device *uacce_alloc(struct device *parent,
 	return uacce;
 
 err_with_uacce:
-	if (flags & UACCE_DEV_SVA)
+	if (flags & UACCE_DEV_SVA) {
 		iommu_dev_disable_feature(uacce->parent, IOMMU_DEV_FEAT_SVA);
+		iommu_dev_disable_feature(uacce->parent, IOMMU_DEV_FEAT_IOPF);
+	}
 	kfree(uacce);
 	return ERR_PTR(ret);
 }
@@ -487,8 +503,10 @@  void uacce_remove(struct uacce_device *uacce)
 	mutex_unlock(&uacce->queues_lock);
 
 	/* disable sva now since no opened queues */
-	if (uacce->flags & UACCE_DEV_SVA)
+	if (uacce->flags & UACCE_DEV_SVA) {
 		iommu_dev_disable_feature(uacce->parent, IOMMU_DEV_FEAT_SVA);
+		iommu_dev_disable_feature(uacce->parent, IOMMU_DEV_FEAT_IOPF);
+	}
 
 	if (uacce->cdev)
 		cdev_device_del(uacce->cdev, &uacce->dev);