fix DMA ops layering violations in vmwgfx

fix DMA ops layering violations in vmwgfx

[Christoph Hellwig]

Hi Thomas,

vmwgfx has been doing some odd checks based on DMA ops which rely
on deep DMA mapping layer internals, and I think the changes in
Linux 4.21 finally broke most of these implicit assumptions.

The real fix is in patch 3, but I think the others are important
to make it clear what is actually going on.

[PATCH 1/4] drm/vmwgfx: remove CONFIG_X86 ifdefs

[Christoph Hellwig]

The driver depends on CONFIG_X86 so these are dead code.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/gpu/drm/vmwgfx/vmwgfx_drv.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
index 25afb1d594e3..69e325b2d954 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
@@ -565,7 +565,6 @@ static int vmw_dma_select_mode(struct vmw_private *dev_priv)
 		[vmw_dma_alloc_coherent] = "Using coherent TTM pages.",
 		[vmw_dma_map_populate] = "Keeping DMA mappings.",
 		[vmw_dma_map_bind] = "Giving up DMA mappings early."};
-#ifdef CONFIG_X86
 	const struct dma_map_ops *dma_ops = get_dma_ops(dev_priv->dev->dev);
 
 #ifdef CONFIG_INTEL_IOMMU
@@ -607,11 +606,6 @@ static int vmw_dma_select_mode(struct vmw_private *dev_priv)
 	if (dev_priv->map_mode == vmw_dma_alloc_coherent)
 		return -EINVAL;
 #endif
-
-#else /* CONFIG_X86 */
-	dev_priv->map_mode = vmw_dma_map_populate;
-#endif /* CONFIG_X86 */
-
 	DRM_INFO("DMA map mode: %s\n", names[dev_priv->map_mode]);
 
 	return 0;
-- 
2.20.1

[PATCH 2/4] drm/vmwgfx: remove CONFIG_INTEL_IOMMU ifdefs

[Christoph Hellwig]

intel_iommu_enabled is defined as always false for !CONFIG_INTEL_IOMMU,
so remove the ifdefs around it.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/gpu/drm/vmwgfx/vmwgfx_drv.c | 18 ------------------
 1 file changed, 18 deletions(-)

diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
index 69e325b2d954..236052ad233c 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
@@ -567,12 +567,10 @@ static int vmw_dma_select_mode(struct vmw_private *dev_priv)
 		[vmw_dma_map_bind] = "Giving up DMA mappings early."};
 	const struct dma_map_ops *dma_ops = get_dma_ops(dev_priv->dev->dev);
 
-#ifdef CONFIG_INTEL_IOMMU
 	if (intel_iommu_enabled) {
 		dev_priv->map_mode = vmw_dma_map_populate;
 		goto out_fixup;
 	}
-#endif
 
 	if (!(vmw_force_iommu || vmw_force_coherent)) {
 		dev_priv->map_mode = vmw_dma_phys;
@@ -589,9 +587,7 @@ static int vmw_dma_select_mode(struct vmw_private *dev_priv)
 		dev_priv->map_mode = vmw_dma_map_populate;
 #endif
 
-#ifdef CONFIG_INTEL_IOMMU
 out_fixup:
-#endif
 	if (dev_priv->map_mode == vmw_dma_map_populate &&
 	    vmw_restrict_iommu)
 		dev_priv->map_mode = vmw_dma_map_bind;
@@ -599,13 +595,6 @@ static int vmw_dma_select_mode(struct vmw_private *dev_priv)
 	if (vmw_force_coherent)
 		dev_priv->map_mode = vmw_dma_alloc_coherent;
 
-#if !defined(CONFIG_SWIOTLB) && !defined(CONFIG_INTEL_IOMMU)
-	/*
-	 * No coherent page pool
-	 */
-	if (dev_priv->map_mode == vmw_dma_alloc_coherent)
-		return -EINVAL;
-#endif
 	DRM_INFO("DMA map mode: %s\n", names[dev_priv->map_mode]);
 
 	return 0;
@@ -619,7 +608,6 @@ static int vmw_dma_select_mode(struct vmw_private *dev_priv)
  * With 32-bit we can only handle 32 bit PFNs. Optionally set that
  * restriction also for 64-bit systems.
  */
-#ifdef CONFIG_INTEL_IOMMU
 static int vmw_dma_masks(struct vmw_private *dev_priv)
 {
 	struct drm_device *dev = dev_priv->dev;
@@ -631,12 +619,6 @@ static int vmw_dma_masks(struct vmw_private *dev_priv)
 	}
 	return 0;
 }
