On Tue, 13 Feb 2018 00:45:52 +0100, Jackie Li <yaodong.li@xxxxxxxxx> 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
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 reduntant 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
Cc: Michal Wajdeczko <michal.wajdeczko@xxxxxxxxx>
Cc: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
Cc: Joonas Lahtinen <joonas.lahtinen@xxxxxxxxxxxxxxx>
Signed-off-by: Jackie Li <yaodong.li@xxxxxxxxx>
---
drivers/gpu/drm/i915/intel_guc_reg.h | 2 +
drivers/gpu/drm/i915/intel_guc_wopcm.c | 151
++++++++++++++++++++++++++++++++-
drivers/gpu/drm/i915/intel_guc_wopcm.h | 10 ++-
drivers/gpu/drm/i915/intel_uc.c | 9 +-
4 files changed, 162 insertions(+), 10 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_guc_reg.h
b/drivers/gpu/drm/i915/intel_guc_reg.h
index c482df5..a550078 100644
--- a/drivers/gpu/drm/i915/intel_guc_reg.h
+++ b/drivers/gpu/drm/i915/intel_guc_reg.h
@@ -68,6 +68,8 @@
#define DMA_GUC_WOPCM_OFFSET _MMIO(0xc340)
#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)
diff --git a/drivers/gpu/drm/i915/intel_guc_wopcm.c
b/drivers/gpu/drm/i915/intel_guc_wopcm.c
index 0194266..4043798 100644
--- a/drivers/gpu/drm/i915/intel_guc_wopcm.c
+++ b/drivers/gpu/drm/i915/intel_guc_wopcm.c
@@ -83,6 +83,12 @@ void intel_guc_wopcm_init_early(struct
intel_guc_wopcm *guc_wopcm)
guc_wopcm->top = WOPCM_DEFAULT_SIZE;
}
+static inline
+struct intel_guc *guc_wopcm_to_guc(struct intel_guc_wopcm *guc_wopcm)
+{
+ return container_of(guc_wopcm, struct intel_guc, wopcm);
+}
+
This can be moved to patch 3/7
/**
* intel_guc_wopcm_init() - Initialize the GuC WOPCM.
* @guc_wopcm: pointer to intel_guc_wopcm.
@@ -101,13 +107,13 @@ 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_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 = context_reserved_size(guc);
u32 offset, size, top;
int err;
GEM_BUG_ON(!guc_fw_size);
+ GEM_BUG_ON(guc->wopcm.valid);
offset = huc_fw_size + WOPCM_RESERVED_SIZE;
@@ -138,6 +144,7 @@ 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;
+ guc->wopcm.load_huc_fw = huc_fw_size > 0;
err = guc_wopcm_size_check(guc, huc_fw_size);
if (err) {
@@ -152,3 +159,143 @@ int intel_guc_wopcm_init(struct intel_guc_wopcm
*guc_wopcm, u32 guc_fw_size,
return 0;
}
+
+struct lockable_reg {
+ const char *name;
+ struct intel_guc *guc;
+ i915_reg_t reg;
+ u32 reg_val;
+ u32 lock_bit;
+ bool (*validate)(struct lockable_reg *lreg, u32 reg_val, u32 new_val);
+};
<quote>
Feels like a bit over engineering in this way.
</quote>
+
+static inline u32 lockable_reg_read(struct lockable_reg *lreg)
+{
+ struct drm_i915_private *dev_priv = guc_to_i915(lreg->guc);
+
+ lreg->reg_val = I915_READ(lreg->reg);
+
+ return lreg->reg_val;
+}
+
+static inline bool lockable_reg_validate(struct lockable_reg *lreg, u32
new_val)
+{
+ GEM_BUG_ON(!lreg->validate);
+
+ return lreg->validate(lreg, lreg->reg_val, new_val);
+}
+
+static inline bool lockable_reg_locked(struct lockable_reg *lreg)
+{
+ u32 reg_val = lockable_reg_read(lreg);
+
+ return reg_val & lreg->lock_bit;
+}
+
+static inline bool lockable_reg_locked_and_valid(struct lockable_reg
*lreg,
+ u32 new_val)
+{
+ if (lockable_reg_locked(lreg)) {
+ if (lockable_reg_validate(lreg, new_val))
+ return true;
+
+ return false;
+ }
+
+ return false;
+}
+
+static inline bool lockable_reg_write(struct lockable_reg *lreg, u32
val)
+{
+ struct drm_i915_private *dev_priv = guc_to_i915(lreg->guc);
+
+ /*
+ * Write-once register was locked which may happen with either a faulty
+ * BIOS code or driver module reloading. We should still return success
+ * for the write if the register was locked with a valid value.
+ */
+ if (lockable_reg_locked(lreg)) {
+ if (lockable_reg_validate(lreg, val))
+ goto out;
+
+ DRM_DEBUG_DRIVER("Register %s was locked with invalid value\n",
+ lreg->name);
+
+ return false;
+ }
+
+ I915_WRITE(lreg->reg, val);
+
+ if (!lockable_reg_locked_and_valid(lreg, val)) {
+ DRM_DEBUG_DRIVER("Failed to lock Register %s\n", lreg->name);
+ return false;
+ }
As we acknowledge that there are scenarios where registers can be already
locked, do we really need to make our code so complex ? Maybe
int write_and_verify(struct drm_i915_private *dev_priv,
i915_reg_t reg, u32 value, u32 locked_bit)
{
I915_WRITE(reg, value);
return I915_READ(reg) != (value | locked_bit) ? -EIO : 0;
}
+
+out:
+ DRM_DEBUG_DRIVER("Write-once register %s locked with valid value
%#x\n",
+ lreg->name, lreg->reg_val);
+
+ return true;
+}
+
+static inline bool size_reg_value_validate(struct lockable_reg *lreg,
+ u32 reg_val, u32 new_val)
+{
+ u32 reg_size = reg_val & GUC_WOPCM_SIZE_MASK;
+ u32 target_size = new_val & GUC_WOPCM_SIZE_MASK;
+
+ return reg_size == target_size;
+}
+
+static inline bool offset_reg_value_validate(struct lockable_reg *lreg,
+ u32 reg_val, u32 new_val)
+{
+ u32 mask = GUC_WOPCM_OFFSET_MASK | HUC_LOADING_AGENT_GUC;
+ u32 reg_offset = reg_val & mask;
+ u32 target_offset = new_val & mask;
+
+ return reg_offset == target_offset;
+}
+
+#define DEFINE_LOCKABLE_REG(var, rg, lb, func) \
+ struct lockable_reg var = { \
+ .name = #rg, \
+ .guc = guc_wopcm_to_guc(guc_wopcm), \
btw, implicit macro params are evil...
+ .reg = rg, \
+ .reg_val = 0, \
+ .lock_bit = lb, \
+ .validate = func, \
...and macro names should be always wrapped into ()
+ }
+
+/**
+ * intel_guc_wopcm_init_hw() - Setup GuC WOPCM registers.
+ * @guc_wopcm: pointer to 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. -EIO if registers were locked with incorrect
values.
+ */
+int intel_guc_wopcm_init_hw(struct intel_guc_wopcm *guc_wopcm)
+{
+ DEFINE_LOCKABLE_REG(guc_wopcm_size_reg, GUC_WOPCM_SIZE, BIT(0),
+ size_reg_value_validate);
+ DEFINE_LOCKABLE_REG(guc_wopcm_offset_reg, DMA_GUC_WOPCM_OFFSET, BIT(0),
+ offset_reg_value_validate);
+ u32 reg_val;
+
+ GEM_BUG_ON(!guc_wopcm->valid);
+
+ reg_val = guc_wopcm->size;
+ if (!lockable_reg_write(&guc_wopcm_size_reg, reg_val))
+ return -EIO;
+
+ reg_val = guc_wopcm->offset;
+ if (guc_wopcm->load_huc_fw)
+ reg_val |= HUC_LOADING_AGENT_GUC;
+ if (!lockable_reg_write(&guc_wopcm_offset_reg, reg_val))
+ return -EIO;
+
+ return 0;
+}
diff --git a/drivers/gpu/drm/i915/intel_guc_wopcm.h
b/drivers/gpu/drm/i915/intel_guc_wopcm.h
index 13fcab6..89dd44c 100644
--- a/drivers/gpu/drm/i915/intel_guc_wopcm.h
+++ b/drivers/gpu/drm/i915/intel_guc_wopcm.h
@@ -66,7 +66,8 @@ 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.
+ * @load_huc_fw: whether need to configure GuC to load HuC firmware.
I'm not sure that we need to track this flag inside structure.
It is just a parameter for doing partitioning and final check.
As mentioned before, we can avoid this flag and "valid" flag if do
partitioning and validation *before* writing final results to the
struct.
*
* 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.
@@ -75,11 +76,14 @@ struct intel_guc_wopcm {
u32 offset;
u32 size;
u32 top;
- u32 valid;
+
+ /* GuC WOPCM flags below. */
+ u32 valid:1;
+ u32 load_huc_fw:1;
};
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);
+ 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))
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
