[3/3] drm/i915/bios: do not discard address space
diff mbox series

Message ID 20191108211353.22288-3-lucas.demarchi@intel.com
State New
Headers show
Series
  • [1/3] drm/i915/bios: rename bios to oprom when mapping pci rom
Related show

Commit Message

Lucas De Marchi Nov. 8, 2019, 9:13 p.m. UTC
When we map the VBT through pci_map_rom() we may not be allowed
to simply discard the address space and go on reading the memory.
That doesn't work on my test system, but by dumping the rom via
sysfs I can can get the correct vbt. So change our find_vbt() to do
the same as done by pci_read_rom(), i.e. use memcpy_fromio().

v2: the just the minimal changes by not bothering with the unaligned io
reads: this can be done on top (from Ville and Jani)

Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
---
 drivers/gpu/drm/i915/display/intel_bios.c | 51 +++++++++++++++++------
 1 file changed, 39 insertions(+), 12 deletions(-)

Comments

Ville Syrjälä Nov. 15, 2019, 5:40 p.m. UTC | #1
On Fri, Nov 08, 2019 at 01:13:53PM -0800, Lucas De Marchi wrote:
> When we map the VBT through pci_map_rom() we may not be allowed
> to simply discard the address space and go on reading the memory.
> That doesn't work on my test system, but by dumping the rom via
> sysfs I can can get the correct vbt. So change our find_vbt() to do
> the same as done by pci_read_rom(), i.e. use memcpy_fromio().
> 
> v2: the just the minimal changes by not bothering with the unaligned io
> reads: this can be done on top (from Ville and Jani)
> 
> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_bios.c | 51 +++++++++++++++++------
>  1 file changed, 39 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_bios.c b/drivers/gpu/drm/i915/display/intel_bios.c
> index c79781e1ccbf..c079febae9c8 100644
> --- a/drivers/gpu/drm/i915/display/intel_bios.c
> +++ b/drivers/gpu/drm/i915/display/intel_bios.c
> @@ -1811,28 +1811,52 @@ bool intel_bios_is_valid_vbt(const void *buf, size_t size)
>  	return vbt;
>  }
>  
> -static const struct vbt_header *find_vbt(void __iomem *oprom, size_t size)
> +static const struct vbt_header *copy_vbt(void __iomem *oprom, size_t size)
>  {
> +	void __iomem *p = NULL;
> +	struct vbt_header *vbt;
> +	u16 vbt_size;
>  	size_t i;
>  
>  	/* Scour memory looking for the VBT signature. */
>  	for (i = 0; i + 4 < size; i++) {
> -		void *vbt;
> -
>  		if (ioread32(oprom + i) != *((const u32 *)"$VBT"))
>  			continue;
>  
> -		/*
> -		 * This is the one place where we explicitly discard the address
> -		 * space (__iomem) of the BIOS/VBT.
> -		 */
> -		vbt = (void __force *)oprom + i;
> -		if (intel_bios_is_valid_vbt(vbt, size - i))
> -			return vbt;
> -
> +		p = oprom + i;
> +		size -= i;
>  		break;
>  	}
>  
> +	if (!p)
> +		return NULL;
> +
> +	if (sizeof(struct vbt_header) > size) {
> +		DRM_DEBUG_DRIVER("VBT header incomplete\n");
> +		return NULL;
> +	}
> +
> +	vbt_size = ioread16(p + offsetof(struct vbt_header, vbt_size));
> +	if (vbt_size > size) {
> +		DRM_DEBUG_DRIVER("VBT incomplete (vbt_size overflows)\n");
> +		return NULL;
> +	}
> +
> +	/* The rest will be validated by intel_bios_is_valid_vbt() */
> +	vbt = kmalloc(vbt_size, GFP_KERNEL);
> +	if (!vbt)
> +		return NULL;
> +
> +	memcpy_fromio(vbt, p, vbt_size);
> +
> +	if (!intel_bios_is_valid_vbt(vbt, vbt_size))
> +		goto err_free_vbt;
> +
> +	return vbt;
> +
> +err_free_vbt:
> +	kfree(vbt);
> +
>  	return NULL;
>  }
>  
> @@ -1866,7 +1890,7 @@ void intel_bios_init(struct drm_i915_private *dev_priv)
>  		if (!oprom)
>  			goto out;
>  
> -		vbt = find_vbt(oprom, size);
> +		vbt = copy_vbt(oprom, size);
>  		if (!vbt)
>  			goto out;
>  
> @@ -1902,6 +1926,9 @@ void intel_bios_init(struct drm_i915_private *dev_priv)
>  
>  	if (oprom)
>  		pci_unmap_rom(pdev, oprom);

