Nouveau - Sep 2019 - [PATCH 08/11] drm/nouveau: tegra: Skip IOMMU initialization if already attached

Hi Thierry,

On 16/09/2019 16:04, Thierry Reding wrote:
> From: Thierry Reding <treding at nvidia.com>
> 
> If the GPU is already attached to an IOMMU, don't detach it and setup
an
> explicit IOMMU domain. Since Nouveau can now properly handle the case of
> the DMA API being backed by an IOMMU, just continue using the DMA API.
> 
> Signed-off-by: Thierry Reding <treding at nvidia.com>
> ---
>   .../drm/nouveau/nvkm/engine/device/tegra.c    | 19 +++++++------------
>   1 file changed, 7 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/nouveau/nvkm/engine/device/tegra.c
b/drivers/gpu/drm/nouveau/nvkm/engine/device/tegra.c
> index d0d52c1d4aee..fc652aaa41c7 100644
> --- a/drivers/gpu/drm/nouveau/nvkm/engine/device/tegra.c
> +++ b/drivers/gpu/drm/nouveau/nvkm/engine/device/tegra.c
> @@ -23,10 +23,6 @@
>   #ifdef CONFIG_NOUVEAU_PLATFORM_DRIVER
>   #include "priv.h"
>   
> -#if IS_ENABLED(CONFIG_ARM_DMA_USE_IOMMU)
> -#include <asm/dma-iommu.h>
> -#endif
> -
>   static int
>   nvkm_device_tegra_power_up(struct nvkm_device_tegra *tdev)
>   {
> @@ -109,14 +105,13 @@ nvkm_device_tegra_probe_iommu(struct
nvkm_device_tegra *tdev)
>   	unsigned long pgsize_bitmap;
>   	int ret;
>   
> -#if IS_ENABLED(CONFIG_ARM_DMA_USE_IOMMU)
> -	if (dev->archdata.mapping) {
> -		struct dma_iommu_mapping *mapping = to_dma_iommu_mapping(dev);
> -
> -		arm_iommu_detach_device(dev);
> -		arm_iommu_release_mapping(mapping);
> -	}
> -#endif
> +	/*
> +	 * Skip explicit IOMMU initialization if the GPU is already attached
> +	 * to an IOMMU domain. This can happen if the DMA API is backed by an
> +	 * IOMMU.
> +	 */
> +	if (iommu_get_domain_for_dev(dev))
> +		return;
Beware of "iommu.passthrough=1" - you could get a valid default domain
here yet still have direct/SWIOTLB DMA ops. I guess you probably want to 
double-check the domain type as well.

Robin.
>   
>   	if (!tdev->func->iommu_bit)
>   		return;
>

On Mon, Sep 16, 2019 at 04:29:18PM +0100, Robin Murphy
wrote:
> Hi Thierry,
> 
> On 16/09/2019 16:04, Thierry Reding wrote:
> > From: Thierry Reding <treding at nvidia.com>
> > 
> > If the GPU is already attached to an IOMMU, don't detach it and
setup an
> > explicit IOMMU domain. Since Nouveau can now properly handle the case
of
> > the DMA API being backed by an IOMMU, just continue using the DMA API.
> > 
> > Signed-off-by: Thierry Reding <treding at nvidia.com>
> > ---
> >   .../drm/nouveau/nvkm/engine/device/tegra.c    | 19
+++++++------------
> >   1 file changed, 7 insertions(+), 12 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/nouveau/nvkm/engine/device/tegra.c
b/drivers/gpu/drm/nouveau/nvkm/engine/device/tegra.c
> > index d0d52c1d4aee..fc652aaa41c7 100644
> > --- a/drivers/gpu/drm/nouveau/nvkm/engine/device/tegra.c
> > +++ b/drivers/gpu/drm/nouveau/nvkm/engine/device/tegra.c
> > @@ -23,10 +23,6 @@
> >   #ifdef CONFIG_NOUVEAU_PLATFORM_DRIVER
> >   #include "priv.h"
> > -#if IS_ENABLED(CONFIG_ARM_DMA_USE_IOMMU)
> > -#include <asm/dma-iommu.h>
> > -#endif
> > -
> >   static int
> >   nvkm_device_tegra_power_up(struct nvkm_device_tegra *tdev)
> >   {
> > @@ -109,14 +105,13 @@ nvkm_device_tegra_probe_iommu(struct
nvkm_device_tegra *tdev)
> >   	unsigned long pgsize_bitmap;
> >   	int ret;
> > -#if IS_ENABLED(CONFIG_ARM_DMA_USE_IOMMU)
> > -	if (dev->archdata.mapping) {
> > -		struct dma_iommu_mapping *mapping = to_dma_iommu_mapping(dev);
> > -
> > -		arm_iommu_detach_device(dev);
> > -		arm_iommu_release_mapping(mapping);
> > -	}
> > -#endif
> > +	/*
> > +	 * Skip explicit IOMMU initialization if the GPU is already attached
> > +	 * to an IOMMU domain. This can happen if the DMA API is backed by
an
> > +	 * IOMMU.
> > +	 */
> > +	if (iommu_get_domain_for_dev(dev))
> > +		return;
> 
> Beware of "iommu.passthrough=1" - you could get a valid default
domain here
> yet still have direct/SWIOTLB DMA ops. I guess you probably want to
> double-check the domain type as well.
Good point. An earlier version of this patch had an additional check for
IOMMU_DOMAIN_DMA, but then that failed on 32-bit ARM because there the
DMA API can also use IOMMU_DOMAIN_UNMANAGED type domains. Checking for
IOMMU_DOMAIN_IDENTIFY should be safe, though. That doesn't seem to
appear in arch/arm, arch/arm64 or drivers/iommu/dma-iommu.c.

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL:
<https://lists.freedesktop.org/archives/nouveau/attachments/20190916/1e2d355f/attachment.sig>

On 16/09/2019 16:57, Thierry Reding wrote:
> On Mon, Sep 16, 2019 at 04:29:18PM +0100, Robin Murphy wrote:
>> Hi Thierry,
>>
>> On 16/09/2019 16:04, Thierry Reding wrote:
>>> From: Thierry Reding <treding at nvidia.com>
>>>
>>> If the GPU is already attached to an IOMMU, don't detach it and
setup an
>>> explicit IOMMU domain. Since Nouveau can now properly handle the
case of
>>> the DMA API being backed by an IOMMU, just continue using the DMA
API.
>>>
>>> Signed-off-by: Thierry Reding <treding at nvidia.com>
>>> ---
>>>    .../drm/nouveau/nvkm/engine/device/tegra.c    | 19
+++++++------------
>>>    1 file changed, 7 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/nouveau/nvkm/engine/device/tegra.c
b/drivers/gpu/drm/nouveau/nvkm/engine/device/tegra.c
>>> index d0d52c1d4aee..fc652aaa41c7 100644
>>> --- a/drivers/gpu/drm/nouveau/nvkm/engine/device/tegra.c
>>> +++ b/drivers/gpu/drm/nouveau/nvkm/engine/device/tegra.c
>>> @@ -23,10 +23,6 @@
>>>    #ifdef CONFIG_NOUVEAU_PLATFORM_DRIVER
>>>    #include "priv.h"
>>> -#if IS_ENABLED(CONFIG_ARM_DMA_USE_IOMMU)
>>> -#include <asm/dma-iommu.h>
>>> -#endif
>>> -
>>>    static int
>>>    nvkm_device_tegra_power_up(struct nvkm_device_tegra *tdev)
>>>    {
>>> @@ -109,14 +105,13 @@ nvkm_device_tegra_probe_iommu(struct
nvkm_device_tegra *tdev)
>>>    	unsigned long pgsize_bitmap;
>>>    	int ret;
>>> -#if IS_ENABLED(CONFIG_ARM_DMA_USE_IOMMU)
>>> -	if (dev->archdata.mapping) {
>>> -		struct dma_iommu_mapping *mapping = to_dma_iommu_mapping(dev);
>>> -
>>> -		arm_iommu_detach_device(dev);
>>> -		arm_iommu_release_mapping(mapping);
>>> -	}
>>> -#endif
>>> +	/*
>>> +	 * Skip explicit IOMMU initialization if the GPU is already
attached
>>> +	 * to an IOMMU domain. This can happen if the DMA API is backed
by an
>>> +	 * IOMMU.
>>> +	 */
>>> +	if (iommu_get_domain_for_dev(dev))
>>> +		return;
>>
>> Beware of "iommu.passthrough=1" - you could get a valid
default domain here
>> yet still have direct/SWIOTLB DMA ops. I guess you probably want to
>> double-check the domain type as well.
> 
> Good point. An earlier version of this patch had an additional check for
> IOMMU_DOMAIN_DMA, but then that failed on 32-bit ARM because there the
> DMA API can also use IOMMU_DOMAIN_UNMANAGED type domains. Checking for
> IOMMU_DOMAIN_IDENTIFY should be safe, though. That doesn't seem to
> appear in arch/arm, arch/arm64 or drivers/iommu/dma-iommu.c.
Right, "domain && domain->type != IOMMU_DOMAIN_IDENTITY"
should be
sufficient to answer "is the DMA layer managing my address space for 
me?" unless and until some massive API change happens (which I certainly 
don't foresee).

Robin.