[Intel-gfx] [PATCH v9 6/7] drm/i915/guc: Check the locking status of GuC WOPCM registers
Michal Wajdeczko
michal.wajdeczko at intel.com
Fri Feb 9 19:42:58 UTC 2018
On Fri, 09 Feb 2018 00:03:54 +0100, Jackie Li <yaodong.li at 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 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/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
More information about the Intel-gfx
mailing list