OFS / linux-dfl-backport

Backport version of the linux-dfl (Device Feature List) kernel driver for FPGA devices. This is an out-of-tree driver, designed to be built, packaged, and installed as a stand-alone set of driver modules.
GNU General Public License v2.0
3 stars 11 forks source link

fpga: dfl-pci-sva: fix getting pasid with Linux 6.8 #113

Closed pcolberg closed 9 months ago

pcolberg commented 9 months ago

Commit 2396046d75d3 ("iommu: Add mm_get_enqcmd_pasid() helper function") added a wrapper for pasid, which was moved to a private data structure in commit 092edaddb660 ("iommu: Support mm PASID 1:n with sva domains").

pcolberg commented 9 months ago

This follows on https://github.com/OFS/linux-dfl-backport/pull/110, which was automatically closed when the intel-test/* branches were deleted.

michael-adler commented 9 months ago

I bet we should be calling iommu_sva_get_pasid() and not mm_get_enqcmd_pasid()

pcolberg commented 9 months ago

Thanks @michael-adler, that is even better. https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/iommu/iommu-sva.c?h=v6.8-rc6#n171

u32 iommu_sva_get_pasid(struct iommu_sva *handle)
{
    struct iommu_domain *domain = handle->domain;

    return mm_get_enqcmd_pasid(domain->mm);
}
EXPORT_SYMBOL_GPL(iommu_sva_get_pasid);
pcolberg commented 9 months ago

@michael-adler Currently pci_info() is invoked before checking whether handle is valid, which means iommu_sva_get_pasid() cannot be used. Can pci_info() be moved as follows?

Unlike mm_get_enqcmd_pasid(), iommu_sva_get_pasid() is not an inline function, so the pasid should probably be cached as well.

--- a/drivers/fpga/dfl-pci-sva.c
+++ b/drivers/fpga/dfl-pci-sva.c
@@ -177,21 +177,21 @@ static long ioctl_sva_bind_dev(struct dfl_sva_dev *dev, struct iommu_sva **sva_h
        if (!current->mm)
                return -EINVAL;

-       if (*sva_handle_p)
-               return mm_get_enqcmd_pasid(current->mm);
+       if ((handle = *sva_handle_p))
+               return iommu_sva_get_pasid(handle);

        handle = iommu_sva_bind_device(&dev->pdev->dev, current->mm);
-       pci_info(dev->pdev, "%s: pid %d, bind sva_handle %p, pasid = %d\n",
-                __func__, task_pid_nr(current),
-                handle, mm_get_enqcmd_pasid(current->mm));
-
        if (!handle)
                return -ENODEV;
        if (IS_ERR(handle))
                return PTR_ERR(handle);

+       pci_info(dev->pdev, "%s: pid %d, bind sva_handle %p, pasid = %d\n",
+                __func__, task_pid_nr(current),
+                handle, iommu_sva_get_pasid(handle));
+
        *sva_handle_p = handle;
-       return mm_get_enqcmd_pasid(current->mm);
+       return iommu_sva_get_pasid(handle);
 }

 static long ioctl_sva_unbind_dev(struct dfl_sva_dev *dev, struct iommu_sva **sva_handle_p)
michael-adler commented 9 months ago

Your update looks good to me. Thanks for figuring it out.

No need to cache the pasid. This function is usually called once per opened device.