lgtm
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

As a followup we could now move the rom map/unamp into copy_vbt() as
there's no longer any need to keep it mapped across the whole thing.

> +
> +	if (vbt != dev_priv->opregion.vbt)
> +		kfree(vbt);
>  }
>  
>  /**
> -- 
> 2.24.0
Lucas De Marchi Nov. 15, 2019, 5:45 p.m. UTC | #2
On Fri, Nov 15, 2019 at 07:40:15PM +0200, Ville Syrjälä wrote:
>On Fri, Nov 08, 2019 at 01:13:53PM -0800, Lucas De Marchi wrote:
>> When we map the VBT through pci_map_rom() we may not be allowed
>> to simply discard the address space and go on reading the memory.
>> That doesn't work on my test system, but by dumping the rom via
>> sysfs I can can get the correct vbt. So change our find_vbt() to do
>> the same as done by pci_read_rom(), i.e. use memcpy_fromio().
>>
>> v2: the just the minimal changes by not bothering with the unaligned io
>> reads: this can be done on top (from Ville and Jani)
>>
>> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
>> ---
>>  drivers/gpu/drm/i915/display/intel_bios.c | 51 +++++++++++++++++------
>>  1 file changed, 39 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/display/intel_bios.c b/drivers/gpu/drm/i915/display/intel_bios.c
>> index c79781e1ccbf..c079febae9c8 100644
>> --- a/drivers/gpu/drm/i915/display/intel_bios.c
>> +++ b/drivers/gpu/drm/i915/display/intel_bios.c
>> @@ -1811,28 +1811,52 @@ bool intel_bios_is_valid_vbt(const void *buf, size_t size)
>>  	return vbt;
>>  }
>>
>> -static const struct vbt_header *find_vbt(void __iomem *oprom, size_t size)
>> +static const struct vbt_header *copy_vbt(void __iomem *oprom, size_t size)

just noticed this shouldn't be const anymore, since the caller is
supposed to free the return

>>  {
>> +	void __iomem *p = NULL;
>> +	struct vbt_header *vbt;
>> +	u16 vbt_size;
>>  	size_t i;
>>
>>  	/* Scour memory looking for the VBT signature. */
>>  	for (i = 0; i + 4 < size; i++) {
>> -		void *vbt;
>> -
>>  		if (ioread32(oprom + i) != *((const u32 *)"$VBT"))
>>  			continue;
>>
>> -		/*
>> -		 * This is the one place where we explicitly discard the address
>> -		 * space (__iomem) of the BIOS/VBT.
>> -		 */
>> -		vbt = (void __force *)oprom + i;
>> -		if (intel_bios_is_valid_vbt(vbt, size - i))
>> -			return vbt;
>> -
>> +		p = oprom + i;
>> +		size -= i;
>>  		break;
>>  	}
>>
>> +	if (!p)
>> +		return NULL;
>> +
>> +	if (sizeof(struct vbt_header) > size) {
>> +		DRM_DEBUG_DRIVER("VBT header incomplete\n");
>> +		return NULL;
>> +	}
>> +
>> +	vbt_size = ioread16(p + offsetof(struct vbt_header, vbt_size));
>> +	if (vbt_size > size) {
>> +		DRM_DEBUG_DRIVER("VBT incomplete (vbt_size overflows)\n");
>> +		return NULL;
>> +	}
>> +
>> +	/* The rest will be validated by intel_bios_is_valid_vbt() */
>> +	vbt = kmalloc(vbt_size, GFP_KERNEL);
>> +	if (!vbt)
>> +		return NULL;
>> +
>> +	memcpy_fromio(vbt, p, vbt_size);
>> +
>> +	if (!intel_bios_is_valid_vbt(vbt, vbt_size))
>> +		goto err_free_vbt;
>> +
>> +	return vbt;
>> +
>> +err_free_vbt:
>> +	kfree(vbt);
>> +
>>  	return NULL;
>>  }
>>
>> @@ -1866,7 +1890,7 @@ void intel_bios_init(struct drm_i915_private *dev_priv)
>>  		if (!oprom)
>>  			goto out;
>>
>> -		vbt = find_vbt(oprom, size);
>> +		vbt = copy_vbt(oprom, size);
>>  		if (!vbt)
>>  			goto out;
>>
>> @@ -1902,6 +1926,9 @@ void intel_bios_init(struct drm_i915_private *dev_priv)
>>
>>  	if (oprom)
>>  		pci_unmap_rom(pdev, oprom);
>
>lgtm
>Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
>As a followup we could now move the rom map/unamp into copy_vbt() as
>there's no longer any need to keep it mapped across the whole thing.

yep, I will add this and a tentative to avoid the unaligned reads

thanks
Lucas De Marchi

>
>> +
>> +	if (vbt != dev_priv->opregion.vbt)
>> +		kfree(vbt);
>>  }
>>
>>  /**
>> --
>> 2.24.0
>
>-- 
>Ville Syrjälä
>Intel
>_______________________________________________
>Intel-gfx mailing list
>Intel-gfx@lists.freedesktop.org
>https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Patch
diff mbox series

diff --git a/drivers/gpu/drm/i915/display/intel_bios.c b/drivers/gpu/drm/i915/display/intel_bios.c
index c79781e1ccbf..c079febae9c8 100644
--- a/drivers/gpu/drm/i915/display/intel_bios.c
+++ b/drivers/gpu/drm/i915/display/intel_bios.c
@@ -1811,28 +1811,52 @@  bool intel_bios_is_valid_vbt(const void *buf, size_t size)
 	return vbt;
 }
 
-static const struct vbt_header *find_vbt(void __iomem *oprom, size_t size)
+static const struct vbt_header *copy_vbt(void __iomem *oprom, size_t size)
 {
+	void __iomem *p = NULL;
+	struct vbt_header *vbt;
+	u16 vbt_size;
 	size_t i;
 
 	/* Scour memory looking for the VBT signature. */
 	for (i = 0; i + 4 < size; i++) {
-		void *vbt;
-
 		if (ioread32(oprom + i) != *((const u32 *)"$VBT"))
 			continue;
 
-		/*
-		 * This is the one place where we explicitly discard the address
-		 * space (__iomem) of the BIOS/VBT.
-		 */
-		vbt = (void __force *)oprom + i;
-		if (intel_bios_is_valid_vbt(vbt, size - i))
-			return vbt;
-
+		p = oprom + i;
+		size -= i;
 		break;
 	}
 
+	if (!p)
+		return NULL;
+
+	if (sizeof(struct vbt_header) > size) {
+		DRM_DEBUG_DRIVER("VBT header incomplete\n");
+		return NULL;
+	}
+
+	vbt_size = ioread16(p + offsetof(struct vbt_header, vbt_size));
+	if (vbt_size > size) {
+		DRM_DEBUG_DRIVER("VBT incomplete (vbt_size overflows)\n");
+		return NULL;
+	}
+
+	/* The rest will be validated by intel_bios_is_valid_vbt() */
+	vbt = kmalloc(vbt_size, GFP_KERNEL);
+	if (!vbt)
+		return NULL;
+
+	memcpy_fromio(vbt, p, vbt_size);
+
+	if (!intel_bios_is_valid_vbt(vbt, vbt_size))
+		goto err_free_vbt;
+
+	return vbt;
+
+err_free_vbt:
+	kfree(vbt);
+
 	return NULL;
 }
 
@@ -1866,7 +1890,7 @@  void intel_bios_init(struct drm_i915_private *dev_priv)
 		if (!oprom)
 			goto out;
 
-		vbt = find_vbt(oprom, size);
+		vbt = copy_vbt(oprom, size);
 		if (!vbt)
 			goto out;
 
@@ -1902,6 +1926,9 @@  void intel_bios_init(struct drm_i915_private *dev_priv)
 
 	if (oprom)
 		pci_unmap_rom(pdev, oprom);
+
+	if (vbt != dev_priv->opregion.vbt)
+		kfree(vbt);
 }
 
 /**