Re: [PATCH v3 10/16] iommu: introduce device fault report API

[Jacob Pan]

On Thu, 18 Jan 2018 19:24:52 +
Jean-Philippe Brucker  wrote:

> Hi Jacob,
> 
> I've got minor comments after working with this patch, sorry for the
> multiple replies
> 
> On 17/11/17 18:55, Jacob Pan wrote:
> [...]
> > diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> > index 829e9e9..97b7990 100644
> > --- a/drivers/iommu/iommu.c
> > +++ b/drivers/iommu/iommu.c
> > @@ -581,6 +581,12 @@ int iommu_group_add_device(struct iommu_group
> > *group, struct device *dev) goto err_free_name;
> > }
> >  
> > +   dev->iommu_param = kzalloc(sizeof(struct
> > iommu_fault_param), GFP_KERNEL);  
> 
> This should be "sizeof(struct iommu_param)" or maybe
> "sizeof(*dev->iommu_param)".
> 
good catch, thanks,
> > +   if (!dev->iommu_param) {
> > +   ret = -ENOMEM;
> > +   goto err_free_name;
> > +   }
> > +
> > kobject_get(group->devices_kobj);
> >  
> > dev->iommu_group = group;
> > @@ -657,7 +663,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;
> > @@ -791,6 +797,61 @@ int iommu_group_unregister_notifier(struct
> > iommu_group *group, }
> >  EXPORT_SYMBOL_GPL(iommu_group_unregister_notifier);
> > 
> > +int iommu_register_device_fault_handler(struct device *dev,
> > +   iommu_dev_fault_handler_t
> > handler,
> > +   void *data)
> > +{
> > +   struct iommu_param *idata = dev->iommu_param;
> > +
> > +   /*
> > +* Device iommu_param should have been allocated when
> > device is
> > +* added to its iommu_group.
> > +*/
> > +   if (!idata)
> > +   return -EINVAL;
> > +   /* Only allow one fault handler registered for each device
> > */
> > +   if (idata->fault_param)
> > +   return -EBUSY;
> > +   get_device(dev);
> > +   idata->fault_param =
> > +   kzalloc(sizeof(struct iommu_fault_param),
> > GFP_KERNEL);
> > +   if (!idata->fault_param)
> > +   return -ENOMEM;
> > +   idata->fault_param->handler = handler;
> > +   idata->fault_param->data = data;
> > +
> > +   return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(iommu_register_device_fault_handler);
> > +
> > +int iommu_unregister_device_fault_handler(struct device *dev)
> > +{
> > +   struct iommu_param *idata = dev->iommu_param;
> > +
> > +   if (!idata)
> > +   return -EINVAL;
> > +
> > +   kfree(idata->fault_param);
> > +   idata->fault_param = NULL;
> > +   put_device(dev);
> > +
> > +   return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(iommu_unregister_device_fault_handler);  
> 
> We should probably document register() and unregister() functions
> since they are part of the device driver API. If it helps I came up
> with:
> 
> /**
>  * 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. */
> 
> /**
>  * 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.
>  */
> 
agreed. thanks. sorry about the delay.
> Thanks,
> Jean

[Jacob Pan]

Re: [PATCH v3 10/16] iommu: introduce device fault report API

[Jacob Pan]

On Thu, 18 Jan 2018 19:24:52 +
Jean-Philippe Brucker  wrote:

> Hi Jacob,
> 
> I've got minor comments after working with this patch, sorry for the
> multiple replies
> 
> On 17/11/17 18:55, Jacob Pan wrote:
> [...]
> > diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> > index 829e9e9..97b7990 100644
> > --- a/drivers/iommu/iommu.c
> > +++ b/drivers/iommu/iommu.c
> > @@ -581,6 +581,12 @@ int iommu_group_add_device(struct iommu_group
> > *group, struct device *dev) goto err_free_name;
> > }
> >  
> > +   dev->iommu_param = kzalloc(sizeof(struct
> > iommu_fault_param), GFP_KERNEL);  
> 
> This should be "sizeof(struct iommu_param)" or maybe
> "sizeof(*dev->iommu_param)".
> 
good catch, thanks,
> > +   if (!dev->iommu_param) {
> > +   ret = -ENOMEM;
> > +   goto err_free_name;
> > +   }
> > +
> > kobject_get(group->devices_kobj);
> >  
> > dev->iommu_group = group;
> > @@ -657,7 +663,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;
> > @@ -791,6 +797,61 @@ int iommu_group_unregister_notifier(struct
> > iommu_group *group, }
> >  EXPORT_SYMBOL_GPL(iommu_group_unregister_notifier);
> > 
> > +int iommu_register_device_fault_handler(struct device *dev,
> > +   iommu_dev_fault_handler_t
> > handler,
> > +   void *data)
> > +{
> > +   struct iommu_param *idata = dev->iommu_param;
> > +
> > +   /*
> > +* Device iommu_param should have been allocated when
> > device is
> > +* added to its iommu_group.
> > +*/
> > +   if (!idata)
> > +   return -EINVAL;
> > +   /* Only allow one fault handler registered for each device
> > */
> > +   if (idata->fault_param)
> > +   return -EBUSY;
> > +   get_device(dev);
> > +   idata->fault_param =
> > +   kzalloc(sizeof(struct iommu_fault_param),
> > GFP_KERNEL);
> > +   if (!idata->fault_param)
> > +   return -ENOMEM;
> > +   idata->fault_param->handler = handler;
> > +   idata->fault_param->data = data;
> > +
> > +   return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(iommu_register_device_fault_handler);
> > +
> > +int iommu_unregister_device_fault_handler(struct device *dev)
> > +{
> > +   struct iommu_param *idata = dev->iommu_param;
> > +
> > +   if (!idata)
> > +   return -EINVAL;
> > +
> > +   kfree(idata->fault_param);
> > +   idata->fault_param = NULL;
> > +   put_device(dev);
> > +
> > +   return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(iommu_unregister_device_fault_handler);  
> 
> We should probably document register() and unregister() functions
> since they are part of the device driver API. If it helps I came up
> with:
> 
> /**
>  * 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. */
> 
> /**
>  * 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.
>  */
> 
agreed. thanks. sorry about the delay.
> Thanks,
> Jean

[Jacob Pan]

Re: [PATCH v3 10/16] iommu: introduce device fault report API

[Jean-Philippe Brucker]

Hi Jacob,

I've got minor comments after working with this patch, sorry for the
multiple replies

On 17/11/17 18:55, Jacob Pan wrote:
[...]
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index 829e9e9..97b7990 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -581,6 +581,12 @@ int iommu_group_add_device(struct iommu_group *group, 
> struct device *dev)
>   goto err_free_name;
>   }
>  
> + dev->iommu_param = kzalloc(sizeof(struct iommu_fault_param), 
> GFP_KERNEL);

This should be "sizeof(struct iommu_param)" or maybe
"sizeof(*dev->iommu_param)".

> + if (!dev->iommu_param) {
> + ret = -ENOMEM;
> + goto err_free_name;
> + }
> +
>   kobject_get(group->devices_kobj);
>  
>   dev->iommu_group = group;
> @@ -657,7 +663,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;
> @@ -791,6 +797,61 @@ int iommu_group_unregister_notifier(struct iommu_group 
> *group,
>  }
>  EXPORT_SYMBOL_GPL(iommu_group_unregister_notifier);
> 
> +int iommu_register_device_fault_handler(struct device *dev,
> + iommu_dev_fault_handler_t handler,
> + void *data)
> +{
> + struct iommu_param *idata = dev->iommu_param;
> +
> + /*
> +  * Device iommu_param should have been allocated when device is
> +  * added to its iommu_group.
> +  */
> + if (!idata)
> + return -EINVAL;
> + /* Only allow one fault handler registered for each device */
> + if (idata->fault_param)
> + return -EBUSY;
> + get_device(dev);
> + idata->fault_param =
> + kzalloc(sizeof(struct iommu_fault_param), GFP_KERNEL);
> + if (!idata->fault_param)
> + return -ENOMEM;
> + idata->fault_param->handler = handler;
> + idata->fault_param->data = data;
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(iommu_register_device_fault_handler);
> +
> +int iommu_unregister_device_fault_handler(struct device *dev)
> +{
> + struct iommu_param *idata = dev->iommu_param;
> +
> + if (!idata)
> + return -EINVAL;
> +
> + kfree(idata->fault_param);
> + idata->fault_param = NULL;
> + put_device(dev);
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(iommu_unregister_device_fault_handler);

We should probably document register() and unregister() functions since
they are part of the device driver API. If it helps I came up with:

/**
 * 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.
 */

/**
 * 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.
 */

Thanks,
Jean

Re: [PATCH v3 10/16] iommu: introduce device fault report API

[Jean-Philippe Brucker]

Hi Jacob,

I've got minor comments after working with this patch, sorry for the
multiple replies

On 17/11/17 18:55, Jacob Pan wrote:
[...]
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index 829e9e9..97b7990 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -581,6 +581,12 @@ int iommu_group_add_device(struct iommu_group *group, 
> struct device *dev)
>   goto err_free_name;
>   }
>  
> + dev->iommu_param = kzalloc(sizeof(struct iommu_fault_param), 
> GFP_KERNEL);

This should be "sizeof(struct iommu_param)" or maybe
"sizeof(*dev->iommu_param)".

> + if (!dev->iommu_param) {
> + ret = -ENOMEM;
> + goto err_free_name;
> + }
> +
>   kobject_get(group->devices_kobj);
>  
>   dev->iommu_group = group;
> @@ -657,7 +663,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;
> @@ -791,6 +797,61 @@ int iommu_group_unregister_notifier(struct iommu_group 
> *group,
>  }
>  EXPORT_SYMBOL_GPL(iommu_group_unregister_notifier);
> 
> +int iommu_register_device_fault_handler(struct device *dev,
> + iommu_dev_fault_handler_t handler,
> + void *data)
> +{
> + struct iommu_param *idata = dev->iommu_param;
> +
> + /*
> +  * Device iommu_param should have been allocated when device is
> +  * added to its iommu_group.
> +  */
> + if (!idata)
> + return -EINVAL;
> + /* Only allow one fault handler registered for each device */
> + if (idata->fault_param)
> + return -EBUSY;
> + get_device(dev);
> + idata->fault_param =
> + kzalloc(sizeof(struct iommu_fault_param), GFP_KERNEL);
> + if (!idata->fault_param)
> + return -ENOMEM;
> + idata->fault_param->handler = handler;
> + idata->fault_param->data = data;
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(iommu_register_device_fault_handler);
> +
> +int iommu_unregister_device_fault_handler(struct device *dev)
> +{
> + struct iommu_param *idata = dev->iommu_param;
> +
> + if (!idata)
> + return -EINVAL;
> +
> + kfree(idata->fault_param);
> + idata->fault_param = NULL;
> + put_device(dev);
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(iommu_unregister_device_fault_handler);

We should probably document register() and unregister() functions since
they are part of the device driver API. If it helps I came up with:

/**
 * 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.
 */

/**
 * 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.
 */

Thanks,
Jean

Re: [PATCH v3 10/16] iommu: introduce device fault report API

[Jean-Philippe Brucker]

On 17/11/17 18:55, Jacob Pan wrote:
[...]
> +static inline int iommu_register_device_fault_handler(struct device *dev,
> + iommu_dev_fault_handler_t 
> handler,
> + void *data)
> +{
> + return 0;> +}
> +
> +static inline int iommu_unregister_device_fault_handler(struct device *dev)
> +{
> + return 0;
> +}
> +
> +static inline bool iommu_has_device_fault_handler(struct device *dev)
> +{
> + return false;
> +}
> +
> +static inline int iommu_report_device_fault(struct device *dev, struct 
> iommu_fault_event *evt)
> +{
> + return 0;
> +}

Not too important but these stubs, when CONFIG_IOMMU_API is disabled,
usually return an error value (-ENODEV) instead of 0.

Thanks,
Jean

> +
>  static inline int iommu_group_id(struct iommu_group *group)
>  {
>   return -ENODEV;
> 

Re: [PATCH v3 10/16] iommu: introduce device fault report API

[Jean-Philippe Brucker]

On 17/11/17 18:55, Jacob Pan wrote:
[...]
> +static inline int iommu_register_device_fault_handler(struct device *dev,
> + iommu_dev_fault_handler_t 
> handler,
> + void *data)
> +{
> + return 0;> +}
> +
> +static inline int iommu_unregister_device_fault_handler(struct device *dev)
> +{
> + return 0;
> +}
> +
> +static inline bool iommu_has_device_fault_handler(struct device *dev)
> +{
> + return false;
> +}
> +
> +static inline int iommu_report_device_fault(struct device *dev, struct 
> iommu_fault_event *evt)
> +{
> + return 0;
> +}

Not too important but these stubs, when CONFIG_IOMMU_API is disabled,
usually return an error value (-ENODEV) instead of 0.

Thanks,
Jean

> +
>  static inline int iommu_group_id(struct iommu_group *group)
>  {
>   return -ENODEV;
> 

Re: [PATCH v3 10/16] iommu: introduce device fault report API

[Jacob Pan]

On Fri, 8 Dec 2017 13:59:09 -0700
Alex Williamson  wrote:

> > > 
> > > Isn't this all rather racy?  I see that we can have multiple
> > > callers to register racing.
> > I agree, should use a lock here to prevent unregister. For multiple
> > caller race, it won't happen since there is only one caller can
> > register handler.  
> 
> If you have multiple simultaneous callers to
> iommu_register_device_fault_handler, they can all get past the test
> for fault_param (testing and setting is not atomic), then it's
> indeterminate which handler gets installed.  Thanks,
> 
I see, having the mutex would prevent it. Later callers would get
-EBUSY.
Thanks a lot!
> Alex

Re: [PATCH v3 10/16] iommu: introduce device fault report API

[Jacob Pan]

On Fri, 8 Dec 2017 13:59:09 -0700
Alex Williamson  wrote:

> > > 
> > > Isn't this all rather racy?  I see that we can have multiple
> > > callers to register racing.
> > I agree, should use a lock here to prevent unregister. For multiple
> > caller race, it won't happen since there is only one caller can
> > register handler.  
> 
> If you have multiple simultaneous callers to
> iommu_register_device_fault_handler, they can all get past the test
> for fault_param (testing and setting is not atomic), then it's
> indeterminate which handler gets installed.  Thanks,
> 
I see, having the mutex would prevent it. Later callers would get
-EBUSY.
Thanks a lot!
> Alex

Re: [PATCH v3 10/16] iommu: introduce device fault report API

[Jacob Pan]

On Tue, 5 Dec 2017 14:22:41 +0800
Lu Baolu  wrote:

> > +int iommu_report_device_fault(struct device *dev, struct
> > iommu_fault_event *evt) +{
> > +   /* we only report device fault if there is a handler
> > registered */
> > +   if (!dev->iommu_param || !dev->iommu_param->fault_param ||
> > +   !dev->iommu_param->fault_param->handler)  
> 
> Can this replaced by:
> 
> if (!iommu_has_device_fault_handler(dev))
> 
right, and under a lock too.

Thanks,

Jacob

Re: [PATCH v3 10/16] iommu: introduce device fault report API

[Jacob Pan]

On Tue, 5 Dec 2017 14:22:41 +0800
Lu Baolu  wrote:

> > +int iommu_report_device_fault(struct device *dev, struct
> > iommu_fault_event *evt) +{
> > +   /* we only report device fault if there is a handler
> > registered */
> > +   if (!dev->iommu_param || !dev->iommu_param->fault_param ||
> > +   !dev->iommu_param->fault_param->handler)  
> 
> Can this replaced by:
> 
> if (!iommu_has_device_fault_handler(dev))
> 
right, and under a lock too.

Thanks,

Jacob

Re: [PATCH v3 10/16] iommu: introduce device fault report API

[Alex Williamson]

On Fri, 8 Dec 2017 12:23:58 -0800
Jacob Pan  wrote:

> On Thu, 7 Dec 2017 14:27:25 -0700
> Alex Williamson  wrote:
> 
> > On Fri, 17 Nov 2017 10:55:08 -0800
> > Jacob Pan  wrote:
> >   
> > > 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 
> > > ---
> > >  drivers/iommu/iommu.c | 63
> > > ++-
> > > include/linux/iommu.h | 36 + 2 files
> > > changed, 98 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> > > index 829e9e9..97b7990 100644
> > > --- a/drivers/iommu/iommu.c
> > > +++ b/drivers/iommu/iommu.c
> > > @@ -581,6 +581,12 @@ int iommu_group_add_device(struct iommu_group
> > > *group, struct device *dev) goto err_free_name;
> > >   }
> > >  
> > > + dev->iommu_param = kzalloc(sizeof(struct
> > > iommu_fault_param), GFP_KERNEL);
> > > + if (!dev->iommu_param) {
> > > + ret = -ENOMEM;
> > > + goto err_free_name;
> > > + }
> > > +
> > >   kobject_get(group->devices_kobj);
> > >  
> > >   dev->iommu_group = group;
> > > @@ -657,7 +663,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;
> > > @@ -791,6 +797,61 @@ int iommu_group_unregister_notifier(struct
> > > iommu_group *group, }
> > >  EXPORT_SYMBOL_GPL(iommu_group_unregister_notifier);
> > >  
> > > +int iommu_register_device_fault_handler(struct device *dev,
> > > + iommu_dev_fault_handler_t
> > > handler,
> > > + void *data)
> > > +{
> > > + struct iommu_param *idata = dev->iommu_param;
> > > +
> > > + /*
> > > +  * Device iommu_param should have been allocated when
> > > device is
> > > +  * added to its iommu_group.
> > > +  */
> > > + if (!idata)
> > > + return -EINVAL;
> > > + /* Only allow one fault handler registered for each device
> > > */
> > > + if (idata->fault_param)
> > > + return -EBUSY;
> > > + get_device(dev);
> > > + idata->fault_param =
> > > + kzalloc(sizeof(struct iommu_fault_param),
> > > GFP_KERNEL);
> > > + if (!idata->fault_param)
> > > + return -ENOMEM;
> > > + idata->fault_param->handler = handler;
> > > + idata->fault_param->data = data;
> > > +
> > > + return 0;
> > > +}
> > > +EXPORT_SYMBOL_GPL(iommu_register_device_fault_handler);
> > > +
> > > +int iommu_unregister_device_fault_handler(struct device *dev)
> > > +{
> > > + struct iommu_param *idata = dev->iommu_param;
> > > +
> > > + if (!idata)
> > > + return -EINVAL;
> > > +
> > > + kfree(idata->fault_param);
> > > + idata->fault_param = NULL;
> > > + put_device(dev);
> > > +
> > > + return 0;
> > > +}
> > > +EXPORT_SYMBOL_GPL(iommu_unregister_device_fault_handler);
> > > +
> > > +
> > > +int 

Re: [PATCH v3 10/16] iommu: introduce device fault report API

[Alex Williamson]

On Fri, 8 Dec 2017 12:23:58 -0800
Jacob Pan  wrote:

> On Thu, 7 Dec 2017 14:27:25 -0700
> Alex Williamson  wrote:
> 
> > On Fri, 17 Nov 2017 10:55:08 -0800
> > Jacob Pan  wrote:
> >   
> > > 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 
> > > ---
> > >  drivers/iommu/iommu.c | 63
> > > ++-
> > > include/linux/iommu.h | 36 + 2 files
> > > changed, 98 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> > > index 829e9e9..97b7990 100644
> > > --- a/drivers/iommu/iommu.c
> > > +++ b/drivers/iommu/iommu.c
> > > @@ -581,6 +581,12 @@ int iommu_group_add_device(struct iommu_group
> > > *group, struct device *dev) goto err_free_name;
> > >   }
> > >  
> > > + dev->iommu_param = kzalloc(sizeof(struct
> > > iommu_fault_param), GFP_KERNEL);
> > > + if (!dev->iommu_param) {
> > > + ret = -ENOMEM;
> > > + goto err_free_name;
> > > + }
> > > +
> > >   kobject_get(group->devices_kobj);
> > >  
> > >   dev->iommu_group = group;
> > > @@ -657,7 +663,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;
> > > @@ -791,6 +797,61 @@ int iommu_group_unregister_notifier(struct
> > > iommu_group *group, }
> > >  EXPORT_SYMBOL_GPL(iommu_group_unregister_notifier);
> > >  
> > > +int iommu_register_device_fault_handler(struct device *dev,
> > > + iommu_dev_fault_handler_t
> > > handler,
> > > + void *data)
> > > +{
> > > + struct iommu_param *idata = dev->iommu_param;
> > > +
> > > + /*
> > > +  * Device iommu_param should have been allocated when
> > > device is
> > > +  * added to its iommu_group.
> > > +  */
> > > + if (!idata)
> > > + return -EINVAL;
> > > + /* Only allow one fault handler registered for each device
> > > */
> > > + if (idata->fault_param)
> > > + return -EBUSY;
> > > + get_device(dev);
> > > + idata->fault_param =
> > > + kzalloc(sizeof(struct iommu_fault_param),
> > > GFP_KERNEL);
> > > + if (!idata->fault_param)
> > > + return -ENOMEM;
> > > + idata->fault_param->handler = handler;
> > > + idata->fault_param->data = data;
> > > +
> > > + return 0;
> > > +}
> > > +EXPORT_SYMBOL_GPL(iommu_register_device_fault_handler);
> > > +
> > > +int iommu_unregister_device_fault_handler(struct device *dev)
> > > +{
> > > + struct iommu_param *idata = dev->iommu_param;
> > > +
> > > + if (!idata)
> > > + return -EINVAL;
> > > +
> > > + kfree(idata->fault_param);
> > > + idata->fault_param = NULL;
> > > + put_device(dev);
> > > +
> > > + return 0;
> > > +}
> > > +EXPORT_SYMBOL_GPL(iommu_unregister_device_fault_handler);
> > > +
> > > +
> > > +int iommu_report_device_fault(struct device *dev, struct
> > > iommu_fault_event *evt) +{
> > > + /* we only report device fault if there is a handler
> > > 

Re: [PATCH v3 10/16] iommu: introduce device fault report API

[Jacob Pan]

On Thu, 7 Dec 2017 14:27:25 -0700
Alex Williamson  wrote:

> On Fri, 17 Nov 2017 10:55:08 -0800
> Jacob Pan  wrote:
> 
> > 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 
> > ---
> >  drivers/iommu/iommu.c | 63
> > ++-
> > include/linux/iommu.h | 36 + 2 files
> > changed, 98 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> > index 829e9e9..97b7990 100644
> > --- a/drivers/iommu/iommu.c
> > +++ b/drivers/iommu/iommu.c
> > @@ -581,6 +581,12 @@ int iommu_group_add_device(struct iommu_group
> > *group, struct device *dev) goto err_free_name;
> > }
> >  
> > +   dev->iommu_param = kzalloc(sizeof(struct
> > iommu_fault_param), GFP_KERNEL);
> > +   if (!dev->iommu_param) {
> > +   ret = -ENOMEM;
> > +   goto err_free_name;
> > +   }
> > +
> > kobject_get(group->devices_kobj);
> >  
> > dev->iommu_group = group;
> > @@ -657,7 +663,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;
> > @@ -791,6 +797,61 @@ int iommu_group_unregister_notifier(struct
> > iommu_group *group, }
> >  EXPORT_SYMBOL_GPL(iommu_group_unregister_notifier);
> >  
> > +int iommu_register_device_fault_handler(struct device *dev,
> > +   iommu_dev_fault_handler_t
> > handler,
> > +   void *data)
> > +{
> > +   struct iommu_param *idata = dev->iommu_param;
> > +
> > +   /*
> > +* Device iommu_param should have been allocated when
> > device is
> > +* added to its iommu_group.
> > +*/
> > +   if (!idata)
> > +   return -EINVAL;
> > +   /* Only allow one fault handler registered for each device
> > */
> > +   if (idata->fault_param)
> > +   return -EBUSY;
> > +   get_device(dev);
> > +   idata->fault_param =
> > +   kzalloc(sizeof(struct iommu_fault_param),
> > GFP_KERNEL);
> > +   if (!idata->fault_param)
> > +   return -ENOMEM;
> > +   idata->fault_param->handler = handler;
> > +   idata->fault_param->data = data;
> > +
> > +   return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(iommu_register_device_fault_handler);
> > +
> > +int iommu_unregister_device_fault_handler(struct device *dev)
> > +{
> > +   struct iommu_param *idata = dev->iommu_param;
> > +
> > +   if (!idata)
> > +   return -EINVAL;
> > +
> > +   kfree(idata->fault_param);
> > +   idata->fault_param = NULL;
> > +   put_device(dev);
> > +
> > +   return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(iommu_unregister_device_fault_handler);
> > +
> > +
> > +int iommu_report_device_fault(struct device *dev, struct
> > iommu_fault_event *evt) +{
> > +   /* we only report device fault if there is a handler
> > registered */
> > +   if (!dev->iommu_param || !dev->iommu_param->fault_param ||
> > +   

Re: [PATCH v3 10/16] iommu: introduce device fault report API

[Jacob Pan]

On Thu, 7 Dec 2017 14:27:25 -0700
Alex Williamson  wrote:

> On Fri, 17 Nov 2017 10:55:08 -0800
> Jacob Pan  wrote:
> 
> > 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 
> > ---
> >  drivers/iommu/iommu.c | 63
> > ++-
> > include/linux/iommu.h | 36 + 2 files
> > changed, 98 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> > index 829e9e9..97b7990 100644
> > --- a/drivers/iommu/iommu.c
> > +++ b/drivers/iommu/iommu.c
> > @@ -581,6 +581,12 @@ int iommu_group_add_device(struct iommu_group
> > *group, struct device *dev) goto err_free_name;
> > }
> >  
> > +   dev->iommu_param = kzalloc(sizeof(struct
> > iommu_fault_param), GFP_KERNEL);
> > +   if (!dev->iommu_param) {
> > +   ret = -ENOMEM;
> > +   goto err_free_name;
> > +   }
> > +
> > kobject_get(group->devices_kobj);
> >  
> > dev->iommu_group = group;
> > @@ -657,7 +663,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;
> > @@ -791,6 +797,61 @@ int iommu_group_unregister_notifier(struct
> > iommu_group *group, }
> >  EXPORT_SYMBOL_GPL(iommu_group_unregister_notifier);
> >  
> > +int iommu_register_device_fault_handler(struct device *dev,
> > +   iommu_dev_fault_handler_t
> > handler,
> > +   void *data)
> > +{
> > +   struct iommu_param *idata = dev->iommu_param;
> > +
> > +   /*
> > +* Device iommu_param should have been allocated when
> > device is
> > +* added to its iommu_group.
> > +*/
> > +   if (!idata)
> > +   return -EINVAL;
> > +   /* Only allow one fault handler registered for each device
> > */
> > +   if (idata->fault_param)
> > +   return -EBUSY;
> > +   get_device(dev);
> > +   idata->fault_param =
> > +   kzalloc(sizeof(struct iommu_fault_param),
> > GFP_KERNEL);
> > +   if (!idata->fault_param)
> > +   return -ENOMEM;
> > +   idata->fault_param->handler = handler;
> > +   idata->fault_param->data = data;
> > +
> > +   return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(iommu_register_device_fault_handler);
> > +
> > +int iommu_unregister_device_fault_handler(struct device *dev)
> > +{
> > +   struct iommu_param *idata = dev->iommu_param;
> > +
> > +   if (!idata)
> > +   return -EINVAL;
> > +
> > +   kfree(idata->fault_param);
> > +   idata->fault_param = NULL;
> > +   put_device(dev);
> > +
> > +   return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(iommu_unregister_device_fault_handler);
> > +
> > +
> > +int iommu_report_device_fault(struct device *dev, struct
> > iommu_fault_event *evt) +{
> > +   /* we only report device fault if there is a handler
> > registered */
> > +   if (!dev->iommu_param || !dev->iommu_param->fault_param ||
> > +   !dev->iommu_param->fault_param->handler)
> > +   return -ENOSYS;
> > +
> > +   return dev->iommu_param->fault_param->handler(evt,
> > +
> > 

Re: [PATCH v3 10/16] iommu: introduce device fault report API

[Alex Williamson]

On Fri, 17 Nov 2017 10:55:08 -0800
Jacob Pan  wrote:

> 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 
> ---
>  drivers/iommu/iommu.c | 63 
> ++-
>  include/linux/iommu.h | 36 +
>  2 files changed, 98 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index 829e9e9..97b7990 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -581,6 +581,12 @@ int iommu_group_add_device(struct iommu_group *group, 
> struct device *dev)
>   goto err_free_name;
>   }
>  
> + dev->iommu_param = kzalloc(sizeof(struct iommu_fault_param), 
> GFP_KERNEL);
> + if (!dev->iommu_param) {
> + ret = -ENOMEM;
> + goto err_free_name;
> + }
> +
>   kobject_get(group->devices_kobj);
>  
>   dev->iommu_group = group;
> @@ -657,7 +663,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;
> @@ -791,6 +797,61 @@ int iommu_group_unregister_notifier(struct iommu_group 
> *group,
>  }
>  EXPORT_SYMBOL_GPL(iommu_group_unregister_notifier);
>  
> +int iommu_register_device_fault_handler(struct device *dev,
> + iommu_dev_fault_handler_t handler,
> + void *data)
> +{
> + struct iommu_param *idata = dev->iommu_param;
> +
> + /*
> +  * Device iommu_param should have been allocated when device is
> +  * added to its iommu_group.
> +  */
> + if (!idata)
> + return -EINVAL;
> + /* Only allow one fault handler registered for each device */
> + if (idata->fault_param)
> + return -EBUSY;
> + get_device(dev);
> + idata->fault_param =
> + kzalloc(sizeof(struct iommu_fault_param), GFP_KERNEL);
> + if (!idata->fault_param)
> + return -ENOMEM;
> + idata->fault_param->handler = handler;
> + idata->fault_param->data = data;
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(iommu_register_device_fault_handler);
> +
> +int iommu_unregister_device_fault_handler(struct device *dev)
> +{
> + struct iommu_param *idata = dev->iommu_param;
> +
> + if (!idata)
> + return -EINVAL;
> +
> + kfree(idata->fault_param);
> + idata->fault_param = NULL;
> + put_device(dev);
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(iommu_unregister_device_fault_handler);
> +
> +
> +int iommu_report_device_fault(struct device *dev, struct iommu_fault_event 
> *evt)
> +{
> + /* we only report device fault if there is a handler registered */
> + if (!dev->iommu_param || !dev->iommu_param->fault_param ||
> + !dev->iommu_param->fault_param->handler)
> + return -ENOSYS;
> +
> + return dev->iommu_param->fault_param->handler(evt,
> + 
> dev->iommu_param->fault_param->data);
> +}
> 

Re: [PATCH v3 10/16] iommu: introduce device fault report API

[Alex Williamson]

On Fri, 17 Nov 2017 10:55:08 -0800
Jacob Pan  wrote:

> 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 
> ---
>  drivers/iommu/iommu.c | 63 
> ++-
>  include/linux/iommu.h | 36 +
>  2 files changed, 98 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index 829e9e9..97b7990 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -581,6 +581,12 @@ int iommu_group_add_device(struct iommu_group *group, 
> struct device *dev)
>   goto err_free_name;
>   }
>  
> + dev->iommu_param = kzalloc(sizeof(struct iommu_fault_param), 
> GFP_KERNEL);
> + if (!dev->iommu_param) {
> + ret = -ENOMEM;
> + goto err_free_name;
> + }
> +
>   kobject_get(group->devices_kobj);
>  
>   dev->iommu_group = group;
> @@ -657,7 +663,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;
> @@ -791,6 +797,61 @@ int iommu_group_unregister_notifier(struct iommu_group 
> *group,
>  }
>  EXPORT_SYMBOL_GPL(iommu_group_unregister_notifier);
>  
> +int iommu_register_device_fault_handler(struct device *dev,
> + iommu_dev_fault_handler_t handler,
> + void *data)
> +{
> + struct iommu_param *idata = dev->iommu_param;
> +
> + /*
> +  * Device iommu_param should have been allocated when device is
> +  * added to its iommu_group.
> +  */
> + if (!idata)
> + return -EINVAL;
> + /* Only allow one fault handler registered for each device */
> + if (idata->fault_param)
> + return -EBUSY;
> + get_device(dev);
> + idata->fault_param =
> + kzalloc(sizeof(struct iommu_fault_param), GFP_KERNEL);
> + if (!idata->fault_param)
> + return -ENOMEM;
> + idata->fault_param->handler = handler;
> + idata->fault_param->data = data;
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(iommu_register_device_fault_handler);
> +
> +int iommu_unregister_device_fault_handler(struct device *dev)
> +{
> + struct iommu_param *idata = dev->iommu_param;
> +
> + if (!idata)
> + return -EINVAL;
> +
> + kfree(idata->fault_param);
> + idata->fault_param = NULL;
> + put_device(dev);
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(iommu_unregister_device_fault_handler);
> +
> +
> +int iommu_report_device_fault(struct device *dev, struct iommu_fault_event 
> *evt)
> +{
> + /* we only report device fault if there is a handler registered */
> + if (!dev->iommu_param || !dev->iommu_param->fault_param ||
> + !dev->iommu_param->fault_param->handler)
> + return -ENOSYS;
> +
> + return dev->iommu_param->fault_param->handler(evt,
> + 
> dev->iommu_param->fault_param->data);
> +}
> +EXPORT_SYMBOL_GPL(iommu_report_device_fault);
> +

Isn't this all rather racy?  I see that we can have multiple callers to

Re: [PATCH v3 10/16] iommu: introduce device fault report API

[Lu Baolu]

Hi,

On 11/18/2017 02:55 AM, Jacob Pan wrote:
> 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 
> ---
>  drivers/iommu/iommu.c | 63 
> ++-
>  include/linux/iommu.h | 36 +
>  2 files changed, 98 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index 829e9e9..97b7990 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -581,6 +581,12 @@ int iommu_group_add_device(struct iommu_group *group, 
> struct device *dev)
>   goto err_free_name;
>   }
>  
> + dev->iommu_param = kzalloc(sizeof(struct iommu_fault_param), 
> GFP_KERNEL);
> + if (!dev->iommu_param) {
> + ret = -ENOMEM;
> + goto err_free_name;
> + }
> +
>   kobject_get(group->devices_kobj);
>  
>   dev->iommu_group = group;
> @@ -657,7 +663,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;
> @@ -791,6 +797,61 @@ int iommu_group_unregister_notifier(struct iommu_group 
> *group,
>  }
>  EXPORT_SYMBOL_GPL(iommu_group_unregister_notifier);
>  
> +int iommu_register_device_fault_handler(struct device *dev,
> + iommu_dev_fault_handler_t handler,
> + void *data)
> +{
> + struct iommu_param *idata = dev->iommu_param;
> +
> + /*
> +  * Device iommu_param should have been allocated when device is
> +  * added to its iommu_group.
> +  */
> + if (!idata)
> + return -EINVAL;
> + /* Only allow one fault handler registered for each device */
> + if (idata->fault_param)
> + return -EBUSY;
> + get_device(dev);
> + idata->fault_param =
> + kzalloc(sizeof(struct iommu_fault_param), GFP_KERNEL);
> + if (!idata->fault_param)
> + return -ENOMEM;
> + idata->fault_param->handler = handler;
> + idata->fault_param->data = data;
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(iommu_register_device_fault_handler);
> +
> +int iommu_unregister_device_fault_handler(struct device *dev)
> +{
> + struct iommu_param *idata = dev->iommu_param;
> +
> + if (!idata)
> + return -EINVAL;
> +
> + kfree(idata->fault_param);
> + idata->fault_param = NULL;
> + put_device(dev);
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(iommu_unregister_device_fault_handler);
> +
> +
> +int iommu_report_device_fault(struct device *dev, struct iommu_fault_event 
> *evt)
> +{
> + /* we only report device fault if there is a handler registered */
> + if (!dev->iommu_param || !dev->iommu_param->fault_param ||
> + !dev->iommu_param->fault_param->handler)

Can this replaced by:

if (!iommu_has_device_fault_handler(dev))

?

Best regards,
Lu Baolu

> + return -ENOSYS;
> +
> + return dev->iommu_param->fault_param->handler(evt,
> + 
> dev->iommu_param->fault_param->data);

Re: [PATCH v3 10/16] iommu: introduce device fault report API

[Lu Baolu]

Hi,

On 11/18/2017 02:55 AM, Jacob Pan wrote:
> 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 
> ---
>  drivers/iommu/iommu.c | 63 
> ++-
>  include/linux/iommu.h | 36 +
>  2 files changed, 98 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index 829e9e9..97b7990 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -581,6 +581,12 @@ int iommu_group_add_device(struct iommu_group *group, 
> struct device *dev)
>   goto err_free_name;
>   }
>  
> + dev->iommu_param = kzalloc(sizeof(struct iommu_fault_param), 
> GFP_KERNEL);
> + if (!dev->iommu_param) {
> + ret = -ENOMEM;
> + goto err_free_name;
> + }
> +
>   kobject_get(group->devices_kobj);
>  
>   dev->iommu_group = group;
> @@ -657,7 +663,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;
> @@ -791,6 +797,61 @@ int iommu_group_unregister_notifier(struct iommu_group 
> *group,
>  }
>  EXPORT_SYMBOL_GPL(iommu_group_unregister_notifier);
>  
> +int iommu_register_device_fault_handler(struct device *dev,
> + iommu_dev_fault_handler_t handler,
> + void *data)
> +{
> + struct iommu_param *idata = dev->iommu_param;
> +
> + /*
> +  * Device iommu_param should have been allocated when device is
> +  * added to its iommu_group.
> +  */
> + if (!idata)
> + return -EINVAL;
> + /* Only allow one fault handler registered for each device */
> + if (idata->fault_param)
> + return -EBUSY;
> + get_device(dev);
> + idata->fault_param =
> + kzalloc(sizeof(struct iommu_fault_param), GFP_KERNEL);
> + if (!idata->fault_param)
> + return -ENOMEM;
> + idata->fault_param->handler = handler;
> + idata->fault_param->data = data;
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(iommu_register_device_fault_handler);
> +
> +int iommu_unregister_device_fault_handler(struct device *dev)
> +{
> + struct iommu_param *idata = dev->iommu_param;
> +
> + if (!idata)
> + return -EINVAL;
> +
> + kfree(idata->fault_param);
> + idata->fault_param = NULL;
> + put_device(dev);
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(iommu_unregister_device_fault_handler);
> +
> +
> +int iommu_report_device_fault(struct device *dev, struct iommu_fault_event 
> *evt)
> +{
> + /* we only report device fault if there is a handler registered */
> + if (!dev->iommu_param || !dev->iommu_param->fault_param ||
> + !dev->iommu_param->fault_param->handler)

Can this replaced by:

if (!iommu_has_device_fault_handler(dev))

?

Best regards,
Lu Baolu

> + return -ENOSYS;
> +
> + return dev->iommu_param->fault_param->handler(evt,
> + 
> dev->iommu_param->fault_param->data);
> +}
> 

[PATCH v3 10/16] 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 
---
 drivers/iommu/iommu.c | 63 ++-
 include/linux/iommu.h | 36 +
 2 files changed, 98 insertions(+), 1 deletion(-)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 829e9e9..97b7990 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -581,6 +581,12 @@ int iommu_group_add_device(struct iommu_group *group, 
struct device *dev)
goto err_free_name;
}
 
+   dev->iommu_param = kzalloc(sizeof(struct iommu_fault_param), 
GFP_KERNEL);
+   if (!dev->iommu_param) {
+   ret = -ENOMEM;
+   goto err_free_name;
+   }
+
kobject_get(group->devices_kobj);
 
dev->iommu_group = group;
@@ -657,7 +663,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;
@@ -791,6 +797,61 @@ int iommu_group_unregister_notifier(struct iommu_group 
*group,
 }
 EXPORT_SYMBOL_GPL(iommu_group_unregister_notifier);
 
+int iommu_register_device_fault_handler(struct device *dev,
+   iommu_dev_fault_handler_t handler,
+   void *data)
+{
+   struct iommu_param *idata = dev->iommu_param;
+
+   /*
+* Device iommu_param should have been allocated when device is
+* added to its iommu_group.
+*/
+   if (!idata)
+   return -EINVAL;
+   /* Only allow one fault handler registered for each device */
+   if (idata->fault_param)
+   return -EBUSY;
+   get_device(dev);
+   idata->fault_param =
+   kzalloc(sizeof(struct iommu_fault_param), GFP_KERNEL);
+   if (!idata->fault_param)
+   return -ENOMEM;
+   idata->fault_param->handler = handler;
+   idata->fault_param->data = data;
+
+   return 0;
+}
+EXPORT_SYMBOL_GPL(iommu_register_device_fault_handler);
+
+int iommu_unregister_device_fault_handler(struct device *dev)
+{
+   struct iommu_param *idata = dev->iommu_param;
+
+   if (!idata)
+   return -EINVAL;
+
+   kfree(idata->fault_param);
+   idata->fault_param = NULL;
+   put_device(dev);
+
+   return 0;
+}
+EXPORT_SYMBOL_GPL(iommu_unregister_device_fault_handler);
+
+
+int iommu_report_device_fault(struct device *dev, struct iommu_fault_event 
*evt)
+{
+   /* we only report device fault if there is a handler registered */
+   if (!dev->iommu_param || !dev->iommu_param->fault_param ||
+   !dev->iommu_param->fault_param->handler)
+   return -ENOSYS;
+
+   return dev->iommu_param->fault_param->handler(evt,
+   
dev->iommu_param->fault_param->data);
+}
+EXPORT_SYMBOL_GPL(iommu_report_device_fault);
+
 /**
  * iommu_group_id - Return ID for a group
  * @group: the group to ID
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index dfda89b..841c044 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -463,6 +463,14 @@ extern int 

[PATCH v3 10/16] 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 
---
 drivers/iommu/iommu.c | 63 ++-
 include/linux/iommu.h | 36 +
 2 files changed, 98 insertions(+), 1 deletion(-)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 829e9e9..97b7990 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -581,6 +581,12 @@ int iommu_group_add_device(struct iommu_group *group, 
struct device *dev)
goto err_free_name;
}
 
+   dev->iommu_param = kzalloc(sizeof(struct iommu_fault_param), 
GFP_KERNEL);
+   if (!dev->iommu_param) {
+   ret = -ENOMEM;
+   goto err_free_name;
+   }
+
kobject_get(group->devices_kobj);
 
dev->iommu_group = group;
@@ -657,7 +663,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;
@@ -791,6 +797,61 @@ int iommu_group_unregister_notifier(struct iommu_group 
*group,
 }
 EXPORT_SYMBOL_GPL(iommu_group_unregister_notifier);
 
+int iommu_register_device_fault_handler(struct device *dev,
+   iommu_dev_fault_handler_t handler,
+   void *data)
+{
+   struct iommu_param *idata = dev->iommu_param;
+
+   /*
+* Device iommu_param should have been allocated when device is
+* added to its iommu_group.
+*/
+   if (!idata)
+   return -EINVAL;
+   /* Only allow one fault handler registered for each device */
+   if (idata->fault_param)
+   return -EBUSY;
+   get_device(dev);
+   idata->fault_param =
+   kzalloc(sizeof(struct iommu_fault_param), GFP_KERNEL);
+   if (!idata->fault_param)
+   return -ENOMEM;
+   idata->fault_param->handler = handler;
+   idata->fault_param->data = data;
+
+   return 0;
+}
+EXPORT_SYMBOL_GPL(iommu_register_device_fault_handler);
+
+int iommu_unregister_device_fault_handler(struct device *dev)
+{
+   struct iommu_param *idata = dev->iommu_param;
+
+   if (!idata)
+   return -EINVAL;
+
+   kfree(idata->fault_param);
+   idata->fault_param = NULL;
+   put_device(dev);
+
+   return 0;
+}
+EXPORT_SYMBOL_GPL(iommu_unregister_device_fault_handler);
+
+
+int iommu_report_device_fault(struct device *dev, struct iommu_fault_event 
*evt)
+{
+   /* we only report device fault if there is a handler registered */
+   if (!dev->iommu_param || !dev->iommu_param->fault_param ||
+   !dev->iommu_param->fault_param->handler)
+   return -ENOSYS;
+
+   return dev->iommu_param->fault_param->handler(evt,
+   
dev->iommu_param->fault_param->data);
+}
+EXPORT_SYMBOL_GPL(iommu_report_device_fault);
+
 /**
  * iommu_group_id - Return ID for a group
  * @group: the group to ID
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index dfda89b..841c044 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -463,6 +463,14 @@ extern int iommu_group_register_notifier(struct 
iommu_group