Messages in this thread Patch in this message | | | Date | Mon, 21 Mar 2022 11:39:16 +0000 | From | Jean-Philippe Brucker <> | Subject | Re: [PATCH RFC 10/11] iommu: Make IOPF handling framework generic |
| |
On Sun, Mar 20, 2022 at 02:40:29PM +0800, Lu Baolu wrote: > The existing IOPF handling framework only handles the I/O page faults for > SVA. Ginven that we are able to link iommu domain with each I/O page fault, > we can now make the I/O page fault handling framework more general for > more types of page faults. > > Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com> > --- > include/linux/iommu.h | 4 +++ > drivers/iommu/io-pgfault.c | 67 ++++++----------------------------- > drivers/iommu/iommu-sva-lib.c | 59 ++++++++++++++++++++++++++++++ > 3 files changed, 73 insertions(+), 57 deletions(-) > > diff --git a/include/linux/iommu.h b/include/linux/iommu.h > index 803e7b07605e..11c65a7bed88 100644 > --- a/include/linux/iommu.h > +++ b/include/linux/iommu.h > @@ -50,6 +50,8 @@ struct iommu_dma_cookie; > 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 *); > +typedef enum iommu_page_response_code (*iommu_domain_iopf_handler_t) > + (struct iommu_fault *, void *); > > struct iommu_domain_geometry { > dma_addr_t aperture_start; /* First address that can be mapped */ > @@ -101,6 +103,8 @@ struct iommu_domain { > struct iommu_domain_geometry geometry; > struct iommu_dma_cookie *iova_cookie; > struct mm_struct *sva_cookie; > + iommu_domain_iopf_handler_t fault_handler; > + void *fault_data; > }; > > static inline bool iommu_is_dma_domain(struct iommu_domain *domain) > diff --git a/drivers/iommu/io-pgfault.c b/drivers/iommu/io-pgfault.c > index 1df8c1dcae77..dad0e40cd8d2 100644 > --- a/drivers/iommu/io-pgfault.c > +++ b/drivers/iommu/io-pgfault.c > @@ -69,62 +69,6 @@ static int iopf_complete_group(struct device *dev, struct iopf_fault *iopf, > return iommu_page_response(dev, &resp); > } > > -static enum iommu_page_response_code > -iopf_handle_single(struct iopf_fault *iopf) > -{ > - vm_fault_t ret; > - struct mm_struct *mm; > - struct vm_area_struct *vma; > - unsigned int access_flags = 0; > - unsigned int fault_flags = FAULT_FLAG_REMOTE; > - struct iommu_fault_page_request *prm = &iopf->fault.prm; > - enum iommu_page_response_code status = IOMMU_PAGE_RESP_INVALID; > - > - if (!(prm->flags & IOMMU_FAULT_PAGE_REQUEST_PASID_VALID)) > - return status; > - > - mm = iommu_sva_find(prm->pasid); > - if (IS_ERR_OR_NULL(mm)) > - return status; > - > - mmap_read_lock(mm); > - > - vma = find_extend_vma(mm, prm->addr); > - if (!vma) > - /* Unmapped area */ > - goto out_put_mm; > - > - if (prm->perm & IOMMU_FAULT_PERM_READ) > - access_flags |= VM_READ; > - > - if (prm->perm & IOMMU_FAULT_PERM_WRITE) { > - access_flags |= VM_WRITE; > - fault_flags |= FAULT_FLAG_WRITE; > - } > - > - if (prm->perm & IOMMU_FAULT_PERM_EXEC) { > - access_flags |= VM_EXEC; > - fault_flags |= FAULT_FLAG_INSTRUCTION; > - } > - > - if (!(prm->perm & IOMMU_FAULT_PERM_PRIV)) > - fault_flags |= FAULT_FLAG_USER; > - > - if (access_flags & ~vma->vm_flags) > - /* Access fault */ > - goto out_put_mm; > - > - ret = handle_mm_fault(vma, prm->addr, fault_flags, NULL); > - status = ret & VM_FAULT_ERROR ? IOMMU_PAGE_RESP_INVALID : > - IOMMU_PAGE_RESP_SUCCESS; > - > -out_put_mm: > - mmap_read_unlock(mm); > - mmput(mm); > - > - return status; > -} > - > static void iopf_handle_group(struct work_struct *work) > { > struct iopf_group *group; > @@ -134,12 +78,21 @@ static void iopf_handle_group(struct work_struct *work) > group = container_of(work, struct iopf_group, work); > > list_for_each_entry_safe(iopf, next, &group->faults, list) { > + struct iommu_domain *domain; > + > + domain = iommu_get_domain_for_dev_pasid(group->dev, > + iopf->fault.prm.pasid);
Do we have a guarantee that the domain is not freed while we handle the fault? We could prevent unbind() while there are pending faults on this bond. But a refcount on SVA domains could defer freeing, and would also help with keeping the semantics where bind() returns a single refcounted bond for any {dev, mm}.
Given that this path is full of circular locking pitfalls, and to keep the fault handler efficient (well, at least not make it worse), we should probably keep a getter like iommu_sva_find() that does not require locking.
> + > + if (!domain || !domain->fault_handler) > + status = IOMMU_PAGE_RESP_INVALID; > + > /* > * For the moment, errors are sticky: don't handle subsequent > * faults in the group if there is an error. > */ > if (status == IOMMU_PAGE_RESP_SUCCESS) > - status = iopf_handle_single(iopf); > + status = domain->fault_handler(&iopf->fault, > + domain->fault_data);
If we make this a direct call and only use a light getter for the PASID->mm lookup we don't need to look at the domain at all. Or are you planning to add external fault handlers?
> > if (!(iopf->fault.prm.flags & > IOMMU_FAULT_PAGE_REQUEST_LAST_PAGE)) > diff --git a/drivers/iommu/iommu-sva-lib.c b/drivers/iommu/iommu-sva-lib.c > index 47cf98e661ff..01fa8096bd02 100644 > --- a/drivers/iommu/iommu-sva-lib.c > +++ b/drivers/iommu/iommu-sva-lib.c > @@ -87,6 +87,63 @@ static struct iommu_domain *iommu_sva_domain_alloc(struct device *dev) > return domain; > } > > +static enum iommu_page_response_code > +iommu_sva_handle_iopf(struct iommu_fault *fault, void *data) > +{ > + vm_fault_t ret; > + struct mm_struct *mm; > + struct vm_area_struct *vma; > + unsigned int access_flags = 0; > + struct iommu_domain *domain = data; > + unsigned int fault_flags = FAULT_FLAG_REMOTE; > + struct iommu_fault_page_request *prm = &fault->prm; > + enum iommu_page_response_code status = IOMMU_PAGE_RESP_INVALID; > + > + if (!(prm->flags & IOMMU_FAULT_PAGE_REQUEST_PASID_VALID)) > + return status; > + > + mm = domain->sva_cookie; > + if (IS_ERR_OR_NULL(mm)) > + return status; > + > + mmap_read_lock(mm); > + > + vma = find_extend_vma(mm, prm->addr); > + if (!vma) > + /* Unmapped area */ > + goto out_put_mm; > + > + if (prm->perm & IOMMU_FAULT_PERM_READ) > + access_flags |= VM_READ; > + > + if (prm->perm & IOMMU_FAULT_PERM_WRITE) { > + access_flags |= VM_WRITE; > + fault_flags |= FAULT_FLAG_WRITE; > + } > + > + if (prm->perm & IOMMU_FAULT_PERM_EXEC) { > + access_flags |= VM_EXEC; > + fault_flags |= FAULT_FLAG_INSTRUCTION; > + } > + > + if (!(prm->perm & IOMMU_FAULT_PERM_PRIV)) > + fault_flags |= FAULT_FLAG_USER; > + > + if (access_flags & ~vma->vm_flags) > + /* Access fault */ > + goto out_put_mm; > + > + ret = handle_mm_fault(vma, prm->addr, fault_flags, NULL); > + status = ret & VM_FAULT_ERROR ? IOMMU_PAGE_RESP_INVALID : > + IOMMU_PAGE_RESP_SUCCESS; > + > +out_put_mm: > + mmap_read_unlock(mm); > + mmput(mm);
mmget_not_zero() is missing since iommu_sva_find() is gone. I'm guessing we still need it in case the process dies
Thanks, Jean
> + > + return status; > +} > + > /** > * iommu_sva_bind_device() - Bind a process address space to a device > * @dev: the device > @@ -124,6 +181,8 @@ iommu_sva_bind_device(struct device *dev, struct mm_struct *mm, void *drvdata) > goto out; > } > domain->sva_cookie = mm; > + domain->fault_handler = iommu_sva_handle_iopf; > + domain->fault_data = domain; > > ret = iommu_attach_device_pasid(domain, dev, mm->pasid); > if (ret) > -- > 2.25.1 >
| |