| Message ID | 4-v1-21cc72fcfb22+a7a-iommu_all_defdom_jgg@nvidia.com |
|---|---|
| State | New |
| Headers | show |
| Series | iommu: Make default_domain's mandatory | expand |
On 2023-05-01 19:02, Jason Gunthorpe wrote: > It is not clear what this is actually doing, most likely this is IDENTITY > behavior, but I think there is a chance it is BLOCKING given how the PAMU > stuff is oddly used. Logically it has to be identity, since there are no DMA ops interacting with this driver (it's not the TCE IOMMU of arch/powerpc/kernel/iommu.c), so any device using a kernel driver rather than VFIO must be using dma-direct and thus require an identity mapping. At this point I finally got sufficiently fed up of this driver always being the mystery weirdo and tracked down an old QorIQ reference manual, and now I have about half an hours' worth of understanding of how the PAMU actually works. Based on that, what setup_liodns() is doing is indeed setting up identity for everything initially. It also becomes apparent that it's never supported giving a PCI device back to its regular driver after using vfio-pci, since an attach/detach cycle will then leave the PPAACE invalid and thus DMA blocked. Oh well, that's been broken for 10 years; nobody cares. Call it an identity domain and move on. Thanks, Robin. > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> > --- > drivers/iommu/fsl_pamu_domain.c | 29 ++++++++++++++++++++++++++--- > 1 file changed, 26 insertions(+), 3 deletions(-) > > diff --git a/drivers/iommu/fsl_pamu_domain.c b/drivers/iommu/fsl_pamu_domain.c > index bce37229709965..4c65f1adfe7511 100644 > --- a/drivers/iommu/fsl_pamu_domain.c > +++ b/drivers/iommu/fsl_pamu_domain.c > @@ -283,15 +283,28 @@ static int fsl_pamu_attach_device(struct iommu_domain *domain, > return ret; > } > > -static void fsl_pamu_set_platform_dma(struct device *dev) > +/* > + * FIXME: This seems to turn off the iommu HW but it is not obvious what state > + * it leaves the HW in. This is probably really a BLOCKING or IDENTITY domain. > + * For now this ensures that the old detach_dev behavior functions about the > + * same as it always did, and we turn off the IOMMU whenever the UNMANAGED > + * domain is detached. > + */ > +static int fsl_pamu_platform_attach(struct iommu_domain *platform_domain, > + struct device *dev) > { > struct iommu_domain *domain = iommu_get_domain_for_dev(dev); > - struct fsl_dma_domain *dma_domain = to_fsl_dma_domain(domain); > + struct fsl_dma_domain *dma_domain; > const u32 *prop; > int len; > struct pci_dev *pdev = NULL; > struct pci_controller *pci_ctl; > > + if (domain == platform_domain || !domain) > + return 0; > + > + dma_domain = to_fsl_dma_domain(domain); > + > /* > * Use LIODN of the PCI controller while detaching a > * PCI device. > @@ -312,8 +325,18 @@ static void fsl_pamu_set_platform_dma(struct device *dev) > detach_device(dev, dma_domain); > else > pr_debug("missing fsl,liodn property at %pOF\n", dev->of_node); > + return 0; > } > > +static struct iommu_domain_ops fsl_pamu_platform_ops = { > + .attach_dev = fsl_pamu_platform_attach, > +}; > + > +static struct iommu_domain fsl_pamu_platform_domain = { > + .type = IOMMU_DOMAIN_PLATFORM, > + .ops = &fsl_pamu_platform_ops, > +}; > + > /* Set the domain stash attribute */ > int fsl_pamu_configure_l1_stash(struct iommu_domain *domain, u32 cpu) > { > @@ -448,11 +471,11 @@ static struct iommu_device *fsl_pamu_probe_device(struct device *dev) > } > > static const struct iommu_ops fsl_pamu_ops = { > + .default_domain = &fsl_pamu_platform_domain, > .capable = fsl_pamu_capable, > .domain_alloc = fsl_pamu_domain_alloc, > .probe_device = fsl_pamu_probe_device, > .device_group = fsl_pamu_device_group, > - .set_platform_dma_ops = fsl_pamu_set_platform_dma, > .default_domain_ops = &(const struct iommu_domain_ops) { > .attach_dev = fsl_pamu_attach_device, > .iova_to_phys = fsl_pamu_iova_to_phys,
On Wed, May 03, 2023 at 11:57:59AM +0100, Robin Murphy wrote: > On 2023-05-01 19:02, Jason Gunthorpe wrote: > > It is not clear what this is actually doing, most likely this is IDENTITY > > behavior, but I think there is a chance it is BLOCKING given how the PAMU > > stuff is oddly used. > > Logically it has to be identity, since there are no DMA ops interacting with > this driver (it's not the TCE IOMMU of arch/powerpc/kernel/iommu.c), so any > device using a kernel driver rather than VFIO must be using dma-direct and > thus require an identity mapping. Yes, that is the usual argument.. FSL just seems so odd because it has drivers manipulating the iommu so it wasn't clear to me if those drivers were the only drivers hooked here or not. > At this point I finally got sufficiently fed up of this driver always being > the mystery weirdo and tracked down an old QorIQ reference manual, and now I > have about half an hours' worth of understanding of how the PAMU actually > works. I'm going to post my other FSL patches for the group setup then before you forget ;) > Based on that, what setup_liodns() is doing is indeed setting up identity > for everything initially. It also becomes apparent that it's never supported > giving a PCI device back to its regular driver after using vfio-pci, since > an attach/detach cycle will then leave the PPAACE invalid and thus DMA > blocked. Oh well, that's been broken for 10 years; nobody cares. Call it an > identity domain and move on. Okay, easy to do thanks for checking the manual Jason
diff --git a/drivers/iommu/fsl_pamu_domain.c b/drivers/iommu/fsl_pamu_domain.c index bce37229709965..4c65f1adfe7511 100644 --- a/drivers/iommu/fsl_pamu_domain.c +++ b/drivers/iommu/fsl_pamu_domain.c @@ -283,15 +283,28 @@ static int fsl_pamu_attach_device(struct iommu_domain *domain, return ret; } -static void fsl_pamu_set_platform_dma(struct device *dev) +/* + * FIXME: This seems to turn off the iommu HW but it is not obvious what state + * it leaves the HW in. This is probably really a BLOCKING or IDENTITY domain. + * For now this ensures that the old detach_dev behavior functions about the + * same as it always did, and we turn off the IOMMU whenever the UNMANAGED + * domain is detached. + */ +static int fsl_pamu_platform_attach(struct iommu_domain *platform_domain, + struct device *dev) { struct iommu_domain *domain = iommu_get_domain_for_dev(dev); - struct fsl_dma_domain *dma_domain = to_fsl_dma_domain(domain); + struct fsl_dma_domain *dma_domain; const u32 *prop; int len; struct pci_dev *pdev = NULL; struct pci_controller *pci_ctl; + if (domain == platform_domain || !domain) + return 0; + + dma_domain = to_fsl_dma_domain(domain); + /* * Use LIODN of the PCI controller while detaching a * PCI device. @@ -312,8 +325,18 @@ static void fsl_pamu_set_platform_dma(struct device *dev) detach_device(dev, dma_domain); else pr_debug("missing fsl,liodn property at %pOF\n", dev->of_node); + return 0; } +static struct iommu_domain_ops fsl_pamu_platform_ops = { + .attach_dev = fsl_pamu_platform_attach, +}; + +static struct iommu_domain fsl_pamu_platform_domain = { + .type = IOMMU_DOMAIN_PLATFORM, + .ops = &fsl_pamu_platform_ops, +}; + /* Set the domain stash attribute */ int fsl_pamu_configure_l1_stash(struct iommu_domain *domain, u32 cpu) { @@ -448,11 +471,11 @@ static struct iommu_device *fsl_pamu_probe_device(struct device *dev) } static const struct iommu_ops fsl_pamu_ops = { + .default_domain = &fsl_pamu_platform_domain, .capable = fsl_pamu_capable, .domain_alloc = fsl_pamu_domain_alloc, .probe_device = fsl_pamu_probe_device, .device_group = fsl_pamu_device_group, - .set_platform_dma_ops = fsl_pamu_set_platform_dma, .default_domain_ops = &(const struct iommu_domain_ops) { .attach_dev = fsl_pamu_attach_device, .iova_to_phys = fsl_pamu_iova_to_phys,
It is not clear what this is actually doing, most likely this is IDENTITY behavior, but I think there is a chance it is BLOCKING given how the PAMU stuff is oddly used. Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> --- drivers/iommu/fsl_pamu_domain.c | 29 ++++++++++++++++++++++++++--- 1 file changed, 26 insertions(+), 3 deletions(-)