-#else
-static int vmw_dma_masks(struct vmw_private *dev_priv)
-{
-	return 0;
-}
-#endif
 
 static int vmw_driver_load(struct drm_device *dev, unsigned long chipset)
 {
-- 
2.20.1

[PATCH 3/4] drm/vmwgfx: fix the check when to use dma_alloc_coherent

[Christoph Hellwig]

Since Linux 4.21 we merged the swiotlb ops into the DMA direct ops,
so they would always have a the sync_single methods.  But late in
the cicle we also removed the direct ops entirely, so we'd see NULL
DMA ops.  Switch vmw_dma_select_mode to only detect swiotlb presence
using swiotlb_nr_tbl() instead.

Fixes: 55897af630 ("dma-direct: merge swiotlb_dma_ops into the dma_direct code")
Fixes: 356da6d0cd ("dma-mapping: bypass indirect calls for dma-direct")
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/gpu/drm/vmwgfx/vmwgfx_drv.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
index 236052ad233c..c2060f6cc9e8 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
@@ -565,7 +565,6 @@ static int vmw_dma_select_mode(struct vmw_private *dev_priv)
 		[vmw_dma_alloc_coherent] = "Using coherent TTM pages.",
 		[vmw_dma_map_populate] = "Keeping DMA mappings.",
 		[vmw_dma_map_bind] = "Giving up DMA mappings early."};
-	const struct dma_map_ops *dma_ops = get_dma_ops(dev_priv->dev->dev);
 
 	if (intel_iommu_enabled) {
 		dev_priv->map_mode = vmw_dma_map_populate;
@@ -578,14 +577,12 @@ static int vmw_dma_select_mode(struct vmw_private *dev_priv)
 		return 0;
 	}
 
-	dev_priv->map_mode = vmw_dma_map_populate;
-
-	if (dma_ops && dma_ops->sync_single_for_cpu)
-		dev_priv->map_mode = vmw_dma_alloc_coherent;
 #ifdef CONFIG_SWIOTLB
-	if (swiotlb_nr_tbl() == 0)
-		dev_priv->map_mode = vmw_dma_map_populate;
+	if (swiotlb_nr_tbl())
+		dev_priv->map_mode = vmw_dma_alloc_coherent;
+	else
 #endif
+		dev_priv->map_mode = vmw_dma_map_populate;
 
 out_fixup:
 	if (dev_priv->map_mode == vmw_dma_map_populate &&
-- 
2.20.1

[PATCH 4/4] drm/vmwgfx: unwind spaghetti code in vmw_dma_select_mode

[Christoph Hellwig]

Just use a simple if/else chain to select the DMA mode.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/gpu/drm/vmwgfx/vmwgfx_drv.c | 25 ++++++-------------------
 1 file changed, 6 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
index c2060f6cc9e8..86387735a90b 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
@@ -566,34 +566,21 @@ static int vmw_dma_select_mode(struct vmw_private *dev_priv)
 		[vmw_dma_map_populate] = "Keeping DMA mappings.",
 		[vmw_dma_map_bind] = "Giving up DMA mappings early."};
 
-	if (intel_iommu_enabled) {
+	if (vmw_force_coherent)
+		dev_priv->map_mode = vmw_dma_alloc_coherent;
+	else if (intel_iommu_enabled)
 		dev_priv->map_mode = vmw_dma_map_populate;
-		goto out_fixup;
-	}
-
-	if (!(vmw_force_iommu || vmw_force_coherent)) {
+	else if (!vmw_force_iommu)
 		dev_priv->map_mode = vmw_dma_phys;
-		DRM_INFO("DMA map mode: %s\n", names[dev_priv->map_mode]);
-		return 0;
-	}
-
-#ifdef CONFIG_SWIOTLB
-	if (swiotlb_nr_tbl())
+	else if (IS_ENABLED(CONFIG_SWIOTLB) && swiotlb_nr_tbl())
 		dev_priv->map_mode = vmw_dma_alloc_coherent;
 	else
