Re: [PATCH v9 6/7] drm/i915/guc: Check the locking status of GuC WOPCM registers

["Michal Wajdeczko" <michal.wajdeczko@intel.com>]

On Fri, 09 Feb 2018 00:03:54 +0100, Jackie Li <yaodong.li@intel.com> wrote:

Few more comments in addition to Joonas/Chris

> GuC WOPCM registers are write-once registers. Current driver code  
> accesses
> these registers without checking the accessibility to these registers  
> which
> will lead to unpredictable driver behaviors if these registers were touch
> by other components (such as faulty BIOS code).
>
> This patch moves the GuC WOPCM registers updating code into
> intel_guc_wopcm.c and adds check before and after the update to GuC WOPCM
> registers so that we can make sure the driver is in a known state before
> and after writing to these write-once registers.
>
> v6:
>  - Made sure module reloading won't bug the kernel while doing
>    locking status checking
>
> v7:
>  - Fixed patch format issues
>
> v8:
>  - Fixed coding style issue on register lock bit macro definition (Sagar)
>
> v9:
>  - Avoided to use redundant !! to cast uint to bool (Chris)
>  - Return error code instead of GEM_BUG_ON for locked with invalid  
> register
>    values case (Sagar)
>  - Updated guc_wopcm_hw_init to use guc_wopcm as first parameter (Michal)
>  - Added code to set and validate the HuC_LOADING_AGENT_GUC bit in GuC
>    WOPCM offset register based on the presence of HuC firmware (Michal)
>  - Use bit fields instead of macros for GuC WOPCM flags (Michal)
>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Signed-off-by: Jackie Li <yaodong.li@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_guc_reg.h   |   2 +
>  drivers/gpu/drm/i915/intel_guc_wopcm.c | 117  
> ++++++++++++++++++++++++++++++++-
>  drivers/gpu/drm/i915/intel_guc_wopcm.h |  12 +++-
>  drivers/gpu/drm/i915/intel_uc.c        |   9 ++-
>  4 files changed, 130 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_guc_reg.h  
> b/drivers/gpu/drm/i915/intel_guc_reg.h
> index de4f78b..170d9cd 100644
> --- a/drivers/gpu/drm/i915/intel_guc_reg.h
> +++ b/drivers/gpu/drm/i915/intel_guc_reg.h
> @@ -66,6 +66,8 @@
>  #define   UOS_MOVE			  (1<<4)
>  #define   START_DMA			  (1<<0)
>  #define DMA_GUC_WOPCM_OFFSET		_MMIO(0xc340)
> +#define   GUC_WOPCM_OFFSET_SHIFT	14
> +#define   GUC_WOPCM_OFFSET_MASK		  (0x3ffff << GUC_WOPCM_OFFSET_SHIFT)
>  #define   HUC_LOADING_AGENT_VCR		  (0<<1)
>  #define   HUC_LOADING_AGENT_GUC		  (1<<1)
>  #define GUC_MAX_IDLE_COUNT		_MMIO(0xC3E4)
> diff --git a/drivers/gpu/drm/i915/intel_guc_wopcm.c  
> b/drivers/gpu/drm/i915/intel_guc_wopcm.c
> index 2e8e9ec..0af435a 100644
> --- a/drivers/gpu/drm/i915/intel_guc_wopcm.c
> +++ b/drivers/gpu/drm/i915/intel_guc_wopcm.c
> @@ -90,6 +90,69 @@ static inline int guc_wopcm_size_check(struct  
> intel_guc *guc, u32 huc_fw_size)
>  	return 0;
>  }
> +static inline bool __reg_locked(struct drm_i915_private *dev_priv,
> +				i915_reg_t reg)
> +{
> +	/* Set of bit-0 means this write-once register is locked. */
> +	return I915_READ(reg) & BIT(0);
> +}

Hmm, I'm still not happy as not all registers supports write-once bit.
What about something more generic/robust

static inline bool
__reg_test(struct drm_i915_private *dev_priv, i915_reg_t reg, u32 mask,  
u32 value)
{
	return (I915_READ(reg) & mask) == value;
}

static inline bool
__reg_is_set(struct drm_i915_private *dev_priv, i915_reg_t reg, u32 value)
{
	return __reg_test(dev_priv, reg, value, value);
}
...
#define WOPCM_OFFSET_VALID  (1<<0)
...
#define WOPCM_LOCKED        (1<<0)
...
locked = __reg_is_set(i915, GUC_WOPCM_SIZE, WOPCM_LOCKED);
locked = __reg_is_set(i915, DMA_GUC_WOPCM_OFFSET, WOPCM_OFFSET_VALID);

