[PATCH 1/2] drm/i915: Treat stolen memory as DMA addresses

[PATCH 1/2] drm/i915: Treat stolen memory as DMA addresses

[Chris Wilson]

The conversion of stolen to use phys_addr_t (from essentially u32)
sparked an interesting discussion. We treat stolen memory as only
accessible from the GPU (the DMA device) - an attempt to use it from the
CPU will generate a MCE on gen6 onwards, although it is in theory a
physical address that can be dereferenced from the CPU as demonstrated
by earlier generations. As such, using phys_addr_t has the wrong
connotations and as we pass the address into the DMA device via
dma_addr_t (through the scatterlists used to program the GTT entries),
we should treat it as dma_addr_t throughout.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h        |  2 +-
 drivers/gpu/drm/i915/i915_gem_stolen.c | 36 +++++++++++++++++-----------------
 2 files changed, 19 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index d0a48cceb15b..a43ba7872d7c 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1467,7 +1467,7 @@ struct i915_gem_mm {
 	struct work_struct free_work;
 
 	/** Usable portion of the GTT for GEM */
-	phys_addr_t stolen_base; /* limited to low memory (32-bit) */
+	dma_addr_t stolen_base; /* limited to low memory (32-bit) */
 
 	/** PPGTT used for aliasing the PPGTT with the GTT */
 	struct i915_hw_ppgtt *aliasing_ppgtt;
diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c
index 7226e4a21097..42bbc4b04fd6 100644
--- a/drivers/gpu/drm/i915/i915_gem_stolen.c
+++ b/drivers/gpu/drm/i915/i915_gem_stolen.c
@@ -79,12 +79,12 @@ void i915_gem_stolen_remove_node(struct drm_i915_private *dev_priv,
 	mutex_unlock(&dev_priv->mm.stolen_lock);
 }
 
-static phys_addr_t i915_stolen_to_physical(struct drm_i915_private *dev_priv)
+static dma_addr_t i915_stolen_to_dma(struct drm_i915_private *dev_priv)
 {
 	struct pci_dev *pdev = dev_priv->drm.pdev;
 	struct i915_ggtt *ggtt = &dev_priv->ggtt;
 	struct resource *r;
-	phys_addr_t base;
+	dma_addr_t base;
 
 	/* Almost universally we can find the Graphics Base of Stolen Memory
 	 * at register BSM (0x5c) in the igfx configuration space. On a few
@@ -196,7 +196,7 @@ static phys_addr_t i915_stolen_to_physical(struct drm_i915_private *dev_priv)
 	if (INTEL_GEN(dev_priv) <= 4 &&
 	    !IS_G33(dev_priv) && !IS_PINEVIEW(dev_priv) && !IS_G4X(dev_priv)) {
 		struct {
-			phys_addr_t start, end;
+			dma_addr_t start, end;
 		} stolen[2] = {
 			{ .start = base, .end = base + ggtt->stolen_size, },
 			{ .start = base, .end = base + ggtt->stolen_size, },
@@ -228,12 +228,12 @@ static phys_addr_t i915_stolen_to_physical(struct drm_i915_private *dev_priv)
 
 		if (stolen[0].start != stolen[1].start ||
 		    stolen[0].end != stolen[1].end) {
-			phys_addr_t end = base + ggtt->stolen_size - 1;
+			dma_addr_t end = base + ggtt->stolen_size - 1;
 
 			DRM_DEBUG_KMS("GTT within stolen memory at 0x%llx-0x%llx\n",
 				      (unsigned long long)ggtt_start,
 				      (unsigned long long)ggtt_end - 1);
-			DRM_DEBUG_KMS("Stolen memory adjusted to %pa - %pa\n",
+			DRM_DEBUG_KMS("Stolen memory adjusted to %pad - %pad\n",
 				      &base, &end);
 		}
 	}
@@ -263,9 +263,9 @@ static phys_addr_t i915_stolen_to_physical(struct drm_i915_private *dev_priv)
 		 * range. Apparently this works.
 		 */
 		if (r == NULL && !IS_GEN3(dev_priv)) {
-			phys_addr_t end = base + ggtt->stolen_size;
+			dma_addr_t end = base + ggtt->stolen_size;
 
-			DRM_ERROR("conflict detected with stolen region: [%pa - %pa]\n",
+			DRM_ERROR("conflict detected with stolen region: [%pad - %pad]\n",
 				  &base, &end);
 			base = 0;
 		}
@@ -285,13 +285,13 @@ void i915_gem_cleanup_stolen(struct drm_device *dev)
 }
 
 static void g4x_get_stolen_reserved(struct drm_i915_private *dev_priv,
-				    phys_addr_t *base, u32 *size)
+				    dma_addr_t *base, u32 *size)
 {
 	struct i915_ggtt *ggtt = &dev_priv->ggtt;
 	uint32_t reg_val = I915_READ(IS_GM45(dev_priv) ?
 				     CTG_STOLEN_RESERVED :
 				     ELK_STOLEN_RESERVED);
-	phys_addr_t stolen_top = dev_priv->mm.stolen_base + ggtt->stolen_size;
+	dma_addr_t stolen_top = dev_priv->mm.stolen_base + ggtt->stolen_size;
 
 	*base = (reg_val & G4X_STOLEN_RESERVED_ADDR2_MASK) << 16;
 
@@ -308,7 +308,7 @@ static void g4x_get_stolen_reserved(struct drm_i915_private *dev_priv,
 }
 
 static void gen6_get_stolen_reserved(struct drm_i915_private *dev_priv,
-				     phys_addr_t *base, u32 *size)
+				     dma_addr_t *base, u32 *size)
 {
 	uint32_t reg_val = I915_READ(GEN6_STOLEN_RESERVED);
 
@@ -334,7 +334,7 @@ static void gen6_get_stolen_reserved(struct drm_i915_private *dev_priv,
 }
 
 static void gen7_get_stolen_reserved(struct drm_i915_private *dev_priv,
-				     phys_addr_t *base, u32 *size)
+				     dma_addr_t *base, u32 *size)
 {
 	uint32_t reg_val = I915_READ(GEN6_STOLEN_RESERVED);
 
@@ -354,7 +354,7 @@ static void gen7_get_stolen_reserved(struct drm_i915_private *dev_priv,
 }
 
 static void chv_get_stolen_reserved(struct drm_i915_private *dev_priv,
-				    phys_addr_t *base, u32 *size)
+				    dma_addr_t *base, u32 *size)
 {
 	uint32_t reg_val = I915_READ(GEN6_STOLEN_RESERVED);
 
@@ -380,11 +380,11 @@ static void chv_get_stolen_reserved(struct drm_i915_private *dev_priv,
 }
 
 static void bdw_get_stolen_reserved(struct drm_i915_private *dev_priv,
-				    phys_addr_t *base, u32 *size)
+				    dma_addr_t *base, u32 *size)
 {
 	struct i915_ggtt *ggtt = &dev_priv->ggtt;
 	uint32_t reg_val = I915_READ(GEN6_STOLEN_RESERVED);
-	phys_addr_t stolen_top;
+	dma_addr_t stolen_top;
 
 	stolen_top = dev_priv->mm.stolen_base + ggtt->stolen_size;
 
@@ -403,7 +403,7 @@ static void bdw_get_stolen_reserved(struct drm_i915_private *dev_priv,
 int i915_gem_init_stolen(struct drm_i915_private *dev_priv)
 {
 	struct i915_ggtt *ggtt = &dev_priv->ggtt;
-	phys_addr_t reserved_base, stolen_top;
+	dma_addr_t reserved_base, stolen_top;
 	u32 reserved_total, reserved_size;
 	u32 stolen_usable_start;
 
@@ -424,7 +424,7 @@ int i915_gem_init_stolen(struct drm_i915_private *dev_priv)
 	if (ggtt->stolen_size == 0)
 		return 0;
 
-	dev_priv->mm.stolen_base = i915_stolen_to_physical(dev_priv);
+	dev_priv->mm.stolen_base = i915_stolen_to_dma(dev_priv);
 	if (dev_priv->mm.stolen_base == 0)
 		return 0;
 
@@ -473,8 +473,8 @@ int i915_gem_init_stolen(struct drm_i915_private *dev_priv)
 
 	if (reserved_base < dev_priv->mm.stolen_base ||
 	    reserved_base + reserved_size > stolen_top) {
-		phys_addr_t reserved_top = reserved_base + reserved_size;
-		DRM_DEBUG_KMS("Stolen reserved area [%pa - %pa] outside stolen memory [%pa - %pa]\n",
+		dma_addr_t reserved_top = reserved_base + reserved_size;
+		DRM_DEBUG_KMS("Stolen reserved area [%pad - %pad] outside stolen memory [%pad - %pad]\n",
 			      &reserved_base, &reserved_top,
 			      &dev_priv->mm.stolen_base, &stolen_top);
 		return 0;
-- 
2.11.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 2/2] drm/i915: Check that the DMA address for stolen fits within dma_addr_t

[Chris Wilson]

Just sanity check that the value we deduce from the stolen memory
register fits within the kernel's dma_addr_t and doesn't overflow.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_gem_stolen.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c
index 42bbc4b04fd6..4f1f3090c0ed 100644
--- a/drivers/gpu/drm/i915/i915_gem_stolen.c
+++ b/drivers/gpu/drm/i915/i915_gem_stolen.c
@@ -211,6 +211,14 @@ static dma_addr_t i915_stolen_to_dma(struct drm_i915_private *dev_priv)
 			ggtt_start &= PGTBL_ADDRESS_LO_MASK;
 		ggtt_end = ggtt_start + ggtt_total_entries(ggtt) * 4;
 
+		if (ggtt_end <= ggtt_start ||
+		    overflows_type(ggtt_end, dma_addr_t)) {
+			DRM_ERROR("DMA address for reserved igfx memory [%llx - %llx] does not fit within the kernel's %db dma_addr_t\n",
+				  ggtt_start, ggtt_end,
+				  (int)sizeof(dma_addr_t) * 8);
+			return 0;
+		}
+
 		if (ggtt_start >= stolen[0].start && ggtt_start < stolen[0].end)
 			stolen[0].end = ggtt_start;
 		if (ggtt_end > stolen[1].start && ggtt_end <= stolen[1].end)
-- 
2.11.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/2] drm/i915: Check that the DMA address for stolen fits within dma_addr_t

[Ville Syrjälä]

On Fri, Jan 27, 2017 at 04:55:31PM +0000, Chris Wilson wrote:
> Just sanity check that the value we deduce from the stolen memory
> register fits within the kernel's dma_addr_t and doesn't overflow.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_gem_stolen.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c
> index 42bbc4b04fd6..4f1f3090c0ed 100644
> --- a/drivers/gpu/drm/i915/i915_gem_stolen.c
> +++ b/drivers/gpu/drm/i915/i915_gem_stolen.c
> @@ -211,6 +211,14 @@ static dma_addr_t i915_stolen_to_dma(struct drm_i915_private *dev_priv)
>  			ggtt_start &= PGTBL_ADDRESS_LO_MASK;
>  		ggtt_end = ggtt_start + ggtt_total_entries(ggtt) * 4;
>  
> +		if (ggtt_end <= ggtt_start ||
> +		    overflows_type(ggtt_end, dma_addr_t)) {
> +			DRM_ERROR("DMA address for reserved igfx memory [%llx - %llx] does not fit within the kernel's %db dma_addr_t\n",
> +				  ggtt_start, ggtt_end,
> +				  (int)sizeof(dma_addr_t) * 8);
> +			return 0;
> +		}

This would only check if the ggtt location fits into dma_addr_t.
We don't need that since we never touch the ggtt directly with
either the CPU or GPU. So I think this piece of code should keep
on using u64 as it needs to be able to hold the 36 address we read
from the hardware/firmware.

> +
>  		if (ggtt_start >= stolen[0].start && ggtt_start < stolen[0].end)
>  			stolen[0].end = ggtt_start;
>  		if (ggtt_end > stolen[1].start && ggtt_end <= stolen[1].end)
> -- 
> 2.11.0

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/2] drm/i915: Check that the DMA address for stolen fits within dma_addr_t

[Chris Wilson]

On Fri, Jan 27, 2017 at 07:05:57PM +0200, Ville Syrjälä wrote:
> On Fri, Jan 27, 2017 at 04:55:31PM +0000, Chris Wilson wrote:
> > Just sanity check that the value we deduce from the stolen memory
> > register fits within the kernel's dma_addr_t and doesn't overflow.
> > 
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_gem_stolen.c | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c
> > index 42bbc4b04fd6..4f1f3090c0ed 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_stolen.c
> > +++ b/drivers/gpu/drm/i915/i915_gem_stolen.c
> > @@ -211,6 +211,14 @@ static dma_addr_t i915_stolen_to_dma(struct drm_i915_private *dev_priv)
> >  			ggtt_start &= PGTBL_ADDRESS_LO_MASK;
> >  		ggtt_end = ggtt_start + ggtt_total_entries(ggtt) * 4;
> >  
> > +		if (ggtt_end <= ggtt_start ||
> > +		    overflows_type(ggtt_end, dma_addr_t)) {
> > +			DRM_ERROR("DMA address for reserved igfx memory [%llx - %llx] does not fit within the kernel's %db dma_addr_t\n",
> > +				  ggtt_start, ggtt_end,
> > +				  (int)sizeof(dma_addr_t) * 8);
> > +			return 0;
> > +		}
> 
> This would only check if the ggtt location fits into dma_addr_t.
> We don't need that since we never touch the ggtt directly with
> either the CPU or GPU. So I think this piece of code should keep
> on using u64 as it needs to be able to hold the 36 address we read
> from the hardware/firmware.

Ok, I was just looking for u64 in the vicinity of dma_addr_t. So we are
just checking that the ggtt doesn't overlap stolen, not creating stolen
addresses from ggtt. Yup, we don't need to check against dma_addr_t
here - that I can leave for future register checking.

The sanity check for stolen would actually just be:

--- a/drivers/gpu/drm/i915/i915_gem_stolen.c
+++ b/drivers/gpu/drm/i915/i915_gem_stolen.c
@@ -189,7 +189,7 @@ static dma_addr_t i915_stolen_to_dma(struct drm_i915_private *dev_priv)
                base = tom - tseg_size - ggtt->stolen_size;
        }
 
-       if (base == 0)
+       if (base == 0 || ggtt->stolen_size + base < base)
                return 0;
		 

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH] drm/i915: Sanity check the computed size and base of stolen memory

[Chris Wilson]

Just do a quick check that the stolen memory address range doesn't
overflow our chosen integer type.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_gem_stolen.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c
index 42bbc4b04fd6..37a26e1c9190 100644
--- a/drivers/gpu/drm/i915/i915_gem_stolen.c
+++ b/drivers/gpu/drm/i915/i915_gem_stolen.c
@@ -189,7 +189,7 @@ static dma_addr_t i915_stolen_to_dma(struct drm_i915_private *dev_priv)
 		base = tom - tseg_size - ggtt->stolen_size;
 	}
 