-#endif
 		dev_priv->map_mode = vmw_dma_map_populate;
 
-out_fixup:
-	if (dev_priv->map_mode == vmw_dma_map_populate &&
-	    vmw_restrict_iommu)
+	if (dev_priv->map_mode == vmw_dma_map_populate && vmw_restrict_iommu)
 		dev_priv->map_mode = vmw_dma_map_bind;
 
-	if (vmw_force_coherent)
-		dev_priv->map_mode = vmw_dma_alloc_coherent;
-
 	DRM_INFO("DMA map mode: %s\n", names[dev_priv->map_mode]);
-
 	return 0;
 }
 
-- 
2.20.1

Re: fix DMA ops layering violations in vmwgfx

[Thomas Hellstrom]

Hi, Christoph,

On Sat, 2019-01-05 at 09:01 +0100, Christoph Hellwig wrote:
> Hi Thomas,
> 
> vmwgfx has been doing some odd checks based on DMA ops which rely
> on deep DMA mapping layer internals, and I think the changes in
> Linux 4.21 finally broke most of these implicit assumptions.

Thanks. 
What we're really trying to do here is to try to detect the situation
where DMA remapping using hardware IOMMUs is going on but memory is
still coherent, since the driver can currently only work with coherent
memory[1]. Currently we use intel_iommu_enabled to detect this
situation, but it would be really helpful if there were a generic bool
that advertizes this situation since we need to deal with other IOMMUs
as well going forward. Any suggestion?

Comments on the patches separately.

Thanks,
Thomas




> 
> The real fix is in patch 3, but I think the others are important
> to make it clear what is actually going on.

Re: [PATCH 2/4] drm/vmwgfx: remove CONFIG_INTEL_IOMMU ifdefs

[Thomas Hellstrom]

Reviewed-by: Thomas Hellstrom <thellstrom@vmware.com>

