[Intel-gfx] [PATCH v11 5/6] drm/i915/guc: Check the locking status of GuC WOPCM registers
Michal Wajdeczko
michal.wajdeczko at intel.com
Thu Mar 1 13:37:26 UTC 2018
On Thu, 01 Mar 2018 02:01:39 +0100, Jackie Li <yaodong.li at intel.com> wrote:
> 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
s/intel_guc_wopcm/intel_wopcm
s/before and after/after
> 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)
>
> v10:
> - Refined variable names, removed redundant comments (Joonas)
> - Introduced lockable_reg to handle the write once register write and
> propagate the write error to caller (Joonas)
> - Used lockable_reg abstraction to avoid locking bit check on generic
> i915_reg_t (Michal)
> - Added log message for error paths (Michal)
> - Removed hw_updated flag and only relies on real hardware status
>
> v11:
> - Replaced lockable_reg with simplified function (Michal)
> - Used new macros for locking bits of WOPCM size/offset registers
> instead
> of using BIT(0) directly (Michal)
> - use intel_wopcm_init_hw() called from intel_gem_init_hw() to do GuC
> WOPCM register setup instead of calling from intel_uc_init_hw()
> (Michal)
>
> BSpec: 10875, 10833
>
> Cc: Michal Wajdeczko <michal.wajdeczko at intel.com>
> Cc: Chris Wilson <chris at chris-wilson.co.uk>
> Cc: Joonas Lahtinen <joonas.lahtinen at linux.intel.com>
> Signed-off-by: Jackie Li <yaodong.li at intel.com>
> ---
> drivers/gpu/drm/i915/i915_gem.c | 6 ++++
> drivers/gpu/drm/i915/intel_guc_reg.h | 3 ++
> drivers/gpu/drm/i915/intel_uc.c | 5 ---
> drivers/gpu/drm/i915/intel_wopcm.c | 63
> ++++++++++++++++++++++++++++++++++++
> drivers/gpu/drm/i915/intel_wopcm.h | 1 +
> 5 files changed, 73 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem.c
> b/drivers/gpu/drm/i915/i915_gem.c
> index d31ad0b..662d670 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -5122,6 +5122,12 @@ int i915_gem_init_hw(struct drm_i915_private
> *dev_priv)
> goto out;
> }
> + ret = intel_wopcm_init_hw(&dev_priv->wopcm);
> + if (ret) {
> + DRM_ERROR("Enabling WOPCM failed (%d)\n", ret);
> + goto out;
> + }
> +
> /* We can't enable contexts until all firmware is loaded */
> ret = intel_uc_init_hw(dev_priv);
> if (ret) {
> diff --git a/drivers/gpu/drm/i915/intel_guc_reg.h
> b/drivers/gpu/drm/i915/intel_guc_reg.h
> index 01963d0..d860847 100644
> --- a/drivers/gpu/drm/i915/intel_guc_reg.h
> +++ b/drivers/gpu/drm/i915/intel_guc_reg.h
> @@ -66,15 +66,18 @@
> #define UOS_MOVE (1<<4)
> #define START_DMA (1<<0)
> #define DMA_GUC_WOPCM_OFFSET _MMIO(0xc340)
> +#define GUC_WOPCM_OFFSET_VALID (1<<0)
> #define HUC_LOADING_AGENT_VCR (0<<1)
> #define HUC_LOADING_AGENT_GUC (1<<1)
> #define GUC_WOPCM_OFFSET_SHIFT 14
> +#define GUC_WOPCM_OFFSET_MASK (0x3ffff << GUC_WOPCM_OFFSET_SHIFT)
> #define GUC_MAX_IDLE_COUNT _MMIO(0xC3E4)
> #define HUC_STATUS2 _MMIO(0xD3B0)
> #define HUC_FW_VERIFIED (1<<7)
> #define GUC_WOPCM_SIZE _MMIO(0xc050)
> +#define GUC_WOPCM_SIZE_LOCKED (1<<0)
> #define GUC_WOPCM_SIZE_SHIFT 12
> #define GUC_WOPCM_SIZE_MASK (0xfffff << GUC_WOPCM_SIZE_SHIFT)
> diff --git a/drivers/gpu/drm/i915/intel_uc.c
> b/drivers/gpu/drm/i915/intel_uc.c
> index 964e49f..58186f2 100644
> --- a/drivers/gpu/drm/i915/intel_uc.c
> +++ b/drivers/gpu/drm/i915/intel_uc.c
> @@ -341,11 +341,6 @@ int intel_uc_init_hw(struct drm_i915_private
> *dev_priv)
> guc_disable_communication(guc);
> gen9_reset_guc_interrupts(dev_priv);
> - /* init WOPCM */
> - I915_WRITE(GUC_WOPCM_SIZE, dev_priv->wopcm.guc.size);
> - I915_WRITE(DMA_GUC_WOPCM_OFFSET,
> - dev_priv->wopcm.guc.base | HUC_LOADING_AGENT_GUC);
> -
> /* WaEnableuKernelHeaderValidFix:skl */
> /* WaEnableGuCBootHashCheckNotSet:skl,bxt,kbl */
> if (IS_GEN9(dev_priv))
> diff --git a/drivers/gpu/drm/i915/intel_wopcm.c
> b/drivers/gpu/drm/i915/intel_wopcm.c
> index b30d7ff..2d9103a 100644
> --- a/drivers/gpu/drm/i915/intel_wopcm.c
> +++ b/drivers/gpu/drm/i915/intel_wopcm.c
> @@ -219,3 +219,66 @@ int intel_wopcm_init(struct intel_wopcm *wopcm)
> return 0;
> }
> +
> +static inline int write_and_verify(struct drm_i915_private *dev_priv,
> + i915_reg_t reg, u32 val, u32 mask,
> + u32 locked_bit)
> +{
> + u32 reg_val;
> +
> + GEM_BUG_ON(val & ~mask);
> +
> + I915_WRITE(reg, val);
> +
> + reg_val = I915_READ(reg);
> +
> + return (reg_val & mask) != (val | locked_bit) ? -EIO : 0;
> +}
> +
> +/**
> + * intel_wopcm_init_hw() - Setup GuC WOPCM registers.
> + * @wopcm: pointer to intel_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.
This comment is not fully valid any more, as we only verify *after* write.
> + *
> + * Return: 0 on success. -EIO if registers were locked with incorrect
> values.
> + */
> +int intel_wopcm_init_hw(struct intel_wopcm *wopcm)
> +{
> + struct drm_i915_private *dev_priv = wopcm_to_i915(wopcm);
> + u32 huc_agent;
> + u32 mask;
> + int err;
> +
> + if (!USES_GUC(dev_priv))
> + return 0;
> +
> + GEM_BUG_ON(!HAS_GUC(dev_priv));
bikeshed: being paranoic
GEM_BUG_ON(!wopcm->guc.base);
GEM_BUG_ON(!wopcm->guc.size);
> +
> + err = write_and_verify(dev_priv, GUC_WOPCM_SIZE,
> + dev_priv->wopcm.guc.size,
you should use wopcm-> instead dev_priv->wopcm. (same below)
> + GUC_WOPCM_SIZE_MASK | GUC_WOPCM_SIZE_LOCKED,
> + GUC_WOPCM_SIZE_LOCKED);
bikeshed: we should set BASE first, then SIZE ;)
> + if (err)
> + goto err_out;
> +
> + huc_agent = USES_HUC(dev_priv) ? HUC_LOADING_AGENT_GUC : 0;
> + mask = GUC_WOPCM_OFFSET_MASK | GUC_WOPCM_OFFSET_VALID | huc_agent;
> + err = write_and_verify(dev_priv, DMA_GUC_WOPCM_OFFSET,
> + dev_priv->wopcm.guc.base | huc_agent, mask,
> + GUC_WOPCM_OFFSET_VALID);
as the only supported HuC scenario for us is always with
HUC_LOADING_AGENT_GUC, maybe we should always set this bit,
but only add to mask for check conditionally? otherwise we
couldn't run first without and later with HuC... just asking
> + if (err)
> + goto err_out;
> +
> + return 0;
> +
> +err_out:
> + DRM_ERROR("Failed to init WOPCM registers:\n");
> + DRM_ERROR("DMA_GUC_WOPCM_OFFSET=%#x\n",
> + I915_READ(DMA_GUC_WOPCM_OFFSET));
> + DRM_ERROR("GUC_WOPCM_SIZE=%#x\n", I915_READ(GUC_WOPCM_SIZE));
> +
> + return err;
> +}
> diff --git a/drivers/gpu/drm/i915/intel_wopcm.h
> b/drivers/gpu/drm/i915/intel_wopcm.h
> index 39d4847..882f54b 100644
> --- a/drivers/gpu/drm/i915/intel_wopcm.h
> +++ b/drivers/gpu/drm/i915/intel_wopcm.h
> @@ -30,5 +30,6 @@ struct intel_wopcm {
> void intel_wopcm_init_early(struct intel_wopcm *wopcm);
> int intel_wopcm_init(struct intel_wopcm *wopcm);
> +int intel_wopcm_init_hw(struct intel_wopcm *wopcm);
> #endif
with function description and use wopcm pointer fixed,
Reviewed-by: Michal Wajdeczko <michal.wajdeczko at intel.com>
More information about the Intel-gfx
mailing list