> +
> +static inline bool guc_wopcm_locked(struct intel_guc *guc)
> +{
> +	struct drm_i915_private *i915 = guc_to_i915(guc);
> +	bool size_reg_locked = __reg_locked(i915, GUC_WOPCM_SIZE);
> +	bool offset_reg_locked = __reg_locked(i915, DMA_GUC_WOPCM_OFFSET);
> +
> +	return size_reg_locked && offset_reg_locked;
> +}
> +
> +static inline void guc_wopcm_hw_update(struct intel_guc *guc)
> +{
> +	struct drm_i915_private *dev_priv = guc_to_i915(guc);
> +	u32 offset_reg_val;
> +
> +	/* GuC WOPCM registers should be unlocked at this point. */
> +	GEM_BUG_ON(__reg_locked(dev_priv, GUC_WOPCM_SIZE));
> +	GEM_BUG_ON(__reg_locked(dev_priv, DMA_GUC_WOPCM_OFFSET));

I'm not sure that GEM_BUG_ON can be used on bits that we don't
fully control

> +
> +	offset_reg_val = guc->wopcm.offset;
> +	if (guc->wopcm.need_load_huc_fw)
> +		offset_reg_val |= HUC_LOADING_AGENT_GUC;
> +
> +	I915_WRITE(GUC_WOPCM_SIZE, guc->wopcm.size);
> +	I915_WRITE(DMA_GUC_WOPCM_OFFSET, offset_reg_val);
> +
> +	GEM_BUG_ON(!__reg_locked(dev_priv, GUC_WOPCM_SIZE));
> +	GEM_BUG_ON(!__reg_locked(dev_priv, DMA_GUC_WOPCM_OFFSET));
> +}
> +
> +static inline bool guc_wopcm_regs_valid(struct intel_guc *guc)

can't we always have intel_guc_wopcm as first param ?

