Re: [PATCH v7 04/24] iommu: Add a page fault handler

[Lu Baolu <baolu.lu@linux.intel.com>]

Hi Jean,

On 2020/11/11 21:57, Jean-Philippe Brucker wrote:
> Hi Baolu,
> 
> Thanks for the review. I'm only now reworking this and realized I've never
> sent a reply, sorry about that.
> 
> On Wed, May 20, 2020 at 02:42:21PM +0800, Lu Baolu wrote:
>> Hi Jean,
>>
>> On 2020/5/20 1:54, Jean-Philippe Brucker 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
>>> 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 succeeded, the IOMMU retries the access.
>>>
>>> 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>
> [...]
>>> +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;
>>> +
>>> +	down_read(&mm->mmap_sem);
>>> +
>>> +	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);
>>> +	status = ret & VM_FAULT_ERROR ? IOMMU_PAGE_RESP_INVALID :
>>
>> Do you mind telling why it's IOMMU_PAGE_RESP_INVALID but not
>> IOMMU_PAGE_RESP_FAILURE?
> 
> PAGE_RESP_FAILURE maps to PRI Response code "Response Failure" which
> indicates a catastrophic error and causes the function to disable PRI.
> Instead PAGE_RESP_INVALID maps to PRI Response code "Invalid request",
> which tells the function that the address is invalid and there is no point
> retrying this particular access.

Thanks for the explanation. I am also working on converting Intel VT-d
to use this framework (and the sva helpers). So far so good.

Best regards,
baolu