-	if (base == 0)
+	if (base == 0 || base + ggtt->stolen_size <= base)
 		return 0;
 
 	/* make sure we don't clobber the GTT if it's within stolen memory */
-- 
2.11.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/2] drm/i915: Treat stolen memory as DMA addresses

[Paulo Zanoni]

Em Sex, 2017-01-27 às 16:55 +0000, Chris Wilson escreveu:
> The conversion of stolen to use phys_addr_t (from essentially u32)
> sparked an interesting discussion. We treat stolen memory as only
> accessible from the GPU (the DMA device) - an attempt to use it from
> the
> CPU will generate a MCE on gen6 onwards, although it is in theory a
> physical address that can be dereferenced from the CPU as
> demonstrated
> by earlier generations. As such, using phys_addr_t has the wrong
> connotations and as we pass the address into the DMA device via
> dma_addr_t (through the scatterlists used to program the GTT
> entries),
> we should treat it as dma_addr_t throughout.

I'm not a specialist here, but from what I could learn/understand, this
seems good. The patch seems to do what it says, so:

Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

But now that I looked some more at the code, I started to think about
the following: shouldn't we convert our "stolen offset" variables from
u32 to dma_addr_t or some other appropriate type? I mean, if we ever
get 64 bit stolen pointers we may also get 64 bit stolen offsets... The
i915_ggtt struct contains 4 of such u32 pointers.

