diff mbox series

[v2,09/25] iommu/fsl_pamu: Implement an IDENTITY domain

Message ID 9-v2-8d1dc464eac9+10f-iommu_all_defdom_jgg@nvidia.com (mailing list archive)
State New, archived
Headers show
Series iommu: Make default_domain's mandatory | expand

Commit Message

Jason Gunthorpe May 16, 2023, midnight UTC
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(-)

Comments

Robin Murphy June 1, 2023, 7:37 p.m. UTC | #1
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,
Jason Gunthorpe June 1, 2023, 7:46 p.m. UTC | #2
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
Robin Murphy June 1, 2023, 7:53 p.m. UTC | #3
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
Jason Gunthorpe June 1, 2023, 8:17 p.m. UTC | #4
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 mbox series

Patch

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,