Re: [PATCH v4 12/22] iommu: introduce device fault report API

[Jacob Pan]

On Mon, 30 Apr 2018 17:53:52 +0100
Jean-Philippe Brucker  wrote:

> Hi,
> 
> I noticed a couple issues when testing
> 
> On 16/04/18 22:49, Jacob Pan wrote:
> > +int iommu_register_device_fault_handler(struct device *dev,
> > +   iommu_dev_fault_handler_t
> > handler,
> > +   void *data)
> > +{
> > +   struct iommu_param *param = dev->iommu_param;
> > +
> > +   /*
> > +* Device iommu_param should have been allocated when
> > device is
> > +* added to its iommu_group.
> > +*/
> > +   if (!param)
> > +   return -EINVAL;
> > +
> > +   /* Only allow one fault handler registered for each device
> > */
> > +   if (param->fault_param)
> > +   return -EBUSY;  
> 
> Should this be inside the param lock? We probably don't expect
> concurrent register/unregister but it seems cleaner
agreed, same as corrections below. Thanks!
> 
>  [...]  
> 
> We should return EINVAL here, if fault_param is NULL. That way users
> can call unregister_fault_handler unconditionally in their cleanup
> paths
> 
> > +   /* we cannot unregister handler if there are pending
> > faults */
> > +   if (list_empty(>fault_param->faults)) {  
> 
> if (!list_empty(...))
> 
> > +   ret = -EBUSY;
> > +   goto unlock;
> > +   }
> > +
> > +   list_del(>fault_param->faults);  
> 
> faults is the list head, no need for list_del
> 
> > +   kfree(param->fault_param);
> > +   param->fault_param = NULL;
> > +   put_device(dev);
> > +
> > +unlock:
> > +   mutex_unlock(>lock);
> > +
> > +   return 0;  
> 
> return ret
> 
> Thanks,
> Jean

[Jacob Pan]
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v4 12/22] iommu: introduce device fault report API

[Jean-Philippe Brucker]

Hi,

I noticed a couple issues when testing

