On newer platforms (starting DG2 G10 B-step and G11 A-step), ownership of HuC loading and authentication has been moved from the GuC to the GSC, with both actions being performed via a single PXP command. Given that the mei code has not fully landed yet (see [1]), we can't implement the new load mechanism, but we can start getting ready for it by taking care of the changes required for the existing code:
- The HuC header is now different from the GuC one. This also means that if the FW is for GSC-loading and the HW fuse is set to legacy load (or vice-versa) we can't load the HuC.
- To send a PXP message to the GSC we need both MEI_GSC and MEI_PXP.
- All legacy HuC loading paths can be skipped.
Note that the HuC fw version for DG2 is still not defined, so the HuC code will be skipped until the define is added.
v2: drop changes in auth checking for legacy paths.
[1] https://patchwork.freedesktop.org/series/102339/
Cc: Alan Previn alan.previn.teres.alexis@intel.com Cc: John Harrison john.c.harrison@intel.com
Daniele Ceraolo Spurio (4): drm/i915/huc: drop intel_huc_is_authenticated drm/i915/huc: Add fetch support for gsc-loaded HuC binary drm/i915/huc: Prepare for GSC-loaded HuC drm/i915/huc: Don't fail the probe if HuC init fails
drivers/gpu/drm/i915/gt/uc/intel_guc_reg.h | 1 + drivers/gpu/drm/i915/gt/uc/intel_huc.c | 97 +++++++++++++++---- drivers/gpu/drm/i915/gt/uc/intel_huc.h | 5 +- drivers/gpu/drm/i915/gt/uc/intel_huc_fw.c | 5 +- drivers/gpu/drm/i915/gt/uc/intel_uc.c | 22 +++-- drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c | 99 ++++++++++++-------- drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h | 2 + drivers/gpu/drm/i915/gt/uc/intel_uc_fw_abi.h | 9 ++ 8 files changed, 172 insertions(+), 68 deletions(-)
The fuction name is confusing, because it doesn't check the actual auth status in HW but the SW status. Given that there is only one user (the huc_auth function itself), just get rid of it and use the FW status checker directly.
Signed-off-by: Daniele Ceraolo Spurio daniele.ceraolospurio@intel.com --- drivers/gpu/drm/i915/gt/uc/intel_huc.c | 2 +- drivers/gpu/drm/i915/gt/uc/intel_huc.h | 5 ----- 2 files changed, 1 insertion(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_huc.c b/drivers/gpu/drm/i915/gt/uc/intel_huc.c index 556829de9c172..7b759b99cf3c8 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_huc.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_huc.c @@ -96,7 +96,7 @@ int intel_huc_auth(struct intel_huc *huc) struct intel_guc *guc = >->uc.guc; int ret;
- GEM_BUG_ON(intel_huc_is_authenticated(huc)); + GEM_BUG_ON(intel_uc_fw_is_running(&huc->fw));
if (!intel_uc_fw_is_loaded(&huc->fw)) return -ENOEXEC; diff --git a/drivers/gpu/drm/i915/gt/uc/intel_huc.h b/drivers/gpu/drm/i915/gt/uc/intel_huc.h index 73ec670800f2b..77d813840d76c 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_huc.h +++ b/drivers/gpu/drm/i915/gt/uc/intel_huc.h @@ -50,11 +50,6 @@ static inline bool intel_huc_is_used(struct intel_huc *huc) return intel_uc_fw_is_available(&huc->fw); }
-static inline bool intel_huc_is_authenticated(struct intel_huc *huc) -{ - return intel_uc_fw_is_running(&huc->fw); -} - void intel_huc_load_status(struct intel_huc *huc, struct drm_printer *p);
#endif
Reviewed-by: Alan Previn alan.previn.teres.alexis@intel.com
On Wed, 2022-05-04 at 13:48 -0700, Daniele Ceraolo Spurio wrote:
The fuction name is confusing, because it doesn't check the actual auth status in HW but the SW status. Given that there is only one user (the huc_auth function itself), just get rid of it and use the FW status checker directly.
Signed-off-by: Daniele Ceraolo Spurio daniele.ceraolospurio@intel.com
drivers/gpu/drm/i915/gt/uc/intel_huc.c | 2 +- drivers/gpu/drm/i915/gt/uc/intel_huc.h | 5 ----- 2 files changed, 1 insertion(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_huc.c b/drivers/gpu/drm/i915/gt/uc/intel_huc.c index 556829de9c172..7b759b99cf3c8 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_huc.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_huc.c @@ -96,7 +96,7 @@ int intel_huc_auth(struct intel_huc *huc) struct intel_guc *guc = >->uc.guc; int ret;
- GEM_BUG_ON(intel_huc_is_authenticated(huc));
GEM_BUG_ON(intel_uc_fw_is_running(&huc->fw));
if (!intel_uc_fw_is_loaded(&huc->fw)) return -ENOEXEC;
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_huc.h b/drivers/gpu/drm/i915/gt/uc/intel_huc.h index 73ec670800f2b..77d813840d76c 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_huc.h +++ b/drivers/gpu/drm/i915/gt/uc/intel_huc.h @@ -50,11 +50,6 @@ static inline bool intel_huc_is_used(struct intel_huc *huc) return intel_uc_fw_is_available(&huc->fw); }
-static inline bool intel_huc_is_authenticated(struct intel_huc *huc) -{
- return intel_uc_fw_is_running(&huc->fw);
-}
void intel_huc_load_status(struct intel_huc *huc, struct drm_printer *p);
#endif
On newer platforms (starting DG2 G10 B-step and G11 A-step), ownership of HuC loading has been moved from the GuC to the GSC. As part of the change, the header format of the HuC binary has been updated and does not match the GuC anymore. The GSC will perform all the required checks on the binary size, so we only need to check that the version matches.
Note that since we still haven't added any gsc-loaded FWs, the loaded_via_gsc variable will always be kept to its initialization value of zero.
v2: Add a note about loaded_via_gsc being zero (Alan)
Signed-off-by: Daniele Ceraolo Spurio daniele.ceraolospurio@intel.com Reviewed-by: Alan Previn alan.previn.teres.alexis@intel.com --- drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c | 99 ++++++++++++-------- drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h | 2 + drivers/gpu/drm/i915/gt/uc/intel_uc_fw_abi.h | 9 ++ 3 files changed, 72 insertions(+), 38 deletions(-)
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c index d078f884b5e32..9361532726d6c 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c @@ -301,45 +301,31 @@ static void __force_fw_fetch_failures(struct intel_uc_fw *uc_fw, int e) } }
-/** - * intel_uc_fw_fetch - fetch uC firmware - * @uc_fw: uC firmware - * - * Fetch uC firmware into GEM obj. - * - * Return: 0 on success, a negative errno code on failure. - */ -int intel_uc_fw_fetch(struct intel_uc_fw *uc_fw) +static int check_gsc_manifest(const struct firmware *fw, + struct intel_uc_fw *uc_fw) { - struct drm_i915_private *i915 = __uc_fw_to_gt(uc_fw)->i915; - struct device *dev = i915->drm.dev; - struct drm_i915_gem_object *obj; - const struct firmware *fw = NULL; - struct uc_css_header *css; - size_t size; - int err; + u32 *dw = (u32 *)fw->data; + u32 version = dw[HUC_GSC_VERSION_DW];
- GEM_BUG_ON(!i915->wopcm.size); - GEM_BUG_ON(!intel_uc_fw_is_enabled(uc_fw)); - - err = i915_inject_probe_error(i915, -ENXIO); - if (err) - goto fail; + uc_fw->major_ver_found = FIELD_GET(HUC_GSC_MAJOR_VER_MASK, version); + uc_fw->minor_ver_found = FIELD_GET(HUC_GSC_MINOR_VER_MASK, version);
- __force_fw_fetch_failures(uc_fw, -EINVAL); - __force_fw_fetch_failures(uc_fw, -ESTALE); + return 0; +}
- err = request_firmware(&fw, uc_fw->path, dev); - if (err) - goto fail; +static int check_ccs_header(struct drm_i915_private *i915, + const struct firmware *fw, + struct intel_uc_fw *uc_fw) +{ + struct uc_css_header *css; + size_t size;
/* Check the size of the blob before examining buffer contents */ if (unlikely(fw->size < sizeof(struct uc_css_header))) { drm_warn(&i915->drm, "%s firmware %s: invalid size: %zu < %zu\n", intel_uc_fw_type_repr(uc_fw->type), uc_fw->path, fw->size, sizeof(struct uc_css_header)); - err = -ENODATA; - goto fail; + return -ENODATA; }
css = (struct uc_css_header *)fw->data; @@ -352,8 +338,7 @@ int intel_uc_fw_fetch(struct intel_uc_fw *uc_fw) "%s firmware %s: unexpected header size: %zu != %zu\n", intel_uc_fw_type_repr(uc_fw->type), uc_fw->path, fw->size, sizeof(struct uc_css_header)); - err = -EPROTO; - goto fail; + return -EPROTO; }
/* uCode size must calculated from other sizes */ @@ -368,8 +353,7 @@ int intel_uc_fw_fetch(struct intel_uc_fw *uc_fw) drm_warn(&i915->drm, "%s firmware %s: invalid size: %zu < %zu\n", intel_uc_fw_type_repr(uc_fw->type), uc_fw->path, fw->size, size); - err = -ENOEXEC; - goto fail; + return -ENOEXEC; }
/* Sanity check whether this fw is not larger than whole WOPCM memory */ @@ -378,8 +362,7 @@ int intel_uc_fw_fetch(struct intel_uc_fw *uc_fw) drm_warn(&i915->drm, "%s firmware %s: invalid size: %zu > %zu\n", intel_uc_fw_type_repr(uc_fw->type), uc_fw->path, size, (size_t)i915->wopcm.size); - err = -E2BIG; - goto fail; + return -E2BIG; }
/* Get version numbers from the CSS header */ @@ -388,6 +371,49 @@ int intel_uc_fw_fetch(struct intel_uc_fw *uc_fw) uc_fw->minor_ver_found = FIELD_GET(CSS_SW_VERSION_UC_MINOR, css->sw_version);
+ if (uc_fw->type == INTEL_UC_FW_TYPE_GUC) + uc_fw->private_data_size = css->private_data_size; + + return 0; +} + +/** + * intel_uc_fw_fetch - fetch uC firmware + * @uc_fw: uC firmware + * + * Fetch uC firmware into GEM obj. + * + * Return: 0 on success, a negative errno code on failure. + */ +int intel_uc_fw_fetch(struct intel_uc_fw *uc_fw) +{ + struct drm_i915_private *i915 = __uc_fw_to_gt(uc_fw)->i915; + struct device *dev = i915->drm.dev; + struct drm_i915_gem_object *obj; + const struct firmware *fw = NULL; + int err; + + GEM_BUG_ON(!i915->wopcm.size); + GEM_BUG_ON(!intel_uc_fw_is_enabled(uc_fw)); + + err = i915_inject_probe_error(i915, -ENXIO); + if (err) + goto fail; + + __force_fw_fetch_failures(uc_fw, -EINVAL); + __force_fw_fetch_failures(uc_fw, -ESTALE); + + err = request_firmware(&fw, uc_fw->path, dev); + if (err) + goto fail; + + if (uc_fw->loaded_via_gsc) + err = check_gsc_manifest(fw, uc_fw); + else + err = check_ccs_header(i915, fw, uc_fw); + if (err) + goto fail; + if (uc_fw->major_ver_found != uc_fw->major_ver_wanted || uc_fw->minor_ver_found < uc_fw->minor_ver_wanted) { drm_notice(&i915->drm, "%s firmware %s: unexpected version: %u.%u != %u.%u\n", @@ -400,9 +426,6 @@ int intel_uc_fw_fetch(struct intel_uc_fw *uc_fw) } }
- if (uc_fw->type == INTEL_UC_FW_TYPE_GUC) - uc_fw->private_data_size = css->private_data_size; - if (HAS_LMEM(i915)) { obj = i915_gem_object_create_lmem_from_data(i915, fw->data, fw->size); if (!IS_ERR(obj)) diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h index 3229018877d3d..4f169035f5041 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h @@ -102,6 +102,8 @@ struct intel_uc_fw { u32 ucode_size;
u32 private_data_size; + + bool loaded_via_gsc; };
#ifdef CONFIG_DRM_I915_DEBUG_GUC diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw_abi.h b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw_abi.h index e41ffc7a7fbcb..b05e0e35b734c 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw_abi.h +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw_abi.h @@ -39,6 +39,11 @@ * 3. Length info of each component can be found in header, in dwords. * 4. Modulus and exponent key are not required by driver. They may not appear * in fw. So driver will load a truncated firmware in this case. + * + * Starting from DG2, the HuC is loaded by the GSC instead of i915. The GSC + * firmware performs all the required integrity checks, we just need to check + * the version. Note that the header for GSC-managed blobs is different from the + * CSS used for dma-loaded firmwares. */
struct uc_css_header { @@ -78,4 +83,8 @@ struct uc_css_header { } __packed; static_assert(sizeof(struct uc_css_header) == 128);
+#define HUC_GSC_VERSION_DW 44 +#define HUC_GSC_MAJOR_VER_MASK (0xFF << 0) +#define HUC_GSC_MINOR_VER_MASK (0xFF << 16) + #endif /* _INTEL_UC_FW_ABI_H */
HuC loading via GSC is performed via a PXP command sent through the mei modules, so we need both MEI_GSC and MEI_PXP to be available. Given that the GSC will do both the transfer and the authentication, the legacy HuC loading paths can be safely skipped. Also note that the GSC-loaded HuC survives GT reset.
v2: move the huc_is_authenticated() function to this patch.
Signed-off-by: Daniele Ceraolo Spurio daniele.ceraolospurio@intel.com Reviewed-by: Alan Previn alan.previn.teres.alexis@intel.com #v1 --- drivers/gpu/drm/i915/gt/uc/intel_guc_reg.h | 1 + drivers/gpu/drm/i915/gt/uc/intel_huc.c | 95 ++++++++++++++++++---- drivers/gpu/drm/i915/gt/uc/intel_huc.h | 6 ++ drivers/gpu/drm/i915/gt/uc/intel_huc_fw.c | 5 +- drivers/gpu/drm/i915/gt/uc/intel_uc.c | 11 ++- 5 files changed, 100 insertions(+), 18 deletions(-)
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_reg.h b/drivers/gpu/drm/i915/gt/uc/intel_guc_reg.h index 66027a42cda9e..2516705b9f365 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_reg.h +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_reg.h @@ -96,6 +96,7 @@
#define GUC_SHIM_CONTROL2 _MMIO(0xc068) #define GUC_IS_PRIVILEGED (1<<29) +#define GSC_LOADS_HUC (1<<30)
#define GUC_SEND_INTERRUPT _MMIO(0xc4c8) #define GUC_SEND_TRIGGER (1<<0) diff --git a/drivers/gpu/drm/i915/gt/uc/intel_huc.c b/drivers/gpu/drm/i915/gt/uc/intel_huc.c index 7b759b99cf3c8..c36e2bf9b0f29 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_huc.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_huc.c @@ -6,6 +6,7 @@ #include <linux/types.h>
#include "gt/intel_gt.h" +#include "intel_guc_reg.h" #include "intel_huc.h" #include "i915_drv.h"
@@ -17,11 +18,15 @@ * capabilities by adding HuC specific commands to batch buffers. * * The kernel driver is only responsible for loading the HuC firmware and - * triggering its security authentication, which is performed by the GuC. For - * The GuC to correctly perform the authentication, the HuC binary must be - * loaded before the GuC one. Loading the HuC is optional; however, not using - * the HuC might negatively impact power usage and/or performance of media - * workloads, depending on the use-cases. + * triggering its security authentication, which is performed by the GuC on + * older platforms and by the GSC on newer ones. For the GuC to correctly + * perform the authentication, the HuC binary must be loaded before the GuC one. + * Loading the HuC is optional; however, not using the HuC might negatively + * impact power usage and/or performance of media workloads, depending on the + * use-cases. + * HuC must be reloaded on events that cause the WOPCM to lose its contents + * (S3/S4, FLR); GuC-authenticated HuC must also be reloaded on GuC/GT reset, + * while GSC-managed HuC will survive that. * * See https://github.com/intel/media-driver for the latest details on HuC * functionality. @@ -54,11 +59,51 @@ void intel_huc_init_early(struct intel_huc *huc) } }
+#define HUC_LOAD_MODE_STRING(x) (x ? "GSC" : "legacy") +static int check_huc_loading_mode(struct intel_huc *huc) +{ + struct intel_gt *gt = huc_to_gt(huc); + bool fw_needs_gsc = intel_huc_is_loaded_by_gsc(huc); + bool hw_uses_gsc = false; + + /* + * The fuse for HuC load via GSC is only valid on platforms that have + * GuC deprivilege. + */ + if (HAS_GUC_DEPRIVILEGE(gt->i915)) + hw_uses_gsc = intel_uncore_read(gt->uncore, GUC_SHIM_CONTROL2) & + GSC_LOADS_HUC; + + if (fw_needs_gsc != hw_uses_gsc) { + drm_err(>->i915->drm, + "mismatch between HuC FW (%s) and HW (%s) load modes\n", + HUC_LOAD_MODE_STRING(fw_needs_gsc), + HUC_LOAD_MODE_STRING(hw_uses_gsc)); + return -ENOEXEC; + } + + /* make sure we can access the GSC via the mei driver if we need it */ + if (!(IS_ENABLED(CONFIG_INTEL_MEI_PXP) && IS_ENABLED(CONFIG_INTEL_MEI_GSC)) && + fw_needs_gsc) { + drm_info(>->i915->drm, + "Can't load HuC due to missing MEI modules\n"); + return -EIO; + } + + drm_dbg(>->i915->drm, "GSC loads huc=%s\n", str_yes_no(fw_needs_gsc)); + + return 0; +} + int intel_huc_init(struct intel_huc *huc) { struct drm_i915_private *i915 = huc_to_gt(huc)->i915; int err;
+ err = check_huc_loading_mode(huc); + if (err) + goto out; + err = intel_uc_fw_init(&huc->fw); if (err) goto out; @@ -96,17 +141,20 @@ int intel_huc_auth(struct intel_huc *huc) struct intel_guc *guc = >->uc.guc; int ret;
- GEM_BUG_ON(intel_uc_fw_is_running(&huc->fw)); - if (!intel_uc_fw_is_loaded(&huc->fw)) return -ENOEXEC;
+ /* GSC will do the auth */ + if (intel_huc_is_loaded_by_gsc(huc)) + return -ENODEV; + ret = i915_inject_probe_error(gt->i915, -ENXIO); if (ret) goto fail;
- ret = intel_guc_auth_huc(guc, - intel_guc_ggtt_offset(guc, huc->fw.rsa_data)); + GEM_BUG_ON(intel_uc_fw_is_running(&huc->fw)); + + ret = intel_guc_auth_huc(guc, intel_guc_ggtt_offset(guc, huc->fw.rsa_data)); if (ret) { DRM_ERROR("HuC: GuC did not ack Auth request %d\n", ret); goto fail; @@ -133,6 +181,18 @@ int intel_huc_auth(struct intel_huc *huc) return ret; }
+static bool huc_is_authenticated(struct intel_huc *huc) +{ + struct intel_gt *gt = huc_to_gt(huc); + intel_wakeref_t wakeref; + u32 status = 0; + + with_intel_runtime_pm(gt->uncore->rpm, wakeref) + status = intel_uncore_read(gt->uncore, huc->status.reg); + + return (status & huc->status.mask) == huc->status.value; +} + /** * intel_huc_check_status() - check HuC status * @huc: intel_huc structure @@ -150,10 +210,6 @@ int intel_huc_auth(struct intel_huc *huc) */ int intel_huc_check_status(struct intel_huc *huc) { - struct intel_gt *gt = huc_to_gt(huc); - intel_wakeref_t wakeref; - u32 status = 0; - switch (__intel_uc_fw_status(&huc->fw)) { case INTEL_UC_FIRMWARE_NOT_SUPPORTED: return -ENODEV; @@ -167,10 +223,17 @@ int intel_huc_check_status(struct intel_huc *huc) break; }
- with_intel_runtime_pm(gt->uncore->rpm, wakeref) - status = intel_uncore_read(gt->uncore, huc->status.reg); + return huc_is_authenticated(huc); +}
- return (status & huc->status.mask) == huc->status.value; +void intel_huc_update_auth_status(struct intel_huc *huc) +{ + if (!intel_uc_fw_is_loadable(&huc->fw)) + return; + + if (huc_is_authenticated(huc)) + intel_uc_fw_change_status(&huc->fw, + INTEL_UC_FIRMWARE_RUNNING); }
/** diff --git a/drivers/gpu/drm/i915/gt/uc/intel_huc.h b/drivers/gpu/drm/i915/gt/uc/intel_huc.h index 77d813840d76c..d7e25b6e879eb 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_huc.h +++ b/drivers/gpu/drm/i915/gt/uc/intel_huc.h @@ -27,6 +27,7 @@ int intel_huc_init(struct intel_huc *huc); void intel_huc_fini(struct intel_huc *huc); int intel_huc_auth(struct intel_huc *huc); int intel_huc_check_status(struct intel_huc *huc); +void intel_huc_update_auth_status(struct intel_huc *huc);
static inline int intel_huc_sanitize(struct intel_huc *huc) { @@ -50,6 +51,11 @@ static inline bool intel_huc_is_used(struct intel_huc *huc) return intel_uc_fw_is_available(&huc->fw); }
+static inline bool intel_huc_is_loaded_by_gsc(const struct intel_huc *huc) +{ + return huc->fw.loaded_via_gsc; +} + void intel_huc_load_status(struct intel_huc *huc, struct drm_printer *p);
#endif diff --git a/drivers/gpu/drm/i915/gt/uc/intel_huc_fw.c b/drivers/gpu/drm/i915/gt/uc/intel_huc_fw.c index e5ef509c70e89..9d6ab1e016395 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_huc_fw.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_huc_fw.c @@ -8,7 +8,7 @@ #include "i915_drv.h"
/** - * intel_huc_fw_upload() - load HuC uCode to device + * intel_huc_fw_upload() - load HuC uCode to device via DMA transfer * @huc: intel_huc structure * * Called from intel_uc_init_hw() during driver load, resume from sleep and @@ -21,6 +21,9 @@ */ int intel_huc_fw_upload(struct intel_huc *huc) { + if (intel_huc_is_loaded_by_gsc(huc)) + return -ENODEV; + /* HW doesn't look at destination address for HuC, so set it to 0 */ return intel_uc_fw_upload(&huc->fw, 0, HUC_UKERNEL); } diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.c b/drivers/gpu/drm/i915/gt/uc/intel_uc.c index 8c9ef690ac9d8..0dce94f896a8c 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_uc.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.c @@ -509,7 +509,16 @@ static int __uc_init_hw(struct intel_uc *uc) if (ret) goto err_log_capture;
- intel_huc_auth(huc); + /* + * GSC-loaded HuC is authenticated by the GSC, so we don't need to + * trigger the auth here. However, given that the HuC loaded this way + * survive GT reset, we still need to update our SW bookkeeping to make + * sure it reflects the correct HW status. + */ + if (intel_huc_is_loaded_by_gsc(huc)) + intel_huc_update_auth_status(huc); + else + intel_huc_auth(huc);
if (intel_uc_uses_guc_submission(uc)) intel_guc_submission_enable(guc);
Because i reviewed this already and the only new change is the relocation of the function "huc_is_authenticated()" from Patch 1 to this patch while maintaining the same logic as rev-1, thus:
Acked-by: Alan Previn alan.previn.teres.alexis@intel.com
On Wed, 2022-05-04 at 13:48 -0700, Daniele Ceraolo Spurio wrote:
HuC loading via GSC is performed via a PXP command sent through the mei modules, so we need both MEI_GSC and MEI_PXP to be available. Given that the GSC will do both the transfer and the authentication, the legacy HuC loading paths can be safely skipped. Also note that the GSC-loaded HuC survives GT reset.
v2: move the huc_is_authenticated() function to this patch.
Signed-off-by: Daniele Ceraolo Spurio daniele.ceraolospurio@intel.com Reviewed-by: Alan Previn alan.previn.teres.alexis@intel.com #v1
drivers/gpu/drm/i915/gt/uc/intel_guc_reg.h | 1 + drivers/gpu/drm/i915/gt/uc/intel_huc.c | 95 ++++++++++++++++++---- drivers/gpu/drm/i915/gt/uc/intel_huc.h | 6 ++ drivers/gpu/drm/i915/gt/uc/intel_huc_fw.c | 5 +- drivers/gpu/drm/i915/gt/uc/intel_uc.c | 11 ++- 5 files changed, 100 insertions(+), 18 deletions(-)
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_reg.h b/drivers/gpu/drm/i915/gt/uc/intel_guc_reg.h index 66027a42cda9e..2516705b9f365 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_reg.h +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_reg.h @@ -96,6 +96,7 @@
#define GUC_SHIM_CONTROL2 _MMIO(0xc068) #define GUC_IS_PRIVILEGED (1<<29) +#define GSC_LOADS_HUC (1<<30)
#define GUC_SEND_INTERRUPT _MMIO(0xc4c8) #define GUC_SEND_TRIGGER (1<<0) diff --git a/drivers/gpu/drm/i915/gt/uc/intel_huc.c b/drivers/gpu/drm/i915/gt/uc/intel_huc.c index 7b759b99cf3c8..c36e2bf9b0f29 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_huc.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_huc.c @@ -6,6 +6,7 @@ #include <linux/types.h>
#include "gt/intel_gt.h" +#include "intel_guc_reg.h" #include "intel_huc.h" #include "i915_drv.h"
@@ -17,11 +18,15 @@
- capabilities by adding HuC specific commands to batch buffers.
- The kernel driver is only responsible for loading the HuC firmware and
- triggering its security authentication, which is performed by the GuC. For
- The GuC to correctly perform the authentication, the HuC binary must be
- loaded before the GuC one. Loading the HuC is optional; however, not using
- the HuC might negatively impact power usage and/or performance of media
- workloads, depending on the use-cases.
- triggering its security authentication, which is performed by the GuC on
- older platforms and by the GSC on newer ones. For the GuC to correctly
- perform the authentication, the HuC binary must be loaded before the GuC one.
- Loading the HuC is optional; however, not using the HuC might negatively
- impact power usage and/or performance of media workloads, depending on the
- use-cases.
- HuC must be reloaded on events that cause the WOPCM to lose its contents
- (S3/S4, FLR); GuC-authenticated HuC must also be reloaded on GuC/GT reset,
- while GSC-managed HuC will survive that.
- See https://github.com/intel/media-driver for the latest details on HuC
- functionality.
@@ -54,11 +59,51 @@ void intel_huc_init_early(struct intel_huc *huc) } }
+#define HUC_LOAD_MODE_STRING(x) (x ? "GSC" : "legacy") +static int check_huc_loading_mode(struct intel_huc *huc) +{
- struct intel_gt *gt = huc_to_gt(huc);
- bool fw_needs_gsc = intel_huc_is_loaded_by_gsc(huc);
- bool hw_uses_gsc = false;
- /*
* The fuse for HuC load via GSC is only valid on platforms that have* GuC deprivilege.*/- if (HAS_GUC_DEPRIVILEGE(gt->i915))
hw_uses_gsc = intel_uncore_read(gt->uncore, GUC_SHIM_CONTROL2) &GSC_LOADS_HUC;- if (fw_needs_gsc != hw_uses_gsc) {
drm_err(>->i915->drm,"mismatch between HuC FW (%s) and HW (%s) load modes\n",HUC_LOAD_MODE_STRING(fw_needs_gsc),HUC_LOAD_MODE_STRING(hw_uses_gsc));return -ENOEXEC;- }
- /* make sure we can access the GSC via the mei driver if we need it */
- if (!(IS_ENABLED(CONFIG_INTEL_MEI_PXP) && IS_ENABLED(CONFIG_INTEL_MEI_GSC)) &&
fw_needs_gsc) {drm_info(>->i915->drm,"Can't load HuC due to missing MEI modules\n");return -EIO;- }
- drm_dbg(>->i915->drm, "GSC loads huc=%s\n", str_yes_no(fw_needs_gsc));
- return 0;
+}
int intel_huc_init(struct intel_huc *huc) { struct drm_i915_private *i915 = huc_to_gt(huc)->i915; int err;
- err = check_huc_loading_mode(huc);
- if (err)
goto out;- err = intel_uc_fw_init(&huc->fw); if (err) goto out;
@@ -96,17 +141,20 @@ int intel_huc_auth(struct intel_huc *huc) struct intel_guc *guc = >->uc.guc; int ret;
- GEM_BUG_ON(intel_uc_fw_is_running(&huc->fw));
- if (!intel_uc_fw_is_loaded(&huc->fw)) return -ENOEXEC;
- /* GSC will do the auth */
- if (intel_huc_is_loaded_by_gsc(huc))
return -ENODEV;- ret = i915_inject_probe_error(gt->i915, -ENXIO); if (ret) goto fail;
- ret = intel_guc_auth_huc(guc,
intel_guc_ggtt_offset(guc, huc->fw.rsa_data));
- GEM_BUG_ON(intel_uc_fw_is_running(&huc->fw));
- ret = intel_guc_auth_huc(guc, intel_guc_ggtt_offset(guc, huc->fw.rsa_data)); if (ret) { DRM_ERROR("HuC: GuC did not ack Auth request %d\n", ret); goto fail;
@@ -133,6 +181,18 @@ int intel_huc_auth(struct intel_huc *huc) return ret; }
+static bool huc_is_authenticated(struct intel_huc *huc) +{
- struct intel_gt *gt = huc_to_gt(huc);
- intel_wakeref_t wakeref;
- u32 status = 0;
- with_intel_runtime_pm(gt->uncore->rpm, wakeref)
status = intel_uncore_read(gt->uncore, huc->status.reg);- return (status & huc->status.mask) == huc->status.value;
+}
/**
- intel_huc_check_status() - check HuC status
- @huc: intel_huc structure
@@ -150,10 +210,6 @@ int intel_huc_auth(struct intel_huc *huc) */ int intel_huc_check_status(struct intel_huc *huc) {
- struct intel_gt *gt = huc_to_gt(huc);
- intel_wakeref_t wakeref;
- u32 status = 0;
- switch (__intel_uc_fw_status(&huc->fw)) { case INTEL_UC_FIRMWARE_NOT_SUPPORTED: return -ENODEV;
@@ -167,10 +223,17 @@ int intel_huc_check_status(struct intel_huc *huc) break; }
- with_intel_runtime_pm(gt->uncore->rpm, wakeref)
status = intel_uncore_read(gt->uncore, huc->status.reg);
- return huc_is_authenticated(huc);
+}
- return (status & huc->status.mask) == huc->status.value;
+void intel_huc_update_auth_status(struct intel_huc *huc) +{
- if (!intel_uc_fw_is_loadable(&huc->fw))
return;- if (huc_is_authenticated(huc))
intel_uc_fw_change_status(&huc->fw,INTEL_UC_FIRMWARE_RUNNING);}
/** diff --git a/drivers/gpu/drm/i915/gt/uc/intel_huc.h b/drivers/gpu/drm/i915/gt/uc/intel_huc.h index 77d813840d76c..d7e25b6e879eb 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_huc.h +++ b/drivers/gpu/drm/i915/gt/uc/intel_huc.h @@ -27,6 +27,7 @@ int intel_huc_init(struct intel_huc *huc); void intel_huc_fini(struct intel_huc *huc); int intel_huc_auth(struct intel_huc *huc); int intel_huc_check_status(struct intel_huc *huc); +void intel_huc_update_auth_status(struct intel_huc *huc);
static inline int intel_huc_sanitize(struct intel_huc *huc) { @@ -50,6 +51,11 @@ static inline bool intel_huc_is_used(struct intel_huc *huc) return intel_uc_fw_is_available(&huc->fw); }
+static inline bool intel_huc_is_loaded_by_gsc(const struct intel_huc *huc) +{
- return huc->fw.loaded_via_gsc;
+}
void intel_huc_load_status(struct intel_huc *huc, struct drm_printer *p);
#endif diff --git a/drivers/gpu/drm/i915/gt/uc/intel_huc_fw.c b/drivers/gpu/drm/i915/gt/uc/intel_huc_fw.c index e5ef509c70e89..9d6ab1e016395 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_huc_fw.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_huc_fw.c @@ -8,7 +8,7 @@ #include "i915_drv.h"
/**
- intel_huc_fw_upload() - load HuC uCode to device
- intel_huc_fw_upload() - load HuC uCode to device via DMA transfer
- @huc: intel_huc structure
- Called from intel_uc_init_hw() during driver load, resume from sleep and
@@ -21,6 +21,9 @@ */ int intel_huc_fw_upload(struct intel_huc *huc) {
- if (intel_huc_is_loaded_by_gsc(huc))
return -ENODEV;- /* HW doesn't look at destination address for HuC, so set it to 0 */ return intel_uc_fw_upload(&huc->fw, 0, HUC_UKERNEL);
} diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.c b/drivers/gpu/drm/i915/gt/uc/intel_uc.c index 8c9ef690ac9d8..0dce94f896a8c 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_uc.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.c @@ -509,7 +509,16 @@ static int __uc_init_hw(struct intel_uc *uc) if (ret) goto err_log_capture;
- intel_huc_auth(huc);
/*
* GSC-loaded HuC is authenticated by the GSC, so we don't need to* trigger the auth here. However, given that the HuC loaded this way* survive GT reset, we still need to update our SW bookkeeping to make* sure it reflects the correct HW status.*/if (intel_huc_is_loaded_by_gsc(huc))
intel_huc_update_auth_status(huc);else
intel_huc_auth(huc);if (intel_uc_uses_guc_submission(uc)) intel_guc_submission_enable(guc);
The previous patch introduced new failure cases in the HuC init flow that can be hit by simply changing the config, so we want to avoid failing the probe in those scenarios. HuC load failure is already considered a non-fatal error and we have a way to report to userspace if the HuC is not available via a dedicated getparam, so no changes in expectation there. The error message in the HuC init code has also been lowered to info to avoid throwing error message for an expected behavior.
Signed-off-by: Daniele Ceraolo Spurio daniele.ceraolospurio@intel.com Reviewed-by: Rodrigo Vivi rodrigo.vivi@intel.com --- drivers/gpu/drm/i915/gt/uc/intel_huc.c | 2 +- drivers/gpu/drm/i915/gt/uc/intel_uc.c | 11 ++--------- 2 files changed, 3 insertions(+), 10 deletions(-)
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_huc.c b/drivers/gpu/drm/i915/gt/uc/intel_huc.c index c36e2bf9b0f29..3bb8838e325a4 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_huc.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_huc.c @@ -113,7 +113,7 @@ int intel_huc_init(struct intel_huc *huc) return 0;
out: - i915_probe_error(i915, "failed with %d\n", err); + drm_info(&i915->drm, "HuC init failed with %d\n", err); return err; }
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.c b/drivers/gpu/drm/i915/gt/uc/intel_uc.c index 0dce94f896a8c..ecf149c5fdb02 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_uc.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.c @@ -323,17 +323,10 @@ static int __uc_init(struct intel_uc *uc) if (ret) return ret;
- if (intel_uc_uses_huc(uc)) { - ret = intel_huc_init(huc); - if (ret) - goto out_guc; - } + if (intel_uc_uses_huc(uc)) + intel_huc_init(huc);
return 0; - -out_guc: - intel_guc_fini(guc); - return ret; }
static void __uc_fini(struct intel_uc *uc)
dri-devel@lists.freedesktop.org
-
Daniele Ceraolo Spurio -
Teres Alexis, Alan Previn