| Message ID | 9-v2-8d1dc464eac9+10f-iommu_all_defdom_jgg@nvidia.com |
|---|---|
| State | New |
| Headers | show |
| Series | iommu: Make default_domain's mandatory | expand |
On 2023-05-16 01:00, Jason Gunthorpe wrote: > Robin was able to check the documentation and what fsl_pamu has > historically called detach_dev() is really putting the IOMMU into an > IDENTITY mode. Unfortunately it was the other way around - it's the call to fsl_setup_liodns() from fsl_pamu_probe() which leaves everything in bypass by default (the PAACE_ATM_NO_XLATE part, IIRC), whereas the detach_device() call here ends up disabling the given device's LIODN altogether. There doesn't appear to have ever been any code anywhere for putting things *back* into bypass after using a VFIO domain, so as-is these default domains would probably break all DMA :( Thanks, Robin. > Move to the new core support for ARM_DMA_USE_IOMMU by defining > ops->identity_domain. This is a ppc driver without any dma_ops, so ensure > the identity translation is the default domain. > > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> > --- > drivers/iommu/fsl_pamu_domain.c | 32 +++++++++++++++++++++++++++++--- > 1 file changed, 29 insertions(+), 3 deletions(-) > > diff --git a/drivers/iommu/fsl_pamu_domain.c b/drivers/iommu/fsl_pamu_domain.c > index bce37229709965..ca4f5ebf028783 100644 > --- a/drivers/iommu/fsl_pamu_domain.c > +++ b/drivers/iommu/fsl_pamu_domain.c > @@ -283,15 +283,21 @@ static int fsl_pamu_attach_device(struct iommu_domain *domain, > return ret; > } > > -static void fsl_pamu_set_platform_dma(struct device *dev) > +static int fsl_pamu_identity_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 +318,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_identity_ops = { > + .attach_dev = fsl_pamu_identity_attach, > +}; > + > +static struct iommu_domain fsl_pamu_identity_domain = { > + .type = IOMMU_DOMAIN_IDENTITY, > + .ops = &fsl_pamu_identity_ops, > +}; > + > /* Set the domain stash attribute */ > int fsl_pamu_configure_l1_stash(struct iommu_domain *domain, u32 cpu) > { > @@ -447,12 +463,22 @@ static struct iommu_device *fsl_pamu_probe_device(struct device *dev) > return &pamu_iommu; > } > > +static int fsl_pamu_def_domain_type(struct device *dev) > +{ > + /* > + * This platform does not use dma_ops at all so the normally the iommu > + * must be in identity mode > + */ > + return IOMMU_DOMAIN_IDENTITY; > +} > + > static const struct iommu_ops fsl_pamu_ops = { > + .identity_domain = &fsl_pamu_identity_domain, > + .def_domain_type = &fsl_pamu_def_domain_type, > .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 Thu, Jun 01, 2023 at 08:37:45PM +0100, Robin Murphy wrote: > On 2023-05-16 01:00, Jason Gunthorpe wrote: > > Robin was able to check the documentation and what fsl_pamu has > > historically called detach_dev() is really putting the IOMMU into an > > IDENTITY mode. > > Unfortunately it was the other way around - it's the call to > fsl_setup_liodns() from fsl_pamu_probe() which leaves everything in bypass > by default (the PAACE_ATM_NO_XLATE part, IIRC), whereas the detach_device() > call here ends up disabling the given device's LIODN altogether Er, I see.. Let me think about it, you convinced me to change it from PLATFORM, so maybe we should go back to that if it is all wonky. > There doesn't appear to have ever been any code anywhere for putting > things *back* into bypass after using a VFIO domain, so as-is these > default domains would probably break all DMA :( Sounds like it just never worked right. ie going to VFIO mode was always a one way trip and you can't go back to a kernel driver. I don't think this patch makes it worse because we call the identity attach_dev in all the same places we called detach_dev in the first place. We add an extra call at the start of time, but that call is NOP'd by this: > > if (domain == platform_domain || !domain) > > + return 0; > > + (bah, and the variable name needs updating too) Honestly, I don't really want to fix FSL since it seems abandoned, so either this patch or going back to PLATFORM seems like the best option. Jason
On 2023-06-01 20:46, Jason Gunthorpe wrote: > On Thu, Jun 01, 2023 at 08:37:45PM +0100, Robin Murphy wrote: >> On 2023-05-16 01:00, Jason Gunthorpe wrote: >>> Robin was able to check the documentation and what fsl_pamu has >>> historically called detach_dev() is really putting the IOMMU into an >>> IDENTITY mode. >> >> Unfortunately it was the other way around - it's the call to >> fsl_setup_liodns() from fsl_pamu_probe() which leaves everything in bypass >> by default (the PAACE_ATM_NO_XLATE part, IIRC), whereas the detach_device() >> call here ends up disabling the given device's LIODN altogether > > Er, I see.. Let me think about it, you convinced me to change it from > PLATFORM, so maybe we should go back to that if it is all wonky. FWIW I was thinking more along the lines of a token nominal identity domain where attach does nothing at all... >> There doesn't appear to have ever been any code anywhere for putting >> things *back* into bypass after using a VFIO domain, so as-is these >> default domains would probably break all DMA :( > > Sounds like it just never worked right. > > ie going to VFIO mode was always a one way trip and you can't go back > to a kernel driver. ...on the assumption that doing so wouldn't really be any less broken than it always has been :) Thanks, Robin. > I don't think this patch makes it worse because we call the identity > attach_dev in all the same places we called detach_dev in the first > place. > > We add an extra call at the start of time, but that call is NOP'd > by this: > >>> if (domain == platform_domain || !domain) >>> + return 0; >>> + > > (bah, and the variable name needs updating too) > > Honestly, I don't really want to fix FSL since it seems abandoned, so > either this patch or going back to PLATFORM seems like the best option. > > Jason
On Thu, Jun 01, 2023 at 08:53:41PM +0100, Robin Murphy wrote: > On 2023-06-01 20:46, Jason Gunthorpe wrote: > > On Thu, Jun 01, 2023 at 08:37:45PM +0100, Robin Murphy wrote: > > > On 2023-05-16 01:00, Jason Gunthorpe wrote: > > > > Robin was able to check the documentation and what fsl_pamu has > > > > historically called detach_dev() is really putting the IOMMU into an > > > > IDENTITY mode. > > > > > > Unfortunately it was the other way around - it's the call to > > > fsl_setup_liodns() from fsl_pamu_probe() which leaves everything in bypass > > > by default (the PAACE_ATM_NO_XLATE part, IIRC), whereas the detach_device() > > > call here ends up disabling the given device's LIODN altogether > > > > Er, I see.. Let me think about it, you convinced me to change it from > > PLATFORM, so maybe we should go back to that if it is all wonky. > > FWIW I was thinking more along the lines of a token nominal identity domain > where attach does nothing at all... I'm worried that would create security problems for VFIO.. At least the driver currently wipes out the VFIO installed translation which sounds like the right thing to do. So, I think my first patch was right, we should label this PLATFORM/PRIVATE/whatever and just leave it as is with some comments explaining this thread. Based on the same rational as my prior email that we should label things correctly and this detach_dev is doing BLOCKING. Jason
diff --git a/drivers/iommu/fsl_pamu_domain.c b/drivers/iommu/fsl_pamu_domain.c index bce37229709965..ca4f5ebf028783 100644 --- a/drivers/iommu/fsl_pamu_domain.c +++ b/drivers/iommu/fsl_pamu_domain.c @@ -283,15 +283,21 @@ static int fsl_pamu_attach_device(struct iommu_domain *domain, return ret; } -static void fsl_pamu_set_platform_dma(struct device *dev) +static int fsl_pamu_identity_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 +318,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_identity_ops = { + .attach_dev = fsl_pamu_identity_attach, +}; + +static struct iommu_domain fsl_pamu_identity_domain = { + .type = IOMMU_DOMAIN_IDENTITY, + .ops = &fsl_pamu_identity_ops, +}; + /* Set the domain stash attribute */ int fsl_pamu_configure_l1_stash(struct iommu_domain *domain, u32 cpu) { @@ -447,12 +463,22 @@ static struct iommu_device *fsl_pamu_probe_device(struct device *dev) return &pamu_iommu; } +static int fsl_pamu_def_domain_type(struct device *dev) +{ + /* + * This platform does not use dma_ops at all so the normally the iommu + * must be in identity mode + */ + return IOMMU_DOMAIN_IDENTITY; +} + static const struct iommu_ops fsl_pamu_ops = { + .identity_domain = &fsl_pamu_identity_domain, + .def_domain_type = &fsl_pamu_def_domain_type, .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,
Robin was able to check the documentation and what fsl_pamu has historically called detach_dev() is really putting the IOMMU into an IDENTITY mode. Move to the new core support for ARM_DMA_USE_IOMMU by defining ops->identity_domain. This is a ppc driver without any dma_ops, so ensure the identity translation is the default domain. Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> --- drivers/iommu/fsl_pamu_domain.c | 32 +++++++++++++++++++++++++++++--- 1 file changed, 29 insertions(+), 3 deletions(-)