For example, i915_ggtt->stolen_reserved_base is another pointer to
stolen memory, it's generated directly from one of our drm_addr_t
variables.

Of course, this could be a separate patch.

> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h        |  2 +-
>  drivers/gpu/drm/i915/i915_gem_stolen.c | 36 +++++++++++++++++-------
> ----------
>  2 files changed, 19 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h
> b/drivers/gpu/drm/i915/i915_drv.h
> index d0a48cceb15b..a43ba7872d7c 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1467,7 +1467,7 @@ struct i915_gem_mm {
>  	struct work_struct free_work;
>  
>  	/** Usable portion of the GTT for GEM */
> -	phys_addr_t stolen_base; /* limited to low memory (32-bit)
> */
> +	dma_addr_t stolen_base; /* limited to low memory (32-bit) */
>  
>  	/** PPGTT used for aliasing the PPGTT with the GTT */
>  	struct i915_hw_ppgtt *aliasing_ppgtt;
> diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c
> b/drivers/gpu/drm/i915/i915_gem_stolen.c
> index 7226e4a21097..42bbc4b04fd6 100644
> --- a/drivers/gpu/drm/i915/i915_gem_stolen.c
> +++ b/drivers/gpu/drm/i915/i915_gem_stolen.c
> @@ -79,12 +79,12 @@ void i915_gem_stolen_remove_node(struct
> drm_i915_private *dev_priv,
>  	mutex_unlock(&dev_priv->mm.stolen_lock);
>  }
>  
> -static phys_addr_t i915_stolen_to_physical(struct drm_i915_private
> *dev_priv)
> +static dma_addr_t i915_stolen_to_dma(struct drm_i915_private
> *dev_priv)
>  {
>  	struct pci_dev *pdev = dev_priv->drm.pdev;
>  	struct i915_ggtt *ggtt = &dev_priv->ggtt;
>  	struct resource *r;
> -	phys_addr_t base;
> +	dma_addr_t base;
>  
>  	/* Almost universally we can find the Graphics Base of
> Stolen Memory
>  	 * at register BSM (0x5c) in the igfx configuration space.
> On a few
> @@ -196,7 +196,7 @@ static phys_addr_t i915_stolen_to_physical(struct
> drm_i915_private *dev_priv)
>  	if (INTEL_GEN(dev_priv) <= 4 &&
>  	    !IS_G33(dev_priv) && !IS_PINEVIEW(dev_priv) &&
> !IS_G4X(dev_priv)) {
>  		struct {
> -			phys_addr_t start, end;
> +			dma_addr_t start, end;
>  		} stolen[2] = {
>  			{ .start = base, .end = base + ggtt-
> >stolen_size, },
>  			{ .start = base, .end = base + ggtt-
> >stolen_size, },
> @@ -228,12 +228,12 @@ static phys_addr_t
> i915_stolen_to_physical(struct drm_i915_private *dev_priv)
>  
>  		if (stolen[0].start != stolen[1].start ||
>  		    stolen[0].end != stolen[1].end) {
> -			phys_addr_t end = base + ggtt->stolen_size -
> 1;
> +			dma_addr_t end = base + ggtt->stolen_size -
> 1;
>  
>  			DRM_DEBUG_KMS("GTT within stolen memory at
> 0x%llx-0x%llx\n",
>  				      (unsigned long
> long)ggtt_start,
>  				      (unsigned long long)ggtt_end -
> 1);
> -			DRM_DEBUG_KMS("Stolen memory adjusted to %pa
> - %pa\n",
> +			DRM_DEBUG_KMS("Stolen memory adjusted to
> %pad - %pad\n",
>  				      &base, &end);
>  		}
>  	}
> @@ -263,9 +263,9 @@ static phys_addr_t i915_stolen_to_physical(struct
> drm_i915_private *dev_priv)
>  		 * range. Apparently this works.
>  		 */
>  		if (r == NULL && !IS_GEN3(dev_priv)) {
> -			phys_addr_t end = base + ggtt->stolen_size;
> +			dma_addr_t end = base + ggtt->stolen_size;
>  
> -			DRM_ERROR("conflict detected with stolen
> region: [%pa - %pa]\n",
> +			DRM_ERROR("conflict detected with stolen
> region: [%pad - %pad]\n",
>  				  &base, &end);
>  			base = 0;
>  		}
> @@ -285,13 +285,13 @@ void i915_gem_cleanup_stolen(struct drm_device
> *dev)
>  }
>  
>  static void g4x_get_stolen_reserved(struct drm_i915_private
> *dev_priv,
> -				    phys_addr_t *base, u32 *size)
> +				    dma_addr_t *base, u32 *size)
>  {
>  	struct i915_ggtt *ggtt = &dev_priv->ggtt;
>  	uint32_t reg_val = I915_READ(IS_GM45(dev_priv) ?
>  				     CTG_STOLEN_RESERVED :
>  				     ELK_STOLEN_RESERVED);
> -	phys_addr_t stolen_top = dev_priv->mm.stolen_base + ggtt-
> >stolen_size;
> +	dma_addr_t stolen_top = dev_priv->mm.stolen_base + ggtt-
> >stolen_size;
>  
>  	*base = (reg_val & G4X_STOLEN_RESERVED_ADDR2_MASK) << 16;
>  
> @@ -308,7 +308,7 @@ static void g4x_get_stolen_reserved(struct
> drm_i915_private *dev_priv,
>  }
>  
>  static void gen6_get_stolen_reserved(struct drm_i915_private
> *dev_priv,
> -				     phys_addr_t *base, u32 *size)
> +				     dma_addr_t *base, u32 *size)
>  {
>  	uint32_t reg_val = I915_READ(GEN6_STOLEN_RESERVED);
>  
> @@ -334,7 +334,7 @@ static void gen6_get_stolen_reserved(struct
> drm_i915_private *dev_priv,
>  }
>  
>  static void gen7_get_stolen_reserved(struct drm_i915_private
> *dev_priv,
> -				     phys_addr_t *base, u32 *size)
> +				     dma_addr_t *base, u32 *size)
>  {
>  	uint32_t reg_val = I915_READ(GEN6_STOLEN_RESERVED);
>  
> @@ -354,7 +354,7 @@ static void gen7_get_stolen_reserved(struct
> drm_i915_private *dev_priv,
>  }
>  
>  static void chv_get_stolen_reserved(struct drm_i915_private
> *dev_priv,
> -				    phys_addr_t *base, u32 *size)
> +				    dma_addr_t *base, u32 *size)
>  {
>  	uint32_t reg_val = I915_READ(GEN6_STOLEN_RESERVED);
>  
> @@ -380,11 +380,11 @@ static void chv_get_stolen_reserved(struct
> drm_i915_private *dev_priv,
>  }
>  
>  static void bdw_get_stolen_reserved(struct drm_i915_private
> *dev_priv,
> -				    phys_addr_t *base, u32 *size)
> +				    dma_addr_t *base, u32 *size)
>  {
>  	struct i915_ggtt *ggtt = &dev_priv->ggtt;
>  	uint32_t reg_val = I915_READ(GEN6_STOLEN_RESERVED);
> -	phys_addr_t stolen_top;
> +	dma_addr_t stolen_top;
>  
>  	stolen_top = dev_priv->mm.stolen_base + ggtt->stolen_size;
>  
> @@ -403,7 +403,7 @@ static void bdw_get_stolen_reserved(struct
> drm_i915_private *dev_priv,
>  int i915_gem_init_stolen(struct drm_i915_private *dev_priv)
>  {
>  	struct i915_ggtt *ggtt = &dev_priv->ggtt;
> -	phys_addr_t reserved_base, stolen_top;
> +	dma_addr_t reserved_base, stolen_top;
>  	u32 reserved_total, reserved_size;
>  	u32 stolen_usable_start;
>  
> @@ -424,7 +424,7 @@ int i915_gem_init_stolen(struct drm_i915_private
> *dev_priv)
>  	if (ggtt->stolen_size == 0)
>  		return 0;
>  
> -	dev_priv->mm.stolen_base =
> i915_stolen_to_physical(dev_priv);
> +	dev_priv->mm.stolen_base = i915_stolen_to_dma(dev_priv);
>  	if (dev_priv->mm.stolen_base == 0)
>  		return 0;
>  
> @@ -473,8 +473,8 @@ int i915_gem_init_stolen(struct drm_i915_private
> *dev_priv)
>  
>  	if (reserved_base < dev_priv->mm.stolen_base ||
>  	    reserved_base + reserved_size > stolen_top) {
> -		phys_addr_t reserved_top = reserved_base +
> reserved_size;
> -		DRM_DEBUG_KMS("Stolen reserved area [%pa - %pa]
> outside stolen memory [%pa - %pa]\n",
> +		dma_addr_t reserved_top = reserved_base +
> reserved_size;
> +		DRM_DEBUG_KMS("Stolen reserved area [%pad - %pad]
> outside stolen memory [%pad - %pad]\n",
>  			      &reserved_base, &reserved_top,
>  			      &dev_priv->mm.stolen_base,
> &stolen_top);
>  		return 0;
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Sanity check the computed size and base of stolen memory

[Paulo Zanoni]

Em Sex, 2017-01-27 às 17:20 +0000, Chris Wilson escreveu:
> Just do a quick check that the stolen memory address range doesn't
> overflow our chosen integer type.

Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/i915_gem_stolen.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c
> b/drivers/gpu/drm/i915/i915_gem_stolen.c
> index 42bbc4b04fd6..37a26e1c9190 100644
> --- a/drivers/gpu/drm/i915/i915_gem_stolen.c
> +++ b/drivers/gpu/drm/i915/i915_gem_stolen.c
> @@ -189,7 +189,7 @@ static dma_addr_t i915_stolen_to_dma(struct
> drm_i915_private *dev_priv)
>  		base = tom - tseg_size - ggtt->stolen_size;
>  	}
>  
> -	if (base == 0)
> +	if (base == 0 || base + ggtt->stolen_size <= base)
>  		return 0;
>  
>  	/* make sure we don't clobber the GTT if it's within stolen
> memory */
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/2] drm/i915: Treat stolen memory as DMA addresses

[Chris Wilson]

On Fri, Jan 27, 2017 at 06:50:54PM -0200, Paulo Zanoni wrote:
> Em Sex, 2017-01-27 às 16:55 +0000, Chris Wilson escreveu:
> > The conversion of stolen to use phys_addr_t (from essentially u32)
> > sparked an interesting discussion. We treat stolen memory as only
> > accessible from the GPU (the DMA device) - an attempt to use it from
> > the
> > CPU will generate a MCE on gen6 onwards, although it is in theory a
> > physical address that can be dereferenced from the CPU as
> > demonstrated
> > by earlier generations. As such, using phys_addr_t has the wrong
> > connotations and as we pass the address into the DMA device via
> > dma_addr_t (through the scatterlists used to program the GTT
> > entries),
> > we should treat it as dma_addr_t throughout.
> 
> I'm not a specialist here, but from what I could learn/understand, this
> seems good. The patch seems to do what it says, so:
> 
> Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> But now that I looked some more at the code, I started to think about
> the following: shouldn't we convert our "stolen offset" variables from
> u32 to dma_addr_t or some other appropriate type? I mean, if we ever
> get 64 bit stolen pointers we may also get 64 bit stolen offsets... The
> i915_ggtt struct contains 4 of such u32 pointers.
> 
> For example, i915_ggtt->stolen_reserved_base is another pointer to
> stolen memory, it's generated directly from one of our drm_addr_t
> variables.
> 
> Of course, this could be a separate patch.

Oh yes, I advise you take gentle steps down the rabbit hole.

I thought the reserved_base was an offset, I may be wrong. Certainly the
limits and types are also good to review and be wary of for the future.
Something I've only really recently started trying to do is capture
these assumed limits in the code by checking for type overflows -
hopefully to catch the errors early. That would be a useful exercise
when reviewing the current code to annotate where we change types. Isn't
there a gcc flag for warning on type shrinkage?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Sanity check the computed size and base of stolen memory

[Joonas Lahtinen]

On pe, 2017-01-27 at 17:20 +0000, Chris Wilson wrote:
> Just do a quick check that the stolen memory address range doesn't
> overflow our chosen integer type.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>

<SNIP>

> @@ -189,7 +189,7 @@ static dma_addr_t i915_stolen_to_dma(struct drm_i915_private *dev_priv)
>  		base = tom - tseg_size - ggtt->stolen_size;
>  	}
>  
> -	if (base == 0)
> +	if (base == 0 || base + ggtt->stolen_size <= base)

range_overflows?

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Sanity check the computed size and base of stolen memory

[Chris Wilson]

On Mon, Jan 30, 2017 at 02:41:15PM +0200, Joonas Lahtinen wrote:
> On pe, 2017-01-27 at 17:20 +0000, Chris Wilson wrote:
> > Just do a quick check that the stolen memory address range doesn't
> > overflow our chosen integer type.
> > 
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> 
> <SNIP>
> 
> > @@ -189,7 +189,7 @@ static dma_addr_t i915_stolen_to_dma(struct drm_i915_private *dev_priv)
> >  		base = tom - tseg_size - ggtt->stolen_size;
> >  	}
> >  
> > -	if (base == 0)
> > +	if (base == 0 || base + ggtt->stolen_size <= base)
> 
> range_overflows?

if (base == 0 || range_overflows(base, ggtt->stolen_size, U64_MAX /* DMA_ADDR_MAX */))

Bleh.

#define add_overflows(A, B) __builtin_add_overflow_p((A), (B), (typeof((A) + (B)))0)

if (base == 0 || add_overflows(base, ggtt->stolen_size))
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Sanity check the computed size and base of stolen memory

[Joonas Lahtinen]

On ma, 2017-01-30 at 13:15 +0000, Chris Wilson wrote:
> 
> if (base == 0 || range_overflows(base, ggtt->stolen_size, U64_MAX /* DMA_ADDR_MAX */))
> 
> Bleh.
> 
> #define add_overflows(A, B) __builtin_add_overflow_p((A), (B), (typeof((A) + (B)))0)
> 
> if (base == 0 || add_overflows(base, ggtt->stolen_size))

Looks good.

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2] drm/i915: Sanity check the computed size and base of stolen memory

[Chris Wilson]

Just do a quick check that the stolen memory address range doesn't
overflow our chosen integer type.

v2: Add add_overflows() to utils with the promise that gcc7 can do this
better than C and then maybe it will have a proper definition in core.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_gem_stolen.c | 2 +-
 drivers/gpu/drm/i915/i915_utils.h      | 7 +++++++
 2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c
index 42bbc4b04fd6..f3abdc27c5dd 100644
--- a/drivers/gpu/drm/i915/i915_gem_stolen.c
+++ b/drivers/gpu/drm/i915/i915_gem_stolen.c
@@ -189,7 +189,7 @@ static dma_addr_t i915_stolen_to_dma(struct drm_i915_private *dev_priv)
 		base = tom - tseg_size - ggtt->stolen_size;
 	}
 