On 16/04/18 22:49, Jacob Pan wrote:
> +int iommu_register_device_fault_handler(struct device *dev,
> + iommu_dev_fault_handler_t handler,
> + void *data)
> +{
> + struct iommu_param *param = dev->iommu_param;
> +
> + /*
> +  * Device iommu_param should have been allocated when device is
> +  * added to its iommu_group.
> +  */
> + if (!param)
> + return -EINVAL;
> +
> + /* Only allow one fault handler registered for each device */
> + if (param->fault_param)
> + return -EBUSY;

Should this be inside the param lock? We probably don't expect
concurrent register/unregister but it seems cleaner

> +
> + mutex_lock(>lock);
> + get_device(dev);
> + param->fault_param =
> + kzalloc(sizeof(struct iommu_fault_param), GFP_ATOMIC);
> + if (!param->fault_param) {
> + put_device(dev);
> + mutex_unlock(>lock);
> + return -ENOMEM;
> + }
> + mutex_init(>fault_param->lock);
> + param->fault_param->handler = handler;
> + param->fault_param->data = data;
> + INIT_LIST_HEAD(>fault_param->faults);
> +
> + mutex_unlock(>lock);
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(iommu_register_device_fault_handler);
> +
> +/**
> + * iommu_unregister_device_fault_handler() - Unregister the device fault 
> handler
> + * @dev: the device
> + *
> + * Remove the device fault handler installed with
> + * iommu_register_device_fault_handler().
> + *
> + * Return 0 on success, or an error.
> + */
> +int iommu_unregister_device_fault_handler(struct device *dev)
> +{
> + struct iommu_param *param = dev->iommu_param;
> + int ret = 0;
> +
> + if (!param)
> + return -EINVAL;
> +
> + mutex_lock(>lock);

We should return EINVAL here, if fault_param is NULL. That way users can
call unregister_fault_handler unconditionally in their cleanup paths

> + /* we cannot unregister handler if there are pending faults */
> + if (list_empty(>fault_param->faults)) {

if (!list_empty(...))

> + ret = -EBUSY;
> + goto unlock;
> + }
> +
> + list_del(>fault_param->faults);

faults is the list head, no need for list_del

> + kfree(param->fault_param);
> + param->fault_param = NULL;
> + put_device(dev);
> +
> +unlock:
> + mutex_unlock(>lock);
> +
> + return 0;

return ret

Thanks,
Jean
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v4 12/22] iommu: introduce device fault report API

[Jacob Pan]

On Mon, 23 Apr 2018 12:30:13 +0100
Jean-Philippe Brucker  wrote:

> On Mon, Apr 16, 2018 at 10:49:01PM +0100, Jacob Pan wrote:
> [...]
> > +int iommu_register_device_fault_handler(struct device *dev,
> > +   iommu_dev_fault_handler_t
> > handler,
> > +   void *data)
> > +{
> > +   struct iommu_param *param = dev->iommu_param;
> > +
> > +   /*
> > +* Device iommu_param should have been allocated when
> > device is
> > +* added to its iommu_group.
> > +*/
> > +   if (!param)
> > +   return -EINVAL;
> > +
> > +   /* Only allow one fault handler registered for each device
> > */
> > +   if (param->fault_param)
> > +   return -EBUSY;
> > +
> > +   mutex_lock(>lock);
> > +   get_device(dev);
> > +   param->fault_param =
> > +   kzalloc(sizeof(struct iommu_fault_param),
> > GFP_ATOMIC);  
> 
> This can be GFP_KERNEL
> 
yes, will change.
> [...]
> > +int iommu_report_device_fault(struct device *dev, struct
> > iommu_fault_event *evt) +{
> > +   int ret = 0;
> > +   struct iommu_fault_event *evt_pending;
> > +   struct iommu_fault_param *fparam;
> > +
> > +   /* iommu_param is allocated when device is added to group
> > */
> > +   if (!dev->iommu_param | !evt)
> > +   return -EINVAL;
> > +   /* we only report device fault if there is a handler
> > registered */
> > +   mutex_lock(>iommu_param->lock);
> > +   if (!dev->iommu_param->fault_param ||
> > +   !dev->iommu_param->fault_param->handler) {
> > +   ret = -EINVAL;
> > +   goto done_unlock;
> > +   }
> > +   fparam = dev->iommu_param->fault_param;
> > +   if (evt->type == IOMMU_FAULT_PAGE_REQ && evt->last_req) {
> > +   evt_pending = kzalloc(sizeof(*evt_pending),
> > GFP_ATOMIC);  
> 
> We're expecting caller to be a thread at the moment, so this could be
> GFP_KERNEL too. You could also use kmemdup to remove the memcpy below
> 
good idea. will do.
> [...]
> > +static inline int iommu_register_device_fault_handler(struct
> > device *dev,
> > +
> > iommu_dev_fault_handler_t handler,
> > +   void *data)
> > +{
> > +   return 0;  
> 
> Should return -ENODEV
> 
right. thanks.
> Thanks,
> Jean

[Jacob Pan]
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v4 12/22] iommu: introduce device fault report API

[Jean-Philippe Brucker]

On Mon, Apr 16, 2018 at 10:49:01PM +0100, Jacob Pan wrote:
[...]
> +int iommu_register_device_fault_handler(struct device *dev,
> + iommu_dev_fault_handler_t handler,
> + void *data)
> +{
> + struct iommu_param *param = dev->iommu_param;
> +
> + /*
> +  * Device iommu_param should have been allocated when device is
> +  * added to its iommu_group.
> +  */
> + if (!param)
> + return -EINVAL;
> +
> + /* Only allow one fault handler registered for each device */
> + if (param->fault_param)
> + return -EBUSY;
> +
> + mutex_lock(>lock);
> + get_device(dev);
> + param->fault_param =
> + kzalloc(sizeof(struct iommu_fault_param), GFP_ATOMIC);

This can be GFP_KERNEL

[...]
> +int iommu_report_device_fault(struct device *dev, struct iommu_fault_event 
> *evt)
> +{
> + int ret = 0;
> + struct iommu_fault_event *evt_pending;
> + struct iommu_fault_param *fparam;
> +
> + /* iommu_param is allocated when device is added to group */
> + if (!dev->iommu_param | !evt)
> + return -EINVAL;
> + /* we only report device fault if there is a handler registered */
> + mutex_lock(>iommu_param->lock);
> + if (!dev->iommu_param->fault_param ||
> + !dev->iommu_param->fault_param->handler) {
> + ret = -EINVAL;
> + goto done_unlock;
> + }
> + fparam = dev->iommu_param->fault_param;
> + if (evt->type == IOMMU_FAULT_PAGE_REQ && evt->last_req) {
> + evt_pending = kzalloc(sizeof(*evt_pending), GFP_ATOMIC);

We're expecting caller to be a thread at the moment, so this could be
GFP_KERNEL too. You could also use kmemdup to remove the memcpy below

[...]
> +static inline int iommu_register_device_fault_handler(struct device *dev,
> + iommu_dev_fault_handler_t 
> handler,
> + void *data)
> +{
> + return 0;

Should return -ENODEV

Thanks,
Jean
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

[PATCH v4 12/22] iommu: introduce device fault report API

[Jacob Pan]

Traditionally, device specific faults are detected and handled within
their own device drivers. When IOMMU is enabled, faults such as DMA
related transactions are detected by IOMMU. There is no generic
reporting mechanism to report faults back to the in-kernel device
driver or the guest OS in case of assigned devices.

Faults detected by IOMMU is based on the transaction's source ID which
can be reported at per device basis, regardless of the device type is a
PCI device or not.

The fault types include recoverable (e.g. page request) and
unrecoverable faults(e.g. access error). In most cases, faults can be
handled by IOMMU drivers internally. The primary use cases are as
follows:
1. page request fault originated from an SVM capable device that is
assigned to guest via vIOMMU. In this case, the first level page tables
are owned by the guest. Page request must be propagated to the guest to
let guest OS fault in the pages then send page response. In this
mechanism, the direct receiver of IOMMU fault notification is VFIO,
which can relay notification events to QEMU or other user space
software.

2. faults need more subtle handling by device drivers. Other than
simply invoke reset function, there are needs to let device driver
handle the fault with a smaller impact.

This patchset is intended to create a generic fault report API such
that it can scale as follows:
- all IOMMU types
- PCI and non-PCI devices
- recoverable and unrecoverable faults
- VFIO and other other in kernel users
- DMA & IRQ remapping (TBD)
The original idea was brought up by David Woodhouse and discussions
summarized at https://lwn.net/Articles/608914/.

Signed-off-by: Jacob Pan 
Signed-off-by: Ashok Raj 
Signed-off-by: Jean-Philippe Brucker 
---
 drivers/iommu/iommu.c | 147 +-
 include/linux/iommu.h |  35 +++-
 2 files changed, 179 insertions(+), 3 deletions(-)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 784e019..de19c33 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -581,6 +581,13 @@ int iommu_group_add_device(struct iommu_group *group, 
struct device *dev)
goto err_free_name;
}
 
+   dev->iommu_param = kzalloc(sizeof(*dev->iommu_param), GFP_KERNEL);
+   if (!dev->iommu_param) {
+   ret = -ENOMEM;
+   goto err_free_name;
+   }
+   mutex_init(>iommu_param->lock);
+
kobject_get(group->devices_kobj);
 
dev->iommu_group = group;
@@ -611,6 +618,7 @@ int iommu_group_add_device(struct iommu_group *group, 
struct device *dev)
mutex_unlock(>mutex);
dev->iommu_group = NULL;
kobject_put(group->devices_kobj);
+   kfree(dev->iommu_param);
 err_free_name:
kfree(device->name);
 err_remove_link:
@@ -657,7 +665,7 @@ void iommu_group_remove_device(struct device *dev)
sysfs_remove_link(>kobj, "iommu_group");
 
trace_remove_device_from_group(group->id, dev);
-
+   kfree(dev->iommu_param);
kfree(device->name);
kfree(device);
dev->iommu_group = NULL;
@@ -792,6 +800,143 @@ int iommu_group_unregister_notifier(struct iommu_group 
*group,
 EXPORT_SYMBOL_GPL(iommu_group_unregister_notifier);
 
 /**
+ * iommu_register_device_fault_handler() - Register a device fault handler
+ * @dev: the device
+ * @handler: the fault handler
+ * @data: private data passed as argument to the handler
+ *
+ * When an IOMMU fault event is received, call this handler with the fault 
event
+ * and data as argument. The handler should return 0. If the fault is
+ * recoverable (IOMMU_FAULT_PAGE_REQ), the handler must also complete
+ * the fault by calling iommu_page_response() with one of the following
+ * response code:
+ * - IOMMU_PAGE_RESP_SUCCESS: retry the translation
+ * - IOMMU_PAGE_RESP_INVALID: terminate the fault
+ * - IOMMU_PAGE_RESP_FAILURE: terminate the fault and stop reporting
+ *   page faults if possible.
+ *
+ * Return 0 if the fault handler was installed successfully, or an error.
+ */
+int iommu_register_device_fault_handler(struct device *dev,
+   iommu_dev_fault_handler_t handler,
+   void *data)
+{
+   struct iommu_param *param = dev->iommu_param;
+
+   /*
+* Device iommu_param should have been allocated when device is
+* added to its iommu_group.
+*/
+   if (!param)
+   return -EINVAL;
+
+   /* Only allow one fault handler registered for each device */
+   if (param->fault_param)
+   return -EBUSY;
+
+   mutex_lock(>lock);
+   get_device(dev);
+   param->fault_param =
+   kzalloc(sizeof(struct iommu_fault_param), GFP_ATOMIC);
+   if (!param->fault_param) {
+   put_device(dev);
+   mutex_unlock(>lock);
+   return -ENOMEM;
+   }

[PATCH v4 12/22] iommu: introduce device fault report API

[Jacob Pan]

Traditionally, device specific faults are detected and handled within
their own device drivers. When IOMMU is enabled, faults such as DMA
related transactions are detected by IOMMU. There is no generic
reporting mechanism to report faults back to the in-kernel device
driver or the guest OS in case of assigned devices.

Faults detected by IOMMU is based on the transaction's source ID which
can be reported at per device basis, regardless of the device type is a
PCI device or not.

The fault types include recoverable (e.g. page request) and
unrecoverable faults(e.g. access error). In most cases, faults can be
handled by IOMMU drivers internally. The primary use cases are as
follows:
1. page request fault originated from an SVM capable device that is
assigned to guest via vIOMMU. In this case, the first level page tables
are owned by the guest. Page request must be propagated to the guest to
let guest OS fault in the pages then send page response. In this
mechanism, the direct receiver of IOMMU fault notification is VFIO,
which can relay notification events to QEMU or other user space
software.

2. faults need more subtle handling by device drivers. Other than
simply invoke reset function, there are needs to let device driver
handle the fault with a smaller impact.

This patchset is intended to create a generic fault report API such
that it can scale as follows:
- all IOMMU types
- PCI and non-PCI devices
- recoverable and unrecoverable faults
- VFIO and other other in kernel users
- DMA & IRQ remapping (TBD)
The original idea was brought up by David Woodhouse and discussions
summarized at https://lwn.net/Articles/608914/.

Signed-off-by: Jacob Pan 
Signed-off-by: Ashok Raj 
Signed-off-by: Jean-Philippe Brucker 
---
 drivers/iommu/iommu.c | 147 +-
 include/linux/iommu.h |  35 +++-
 2 files changed, 179 insertions(+), 3 deletions(-)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 466f789..cd653ff 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -581,6 +581,13 @@ int iommu_group_add_device(struct iommu_group *group, 
struct device *dev)
goto err_free_name;
}
 
+   dev->iommu_param = kzalloc(sizeof(*dev->iommu_param), GFP_KERNEL);
+   if (!dev->iommu_param) {
+   ret = -ENOMEM;
+   goto err_free_name;
+   }
+   mutex_init(>iommu_param->lock);
+
kobject_get(group->devices_kobj);
 
dev->iommu_group = group;
@@ -611,6 +618,7 @@ int iommu_group_add_device(struct iommu_group *group, 
struct device *dev)
mutex_unlock(>mutex);
dev->iommu_group = NULL;
kobject_put(group->devices_kobj);
+   kfree(dev->iommu_param);
 err_free_name:
kfree(device->name);
 err_remove_link:
@@ -657,7 +665,7 @@ void iommu_group_remove_device(struct device *dev)
sysfs_remove_link(>kobj, "iommu_group");
 
trace_remove_device_from_group(group->id, dev);
-
+   kfree(dev->iommu_param);
kfree(device->name);
kfree(device);
dev->iommu_group = NULL;
@@ -792,6 +800,143 @@ int iommu_group_unregister_notifier(struct iommu_group 
*group,
 EXPORT_SYMBOL_GPL(iommu_group_unregister_notifier);
 
 /**
+ * iommu_register_device_fault_handler() - Register a device fault handler
+ * @dev: the device
+ * @handler: the fault handler
+ * @data: private data passed as argument to the handler
+ *
+ * When an IOMMU fault event is received, call this handler with the fault 
event
+ * and data as argument. The handler should return 0. If the fault is
+ * recoverable (IOMMU_FAULT_PAGE_REQ), the handler must also complete
+ * the fault by calling iommu_page_response() with one of the following
+ * response code:
+ * - IOMMU_PAGE_RESP_SUCCESS: retry the translation
+ * - IOMMU_PAGE_RESP_INVALID: terminate the fault
+ * - IOMMU_PAGE_RESP_FAILURE: terminate the fault and stop reporting
+ *   page faults if possible.
+ *
+ * Return 0 if the fault handler was installed successfully, or an error.
+ */
+int iommu_register_device_fault_handler(struct device *dev,
+   iommu_dev_fault_handler_t handler,
+   void *data)
+{
+   struct iommu_param *param = dev->iommu_param;
+
+   /*
+* Device iommu_param should have been allocated when device is
+* added to its iommu_group.
+*/
+   if (!param)
+   return -EINVAL;
+
+   /* Only allow one fault handler registered for each device */
+   if (param->fault_param)
+   return -EBUSY;
+
+   mutex_lock(>lock);
+   get_device(dev);
+   param->fault_param =
+   kzalloc(sizeof(struct iommu_fault_param), GFP_ATOMIC);
+   if (!param->fault_param) {
+   put_device(dev);
+   mutex_unlock(>lock);
+   return -ENOMEM;
+   }