Message ID | 20231115030226.16700-7-baolu.lu@linux.intel.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | iommu: Prepare to deliver page faults to user space | expand |
On 2023/11/15 11:02, Lu Baolu wrote: > The individual iommu driver reports the iommu page faults by calling > iommu_report_device_fault(), where a pre-registered device fault handler > is called to route the fault to another fault handler installed on the > corresponding iommu domain. > > The pre-registered device fault handler is static and won't be dynamic > as the fault handler is eventually per iommu domain. Replace calling > device fault handler with iommu_queue_iopf(). > > After this replacement, the registering and unregistering fault handler > interfaces are not needed anywhere. Remove the interfaces and the related > data structures to avoid dead code. > > Convert cookie parameter of iommu_queue_iopf() into a device pointer that > is really passed. > > Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com> > Reviewed-by: Kevin Tian <kevin.tian@intel.com> > Reviewed-by: Jason Gunthorpe <jgg@nvidia.com> > Tested-by: Yan Zhao <yan.y.zhao@intel.com> > --- > include/linux/iommu.h | 23 ------ > drivers/iommu/iommu-sva.h | 4 +- > .../iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c | 13 +--- > drivers/iommu/intel/iommu.c | 24 ++---- > drivers/iommu/io-pgfault.c | 6 +- > drivers/iommu/iommu.c | 76 +------------------ > 6 files changed, 13 insertions(+), 133 deletions(-) > > diff --git a/include/linux/iommu.h b/include/linux/iommu.h > index 108ab50da1ad..a45d92cc31ec 100644 > --- a/include/linux/iommu.h > +++ b/include/linux/iommu.h > @@ -128,7 +128,6 @@ struct iommu_page_response { > > typedef int (*iommu_fault_handler_t)(struct iommu_domain *, > struct device *, unsigned long, int, void *); > -typedef int (*iommu_dev_fault_handler_t)(struct iommu_fault *, void *); > > struct iommu_domain_geometry { > dma_addr_t aperture_start; /* First address that can be mapped */ > @@ -589,8 +588,6 @@ struct iommu_fault_event { > > /** > * struct iommu_fault_param - per-device IOMMU fault data > - * @handler: Callback function to handle IOMMU faults at device level > - * @data: handler private data > * @lock: protect pending faults list > * @dev: the device that owns this param > * @queue: IOPF queue > @@ -600,8 +597,6 @@ struct iommu_fault_event { > * @faults: holds the pending faults which needs response > */ > struct iommu_fault_param { > - iommu_dev_fault_handler_t handler; > - void *data; > struct mutex lock; > > struct device *dev; > @@ -724,11 +719,6 @@ extern int iommu_group_for_each_dev(struct iommu_group *group, void *data, > extern struct iommu_group *iommu_group_get(struct device *dev); > extern struct iommu_group *iommu_group_ref_get(struct iommu_group *group); > extern void iommu_group_put(struct iommu_group *group); > -extern int iommu_register_device_fault_handler(struct device *dev, > - iommu_dev_fault_handler_t handler, > - void *data); > - > -extern int iommu_unregister_device_fault_handler(struct device *dev); > > extern int iommu_report_device_fault(struct device *dev, > struct iommu_fault_event *evt); > @@ -1137,19 +1127,6 @@ static inline void iommu_group_put(struct iommu_group *group) > { > } > > -static inline > -int iommu_register_device_fault_handler(struct device *dev, > - iommu_dev_fault_handler_t handler, > - void *data) > -{ > - return -ENODEV; > -} > - > -static inline int iommu_unregister_device_fault_handler(struct device *dev) > -{ > - return 0; > -} > - > static inline > int iommu_report_device_fault(struct device *dev, struct iommu_fault_event *evt) > { > diff --git a/drivers/iommu/iommu-sva.h b/drivers/iommu/iommu-sva.h > index 54946b5a7caf..de7819c796ce 100644 > --- a/drivers/iommu/iommu-sva.h > +++ b/drivers/iommu/iommu-sva.h > @@ -13,7 +13,7 @@ struct iommu_fault; > struct iopf_queue; > > #ifdef CONFIG_IOMMU_SVA > -int iommu_queue_iopf(struct iommu_fault *fault, void *cookie); > +int iommu_queue_iopf(struct iommu_fault *fault, struct device *dev); > > int iopf_queue_add_device(struct iopf_queue *queue, struct device *dev); > int iopf_queue_remove_device(struct iopf_queue *queue, > @@ -26,7 +26,7 @@ enum iommu_page_response_code > iommu_sva_handle_iopf(struct iommu_fault *fault, void *data); > > #else /* CONFIG_IOMMU_SVA */ > -static inline int iommu_queue_iopf(struct iommu_fault *fault, void *cookie) > +static inline int iommu_queue_iopf(struct iommu_fault *fault, struct device *dev) > { > return -ENODEV; > } > diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c > index 353248ab18e7..84c9554144cb 100644 > --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c > +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c > @@ -480,7 +480,6 @@ bool arm_smmu_master_sva_enabled(struct arm_smmu_master *master) > > static int arm_smmu_master_sva_enable_iopf(struct arm_smmu_master *master) > { > - int ret; > struct device *dev = master->dev; > > /* > @@ -493,16 +492,7 @@ static int arm_smmu_master_sva_enable_iopf(struct arm_smmu_master *master) > if (!master->iopf_enabled) > return -EINVAL; > > - ret = iopf_queue_add_device(master->smmu->evtq.iopf, dev); > - if (ret) > - return ret; > - > - ret = iommu_register_device_fault_handler(dev, iommu_queue_iopf, dev); > - if (ret) { > - iopf_queue_remove_device(master->smmu->evtq.iopf, dev); > - return ret; > - } > - return 0; > + return iopf_queue_add_device(master->smmu->evtq.iopf, dev); > } > > static void arm_smmu_master_sva_disable_iopf(struct arm_smmu_master *master) > @@ -512,7 +502,6 @@ static void arm_smmu_master_sva_disable_iopf(struct arm_smmu_master *master) > if (!master->iopf_enabled) > return; > > - iommu_unregister_device_fault_handler(dev); > iopf_queue_remove_device(master->smmu->evtq.iopf, dev); > } > > diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c > index 3531b956556c..cbe65827730d 100644 > --- a/drivers/iommu/intel/iommu.c > +++ b/drivers/iommu/intel/iommu.c > @@ -4616,23 +4616,15 @@ static int intel_iommu_enable_iopf(struct device *dev) > if (ret) > return ret; > > - ret = iommu_register_device_fault_handler(dev, iommu_queue_iopf, dev); > - if (ret) > - goto iopf_remove_device; > - > ret = pci_enable_pri(pdev, PRQ_DEPTH); > - if (ret) > - goto iopf_unregister_handler; > + if (ret) { > + iopf_queue_remove_device(iommu->iopf_queue, dev); > + return ret; > + } > + > info->pri_enabled = 1; > > return 0; > - > -iopf_unregister_handler: > - iommu_unregister_device_fault_handler(dev); > -iopf_remove_device: > - iopf_queue_remove_device(iommu->iopf_queue, dev); > - > - return ret; > } > > static int intel_iommu_disable_iopf(struct device *dev) > @@ -4655,11 +4647,9 @@ static int intel_iommu_disable_iopf(struct device *dev) > info->pri_enabled = 0; > > /* > - * With PRI disabled and outstanding PRQs drained, unregistering > - * fault handler and removing device from iopf queue should never > - * fail. > + * With PRI disabled and outstanding PRQs drained, removing device > + * from iopf queue should never fail. > */ > - WARN_ON(iommu_unregister_device_fault_handler(dev)); > WARN_ON(iopf_queue_remove_device(iommu->iopf_queue, dev)); > > return 0; > diff --git a/drivers/iommu/io-pgfault.c b/drivers/iommu/io-pgfault.c > index b1cf28055525..31832aeacdba 100644 > --- a/drivers/iommu/io-pgfault.c > +++ b/drivers/iommu/io-pgfault.c > @@ -87,7 +87,7 @@ static void iopf_handler(struct work_struct *work) > /** > * iommu_queue_iopf - IO Page Fault handler > * @fault: fault event > - * @cookie: struct device, passed to iommu_register_device_fault_handler. > + * @dev: struct device. > * > * Add a fault to the device workqueue, to be handled by mm. > * > @@ -124,14 +124,12 @@ static void iopf_handler(struct work_struct *work) > * > * Return: 0 on success and <0 on error. > */ > -int iommu_queue_iopf(struct iommu_fault *fault, void *cookie) > +int iommu_queue_iopf(struct iommu_fault *fault, struct device *dev) > { > int ret; > struct iopf_group *group; > struct iopf_fault *iopf, *next; > struct iommu_fault_param *iopf_param; > - > - struct device *dev = cookie; > struct dev_iommu *param = dev->iommu; > > lockdep_assert_held(¶m->lock); > diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c > index 9c9eacfa6761..0c6700b6659a 100644 > --- a/drivers/iommu/iommu.c > +++ b/drivers/iommu/iommu.c > @@ -1301,76 +1301,6 @@ void iommu_group_put(struct iommu_group *group) > } > EXPORT_SYMBOL_GPL(iommu_group_put); > > -/** > - * 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, this handler gets called with the > - * fault event and data as argument. The handler should return 0 on success. If > - * the fault is recoverable (IOMMU_FAULT_PAGE_REQ), the consumer should also > - * complete the fault by calling iommu_page_response() with one of the following > - * response code: > - * - IOMMU_PAGE_RESP_SUCCESS: retry the translation > - * - IOMMU_PAGE_RESP_INVALID: terminate the fault > - * - IOMMU_PAGE_RESP_FAILURE: terminate the fault and stop reporting > - * page faults if possible. > - * > - * Return 0 if the fault handler was installed successfully, or an error. > - */ > -int iommu_register_device_fault_handler(struct device *dev, > - iommu_dev_fault_handler_t handler, > - void *data) > -{ > - struct dev_iommu *param = dev->iommu; > - int ret = 0; > - > - if (!param || !param->fault_param) > - return -EINVAL; > - > - mutex_lock(¶m->lock); > - /* Only allow one fault handler registered for each device */ > - if (param->fault_param->handler) { > - ret = -EBUSY; > - goto done_unlock; > - } > - > - param->fault_param->handler = handler; > - param->fault_param->data = data; > - > -done_unlock: > - mutex_unlock(¶m->lock); > - > - return ret; > -} > -EXPORT_SYMBOL_GPL(iommu_register_device_fault_handler); > - > -/** > - * iommu_unregister_device_fault_handler() - Unregister the device fault handler > - * @dev: the device > - * > - * Remove the device fault handler installed with > - * iommu_register_device_fault_handler(). > - * > - * Return 0 on success, or an error. > - */ > -int iommu_unregister_device_fault_handler(struct device *dev) > -{ > - struct dev_iommu *param = dev->iommu; > - > - if (!param || !param->fault_param) > - return -EINVAL; > - > - mutex_lock(¶m->lock); > - param->fault_param->handler = NULL; > - param->fault_param->data = NULL; > - mutex_unlock(¶m->lock); > - > - return 0; > -} > -EXPORT_SYMBOL_GPL(iommu_unregister_device_fault_handler); > - > /** > * iommu_report_device_fault() - Report fault event to device driver > * @dev: the device > @@ -1395,10 +1325,6 @@ int iommu_report_device_fault(struct device *dev, struct iommu_fault_event *evt) > /* we only report device fault if there is a handler registered */ > mutex_lock(¶m->lock); > fparam = param->fault_param; > - if (!fparam || !fparam->handler) { should it still check the fparam? > - ret = -EINVAL; > - goto done_unlock; > - } > > if (evt->fault.type == IOMMU_FAULT_PAGE_REQ && > (evt->fault.prm.flags & IOMMU_FAULT_PAGE_REQUEST_LAST_PAGE)) { > @@ -1413,7 +1339,7 @@ int iommu_report_device_fault(struct device *dev, struct iommu_fault_event *evt) > mutex_unlock(&fparam->lock); > } > > - ret = fparam->handler(&evt->fault, fparam->data); > + ret = iommu_queue_iopf(&evt->fault, dev); > if (ret && evt_pending) { > mutex_lock(&fparam->lock); > list_del(&evt_pending->list);
On 2023/12/4 20:36, Yi Liu wrote: >> /** >> * iommu_report_device_fault() - Report fault event to device driver >> * @dev: the device >> @@ -1395,10 +1325,6 @@ int iommu_report_device_fault(struct device >> *dev, struct iommu_fault_event *evt) >> /* we only report device fault if there is a handler registered */ >> mutex_lock(¶m->lock); >> fparam = param->fault_param; >> - if (!fparam || !fparam->handler) { > > should it still check the fparam? Yes. Best regards, baolu
diff --git a/include/linux/iommu.h b/include/linux/iommu.h index 108ab50da1ad..a45d92cc31ec 100644 --- a/include/linux/iommu.h +++ b/include/linux/iommu.h @@ -128,7 +128,6 @@ struct iommu_page_response { typedef int (*iommu_fault_handler_t)(struct iommu_domain *, struct device *, unsigned long, int, void *); -typedef int (*iommu_dev_fault_handler_t)(struct iommu_fault *, void *); struct iommu_domain_geometry { dma_addr_t aperture_start; /* First address that can be mapped */ @@ -589,8 +588,6 @@ struct iommu_fault_event { /** * struct iommu_fault_param - per-device IOMMU fault data - * @handler: Callback function to handle IOMMU faults at device level - * @data: handler private data * @lock: protect pending faults list * @dev: the device that owns this param * @queue: IOPF queue @@ -600,8 +597,6 @@ struct iommu_fault_event { * @faults: holds the pending faults which needs response */ struct iommu_fault_param { - iommu_dev_fault_handler_t handler; - void *data; struct mutex lock; struct device *dev; @@ -724,11 +719,6 @@ extern int iommu_group_for_each_dev(struct iommu_group *group, void *data, extern struct iommu_group *iommu_group_get(struct device *dev); extern struct iommu_group *iommu_group_ref_get(struct iommu_group *group); extern void iommu_group_put(struct iommu_group *group); -extern int iommu_register_device_fault_handler(struct device *dev, - iommu_dev_fault_handler_t handler, - void *data); - -extern int iommu_unregister_device_fault_handler(struct device *dev); extern int iommu_report_device_fault(struct device *dev, struct iommu_fault_event *evt); @@ -1137,19 +1127,6 @@ static inline void iommu_group_put(struct iommu_group *group) { } -static inline -int iommu_register_device_fault_handler(struct device *dev, - iommu_dev_fault_handler_t handler, - void *data) -{ - return -ENODEV; -} - -static inline int iommu_unregister_device_fault_handler(struct device *dev) -{ - return 0; -} - static inline int iommu_report_device_fault(struct device *dev, struct iommu_fault_event *evt) { diff --git a/drivers/iommu/iommu-sva.h b/drivers/iommu/iommu-sva.h index 54946b5a7caf..de7819c796ce 100644 --- a/drivers/iommu/iommu-sva.h +++ b/drivers/iommu/iommu-sva.h @@ -13,7 +13,7 @@ struct iommu_fault; struct iopf_queue; #ifdef CONFIG_IOMMU_SVA -int iommu_queue_iopf(struct iommu_fault *fault, void *cookie); +int iommu_queue_iopf(struct iommu_fault *fault, struct device *dev); int iopf_queue_add_device(struct iopf_queue *queue, struct device *dev); int iopf_queue_remove_device(struct iopf_queue *queue, @@ -26,7 +26,7 @@ enum iommu_page_response_code iommu_sva_handle_iopf(struct iommu_fault *fault, void *data); #else /* CONFIG_IOMMU_SVA */ -static inline int iommu_queue_iopf(struct iommu_fault *fault, void *cookie) +static inline int iommu_queue_iopf(struct iommu_fault *fault, struct device *dev) { return -ENODEV; } diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c index 353248ab18e7..84c9554144cb 100644 --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c @@ -480,7 +480,6 @@ bool arm_smmu_master_sva_enabled(struct arm_smmu_master *master) static int arm_smmu_master_sva_enable_iopf(struct arm_smmu_master *master) { - int ret; struct device *dev = master->dev; /* @@ -493,16 +492,7 @@ static int arm_smmu_master_sva_enable_iopf(struct arm_smmu_master *master) if (!master->iopf_enabled) return -EINVAL; - ret = iopf_queue_add_device(master->smmu->evtq.iopf, dev); - if (ret) - return ret; - - ret = iommu_register_device_fault_handler(dev, iommu_queue_iopf, dev); - if (ret) { - iopf_queue_remove_device(master->smmu->evtq.iopf, dev); - return ret; - } - return 0; + return iopf_queue_add_device(master->smmu->evtq.iopf, dev); } static void arm_smmu_master_sva_disable_iopf(struct arm_smmu_master *master) @@ -512,7 +502,6 @@ static void arm_smmu_master_sva_disable_iopf(struct arm_smmu_master *master) if (!master->iopf_enabled) return; - iommu_unregister_device_fault_handler(dev); iopf_queue_remove_device(master->smmu->evtq.iopf, dev); } diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c index 3531b956556c..cbe65827730d 100644 --- a/drivers/iommu/intel/iommu.c +++ b/drivers/iommu/intel/iommu.c @@ -4616,23 +4616,15 @@ static int intel_iommu_enable_iopf(struct device *dev) if (ret) return ret; - ret = iommu_register_device_fault_handler(dev, iommu_queue_iopf, dev); - if (ret) - goto iopf_remove_device; - ret = pci_enable_pri(pdev, PRQ_DEPTH); - if (ret) - goto iopf_unregister_handler; + if (ret) { + iopf_queue_remove_device(iommu->iopf_queue, dev); + return ret; + } + info->pri_enabled = 1; return 0; - -iopf_unregister_handler: - iommu_unregister_device_fault_handler(dev); -iopf_remove_device: - iopf_queue_remove_device(iommu->iopf_queue, dev); - - return ret; } static int intel_iommu_disable_iopf(struct device *dev) @@ -4655,11 +4647,9 @@ static int intel_iommu_disable_iopf(struct device *dev) info->pri_enabled = 0; /* - * With PRI disabled and outstanding PRQs drained, unregistering - * fault handler and removing device from iopf queue should never - * fail. + * With PRI disabled and outstanding PRQs drained, removing device + * from iopf queue should never fail. */ - WARN_ON(iommu_unregister_device_fault_handler(dev)); WARN_ON(iopf_queue_remove_device(iommu->iopf_queue, dev)); return 0; diff --git a/drivers/iommu/io-pgfault.c b/drivers/iommu/io-pgfault.c index b1cf28055525..31832aeacdba 100644 --- a/drivers/iommu/io-pgfault.c +++ b/drivers/iommu/io-pgfault.c @@ -87,7 +87,7 @@ static void iopf_handler(struct work_struct *work) /** * iommu_queue_iopf - IO Page Fault handler * @fault: fault event - * @cookie: struct device, passed to iommu_register_device_fault_handler. + * @dev: struct device. * * Add a fault to the device workqueue, to be handled by mm. * @@ -124,14 +124,12 @@ static void iopf_handler(struct work_struct *work) * * Return: 0 on success and <0 on error. */ -int iommu_queue_iopf(struct iommu_fault *fault, void *cookie) +int iommu_queue_iopf(struct iommu_fault *fault, struct device *dev) { int ret; struct iopf_group *group; struct iopf_fault *iopf, *next; struct iommu_fault_param *iopf_param; - - struct device *dev = cookie; struct dev_iommu *param = dev->iommu; lockdep_assert_held(¶m->lock); diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c index 9c9eacfa6761..0c6700b6659a 100644 --- a/drivers/iommu/iommu.c +++ b/drivers/iommu/iommu.c @@ -1301,76 +1301,6 @@ void iommu_group_put(struct iommu_group *group) } EXPORT_SYMBOL_GPL(iommu_group_put); -/** - * 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, this handler gets called with the - * fault event and data as argument. The handler should return 0 on success. If - * the fault is recoverable (IOMMU_FAULT_PAGE_REQ), the consumer should also - * complete the fault by calling iommu_page_response() with one of the following - * response code: - * - IOMMU_PAGE_RESP_SUCCESS: retry the translation - * - IOMMU_PAGE_RESP_INVALID: terminate the fault - * - IOMMU_PAGE_RESP_FAILURE: terminate the fault and stop reporting - * page faults if possible. - * - * Return 0 if the fault handler was installed successfully, or an error. - */ -int iommu_register_device_fault_handler(struct device *dev, - iommu_dev_fault_handler_t handler, - void *data) -{ - struct dev_iommu *param = dev->iommu; - int ret = 0; - - if (!param || !param->fault_param) - return -EINVAL; - - mutex_lock(¶m->lock); - /* Only allow one fault handler registered for each device */ - if (param->fault_param->handler) { - ret = -EBUSY; - goto done_unlock; - } - - param->fault_param->handler = handler; - param->fault_param->data = data; - -done_unlock: - mutex_unlock(¶m->lock); - - return ret; -} -EXPORT_SYMBOL_GPL(iommu_register_device_fault_handler); - -/** - * iommu_unregister_device_fault_handler() - Unregister the device fault handler - * @dev: the device - * - * Remove the device fault handler installed with - * iommu_register_device_fault_handler(). - * - * Return 0 on success, or an error. - */ -int iommu_unregister_device_fault_handler(struct device *dev) -{ - struct dev_iommu *param = dev->iommu; - - if (!param || !param->fault_param) - return -EINVAL; - - mutex_lock(¶m->lock); - param->fault_param->handler = NULL; - param->fault_param->data = NULL; - mutex_unlock(¶m->lock); - - return 0; -} -EXPORT_SYMBOL_GPL(iommu_unregister_device_fault_handler); - /** * iommu_report_device_fault() - Report fault event to device driver * @dev: the device @@ -1395,10 +1325,6 @@ int iommu_report_device_fault(struct device *dev, struct iommu_fault_event *evt) /* we only report device fault if there is a handler registered */ mutex_lock(¶m->lock); fparam = param->fault_param; - if (!fparam || !fparam->handler) { - ret = -EINVAL; - goto done_unlock; - } if (evt->fault.type == IOMMU_FAULT_PAGE_REQ && (evt->fault.prm.flags & IOMMU_FAULT_PAGE_REQUEST_LAST_PAGE)) { @@ -1413,7 +1339,7 @@ int iommu_report_device_fault(struct device *dev, struct iommu_fault_event *evt) mutex_unlock(&fparam->lock); } - ret = fparam->handler(&evt->fault, fparam->data); + ret = iommu_queue_iopf(&evt->fault, dev); if (ret && evt_pending) { mutex_lock(&fparam->lock); list_del(&evt_pending->list);