-	if (base == 0)
+	if (base == 0 || add_overflows(base, ggtt->stolen_size))
 		return 0;
 
 	/* make sure we don't clobber the GTT if it's within stolen memory */
diff --git a/drivers/gpu/drm/i915/i915_utils.h b/drivers/gpu/drm/i915/i915_utils.h
index 34020873e1f6..69e00c94ee13 100644
--- a/drivers/gpu/drm/i915/i915_utils.h
+++ b/drivers/gpu/drm/i915/i915_utils.h
@@ -25,6 +25,13 @@
 #ifndef __I915_UTILS_H
 #define __I915_UTILS_H
 
+#if GCC_VERSION >= 70000
+#define add_overflows(A, B) \
+	__builtin_add_overflow_p((A), (B), (typeof((A) + (B)))0)
+#else
+#define add_overflows(A, B) ((A) + (B) < (A))
+#endif
+
 #define range_overflows(start, size, max) ({ \
 	typeof(start) start__ = (start); \
 	typeof(size) size__ = (size); \
-- 
2.11.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.BAT: warning for series starting with [1/2] drm/i915: Treat stolen memory as DMA addresses (rev4)

[Patchwork]

== Series Details ==

Series: series starting with [1/2] drm/i915: Treat stolen memory as DMA addresses (rev4)
URL   : https://patchwork.freedesktop.org/series/18697/
State : warning

