Re: [PATCH v4 03/26] iommu: Add a page fault handler
On Wed, Feb 26, 2020 at 01:59:33PM +, Jonathan Cameron wrote:
> > +static int iopf_complete(struct device *dev, struct iopf_fault *iopf,
> > +enum iommu_page_response_code status)
>
> This is called once per group. Should name reflect that?
Ok
[...]
> > +/**
> > + * iommu_queue_iopf - IO Page Fault handler
> > + * @evt: fault event
> > + * @cookie: struct device, passed to iommu_register_device_fault_handler.
> > + *
> > + * Add a fault to the device workqueue, to be handled by mm.
> > + *
> > + * Return: 0 on success and <0 on error.
> > + */
> > +int iommu_queue_iopf(struct iommu_fault *fault, void *cookie)
> > +{
> > + int ret;
> > + struct iopf_group *group;
> > + struct iopf_fault *iopf, *next;
> > + struct iopf_device_param *iopf_param;
> > +
> > + struct device *dev = cookie;
> > + struct iommu_param *param = dev->iommu_param;
> > +
> > + if (WARN_ON(!mutex_is_locked(>lock)))
> > + return -EINVAL;
>
> Just curious...
>
> Why do we always need a runtime check on this rather than say,
> using lockdep_assert_held or similar?
I probably didn't know about lockdep_assert at the time :)
> > + /*
> > +* It is incredibly easy to find ourselves in a deadlock situation if
> > +* we're not careful, because we're taking the opposite path as
> > +* iommu_queue_iopf:
> > +*
> > +* iopf_queue_flush_dev() | PRI queue handler
> > +*lock(>lock) | iommu_queue_iopf()
> > +* queue->flush() |lock(>lock)
> > +* wait PRI queue empty |
> > +*
> > +* So we can't hold the device param lock while flushing. Take a
> > +* reference to the device param instead, to prevent the queue from
> > +* going away.
> > +*/
> > + mutex_lock(>lock);
> > + iopf_param = param->iopf_param;
> > + if (iopf_param) {
> > + queue = param->iopf_param->queue;
> > + iopf_param->busy = true;
>
> Describing this as taking a reference is not great...
> I'd change the comment to set a flag or something like that.
>
> Is there any potential of multiple copies of this running against
> each other? I've not totally gotten my head around when this
> might be called yet.
Yes it's allowed, this should be a refcount
[...]
> > +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 iommu_param *param = dev->iommu_param;
> > +
> > + if (!param || !queue)
> > + return -EINVAL;
> > +
> > + do {
> > + mutex_lock(>lock);
> > + mutex_lock(>lock);
> > + iopf_param = param->iopf_param;
> > + if (iopf_param && iopf_param->queue == queue) {
> > + if (iopf_param->busy) {
> > + ret = -EBUSY;
> > + } else {
> > + list_del(_param->queue_list);
> > + param->iopf_param = NULL;
> > + ret = 0;
> > + }
> > + }
> > + mutex_unlock(>lock);
> > + mutex_unlock(>lock);
> > +
> > + /*
> > +* If there is an ongoing flush, wait for it to complete and
> > +* then retry. iopf_param isn't going away since we're the only
> > +* thread that can free it.
> > +*/
> > + if (ret == -EBUSY)
> > + wait_event(iopf_param->wq_head, !iopf_param->busy);
> > + else if (ret)
> > + return ret;
> > + } while (ret == -EBUSY);
>
> I'm in two minds about the next comment (so up to you)...
>
> Currently this looks a bit odd. Would you be better off just having a
> separate
> parameter for busy and explicit separate handling for the error path?
>
> bool busy;
> int ret = 0;
>
> do {
> mutex_lock(>lock);
> mutex_lock(>lock);
> iopf_param = param->iopf_param;
> if (iopf_param && iopf_param->queue == queue) {
> busy = iopf_param->busy;
> if (!busy) {
> list_del(_param->queue_list);
> param->iopf_param = NULL;
> }
> } else {
> ret = -EINVAL;
> }
> mutex_unlock(>lock);
> mutex_unlock(>lock);
> if (ret)
> return ret;
> if (busy)
> wait_event(iopf_param->wq_head, !iopf_param->busy);
>
> } while (busy);
>
> ..
Sure, I think it looks better
Thanks,
Jean
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v4 03/26] iommu: Add a page fault handler
On Mon, 24 Feb 2020 19:23:38 +0100
Jean-Philippe Brucker wrote:
> From: Jean-Philippe Brucker
>
> Some systems allow devices to handle I/O Page Faults in the core mm. For
> example systems implementing the PCI PRI extension or Arm SMMU stall
> model. Infrastructure for reporting these recoverable page faults was
> recently added to the IOMMU core. Add a page fault handler for host SVA.
>
> IOMMU driver can now instantiate several fault workqueues and link them to
> IOPF-capable devices. Drivers can choose between a single global
> workqueue, one per IOMMU device, one per low-level fault queue, one per
> domain, etc.
>
> When it receives a fault event, supposedly in an IRQ handler, the IOMMU
> driver reports the fault using iommu_report_device_fault(), which calls
> the registered handler. The page fault handler then calls the mm fault
> handler, and reports either success or failure with iommu_page_response().
> When the handler succeeded, the IOMMU retries the access.
>
> The iopf_param pointer could be embedded into iommu_fault_param. But
> putting iopf_param into the iommu_param structure allows us not to care
> about ordering between calls to iopf_queue_add_device() and
> iommu_register_device_fault_handler().
>
> Signed-off-by: Jean-Philippe Brucker
A few more minor comments...
> ---
> drivers/iommu/Kconfig | 4 +
> drivers/iommu/Makefile | 1 +
> drivers/iommu/io-pgfault.c | 451 +
> include/linux/iommu.h | 59 +
> 4 files changed, 515 insertions(+)
> create mode 100644 drivers/iommu/io-pgfault.c
>
> diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
> index acca20e2da2f..e4a42e1708b4 100644
> --- a/drivers/iommu/Kconfig
> +++ b/drivers/iommu/Kconfig
> @@ -109,6 +109,10 @@ config IOMMU_SVA
> select IOMMU_API
> select MMU_NOTIFIER
>
> +config IOMMU_PAGE_FAULT
> + bool
> + select IOMMU_API
> +
> config FSL_PAMU
> bool "Freescale IOMMU support"
> depends on PCI
> diff --git a/drivers/iommu/Makefile b/drivers/iommu/Makefile
> index 40c800dd4e3e..bf5cb4ee8409 100644
> --- a/drivers/iommu/Makefile
> +++ b/drivers/iommu/Makefile
> @@ -4,6 +4,7 @@ obj-$(CONFIG_IOMMU_API) += iommu-traces.o
> obj-$(CONFIG_IOMMU_API) += iommu-sysfs.o
> obj-$(CONFIG_IOMMU_DEBUGFS) += iommu-debugfs.o
> obj-$(CONFIG_IOMMU_DMA) += dma-iommu.o
> +obj-$(CONFIG_IOMMU_PAGE_FAULT) += io-pgfault.o
> obj-$(CONFIG_IOMMU_IO_PGTABLE) += io-pgtable.o
> obj-$(CONFIG_IOMMU_IO_PGTABLE_ARMV7S) += io-pgtable-arm-v7s.o
> obj-$(CONFIG_IOMMU_IO_PGTABLE_LPAE) += io-pgtable-arm.o
> diff --git a/drivers/iommu/io-pgfault.c b/drivers/iommu/io-pgfault.c
> new file mode 100644
> index ..76e153c59fe3
> --- /dev/null
> +++ b/drivers/iommu/io-pgfault.c
> @@ -0,0 +1,451 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Handle device page faults
> + *
> + * Copyright (C) 2018 ARM Ltd.
As before. Date update perhaps?
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +
> +/**
> + * struct iopf_queue - IO Page Fault queue
> + * @wq: the fault workqueue
> + * @flush: low-level flush callback
> + * @flush_arg: flush() argument
> + * @devices: devices attached to this queue
> + * @lock: protects the device list
> + */
> +struct iopf_queue {
> + struct workqueue_struct *wq;
> + iopf_queue_flush_t flush;
> + void*flush_arg;
> + struct list_headdevices;
> + struct mutexlock;
> +};
> +
> +/**
> + * struct iopf_device_param - IO Page Fault data attached to a device
> + * @dev: the device that owns this param
> + * @queue: IOPF queue
> + * @queue_list: index into queue->devices
> + * @partial: faults that are part of a Page Request Group for which the last
> + * request hasn't been submitted yet.
> + * @busy: the param is being used
> + * @wq_head: signal a change to @busy
> + */
> +struct iopf_device_param {
> + struct device *dev;
> + struct iopf_queue *queue;
> + struct list_headqueue_list;
> + struct list_headpartial;
> + boolbusy;
> + wait_queue_head_t wq_head;
> +};
> +
> +struct iopf_fault {
> + struct iommu_fault fault;
> + struct list_headhead;
> +};
> +
> +struct iopf_group {
> + struct iopf_fault last_fault;
> + struct list_headfaults;
> + struct work_struct work;
> + struct device *dev;
> +};
> +
> +static int iopf_complete(struct device *dev, struct iopf_fault *iopf,
> + enum iommu_page_response_code status)
This is called once per group. Should name reflect that?
> +{
> + struct iommu_page_response resp = {
> + .version= IOMMU_PAGE_RESP_VERSION_1,
> + .pasid =
Re: [PATCH v4 03/26] iommu: Add a page fault handler
Hi,
On 2020/2/25 17:25, Jean-Philippe Brucker wrote:
Hi Zaibo,
On Tue, Feb 25, 2020 at 11:30:05AM +0800, Xu Zaibo wrote:
+struct iopf_queue *
+iopf_queue_alloc(const char *name, iopf_queue_flush_t flush, void *cookie)
+{
+ struct iopf_queue *queue;
+
+ queue = kzalloc(sizeof(*queue), GFP_KERNEL);
+ if (!queue)
+ return NULL;
+
+ /*
+* The WQ is unordered because the low-level handler enqueues faults by
+* group. PRI requests within a group have to be ordered, but once
+* that's dealt with, the high-level function can handle groups out of
+* order.
+*/
+ queue->wq = alloc_workqueue("iopf_queue/%s", WQ_UNBOUND, 0, name);
Should this workqueue use 'WQ_HIGHPRI | WQ_UNBOUND' or some flags like this
to decrease the unexpected
latency of I/O PageFault here? Or maybe, workqueue will show an uncontrolled
latency, even in a busy system.
I'll investigate the effect of these flags. So far I've only run on
completely idle systems but it would be interesting to add some
workqueue-heavy load in my tests.
I'm not sure, just my concern. Hopefully, Tejun Heo can give us some
hints. :)
+cc Tejun Heo
Cheers,
Zaibo
.
.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v4 03/26] iommu: Add a page fault handler
Hi Zaibo,
On Tue, Feb 25, 2020 at 11:30:05AM +0800, Xu Zaibo wrote:
> > +struct iopf_queue *
> > +iopf_queue_alloc(const char *name, iopf_queue_flush_t flush, void *cookie)
> > +{
> > + struct iopf_queue *queue;
> > +
> > + queue = kzalloc(sizeof(*queue), GFP_KERNEL);
> > + if (!queue)
> > + return NULL;
> > +
> > + /*
> > +* The WQ is unordered because the low-level handler enqueues faults by
> > +* group. PRI requests within a group have to be ordered, but once
> > +* that's dealt with, the high-level function can handle groups out of
> > +* order.
> > +*/
> > + queue->wq = alloc_workqueue("iopf_queue/%s", WQ_UNBOUND, 0, name);
> Should this workqueue use 'WQ_HIGHPRI | WQ_UNBOUND' or some flags like this
> to decrease the unexpected
> latency of I/O PageFault here? Or maybe, workqueue will show an uncontrolled
> latency, even in a busy system.
I'll investigate the effect of these flags. So far I've only run on
completely idle systems but it would be interesting to add some
workqueue-heavy load in my tests.
Thanks,
Jean
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v4 03/26] iommu: Add a page fault handler
Hi,
On 2020/2/25 2:23, Jean-Philippe Brucker wrote:
From: Jean-Philippe Brucker
Some systems allow devices to handle I/O Page Faults in the core mm. For
example systems implementing the PCI PRI extension or Arm SMMU stall
model. Infrastructure for reporting these recoverable page faults was
recently added to the IOMMU core. Add a page fault handler for host SVA.
IOMMU driver can now instantiate several fault workqueues and link them to
IOPF-capable devices. Drivers can choose between a single global
workqueue, one per IOMMU device, one per low-level fault queue, one per
domain, etc.
When it receives a fault event, supposedly in an IRQ handler, the IOMMU
driver reports the fault using iommu_report_device_fault(), which calls
the registered handler. The page fault handler then calls the mm fault
handler, and reports either success or failure with iommu_page_response().
When the handler succeeded, the IOMMU retries the access.
The iopf_param pointer could be embedded into iommu_fault_param. But
putting iopf_param into the iommu_param structure allows us not to care
about ordering between calls to iopf_queue_add_device() and
iommu_register_device_fault_handler().
Signed-off-by: Jean-Philippe Brucker
---
drivers/iommu/Kconfig | 4 +
drivers/iommu/Makefile | 1 +
drivers/iommu/io-pgfault.c | 451 +
include/linux/iommu.h | 59 +
4 files changed, 515 insertions(+)
create mode 100644 drivers/iommu/io-pgfault.c
diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
index acca20e2da2f..e4a42e1708b4 100644
--- a/drivers/iommu/Kconfig
+++ b/drivers/iommu/Kconfig
@@ -109,6 +109,10 @@ config IOMMU_SVA
select IOMMU_API
select MMU_NOTIFIER
+config IOMMU_PAGE_FAULT
+ bool
+ select IOMMU_API
+
config FSL_PAMU
bool "Freescale IOMMU support"
depends on PCI
diff --git a/drivers/iommu/Makefile b/drivers/iommu/Makefile
index 40c800dd4e3e..bf5cb4ee8409 100644
--- a/drivers/iommu/Makefile
+++ b/drivers/iommu/Makefile
@@ -4,6 +4,7 @@ obj-$(CONFIG_IOMMU_API) += iommu-traces.o
obj-$(CONFIG_IOMMU_API) += iommu-sysfs.o
obj-$(CONFIG_IOMMU_DEBUGFS) += iommu-debugfs.o
obj-$(CONFIG_IOMMU_DMA) += dma-iommu.o
+obj-$(CONFIG_IOMMU_PAGE_FAULT) += io-pgfault.o
obj-$(CONFIG_IOMMU_IO_PGTABLE) += io-pgtable.o
obj-$(CONFIG_IOMMU_IO_PGTABLE_ARMV7S) += io-pgtable-arm-v7s.o
obj-$(CONFIG_IOMMU_IO_PGTABLE_LPAE) += io-pgtable-arm.o
diff --git a/drivers/iommu/io-pgfault.c b/drivers/iommu/io-pgfault.c
new file mode 100644
index ..76e153c59fe3
--- /dev/null
+++ b/drivers/iommu/io-pgfault.c
@@ -0,0 +1,451 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Handle device page faults
+ *
+ * Copyright (C) 2018 ARM Ltd.
+ */
+
+#include
+#include
+#include
+#include
+
+/**
+ * struct iopf_queue - IO Page Fault queue
+ * @wq: the fault workqueue
+ * @flush: low-level flush callback
+ * @flush_arg: flush() argument
+ * @devices: devices attached to this queue
+ * @lock: protects the device list
+ */
+struct iopf_queue {
+ struct workqueue_struct *wq;
+ iopf_queue_flush_t flush;
+ void*flush_arg;
+ struct list_headdevices;
+ struct mutexlock;
+};
+
+/**
+ * struct iopf_device_param - IO Page Fault data attached to a device
+ * @dev: the device that owns this param
+ * @queue: IOPF queue
+ * @queue_list: index into queue->devices
+ * @partial: faults that are part of a Page Request Group for which the last
+ * request hasn't been submitted yet.
+ * @busy: the param is being used
+ * @wq_head: signal a change to @busy
+ */
+struct iopf_device_param {
+ struct device *dev;
+ struct iopf_queue *queue;
+ struct list_headqueue_list;
+ struct list_headpartial;
+ boolbusy;
+ wait_queue_head_t wq_head;
+};
+
+struct iopf_fault {
+ struct iommu_fault fault;
+ struct list_headhead;
+};
+
+struct iopf_group {
+ struct iopf_fault last_fault;
+ struct list_headfaults;
+ struct work_struct work;
+ struct device *dev;
+};
+
[...]
+
+/**
+ * iopf_queue_alloc - Allocate and initialize a fault queue
+ * @name: a unique string identifying the queue (for workqueue)
+ * @flush: a callback that flushes the low-level queue
+ * @cookie: driver-private data passed to the flush callback
+ *
+ * The callback is called before the workqueue is flushed. The IOMMU driver
must
+ * commit all faults that are pending in its low-level queues at the time of
the
+ * call, into the IOPF queue (with iommu_report_device_fault). The callback
+ * takes a device pointer as argument, hinting what endpoint is causing the
+ * flush. When the device is NULL, all faults
[PATCH v4 03/26] iommu: Add a page fault handler
From: Jean-Philippe Brucker
Some systems allow devices to handle I/O Page Faults in the core mm. For
example systems implementing the PCI PRI extension or Arm SMMU stall
model. Infrastructure for reporting these recoverable page faults was
recently added to the IOMMU core. Add a page fault handler for host SVA.
IOMMU driver can now instantiate several fault workqueues and link them to
IOPF-capable devices. Drivers can choose between a single global
workqueue, one per IOMMU device, one per low-level fault queue, one per
domain, etc.
When it receives a fault event, supposedly in an IRQ handler, the IOMMU
driver reports the fault using iommu_report_device_fault(), which calls
the registered handler. The page fault handler then calls the mm fault
handler, and reports either success or failure with iommu_page_response().
When the handler succeeded, the IOMMU retries the access.
The iopf_param pointer could be embedded into iommu_fault_param. But
putting iopf_param into the iommu_param structure allows us not to care
about ordering between calls to iopf_queue_add_device() and
iommu_register_device_fault_handler().
Signed-off-by: Jean-Philippe Brucker
---
drivers/iommu/Kconfig | 4 +
drivers/iommu/Makefile | 1 +
drivers/iommu/io-pgfault.c | 451 +
include/linux/iommu.h | 59 +
4 files changed, 515 insertions(+)
create mode 100644 drivers/iommu/io-pgfault.c
diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
index acca20e2da2f..e4a42e1708b4 100644
--- a/drivers/iommu/Kconfig
+++ b/drivers/iommu/Kconfig
@@ -109,6 +109,10 @@ config IOMMU_SVA
select IOMMU_API
select MMU_NOTIFIER
+config IOMMU_PAGE_FAULT
+ bool
+ select IOMMU_API
+
config FSL_PAMU
bool "Freescale IOMMU support"
depends on PCI
diff --git a/drivers/iommu/Makefile b/drivers/iommu/Makefile
index 40c800dd4e3e..bf5cb4ee8409 100644
--- a/drivers/iommu/Makefile
+++ b/drivers/iommu/Makefile
@@ -4,6 +4,7 @@ obj-$(CONFIG_IOMMU_API) += iommu-traces.o
obj-$(CONFIG_IOMMU_API) += iommu-sysfs.o
obj-$(CONFIG_IOMMU_DEBUGFS) += iommu-debugfs.o
obj-$(CONFIG_IOMMU_DMA) += dma-iommu.o
+obj-$(CONFIG_IOMMU_PAGE_FAULT) += io-pgfault.o
obj-$(CONFIG_IOMMU_IO_PGTABLE) += io-pgtable.o
obj-$(CONFIG_IOMMU_IO_PGTABLE_ARMV7S) += io-pgtable-arm-v7s.o
obj-$(CONFIG_IOMMU_IO_PGTABLE_LPAE) += io-pgtable-arm.o
diff --git a/drivers/iommu/io-pgfault.c b/drivers/iommu/io-pgfault.c
new file mode 100644
index ..76e153c59fe3
--- /dev/null
+++ b/drivers/iommu/io-pgfault.c
@@ -0,0 +1,451 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Handle device page faults
+ *
+ * Copyright (C) 2018 ARM Ltd.
+ */
+
+#include
+#include
+#include
+#include
+
+/**
+ * struct iopf_queue - IO Page Fault queue
+ * @wq: the fault workqueue
+ * @flush: low-level flush callback
+ * @flush_arg: flush() argument
+ * @devices: devices attached to this queue
+ * @lock: protects the device list
+ */
+struct iopf_queue {
+ struct workqueue_struct *wq;
+ iopf_queue_flush_t flush;
+ void*flush_arg;
+ struct list_headdevices;
+ struct mutexlock;
+};
+
+/**
+ * struct iopf_device_param - IO Page Fault data attached to a device
+ * @dev: the device that owns this param
+ * @queue: IOPF queue
+ * @queue_list: index into queue->devices
+ * @partial: faults that are part of a Page Request Group for which the last
+ * request hasn't been submitted yet.
+ * @busy: the param is being used
+ * @wq_head: signal a change to @busy
+ */
+struct iopf_device_param {
+ struct device *dev;
+ struct iopf_queue *queue;
+ struct list_headqueue_list;
+ struct list_headpartial;
+ boolbusy;
+ wait_queue_head_t wq_head;
+};
+
+struct iopf_fault {
+ struct iommu_fault fault;
+ struct list_headhead;
+};
+
+struct iopf_group {
+ struct iopf_fault last_fault;
+ struct list_headfaults;
+ struct work_struct work;
+ struct device *dev;
+};
+
+static int iopf_complete(struct device *dev, struct iopf_fault *iopf,
+enum iommu_page_response_code status)
+{
+ struct iommu_page_response resp = {
+ .version= IOMMU_PAGE_RESP_VERSION_1,
+ .pasid = iopf->fault.prm.pasid,
+ .grpid = iopf->fault.prm.grpid,
+ .code = status,
+ };
+
+ if (iopf->fault.prm.flags & IOMMU_FAULT_PAGE_REQUEST_PASID_VALID)
+ resp.flags = IOMMU_PAGE_RESP_PASID_VALID;
+
+ return iommu_page_response(dev, );
+}
+
+static enum iommu_page_response_code
+iopf_handle_single(struct iopf_fault