On Sat, 2019-01-05 at 09:01 +0100, Christoph Hellwig wrote:
> intel_iommu_enabled is defined as always false for
> !CONFIG_INTEL_IOMMU,
> so remove the ifdefs around it.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  drivers/gpu/drm/vmwgfx/vmwgfx_drv.c | 18 ------------------
>  1 file changed, 18 deletions(-)
> 
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
> b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
> index 69e325b2d954..236052ad233c 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
> @@ -567,12 +567,10 @@ static int vmw_dma_select_mode(struct
> vmw_private *dev_priv)
>  		[vmw_dma_map_bind] = "Giving up DMA mappings early."};
>  	const struct dma_map_ops *dma_ops = get_dma_ops(dev_priv->dev-
> >dev);
>  
> -#ifdef CONFIG_INTEL_IOMMU
>  	if (intel_iommu_enabled) {
>  		dev_priv->map_mode = vmw_dma_map_populate;
>  		goto out_fixup;
>  	}
> -#endif
>  
>  	if (!(vmw_force_iommu || vmw_force_coherent)) {
>  		dev_priv->map_mode = vmw_dma_phys;
> @@ -589,9 +587,7 @@ static int vmw_dma_select_mode(struct vmw_private
> *dev_priv)
>  		dev_priv->map_mode = vmw_dma_map_populate;
>  #endif
>  
> -#ifdef CONFIG_INTEL_IOMMU
>  out_fixup:
> -#endif
>  	if (dev_priv->map_mode == vmw_dma_map_populate &&
>  	    vmw_restrict_iommu)
>  		dev_priv->map_mode = vmw_dma_map_bind;
> @@ -599,13 +595,6 @@ static int vmw_dma_select_mode(struct
> vmw_private *dev_priv)
>  	if (vmw_force_coherent)
>  		dev_priv->map_mode = vmw_dma_alloc_coherent;
>  
> -#if !defined(CONFIG_SWIOTLB) && !defined(CONFIG_INTEL_IOMMU)
> -	/*
> -	 * No coherent page pool
> -	 */
> -	if (dev_priv->map_mode == vmw_dma_alloc_coherent)
> -		return -EINVAL;
> -#endif
>  	DRM_INFO("DMA map mode: %s\n", names[dev_priv->map_mode]);
>  
>  	return 0;
> @@ -619,7 +608,6 @@ static int vmw_dma_select_mode(struct vmw_private
> *dev_priv)
>   * With 32-bit we can only handle 32 bit PFNs. Optionally set that
>   * restriction also for 64-bit systems.
>   */
> -#ifdef CONFIG_INTEL_IOMMU
>  static int vmw_dma_masks(struct vmw_private *dev_priv)
>  {
>  	struct drm_device *dev = dev_priv->dev;
> @@ -631,12 +619,6 @@ static int vmw_dma_masks(struct vmw_private
> *dev_priv)
>  	}
>  	return 0;
>  }
> -#else
> -static int vmw_dma_masks(struct vmw_private *dev_priv)
> -{
> -	return 0;
> -}
> -#endif
>  
>  static int vmw_driver_load(struct drm_device *dev, unsigned long
> chipset)
>  {

Re: [PATCH 2/4] drm/vmwgfx: remove CONFIG_INTEL_IOMMU ifdefs

[Thomas Hellstrom]

Hi,


On Sat, 2019-01-05 at 09:01 +0100, Christoph Hellwig wrote:
...

> intel_iommu_enabled is defined as always false for
> !CONFIG_INTEL_IOMMU,
> so remove the ifdefs around it.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
> 
> -#if !defined(CONFIG_SWIOTLB) && !defined(CONFIG_INTEL_IOMMU)
> -	/*
> -	 * No coherent page pool
> -	 */
> -	if (dev_priv->map_mode == vmw_dma_alloc_coherent)
> -		return -EINVAL;
> -#endif
> 

Actually this hunk is incorrect, it tries to determine whether the TTM
subsystem maintains a coherent page pool or not. If not, we can't use
vmw_dma_alloc_coherent.

/Thomas

Re: [PATCH 4/4] drm/vmwgfx: unwind spaghetti code in vmw_dma_select_mode

[Thomas Hellstrom]

On Sat, 2019-01-05 at 09:01 +0100, Christoph Hellwig wrote:
> Just use a simple if/else chain to select the DMA mode.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Reviewed-by: Thomas Hellstrom <thellstrom@vmware.com>


> ---
>  drivers/gpu/drm/vmwgfx/vmwgfx_drv.c | 25 ++++++-------------------
>  1 file changed, 6 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
> b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
> index c2060f6cc9e8..86387735a90b 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
> @@ -566,34 +566,21 @@ static int vmw_dma_select_mode(struct
> vmw_private *dev_priv)
>  		[vmw_dma_map_populate] = "Keeping DMA mappings.",
>  		[vmw_dma_map_bind] = "Giving up DMA mappings early."};
>  
> -	if (intel_iommu_enabled) {
> +	if (vmw_force_coherent)
> +		dev_priv->map_mode = vmw_dma_alloc_coherent;
> +	else if (intel_iommu_enabled)
>  		dev_priv->map_mode = vmw_dma_map_populate;
> -		goto out_fixup;
> -	}
> -
> -	if (!(vmw_force_iommu || vmw_force_coherent)) {
> +	else if (!vmw_force_iommu)
>  		dev_priv->map_mode = vmw_dma_phys;
> -		DRM_INFO("DMA map mode: %s\n", names[dev_priv-
> >map_mode]);
> -		return 0;
> -	}
> -
> -#ifdef CONFIG_SWIOTLB
> -	if (swiotlb_nr_tbl())
> +	else if (IS_ENABLED(CONFIG_SWIOTLB) && swiotlb_nr_tbl())
>  		dev_priv->map_mode = vmw_dma_alloc_coherent;
>  	else
> -#endif
>  		dev_priv->map_mode = vmw_dma_map_populate;
>  
> -out_fixup:
> -	if (dev_priv->map_mode == vmw_dma_map_populate &&
> -	    vmw_restrict_iommu)
> +	if (dev_priv->map_mode == vmw_dma_map_populate &&
> vmw_restrict_iommu)
>  		dev_priv->map_mode = vmw_dma_map_bind;
>  
> -	if (vmw_force_coherent)
> -		dev_priv->map_mode = vmw_dma_alloc_coherent;
> -
>  	DRM_INFO("DMA map mode: %s\n", names[dev_priv->map_mode]);
> -
>  	return 0;
>  }
>  

