Re: [Intel-gfx] [PATCH v10 6/7] drm/i915/guc: Check the locking status of GuC WOPCM registers
On 02/13/2018 05:18 PM, Yaodong Li wrote: On 02/13/2018 09:39 AM, Michal Wajdeczko wrote: + +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; } Yes, I agree it's too complex at least for the validation part. Thanks! My intention was trying to avoid extra write once we found the reg was locked and to distinguish between faulty SW behavior and hardware locking error? but now I feel it's not worth it.:-( Sorry. I regret:-). I think it makes since to use my complex way to validate the values because of the rsvd bits these regs. a comparison I915_READ(reg) != (value | locked_bit) cannot rule out the possibility that these regs were updated by faulty software code (e.g. BIOS) with these rsvd bit set to random values. and it's aways better to not make any assumption to these rsvd bits. so I will still keep using the complex way (explicitly clear this rsvd bit before comparison) to do this. As for the lock_bit_check->write->verify sequence v.s. write->verify sequence I think I can prefer to first one since it would provide comprehensive info when any error occurs. so let's keep it?:-) + + +#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... Agree. but seems we always use similar approach in I915_READ/WRITE().O:-) I will avoid this implicit macro. + .reg = rg, \ + .reg_val = 0, \ + .lock_bit = lb, \ + .validate = func, \ ...and macro names should be always wrapped into () Thanks! Will Add them. Regards, -Jackie ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v10 6/7] drm/i915/guc: Check the locking status of GuC WOPCM registers
On 02/14/2018 09:07 AM, Michal Wajdeczko wrote: On Wed, 14 Feb 2018 02:18:29 +0100, Yaodong Liwrote: On 02/13/2018 09:39 AM, Michal Wajdeczko wrote: + +#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... Agree. but seems we always use similar approach in I915_READ/WRITE().O:-) True, but the only reason why it is still not fixed is that it requires changes in too many places ... Sure. will avoid to use implicit params then:-) Thanks. + * @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. I think it's related to actual reg configuration. Any suggestions since we do need it in hw_init to setup offset reg? You can do partitioning at intel_wopcm level, but program hw at intel_uc level when you know for sure if HuC is in use. Note that there is no wrong in using results from WOPCM partitioning outside of intel_wopmc. As mentioned before, we can avoid this flag and "valid" flag if do partitioning and validation *before* writing final results to the struct. In current code, we do verify the ggtt offset against wopcm top in our current code which means current code won't trust the fact that ggtt offset would never be used after uc/guc init failed. But after failure in WOPCM partitioning, which should be done sooner than uc init, we should abort driver load, so there shall be no access to ggtt. I would like to have more discussion on intel_wopcm over intel_guc_wopcm. :-) This is the reason for this valid bit (which clearly suggests the struct is ready to use) - I won't I'm not sure that "valid" flag approach is correct - you should trust your code path, otherwise you will have to add "valid" flags to every structure and this is not a scalable solution ;) True. I need some courage here:-). I would doubt myself after seeing guc_ggtt_offset validates every offset against the wopcm->guc.top which is the only reason I kept using this flag:-( Regards, -Jackie ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v10 6/7] drm/i915/guc: Check the locking status of GuC WOPCM registers
On Wed, 14 Feb 2018 02:18:29 +0100, Yaodong Liwrote: On 02/13/2018 09:39 AM, Michal Wajdeczko wrote: + +#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... Agree. but seems we always use similar approach in I915_READ/WRITE().O:-) True, but the only reason why it is still not fixed is that it requires changes in too many places ... + * @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. I think it's related to actual reg configuration. Any suggestions since we do need it in hw_init to setup offset reg? You can do partitioning at intel_wopcm level, but program hw at intel_uc level when you know for sure if HuC is in use. Note that there is no wrong in using results from WOPCM partitioning outside of intel_wopmc. As mentioned before, we can avoid this flag and "valid" flag if do partitioning and validation *before* writing final results to the struct. In current code, we do verify the ggtt offset against wopcm top in our current code which means current code won't trust the fact that ggtt offset would never be used after uc/guc init failed. But after failure in WOPCM partitioning, which should be done sooner than uc init, we should abort driver load, so there shall be no access to ggtt. This is the reason for this valid bit (which clearly suggests the struct is ready to use) - I won't I'm not sure that "valid" flag approach is correct - you should trust your code path, otherwise you will have to add "valid" flags to every structure and this is not a scalable solution ;) assume the ggtt_offset would never be called even if the uc/guc_init returned failure. ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v10 6/7] drm/i915/guc: Check the locking status of GuC WOPCM registers
On 02/13/2018 09:39 AM, Michal Wajdeczko wrote: + +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; } Yes, I agree it's too complex at least for the validation part. Thanks! My intention was trying to avoid extra write once we found the reg was locked and to distinguish between faulty SW behavior and hardware locking error? but now I feel it's not worth it.:-( + + +#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... Agree. but seems we always use similar approach in I915_READ/WRITE().O:-) + .reg = rg, \ + .reg_val = 0, \ + .lock_bit = lb, \ + .validate = func, \ ...and macro names should be always wrapped into () Thanks! 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. I think it's related to actual reg configuration. Any suggestions since we do need it in hw_init to setup offset reg? As mentioned before, we can avoid this flag and "valid" flag if do partitioning and validation *before* writing final results to the struct. In current code, we do verify the ggtt offset against wopcm top in our current code which means current code won't trust the fact that ggtt offset would never be used after uc/guc init failed. This is the reason for this valid bit (which clearly suggests the struct is ready to use) - I won't assume the ggtt_offset would never be called even if the uc/guc_init returned failure. ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v10 6/7] drm/i915/guc: Check the locking status of GuC WOPCM registers
On Tue, 13 Feb 2018 00:45:52 +0100, Jackie Liwrote: 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 Cc: Chris Wilson Cc: Joonas Lahtinen Signed-off-by: Jackie Li --- 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 (0x3 << 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); +}; Feels like a bit over engineering in this way. + +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
[Intel-gfx] [PATCH v10 6/7] drm/i915/guc: Check the locking status of GuC WOPCM registers
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 WajdeczkoCc: Chris Wilson Cc: Joonas Lahtinen Signed-off-by: Jackie Li --- 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 (0x3 << 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); +} + /** * 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); +}; + +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