Re: [PATCH 06/11] drm/i915: refactor VBT validation

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Mon, Dec 14, 2015 at 04:34:50PM +0200, Jani Nikula wrote:
> On Mon, 14 Dec 2015, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> > 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@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.
> 
> This is intentional (see patch 4). In a way this is where the decision
> is made to use the opregion VBT. The opregion code just validates it,
> but doesn't actually *use* it.

But when you add the RVDA thing, the debug print for that is in the
opregion code, so with that we get two debug prints?

> 
> BR,
> Jani.
> 
> 
> 
> >
> >> 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@lists.freedesktop.org
> >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Jani Nikula, Intel Open Source Technology Center

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