== Summary ==

Series 18697v4 Series without cover letter
https://patchwork.freedesktop.org/api/1.0/series/18697/revisions/4/mbox/

Test kms_busy:
        Subgroup basic-flip-default-c:
                pass       -> DMESG-WARN (fi-bsw-n3050)

fi-bdw-5557u     total:246  pass:232  dwarn:0   dfail:0   fail:0   skip:14 
fi-bsw-n3050     total:246  pass:206  dwarn:1   dfail:0   fail:0   skip:39 
fi-bxt-j4205     total:246  pass:224  dwarn:0   dfail:0   fail:0   skip:22 
fi-bxt-t5700     total:78   pass:65   dwarn:0   dfail:0   fail:0   skip:12 
fi-byt-j1900     total:246  pass:219  dwarn:0   dfail:0   fail:0   skip:27 
fi-byt-n2820     total:246  pass:215  dwarn:0   dfail:0   fail:0   skip:31 
fi-hsw-4770      total:246  pass:227  dwarn:0   dfail:0   fail:0   skip:19 
fi-hsw-4770r     total:246  pass:227  dwarn:0   dfail:0   fail:0   skip:19 
fi-ivb-3520m     total:246  pass:225  dwarn:0   dfail:0   fail:0   skip:21 
fi-ivb-3770      total:246  pass:225  dwarn:0   dfail:0   fail:0   skip:21 
fi-kbl-7500u     total:246  pass:223  dwarn:0   dfail:0   fail:2   skip:21 
fi-skl-6260u     total:246  pass:233  dwarn:0   dfail:0   fail:0   skip:13 
fi-skl-6700hq    total:246  pass:226  dwarn:0   dfail:0   fail:0   skip:20 
fi-skl-6700k     total:246  pass:221  dwarn:4   dfail:0   fail:0   skip:21 
fi-skl-6770hq    total:246  pass:233  dwarn:0   dfail:0   fail:0   skip:13 
fi-snb-2520m     total:246  pass:215  dwarn:0   dfail:0   fail:0   skip:31 
fi-snb-2600      total:246  pass:214  dwarn:0   dfail:0   fail:0   skip:32 