Re: fix DMA ops layering violations in vmwgfx

[hch]

On Tue, Jan 08, 2019 at 09:51:45AM +0000, Thomas Hellstrom wrote:
> Hi, Christoph,
> 
> On Sat, 2019-01-05 at 09:01 +0100, Christoph Hellwig wrote:
> > Hi Thomas,
> > 
> > vmwgfx has been doing some odd checks based on DMA ops which rely
> > on deep DMA mapping layer internals, and I think the changes in
> > Linux 4.21 finally broke most of these implicit assumptions.
> 
> Thanks. 
> What we're really trying to do here is to try to detect the situation
> where DMA remapping using hardware IOMMUs is going on but memory is
> still coherent, since the driver can currently only work with coherent
> memory[1]. Currently we use intel_iommu_enabled to detect this
> situation, but it would be really helpful if there were a generic bool
> that advertizes this situation since we need to deal with other IOMMUs
> as well going forward. Any suggestion?

I'm missing the link of the [1] reference above.  But if you need
coherent memory you should simply always use dma_alloc_coherent, that
is the only gurantee you get.  If you use any other dma mapping methods
you will otherwise need to explicitly transfer ownership by mapping/
unmapping or using dma_sync* before and after every device access.

And the whole DMA API bypass using the phys mode is something that
I'd really prefer not to see in any driver as it tends to cause
major problems sooner or later.

Re: fix DMA ops layering violations in vmwgfx

[Thomas Hellstrom]

On Tue, 2019-01-08 at 14:12 +0100, hch@lst.de wrote:
> On Tue, Jan 08, 2019 at 09:51:45AM +0000, Thomas Hellstrom wrote:
> > Hi, Christoph,
> > 
> > On Sat, 2019-01-05 at 09:01 +0100, Christoph Hellwig wrote:
> > > Hi Thomas,
> > > 
> > > vmwgfx has been doing some odd checks based on DMA ops which rely
> > > on deep DMA mapping layer internals, and I think the changes in
> > > Linux 4.21 finally broke most of these implicit assumptions.
> > 
> > Thanks. 
> > What we're really trying to do here is to try to detect the
> > situation
> > where DMA remapping using hardware IOMMUs is going on but memory is
> > still coherent, since the driver can currently only work with
> > coherent
> > memory[1]. Currently we use intel_iommu_enabled to detect this
> > situation, but it would be really helpful if there were a generic
> > bool
> > that advertizes this situation since we need to deal with other
> > IOMMUs
> > as well going forward. Any suggestion?
> 
> I'm missing the link of the [1] reference above. 

Sorry. I meant to remove the reference since the explanation would be
rather lengthy.

> But if you need
> coherent memory you should simply always use dma_alloc_coherent, that
> is the only gurantee you get.  If you use any other dma mapping
> methods
> you will otherwise need to explicitly transfer ownership by mapping/
> unmapping or using dma_sync* before and after every device access.

The problem is that graphics applications typically allocate huge
amounts of DMA memory. The GPU may want to map half of available system
memory or more. Moreover this memory might need to be mappable by
multiple devices, which would rule out dma_alloc_coherent() in many
situations.

Using SWIOTLB with bounce-buffers is also typically out of the question
for performance- and memory usage reasons. Moreover the sync operations
would often affect sub-regions of 2D and 3D textures, which makes the
dma_sync API inefficient even if it maps to no-ops. vmwgfx would rather
not load in these situations.

Finally graphics applications, do not always provide conservative sync
regions.

> 
> And the whole DMA API bypass using the phys mode is something that
> I'd really prefer not to see in any driver as it tends to cause
> major problems sooner or later.

I fully understand this. However, given the above, the DMA API is quite
awkward for graphics operation. It would be easier to motivate a full
removal of the phys mode if we could query the DMA API whether the
dma_sync operations are no-ops or not.

/Thomas