| Message ID | 20210108145217.2254447-1-jean-philippe@linaro.org |
|---|---|
| Headers | show |
| Series | iommu: I/O page faults for SMMUv3 | expand |
On 2021/1/8 下午10:52, Jean-Philippe Brucker wrote: > Add stall support to the SMMUv3, along with a common I/O Page Fault > handler. > > Changes since v8 [1]: > * Added patches 1 and 2 which aren't strictly related to IOPF but need to > be applied in order - 8 depends on 2 which depends on 1. Patch 2 moves > pasid-num-bits to a device property, following Robin's comment on v8. > * Patches 3-5 extract the IOPF feature from the SVA one, to support SVA > implementations that handle I/O page faults through the device driver > rather than the IOMMU driver [2] > * Use device properties for dma-can-stall, instead of a special fwspec > member. > * Dropped PRI support for now, since it doesn't seem to be available in > hardware and adds some complexity. > * Had to drop some Acks and Tested tags unfortunately, due to code > changes. > > As usual, you can get the latest SVA patches from > http://jpbrucker.net/git/linux sva/current > > [1] https://lore.kernel.org/linux-iommu/20201112125519.3987595-1-jean-philippe@linaro.org/ > [2] https://lore.kernel.org/linux-iommu/BY5PR12MB3764F5D07E8EC48327E39C86B3C60@BY5PR12MB3764.namprd12.prod.outlook.com/ > > Jean-Philippe Brucker (10): > iommu: Remove obsolete comment > iommu/arm-smmu-v3: Use device properties for pasid-num-bits > iommu: Separate IOMMU_DEV_FEAT_IOPF from IOMMU_DEV_FEAT_SVA > iommu/vt-d: Support IOMMU_DEV_FEAT_IOPF > uacce: Enable IOMMU_DEV_FEAT_IOPF > iommu: Add a page fault handler > iommu/arm-smmu-v3: Maintain a SID->device structure > dt-bindings: document stall property for IOMMU masters > ACPI/IORT: Enable stall support for platform devices > iommu/arm-smmu-v3: Add stall support for platform devices Thanks Jean I have tested on Hisilicon Kunpeng920 board. Tested-by: Zhangfei Gao <zhangfei.gao@linaro.org> Thanks
On Fri, 8 Jan 2021 15:52:14 +0100 Jean-Philippe Brucker <jean-philippe@linaro.org> wrote: > Some systems allow devices to handle I/O Page Faults in the core mm. For > example systems implementing the PCIe PRI extension or Arm SMMU stall > model. Infrastructure for reporting these recoverable page faults was > added to the IOMMU core by commit 0c830e6b3282 ("iommu: Introduce device > fault report API"). 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 Why "supposedly"? Do you mean "most commonly" > 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 succeeds, the IOMMU retries the access. For PRI that description is perhaps a bit missleading. IIRC the IOMMU will only retry when it gets a new ATS query. > > 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 <jean-philippe@linaro.org> One really minor inconsistency inline that made me look twice.. With or without that tided up FWIW. Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> ... > +/** > + * iopf_queue_add_device - Add producer to the fault queue > + * @queue: IOPF queue > + * @dev: device to add > + * > + * Return: 0 on success and <0 on error. > + */ > +int iopf_queue_add_device(struct iopf_queue *queue, struct device *dev) > +{ > + int ret = -EBUSY; > + struct iopf_device_param *iopf_param; > + struct dev_iommu *param = dev->iommu; > + > + if (!param) > + return -ENODEV; > + > + iopf_param = kzalloc(sizeof(*iopf_param), GFP_KERNEL); > + if (!iopf_param) > + return -ENOMEM; > + > + INIT_LIST_HEAD(&iopf_param->partial); > + iopf_param->queue = queue; > + iopf_param->dev = dev; > + > + mutex_lock(&queue->lock); > + mutex_lock(¶m->lock); > + if (!param->iopf_param) { > + list_add(&iopf_param->queue_list, &queue->devices); > + param->iopf_param = iopf_param; > + ret = 0; > + } > + mutex_unlock(¶m->lock); > + mutex_unlock(&queue->lock); > + > + if (ret) > + kfree(iopf_param); > + > + return ret; > +} > +EXPORT_SYMBOL_GPL(iopf_queue_add_device); > + > +/** > + * iopf_queue_remove_device - Remove producer from fault queue > + * @queue: IOPF queue > + * @dev: device to remove > + * > + * Caller makes sure that no more faults are reported for this device. > + * > + * Return: 0 on success and <0 on error. > + */ > +int iopf_queue_remove_device(struct iopf_queue *queue, struct device *dev) > +{ > + int ret = 0; I'm not that keen that the logic of ret is basically the opposite of that in the previous function. There we had it init to error then set to good, here we do the opposite. Not that important which but right now it just made me do a double take whilst reading. > + struct iopf_fault *iopf, *next; > + struct iopf_device_param *iopf_param; > + struct dev_iommu *param = dev->iommu; > + > + if (!param || !queue) > + return -EINVAL; > + > + mutex_lock(&queue->lock); > + mutex_lock(¶m->lock); > + iopf_param = param->iopf_param; > + if (iopf_param && iopf_param->queue == queue) { > + list_del(&iopf_param->queue_list); > + param->iopf_param = NULL; > + } else { > + ret = -EINVAL; > + } > + mutex_unlock(¶m->lock); > + mutex_unlock(&queue->lock); > + if (ret) > + return ret; > + > + /* Just in case some faults are still stuck */ > + list_for_each_entry_safe(iopf, next, &iopf_param->partial, list) > + kfree(iopf); > + > + kfree(iopf_param); > + > + return 0; > +} > +EXPORT_SYMBOL_GPL(iopf_queue_remove_device); > + ...
On Tue, Jan 19, 2021 at 01:38:19PM +0000, Jonathan Cameron wrote: > On Fri, 8 Jan 2021 15:52:14 +0100 > Jean-Philippe Brucker <jean-philippe@linaro.org> wrote: > > > Some systems allow devices to handle I/O Page Faults in the core mm. For > > example systems implementing the PCIe PRI extension or Arm SMMU stall > > model. Infrastructure for reporting these recoverable page faults was > > added to the IOMMU core by commit 0c830e6b3282 ("iommu: Introduce device > > fault report API"). 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 > > Why "supposedly"? Do you mean "most commonly" > > > 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 succeeds, the IOMMU retries the access. > > For PRI that description is perhaps a bit missleading. IIRC the IOMMU > will only retry when it gets a new ATS query. > > > > > 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 <jean-philippe@linaro.org> > > One really minor inconsistency inline that made me look twice.. > With or without that tided up FWIW. > > Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> Thanks! I'll fix these up and resend Jean