> +{
> +	struct drm_i915_private *dev_priv = guc_to_i915(guc);
> +	u32 size, offset;
> +	bool guc_loads_huc;
> +	u32 reg_val;
> +
> +	reg_val = I915_READ(GUC_WOPCM_SIZE);
> +	/* GuC WOPCM size should be always multiple of 4K pages. */
> +	size = reg_val & PAGE_MASK;
> +
> +	reg_val = I915_READ(DMA_GUC_WOPCM_OFFSET);
> +	guc_loads_huc = reg_val & HUC_LOADING_AGENT_GUC;
> +	offset = reg_val & GUC_WOPCM_OFFSET_MASK;
> +
> +	if (guc->wopcm.need_load_huc_fw && !guc_loads_huc)
> +		return false;
> +
> +	return (size == guc->wopcm.size) && (offset == guc->wopcm.offset);
> +}
> +
> +static inline
> +struct intel_guc *guc_wopcm_to_guc(struct intel_guc_wopcm *guc_wopcm)
> +{
> +	return container_of(guc_wopcm, struct intel_guc, wopcm);
> +}
> +
>  /**
>   * intel_guc_wopcm_init() - Initialize the GuC WOPCM.
>   * @guc_wopcm: intel_guc_wopcm..
> @@ -108,12 +171,13 @@ static inline int guc_wopcm_size_check(struct  
> intel_guc *guc, u32 huc_fw_size)
>  int intel_guc_wopcm_init(struct intel_guc_wopcm *guc_wopcm, u32  
> guc_fw_size,
>  			 u32 huc_fw_size)
>  {
> -	struct intel_guc *guc =
> -		container_of(guc_wopcm, struct intel_guc, wopcm);
> +	struct intel_guc *guc = guc_wopcm_to_guc(guc_wopcm);
>  	u32 reserved = guc_wopcm_context_reserved_size(guc);
>  	u32 offset, size, top;
>  	int err;
> +	GEM_BUG_ON(guc->wopcm.valid);
> +
>  	if (!guc_fw_size)
>  		return -EINVAL;
> @@ -147,6 +211,8 @@ int intel_guc_wopcm_init(struct intel_guc_wopcm  
> *guc_wopcm, u32 guc_fw_size,
>  	guc->wopcm.offset = offset;
>  	guc->wopcm.size = size;
>  	guc->wopcm.top = top;
> +	/* Use GuC to load HuC firmware if HuC firmware is present. */
> +	guc->wopcm.need_load_huc_fw = huc_fw_size ? 1 : 0;
> 	/* Check platform specific restrictions */
>  	err = guc_wopcm_size_check(guc, huc_fw_size);
> @@ -160,3 +226,50 @@ int intel_guc_wopcm_init(struct intel_guc_wopcm  
> *guc_wopcm, u32 guc_fw_size,
> 	return 0;
>  }
> +
> +/**
> + * intel_guc_wopcm_init_hw() - Setup GuC WOPCM registers.
> + * @guc_wopcm: intel_guc_wopcm.
> + *
> + * Setup the GuC WOPCM size and offset registers with the stored  
> values. It will
> + * also check the registers locking status to determine whether these  
> registers
> + * are unlocked and can be updated.
> + *
> + * Return: 0 on success. -EINVAL if registers were locked with  
> incorrect values.
> + */
> +int intel_guc_wopcm_init_hw(struct intel_guc_wopcm *guc_wopcm)
> +{
> +	struct intel_guc *guc = guc_wopcm_to_guc(guc_wopcm);
> +	bool locked = guc_wopcm_locked(guc);
> +
> +	GEM_BUG_ON(!guc->wopcm.valid);
> +
> +	/*
> +	 * Bug if driver hasn't updated the HW Registers and GuC WOPCM has been
> +	 * locked. Return directly if WOPCM was locked and we have updated
> +	 * the registers.
> +	 */
> +	if (locked) {
> +		if (guc->wopcm.hw_updated)
> +			return 0;
> +
> +		/*
> +		 * Mark as updated if registers contained correct values.
> +		 * This will happen while reloading the driver module without
> +		 * rebooting the system.
> +		 */
> +		if (guc_wopcm_regs_valid(guc))
> +			goto out;
> +
> +		/* Register locked without valid values. Abort HW init. */
> +		return -EINVAL;

I'm not sure that EINVAL is correct error code here ... maybe EACCES ?

> +	}
> +
> +	/* Always update registers when GuC WOPCM is not locked. */
> +	guc_wopcm_hw_update(guc);
> +
> +out:
> +	guc->wopcm.hw_updated = 1;
> +
> +	return 0;
> +}
> diff --git a/drivers/gpu/drm/i915/intel_guc_wopcm.h  
> b/drivers/gpu/drm/i915/intel_guc_wopcm.h
> index 1c5ffeb..471fb8e 100644
> --- a/drivers/gpu/drm/i915/intel_guc_wopcm.h
> +++ b/drivers/gpu/drm/i915/intel_guc_wopcm.h
> @@ -86,7 +86,9 @@ struct intel_guc;
>   * @offset: GuC WOPCM offset from the WOPCM base.
>   * @size: size of GuC WOPCM for GuC firmware.
>   * @top: start of the non-GuC WOPCM memory.
> - * @valid: whether this structure contains valid (1-valid, 0-invalid)  
> info.
> + * @valid: whether the values in this struct are valid.
> + * @hw_updated: GuC WOPCM registers has been updated with values in  
> this struct.
> + * @need_load_huc_fw: whether need to configure GuC to load HuC  
> firmware.
>   *
>   * We simply use this structure to track the GuC use of WOPCM. The  
> layout of
>   * WOPCM would be defined by writing to GuC WOPCM offset and size  
> registers.
> @@ -95,7 +97,11 @@ struct intel_guc_wopcm {
>  	u32 offset;
>  	u32 size;
>  	u32 top;
> -	u32 valid;
> +
> +	/* GuC WOPCM flags below. */
> +	u32 valid:1;
> +	u32 hw_updated:1;
> +	u32 need_load_huc_fw:1;
>  };
> /**
> @@ -114,5 +120,5 @@ static inline void intel_guc_wopcm_init_early(struct  
> intel_guc_wopcm *guc_wopcm)
> int intel_guc_wopcm_init(struct intel_guc_wopcm *guc_wopcm, u32 guc_size,
>  			 u32 huc_size);
> -
> +int intel_guc_wopcm_init_hw(struct intel_guc_wopcm *guc_wopcm);
>  #endif
> diff --git a/drivers/gpu/drm/i915/intel_uc.c  
> b/drivers/gpu/drm/i915/intel_uc.c
> index c842f36..8938096 100644
> --- a/drivers/gpu/drm/i915/intel_uc.c
> +++ b/drivers/gpu/drm/i915/intel_uc.c
> @@ -343,14 +343,13 @@ int intel_uc_init_hw(struct drm_i915_private  
> *dev_priv)
> 	GEM_BUG_ON(!HAS_GUC(dev_priv));
> +	ret = intel_guc_wopcm_init_hw(&guc->wopcm);

Again, extra error path without single log message...

> +	if (ret)
> +		goto err_out;
> +
>  	guc_disable_communication(guc);
>  	gen9_reset_guc_interrupts(dev_priv);
> -	/* init WOPCM */
> -	I915_WRITE(GUC_WOPCM_SIZE, guc->wopcm.size);
> -	I915_WRITE(DMA_GUC_WOPCM_OFFSET,
> -		   guc->wopcm.offset | HUC_LOADING_AGENT_GUC);
> -
>  	/* WaEnableuKernelHeaderValidFix:skl */
>  	/* WaEnableGuCBootHashCheckNotSet:skl,bxt,kbl */
>  	if (IS_GEN9(dev_priv))

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