0e127a5125bdf8236d035abdb41567fcb843a934 drm-tip: 2017y-01m-30d-19h-59m-28s UTC integration manifest
a2d868b drm/i915: Sanity check the computed size and base of stolen memory
18634f3 drm/i915: Treat stolen memory as DMA addresses

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_3644/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2] drm/i915: Sanity check the computed size and base of stolen memory

[Joonas Lahtinen]

On ma, 2017-01-30 at 13:47 +0000, Chris Wilson wrote:
> Just do a quick check that the stolen memory address range doesn't
> overflow our chosen integer type.
> 
> v2: Add add_overflows() to utils with the promise that gcc7 can do this
> better than C and then maybe it will have a proper definition in core.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>

Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2] drm/i915: Sanity check the computed size and base of stolen memory

[Chris Wilson]

On Tue, Jan 31, 2017 at 12:13:47PM +0200, Joonas Lahtinen wrote:
> On ma, 2017-01-30 at 13:47 +0000, Chris Wilson wrote:
> > Just do a quick check that the stolen memory address range doesn't
> > overflow our chosen integer type.
> > 
> > v2: Add add_overflows() to utils with the promise that gcc7 can do this
> > better than C and then maybe it will have a proper definition in core.
> > 
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> 
> Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>

Applied, thanks,
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx