[Intel-gfx] [PATCH 06/11] drm/i915: refactor VBT validation

[Ville Syrjälä]

On Mon, Dec 14, 2015 at 12:50:50PM +0200, Jani Nikula wrote:
> Make the validation function a boolean operating on a buffer of given
> size, removing the extra pointer dances.
> 
> Move the OpRegion based VBT validation to intel_opregion_setup(), only
> initializing opregion->vbt if it's valid.
> 
> Signed-off-by: Jani Nikula <jani.nikula at intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h       |  1 +
>  drivers/gpu/drm/i915/intel_bios.c     | 63 ++++++++++++++++++-----------------
>  drivers/gpu/drm/i915/intel_opregion.c |  9 +++--
>  3 files changed, 40 insertions(+), 33 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index a2bd4c6a86a1..ef15f1710b50 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -3354,6 +3354,7 @@ extern void intel_i2c_reset(struct drm_device *dev);
>  
>  /* intel_bios.c */
>  int intel_bios_init(struct drm_device *dev);
> +bool intel_bios_is_valid_vbt(const void *buf, size_t size);
>  
>  /* intel_opregion.c */
>  #ifdef CONFIG_ACPI
> diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
> index f2b79b7d12b8..007b2b759600 100644
> --- a/drivers/gpu/drm/i915/intel_bios.c
> +++ b/drivers/gpu/drm/i915/intel_bios.c
> @@ -1221,37 +1221,40 @@ static const struct bdb_header *get_bdb_header(const struct vbt_header *vbt)
>  	return _vbt + vbt->bdb_offset;
>  }
>  
> -static const struct vbt_header *validate_vbt(const void *base,
> -					     size_t size,
> -					     const void *_vbt)
> +/**
> + * intel_bios_is_valid_vbt - does the given buffer contain a valid VBT
> + * @buf:	pointer to a buffer to validate
> + * @size:	size of the buffer
> + *
> + * Returns true on valid VBT.
> + */
> +bool intel_bios_is_valid_vbt(const void *buf, size_t size)
>  {
> -	size_t offset = _vbt - base;
> -	const struct vbt_header *vbt = _vbt;
> +	const struct vbt_header *vbt = buf;
>  	const struct bdb_header *bdb;
>  
>  	if (!vbt)
> -		return NULL;
> +		return false;
>  
> -	if (offset + sizeof(struct vbt_header) > size) {
> +	if (sizeof(struct vbt_header) > size) {
>  		DRM_DEBUG_DRIVER("VBT header incomplete\n");
> -		return NULL;
> +		return false;
>  	}
>  
>  	if (memcmp(vbt->signature, "$VBT", 4)) {
>  		DRM_DEBUG_DRIVER("VBT invalid signature\n");
> -		return NULL;
> +		return false;
>  	}
>  
> -	offset += vbt->bdb_offset;
> -	if (offset + sizeof(struct bdb_header) > size) {
> +	if (vbt->bdb_offset + sizeof(struct bdb_header) > size) {
>  		DRM_DEBUG_DRIVER("BDB header incomplete\n");
> -		return NULL;
> +		return false;
>  	}
>  
>  	bdb = get_bdb_header(vbt);
> -	if (offset + bdb->bdb_size > size) {
> +	if (vbt->bdb_offset + bdb->bdb_size > size) {
>  		DRM_DEBUG_DRIVER("BDB incomplete\n");
> -		return NULL;
> +		return false;
>  	}
>  
>  	return vbt;
> @@ -1259,26 +1262,27 @@ static const struct vbt_header *validate_vbt(const void *base,
>  
>  static const struct vbt_header *find_vbt(void __iomem *bios, size_t size)
>  {
> -	const struct vbt_header *vbt = NULL;
>  	size_t i;
>  
>  	/* Scour memory looking for the VBT signature. */
>  	for (i = 0; i + 4 < size; i++) {
> -		if (ioread32(bios + i) == *((const u32 *) "$VBT")) {
> -			/*
> -			 * This is the one place where we explicitly discard the
> -			 * address space (__iomem) of the BIOS/VBT. From now on
> -			 * everything is based on 'base', and treated as regular
> -			 * memory.
> -			 */
> -			void *_bios = (void __force *) bios;
> +		void *vbt;
>  
> -			vbt = validate_vbt(_bios, size, _bios + i);
> -			break;
> -		}
> +		if (ioread32(bios + 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 *) bios + i;
> +		if (intel_bios_is_valid_vbt(vbt, size - i))
> +			return vbt;
> +
> +		break;
>  	}
>  
> -	return vbt;
> +	return NULL;
>  }
>  
>  /**
> @@ -1295,7 +1299,7 @@ intel_bios_init(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct pci_dev *pdev = dev->pdev;
> -	const struct vbt_header *vbt;
> +	const struct vbt_header *vbt = dev_priv->opregion.vbt;
>  	const struct bdb_header *bdb;
>  	u8 __iomem *bios = NULL;
>  
> @@ -1304,9 +1308,6 @@ intel_bios_init(struct drm_device *dev)
>  
>  	init_vbt_defaults(dev_priv);
>  
> -	/* XXX Should this validation be moved to intel_opregion.c? */
> -	vbt = validate_vbt(dev_priv->opregion.header, OPREGION_SIZE,
> -			   dev_priv->opregion.vbt);
>  	if (vbt) {
>  		DRM_DEBUG_KMS("Using VBT from ACPI OpRegion: %20s\n",
>  			      vbt->signature);

Maybe the debug message should be moved too? It looks a bit out of place
after the validation gets moved.

> diff --git a/drivers/gpu/drm/i915/intel_opregion.c b/drivers/gpu/drm/i915/intel_opregion.c
> index 5b9fc790d300..9f992e7c6185 100644
> --- a/drivers/gpu/drm/i915/intel_opregion.c
> +++ b/drivers/gpu/drm/i915/intel_opregion.c
> @@ -986,8 +986,13 @@ int intel_opregion_setup(struct drm_device *dev)
>  	if (mboxes & MBOX_ASLE_EXT)
>  		DRM_DEBUG_DRIVER("ASLE extension supported\n");
>  
> -	if (!dmi_check_system(intel_no_opregion_vbt))
> -		opregion->vbt = base + OPREGION_VBT_OFFSET;
> +	if (!dmi_check_system(intel_no_opregion_vbt)) {
> +		void *vbt = base + OPREGION_VBT_OFFSET;
> +		u32 vbt_size = OPREGION_SIZE - OPREGION_VBT_OFFSET;
> +
> +		if (intel_bios_is_valid_vbt(vbt, vbt_size))
> +			opregion->vbt = vbt;
> +	}
>  
>  	return 0;
>  
> -- 
> 2.1.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC