Remove the static iopf_param pointer from struct iommu_fault_param to
save memory.
Signed-off-by: Lu Baolu <[email protected]>
---
include/linux/iommu.h | 2 --
drivers/iommu/io-pgfault.c | 47 +++++++++++++++++++++++---------------
2 files changed, 29 insertions(+), 20 deletions(-)
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index ffd6fe1317f4..5fe37a7c5a55 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -551,7 +551,6 @@ struct iommu_fault_param {
* struct dev_iommu - Collection of per-device IOMMU data
*
* @fault_param: IOMMU detected device fault reporting data
- * @iopf_param: I/O Page Fault queue and data
* @fwspec: IOMMU fwspec data
* @iommu_dev: IOMMU device this device is linked to
* @priv: IOMMU Driver private data
@@ -564,7 +563,6 @@ struct iommu_fault_param {
struct dev_iommu {
struct mutex lock;
struct iommu_fault_param *fault_param;
- struct iopf_device_param *iopf_param;
struct iommu_fwspec *fwspec;
struct iommu_device *iommu_dev;
void *priv;
diff --git a/drivers/iommu/io-pgfault.c b/drivers/iommu/io-pgfault.c
index 1749e0869f2e..6a3a4e08e67e 100644
--- a/drivers/iommu/io-pgfault.c
+++ b/drivers/iommu/io-pgfault.c
@@ -158,7 +158,7 @@ int iommu_queue_iopf(struct iommu_fault *fault, struct device *dev)
* As long as we're holding param->lock, the queue can't be unlinked
* from the device and therefore cannot disappear.
*/
- iopf_param = param->iopf_param;
+ iopf_param = iommu_get_device_fault_cookie(dev, 0);
if (!iopf_param)
return -ENODEV;
@@ -235,7 +235,7 @@ int iopf_queue_flush_dev(struct device *dev)
return -ENODEV;
mutex_lock(¶m->lock);
- iopf_param = param->iopf_param;
+ iopf_param = iommu_get_device_fault_cookie(dev, 0);
if (iopf_param)
flush_workqueue(iopf_param->queue->wq);
else
@@ -286,9 +286,9 @@ EXPORT_SYMBOL_GPL(iopf_queue_discard_partial);
*/
int iopf_queue_add_device(struct iopf_queue *queue, struct device *dev)
{
- int ret = -EBUSY;
- struct iopf_device_param *iopf_param;
+ struct iopf_device_param *iopf_param, *curr;
struct dev_iommu *param = dev->iommu;
+ int ret;
if (!param)
return -ENODEV;
@@ -303,16 +303,27 @@ int iopf_queue_add_device(struct iopf_queue *queue, struct device *dev)
mutex_lock(&queue->lock);
mutex_lock(¶m->lock);
- if (!param->iopf_param) {
- list_add(&iopf_param->queue_list, &queue->devices);
- param->iopf_param = iopf_param;
- ret = 0;
+ curr = iommu_set_device_fault_cookie(dev, 0, iopf_param);
+ if (IS_ERR(curr)) {
+ ret = PTR_ERR(curr);
+ goto err_free;
}
+
+ if (curr) {
+ ret = -EBUSY;
+ goto err_restore;
+ }
+ list_add(&iopf_param->queue_list, &queue->devices);
mutex_unlock(¶m->lock);
mutex_unlock(&queue->lock);
- if (ret)
- kfree(iopf_param);
+ return 0;
+err_restore:
+ iommu_set_device_fault_cookie(dev, 0, curr);
+err_free:
+ mutex_unlock(¶m->lock);
+ mutex_unlock(&queue->lock);
+ kfree(iopf_param);
return ret;
}
@@ -329,7 +340,6 @@ EXPORT_SYMBOL_GPL(iopf_queue_add_device);
*/
int iopf_queue_remove_device(struct iopf_queue *queue, struct device *dev)
{
- int ret = -EINVAL;
struct iopf_fault *iopf, *next;
struct iopf_device_param *iopf_param;
struct dev_iommu *param = dev->iommu;
@@ -339,16 +349,17 @@ int iopf_queue_remove_device(struct iopf_queue *queue, struct device *dev)
mutex_lock(&queue->lock);
mutex_lock(¶m->lock);
- iopf_param = param->iopf_param;
- if (iopf_param && iopf_param->queue == queue) {
- list_del(&iopf_param->queue_list);
- param->iopf_param = NULL;
- ret = 0;
+ iopf_param = iommu_get_device_fault_cookie(dev, 0);
+ if (!iopf_param || iopf_param->queue != queue) {
+ mutex_unlock(¶m->lock);
+ mutex_unlock(&queue->lock);
+ return -EINVAL;
}
+
+ list_del(&iopf_param->queue_list);
+ iommu_set_device_fault_cookie(dev, 0, NULL);
mutex_unlock(¶m->lock);
mutex_unlock(&queue->lock);
- if (ret)
- return ret;
/* Just in case some faults are still stuck */
list_for_each_entry_safe(iopf, next, &iopf_param->partial, list)
--
2.34.1
> From: Lu Baolu <[email protected]>
> Sent: Tuesday, July 11, 2023 9:07 AM
>
> Remove the static iopf_param pointer from struct iommu_fault_param to
> save memory.
why is there memory saving? you replace a single pointer with a xarray now...
> @@ -303,16 +303,27 @@ int iopf_queue_add_device(struct iopf_queue
> *queue, struct device *dev)
>
> mutex_lock(&queue->lock);
> mutex_lock(¶m->lock);
> - if (!param->iopf_param) {
> - list_add(&iopf_param->queue_list, &queue->devices);
> - param->iopf_param = iopf_param;
> - ret = 0;
> + curr = iommu_set_device_fault_cookie(dev, 0, iopf_param);
> + if (IS_ERR(curr)) {
> + ret = PTR_ERR(curr);
> + goto err_free;
> }
So although the new xarray is called a per-pasid storage, here only
slot#0 is used for sva which includes a list containing partial req's
for many pasid's. It doesn't sound clean...
Hi BaoLu,
On Tue, 11 Jul 2023 09:06:42 +0800, Lu Baolu <[email protected]>
wrote:
> Remove the static iopf_param pointer from struct iommu_fault_param to
> save memory.
>
> Signed-off-by: Lu Baolu <[email protected]>
> ---
> include/linux/iommu.h | 2 --
> drivers/iommu/io-pgfault.c | 47 +++++++++++++++++++++++---------------
> 2 files changed, 29 insertions(+), 20 deletions(-)
>
> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> index ffd6fe1317f4..5fe37a7c5a55 100644
> --- a/include/linux/iommu.h
> +++ b/include/linux/iommu.h
> @@ -551,7 +551,6 @@ struct iommu_fault_param {
> * struct dev_iommu - Collection of per-device IOMMU data
> *
> * @fault_param: IOMMU detected device fault reporting data
> - * @iopf_param: I/O Page Fault queue and data
> * @fwspec: IOMMU fwspec data
> * @iommu_dev: IOMMU device this device is linked to
> * @priv: IOMMU Driver private data
> @@ -564,7 +563,6 @@ struct iommu_fault_param {
> struct dev_iommu {
> struct mutex lock;
> struct iommu_fault_param *fault_param;
> - struct iopf_device_param *iopf_param;
> struct iommu_fwspec *fwspec;
> struct iommu_device *iommu_dev;
> void *priv;
> diff --git a/drivers/iommu/io-pgfault.c b/drivers/iommu/io-pgfault.c
> index 1749e0869f2e..6a3a4e08e67e 100644
> --- a/drivers/iommu/io-pgfault.c
> +++ b/drivers/iommu/io-pgfault.c
> @@ -158,7 +158,7 @@ int iommu_queue_iopf(struct iommu_fault *fault,
> struct device *dev)
> * As long as we're holding param->lock, the queue can't be
> unlinked
> * from the device and therefore cannot disappear.
> */
> - iopf_param = param->iopf_param;
> + iopf_param = iommu_get_device_fault_cookie(dev, 0);
I am not sure I understand how does it know the cookie type is iopf_param
for PASID 0?
Between IOPF and IOMMUFD use of the cookie, cookie types are different,
right?
> if (!iopf_param)
> return -ENODEV;
>
> @@ -235,7 +235,7 @@ int iopf_queue_flush_dev(struct device *dev)
> return -ENODEV;
>
> mutex_lock(¶m->lock);
> - iopf_param = param->iopf_param;
> + iopf_param = iommu_get_device_fault_cookie(dev, 0);
> if (iopf_param)
> flush_workqueue(iopf_param->queue->wq);
> else
> @@ -286,9 +286,9 @@ EXPORT_SYMBOL_GPL(iopf_queue_discard_partial);
> */
> int iopf_queue_add_device(struct iopf_queue *queue, struct device *dev)
> {
> - int ret = -EBUSY;
> - struct iopf_device_param *iopf_param;
> + struct iopf_device_param *iopf_param, *curr;
> struct dev_iommu *param = dev->iommu;
> + int ret;
>
> if (!param)
> return -ENODEV;
> @@ -303,16 +303,27 @@ int iopf_queue_add_device(struct iopf_queue *queue,
> struct device *dev)
> mutex_lock(&queue->lock);
> mutex_lock(¶m->lock);
> - if (!param->iopf_param) {
> - list_add(&iopf_param->queue_list, &queue->devices);
> - param->iopf_param = iopf_param;
> - ret = 0;
> + curr = iommu_set_device_fault_cookie(dev, 0, iopf_param);
> + if (IS_ERR(curr)) {
> + ret = PTR_ERR(curr);
> + goto err_free;
> }
> +
> + if (curr) {
> + ret = -EBUSY;
> + goto err_restore;
> + }
> + list_add(&iopf_param->queue_list, &queue->devices);
> mutex_unlock(¶m->lock);
> mutex_unlock(&queue->lock);
>
> - if (ret)
> - kfree(iopf_param);
> + return 0;
> +err_restore:
> + iommu_set_device_fault_cookie(dev, 0, curr);
> +err_free:
> + mutex_unlock(¶m->lock);
> + mutex_unlock(&queue->lock);
> + kfree(iopf_param);
>
> return ret;
> }
> @@ -329,7 +340,6 @@ EXPORT_SYMBOL_GPL(iopf_queue_add_device);
> */
> int iopf_queue_remove_device(struct iopf_queue *queue, struct device
> *dev) {
> - int ret = -EINVAL;
> struct iopf_fault *iopf, *next;
> struct iopf_device_param *iopf_param;
> struct dev_iommu *param = dev->iommu;
> @@ -339,16 +349,17 @@ int iopf_queue_remove_device(struct iopf_queue
> *queue, struct device *dev)
> mutex_lock(&queue->lock);
> mutex_lock(¶m->lock);
> - iopf_param = param->iopf_param;
> - if (iopf_param && iopf_param->queue == queue) {
> - list_del(&iopf_param->queue_list);
> - param->iopf_param = NULL;
> - ret = 0;
> + iopf_param = iommu_get_device_fault_cookie(dev, 0);
> + if (!iopf_param || iopf_param->queue != queue) {
> + mutex_unlock(¶m->lock);
> + mutex_unlock(&queue->lock);
> + return -EINVAL;
> }
> +
> + list_del(&iopf_param->queue_list);
> + iommu_set_device_fault_cookie(dev, 0, NULL);
> mutex_unlock(¶m->lock);
> mutex_unlock(&queue->lock);
> - if (ret)
> - return ret;
>
> /* Just in case some faults are still stuck */
> list_for_each_entry_safe(iopf, next, &iopf_param->partial, list)
Thanks,
Jacob
On 2023/7/11 14:26, Tian, Kevin wrote:
>> From: Lu Baolu <[email protected]>
>> Sent: Tuesday, July 11, 2023 9:07 AM
>>
>> Remove the static iopf_param pointer from struct iommu_fault_param to
>> save memory.
>
> why is there memory saving? you replace a single pointer with a xarray now...
iopf_param is duplicate with the fault cookie. So replace it with the
fault cookie to remove duplication and save memory.
>
>> @@ -303,16 +303,27 @@ int iopf_queue_add_device(struct iopf_queue
>> *queue, struct device *dev)
>>
>> mutex_lock(&queue->lock);
>> mutex_lock(¶m->lock);
>> - if (!param->iopf_param) {
>> - list_add(&iopf_param->queue_list, &queue->devices);
>> - param->iopf_param = iopf_param;
>> - ret = 0;
>> + curr = iommu_set_device_fault_cookie(dev, 0, iopf_param);
>> + if (IS_ERR(curr)) {
>> + ret = PTR_ERR(curr);
>> + goto err_free;
>> }
>
> So although the new xarray is called a per-pasid storage, here only
> slot#0 is used for sva which includes a list containing partial req's
> for many pasid's. It doesn't sound clean...
Just to make it generic so that IOMMUFD can also use it. IOMMUFD will
use it to store the per-{device, pasid} object id (and possibly other
data) so that it can be quickly retrieved in the critical fault
delivering patch.
Best regards,
baolu
On 2023/7/12 6:02, Jacob Pan wrote:
> On Tue, 11 Jul 2023 09:06:42 +0800, Lu Baolu<[email protected]>
> wrote:
>
>> Remove the static iopf_param pointer from struct iommu_fault_param to
>> save memory.
>>
>> Signed-off-by: Lu Baolu<[email protected]>
>> ---
>> include/linux/iommu.h | 2 --
>> drivers/iommu/io-pgfault.c | 47 +++++++++++++++++++++++---------------
>> 2 files changed, 29 insertions(+), 20 deletions(-)
>>
>> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
>> index ffd6fe1317f4..5fe37a7c5a55 100644
>> --- a/include/linux/iommu.h
>> +++ b/include/linux/iommu.h
>> @@ -551,7 +551,6 @@ struct iommu_fault_param {
>> * struct dev_iommu - Collection of per-device IOMMU data
>> *
>> * @fault_param: IOMMU detected device fault reporting data
>> - * @iopf_param: I/O Page Fault queue and data
>> * @fwspec: IOMMU fwspec data
>> * @iommu_dev: IOMMU device this device is linked to
>> * @priv: IOMMU Driver private data
>> @@ -564,7 +563,6 @@ struct iommu_fault_param {
>> struct dev_iommu {
>> struct mutex lock;
>> struct iommu_fault_param *fault_param;
>> - struct iopf_device_param *iopf_param;
>> struct iommu_fwspec *fwspec;
>> struct iommu_device *iommu_dev;
>> void *priv;
>> diff --git a/drivers/iommu/io-pgfault.c b/drivers/iommu/io-pgfault.c
>> index 1749e0869f2e..6a3a4e08e67e 100644
>> --- a/drivers/iommu/io-pgfault.c
>> +++ b/drivers/iommu/io-pgfault.c
>> @@ -158,7 +158,7 @@ int iommu_queue_iopf(struct iommu_fault *fault,
>> struct device *dev)
>> * As long as we're holding param->lock, the queue can't be
>> unlinked
>> * from the device and therefore cannot disappear.
>> */
>> - iopf_param = param->iopf_param;
>> + iopf_param = iommu_get_device_fault_cookie(dev, 0);
> I am not sure I understand how does it know the cookie type is iopf_param
> for PASID 0?
>
> Between IOPF and IOMMUFD use of the cookie, cookie types are different,
> right?
>
The fault cookie is managed by the code that delivers or handles the
faults. The sva and IOMMUFD paths are exclusive.
Best regards,
baolu
> From: Baolu Lu <[email protected]>
> Sent: Wednesday, July 12, 2023 11:13 AM
>
> On 2023/7/12 6:02, Jacob Pan wrote:
> > On Tue, 11 Jul 2023 09:06:42 +0800, Lu Baolu<[email protected]>
> > wrote:
> >
> >> @@ -158,7 +158,7 @@ int iommu_queue_iopf(struct iommu_fault *fault,
> >> struct device *dev)
> >> * As long as we're holding param->lock, the queue can't be
> >> unlinked
> >> * from the device and therefore cannot disappear.
> >> */
> >> - iopf_param = param->iopf_param;
> >> + iopf_param = iommu_get_device_fault_cookie(dev, 0);
> > I am not sure I understand how does it know the cookie type is iopf_param
> > for PASID 0?
> >
> > Between IOPF and IOMMUFD use of the cookie, cookie types are different,
> > right?
> >
>
> The fault cookie is managed by the code that delivers or handles the
> faults. The sva and IOMMUFD paths are exclusive.
>
what about siov? A siov-capable device can support sva and iommufd
simultaneously.
On 2023/7/13 11:24, Tian, Kevin wrote:
>> From: Baolu Lu <[email protected]>
>> Sent: Wednesday, July 12, 2023 11:13 AM
>>
>> On 2023/7/12 6:02, Jacob Pan wrote:
>>> On Tue, 11 Jul 2023 09:06:42 +0800, Lu Baolu<[email protected]>
>>> wrote:
>>>
>>>> @@ -158,7 +158,7 @@ int iommu_queue_iopf(struct iommu_fault *fault,
>>>> struct device *dev)
>>>> * As long as we're holding param->lock, the queue can't be
>>>> unlinked
>>>> * from the device and therefore cannot disappear.
>>>> */
>>>> - iopf_param = param->iopf_param;
>>>> + iopf_param = iommu_get_device_fault_cookie(dev, 0);
>>> I am not sure I understand how does it know the cookie type is iopf_param
>>> for PASID 0?
>>>
>>> Between IOPF and IOMMUFD use of the cookie, cookie types are different,
>>> right?
>>>
>>
>> The fault cookie is managed by the code that delivers or handles the
>> faults. The sva and IOMMUFD paths are exclusive.
>>
>
> what about siov? A siov-capable device can support sva and iommufd
> simultaneously.
For siov case, the pasid should be global. RID and each pasid are still
exclusive, so I don't see any problem. Did I overlook anything?
Best regards,
baolu
> From: Baolu Lu <[email protected]>
> Sent: Thursday, July 13, 2023 11:44 AM
>
> On 2023/7/13 11:24, Tian, Kevin wrote:
> >> From: Baolu Lu <[email protected]>
> >> Sent: Wednesday, July 12, 2023 11:13 AM
> >>
> >> On 2023/7/12 6:02, Jacob Pan wrote:
> >>> On Tue, 11 Jul 2023 09:06:42 +0800, Lu Baolu<[email protected]>
> >>> wrote:
> >>>
> >>>> @@ -158,7 +158,7 @@ int iommu_queue_iopf(struct iommu_fault
> *fault,
> >>>> struct device *dev)
> >>>> * As long as we're holding param->lock, the queue can't be
> >>>> unlinked
> >>>> * from the device and therefore cannot disappear.
> >>>> */
> >>>> - iopf_param = param->iopf_param;
> >>>> + iopf_param = iommu_get_device_fault_cookie(dev, 0);
> >>> I am not sure I understand how does it know the cookie type is
> iopf_param
> >>> for PASID 0?
> >>>
> >>> Between IOPF and IOMMUFD use of the cookie, cookie types are
> different,
> >>> right?
> >>>
> >>
> >> The fault cookie is managed by the code that delivers or handles the
> >> faults. The sva and IOMMUFD paths are exclusive.
> >>
> >
> > what about siov? A siov-capable device can support sva and iommufd
> > simultaneously.
>
> For siov case, the pasid should be global. RID and each pasid are still
> exclusive, so I don't see any problem. Did I overlook anything?
>
they are exclusive but it's weird to see some pasids (for sva) on this
device are tracked by slot#0 while other pasids (for iommufd) occupies
per-pasid slot.
why not generalizing them given you name it as "per-pasid fault cookie"?
On 2023/7/13 16:01, Tian, Kevin wrote:
>> From: Baolu Lu <[email protected]>
>> Sent: Thursday, July 13, 2023 11:44 AM
>>
>> On 2023/7/13 11:24, Tian, Kevin wrote:
>>>> From: Baolu Lu <[email protected]>
>>>> Sent: Wednesday, July 12, 2023 11:13 AM
>>>>
>>>> On 2023/7/12 6:02, Jacob Pan wrote:
>>>>> On Tue, 11 Jul 2023 09:06:42 +0800, Lu Baolu<[email protected]>
>>>>> wrote:
>>>>>
>>>>>> @@ -158,7 +158,7 @@ int iommu_queue_iopf(struct iommu_fault
>> *fault,
>>>>>> struct device *dev)
>>>>>> * As long as we're holding param->lock, the queue can't be
>>>>>> unlinked
>>>>>> * from the device and therefore cannot disappear.
>>>>>> */
>>>>>> - iopf_param = param->iopf_param;
>>>>>> + iopf_param = iommu_get_device_fault_cookie(dev, 0);
>>>>> I am not sure I understand how does it know the cookie type is
>> iopf_param
>>>>> for PASID 0?
>>>>>
>>>>> Between IOPF and IOMMUFD use of the cookie, cookie types are
>> different,
>>>>> right?
>>>>>
>>>>
>>>> The fault cookie is managed by the code that delivers or handles the
>>>> faults. The sva and IOMMUFD paths are exclusive.
>>>>
>>>
>>> what about siov? A siov-capable device can support sva and iommufd
>>> simultaneously.
>>
>> For siov case, the pasid should be global. RID and each pasid are still
>> exclusive, so I don't see any problem. Did I overlook anything?
>>
>
> they are exclusive but it's weird to see some pasids (for sva) on this
> device are tracked by slot#0 while other pasids (for iommufd) occupies
> per-pasid slot.
>
> why not generalizing them given you name it as "per-pasid fault cookie"?
Yeah! Get your point now. At least the partial list should be per-pasid.
Let me invest more time on this.
Best regards,
baolu