On Wed, Jun 28, 2023 at 10:00:56AM +0800, Baolu Lu wrote: > > If the driver created a SVA domain then the op should point to some > > generic 'handle sva fault' function. There shouldn't be weird SVA > > stuff in the core code. > > > > The weird SVA stuff is really just a generic per-device workqueue > > dispatcher, so if we think that is valuable then it should be > > integrated into the iommu_domain (domain->ops->use_iopf_workqueue = > > true for instance). Then it could route the fault through the > > workqueue and still invoke domain->ops->iopf_handler. > > > > The word "SVA" should not appear in any of this. > > Yes. We should make it generic. The domain->use_iopf_workqueue flag > denotes that the page faults of a fault group should be put together and > then be handled and responded in a workqueue. Otherwise, the page fault > is dispatched to domain->iopf_handler directly.
It might be better to have iopf_handler and iopf_handler_work function pointers to distinguish to two cases. > > Not sure what iommu_register_device_fault_handler() has to do with all > > of this.. Setting up the dev_iommu stuff to allow for the workqueue > > should happen dynamically during domain attach, ideally in the core > > code before calling to the driver. > > There are two pointers under struct dev_iommu for fault handling. > > /** > * struct dev_iommu - Collection of per-device IOMMU data > * > * @fault_param: IOMMU detected device fault reporting data > * @iopf_param: I/O Page Fault queue and data > > [...] > > struct dev_iommu { > struct mutex lock; > struct iommu_fault_param *fault_param; > struct iopf_device_param *iopf_param; > > My understanding is that @fault_param is a place holder for generic > things, while @iopf_param is workqueue specific. Well, lets look struct iommu_fault_param { iommu_dev_fault_handler_t handler; void *data; These two make no sense now. handler is always iommu_queue_iopf. Given our domain centric design we want the function pointer in the domain, not in the device. So delete it. struct list_head faults; struct mutex lock; Queue of unhandled/unacked faults? Seems sort of reasonable > @iopf_param could be allocated on demand. (perhaps renaming it to a more > meaningful one?) It happens before a domain with use_iopf_workqueue flag > set attaches to a device. iopf_param keeps alive until device_release. Yes Do this for the iommu_fault_param as well, in fact, probably just put the two things together in one allocation and allocate if we attach a PRI using domain. I don't think we need to micro optimze further.. Jason _______________________________________________ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization