| Message ID | 20230727054837.147050-13-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 Thu, Jul 27, 2023 at 01:48:37PM +0800, Lu Baolu wrote: > To avoid open code everywhere. > > Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com> > --- > include/linux/iommu.h | 11 ++++++++++- > drivers/iommu/iommu.c | 20 ++++++++++++++++++-- > 2 files changed, 28 insertions(+), 3 deletions(-) Seems like overkill at this point.. Also, I think this is probably upside down. We want to create the domains as fault enabled in the first place. A fault enabled domain should never be attached to something that cannot support faults. It should also not support changing the fault handler while it exists. Thus at the creation point would be the time to supply the fault handler as part of requesting faulting. Taking an existing domain and making it faulting enabled is going to be really messy in all the corner cases. My advice (and Robin will probably hate me), is to define a new op: struct domain_alloc_paging_args { struct fault_handler *fault_handler; void *fault_data }; struct iommu_domain *domain_alloc_paging2(struct device *dev, struct domain_alloc_paging_args *args) The point would be to leave the majority of drivers using the simplified, core assisted, domain_alloc_paging() interface and they just don't have to touch any of this stuff at all. Obviously if handler is given then the domain will be initialized as faulting. Jason
On 2023/8/11 3:18, Jason Gunthorpe wrote: > On Thu, Jul 27, 2023 at 01:48:37PM +0800, Lu Baolu wrote: >> To avoid open code everywhere. >> >> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com> >> --- >> include/linux/iommu.h | 11 ++++++++++- >> drivers/iommu/iommu.c | 20 ++++++++++++++++++-- >> 2 files changed, 28 insertions(+), 3 deletions(-) > > Seems like overkill at this point.. > > Also, I think this is probably upside down. > > We want to create the domains as fault enabled in the first place. > > A fault enabled domain should never be attached to something that > cannot support faults. It should also not support changing the fault > handler while it exists. > > Thus at the creation point would be the time to supply the fault handler > as part of requesting faulting. > > Taking an existing domain and making it faulting enabled is going to > be really messy in all the corner cases. Yes. Agreed. > > My advice (and Robin will probably hate me), is to define a new op: > > struct domain_alloc_paging_args { > struct fault_handler *fault_handler; > void *fault_data > }; > > struct iommu_domain *domain_alloc_paging2(struct device *dev, struct > domain_alloc_paging_args *args) > > The point would be to leave the majority of drivers using the > simplified, core assisted, domain_alloc_paging() interface and they > just don't have to touch any of this stuff at all. > > Obviously if handler is given then the domain will be initialized as > faulting. Perhaps we also need an internal helper for iommu drivers to check the iopf capability of the domain. Best regards, baolu
On Fri, Aug 11, 2023 at 10:40:15AM +0800, Baolu Lu wrote: > On 2023/8/11 3:18, Jason Gunthorpe wrote: > > On Thu, Jul 27, 2023 at 01:48:37PM +0800, Lu Baolu wrote: > > > To avoid open code everywhere. > > > > > > Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com> > > > --- > > > include/linux/iommu.h | 11 ++++++++++- > > > drivers/iommu/iommu.c | 20 ++++++++++++++++++-- > > > 2 files changed, 28 insertions(+), 3 deletions(-) > > > > Seems like overkill at this point.. > > > > Also, I think this is probably upside down. > > > > We want to create the domains as fault enabled in the first place. > > > > A fault enabled domain should never be attached to something that > > cannot support faults. It should also not support changing the fault > > handler while it exists. > > > > Thus at the creation point would be the time to supply the fault handler > > as part of requesting faulting. > > > > Taking an existing domain and making it faulting enabled is going to > > be really messy in all the corner cases. > > Yes. Agreed. > > > > > My advice (and Robin will probably hate me), is to define a new op: > > > > struct domain_alloc_paging_args { > > struct fault_handler *fault_handler; > > void *fault_data > > }; > > > > struct iommu_domain *domain_alloc_paging2(struct device *dev, struct > > domain_alloc_paging_args *args) > > > > The point would be to leave the majority of drivers using the > > simplified, core assisted, domain_alloc_paging() interface and they > > just don't have to touch any of this stuff at all. > > > > Obviously if handler is given then the domain will be initialized as > > faulting. > > Perhaps we also need an internal helper for iommu drivers to check the > iopf capability of the domain. Yeah, maybe. I've been mulling over this for a a bit here Robin suggested to wrap everything in a arg to domain_alloc and build a giant super multiplexor I don't really like that because it makes it quite complicated for the driver and multiplexor APIs are rarely good. So for simple drivers I like the 'domain_alloc_paging' as the only op they implement and it is obviously simple and hard to implement wrong. Most drivers would do this. We also need a: struct iommu_domain *domain_alloc_sva(struct device *dev, struct mm_struct *mm) So SVA can be fully setup at allocation time. SVA doesn't have any legal permutations so it can be kept simple. Then we need something to bundle: - Dirty tracking yes/no - The iommufd user space blob - Fault handling yes/no For complex drivers. So maybe we should just have a 3rd option // I'm a complex driver and many people checked that I implemented // this right: struct domain_alloc_args { struct device *dev; unsigned int flags; // For requesting dirty tracking // alloc_domain_user interface struct iommu_domain *parent; void *user_data; size_t user_len; // Faulting struct fault_handler *fault_handler; void *fault_data; }; struct iommu_domain *domain_alloc(struct domain_alloc_args *args); ? IDK, multiplexor APIs are rarely good, but maybe this is the right direction? Jason
diff --git a/include/linux/iommu.h b/include/linux/iommu.h index d725500344a4..4e0b75befefc 100644 --- a/include/linux/iommu.h +++ b/include/linux/iommu.h @@ -132,6 +132,7 @@ struct iommu_page_response { typedef int (*iommu_fault_handler_t)(struct iommu_domain *, struct device *, unsigned long, int, void *); +typedef int (*iommu_iopf_handler_t)(struct iopf_group *group); struct iommu_domain_geometry { dma_addr_t aperture_start; /* First address that can be mapped */ @@ -181,7 +182,7 @@ struct iommu_domain { unsigned long pgsize_bitmap; /* Bitmap of page sizes in use */ struct iommu_domain_geometry geometry; struct iommu_dma_cookie *iova_cookie; - int (*iopf_handler)(struct iopf_group *group); + iommu_iopf_handler_t iopf_handler; void *fault_data; union { struct { @@ -581,6 +582,8 @@ extern ssize_t iommu_map_sg(struct iommu_domain *domain, unsigned long iova, extern phys_addr_t iommu_iova_to_phys(struct iommu_domain *domain, dma_addr_t iova); extern void iommu_set_fault_handler(struct iommu_domain *domain, iommu_fault_handler_t handler, void *token); +void iommu_domain_set_iopf_handler(struct iommu_domain *domain, + iommu_iopf_handler_t handler, void *data); extern void iommu_get_resv_regions(struct device *dev, struct list_head *list); extern void iommu_put_resv_regions(struct device *dev, struct list_head *list); @@ -904,6 +907,12 @@ static inline void iommu_set_fault_handler(struct iommu_domain *domain, { } +static inline void iommu_domain_set_iopf_handler(struct iommu_domain *domain, + iommu_iopf_handler_t handler, + void *data) +{ +} + static inline void iommu_get_resv_regions(struct device *dev, struct list_head *list) { diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c index 9d964d16d0ad..2254b0ef91fb 100644 --- a/drivers/iommu/iommu.c +++ b/drivers/iommu/iommu.c @@ -1839,6 +1839,23 @@ void iommu_set_fault_handler(struct iommu_domain *domain, } EXPORT_SYMBOL_GPL(iommu_set_fault_handler); +/** + * iommu_domain_set_iopf_handler() - set io page fault handler for a domain + * @domain: iommu domain + * @handler: fault handler + * @data: user data, will be passed back to the fault handler + * + * This function should be used by iommu domain users which want to be notified + * whenever an IOMMU I/O page fault happens. + */ +void iommu_domain_set_iopf_handler(struct iommu_domain *domain, + iommu_iopf_handler_t handler, void *data) +{ + domain->iopf_handler = handler; + domain->fault_data = data; +} +EXPORT_SYMBOL_GPL(iommu_domain_set_iopf_handler); + static struct iommu_domain *__iommu_domain_alloc(const struct bus_type *bus, unsigned type) { @@ -3328,8 +3345,7 @@ struct iommu_domain *iommu_sva_domain_alloc(struct device *dev, domain->type = IOMMU_DOMAIN_SVA; mmgrab(mm); domain->mm = mm; - domain->iopf_handler = iommu_sva_handle_iopf_group; - domain->fault_data = mm; + iommu_domain_set_iopf_handler(domain, iommu_sva_handle_iopf_group, mm); return domain; }
To avoid open code everywhere. Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com> --- include/linux/iommu.h | 11 ++++++++++- drivers/iommu/iommu.c | 20 ++++++++++++++++++-- 2 files changed, 28 insertions(+), 3 deletions(-)