On 5/30/2023 17:14, Ceraolo Spurio, Daniele wrote:
Indeed. I'm just wondering if you know if there was a good reason for it being added in the original message?On 5/30/2023 5:01 PM, John Harrison wrote:On 5/30/2023 08:29, Daniele Ceraolo Spurio wrote:Before we add the second step of the MTL HuC auth (via GSC), we need to have the ability to differentiate between them. To do so, the huc authentication check is duplicated for GuC and GSC auth, with meumeu?leftover from the meu drop. Will reword.Not a blocker but should this have an exclamation? It sort of implies the authentication is unexpected.binaries being considered fully authenticated only after the GSC auth step. To report the difference between the 2 auth steps, a new case is added to the HuC getparam. This way, the clear media driver can start submitting before full auth, as partial auth is enough for those workloads. v2: fix authentication status check for DG2 v3: add a better comment at the top of the HuC file to explain the different approaches to load and auth (John) v4: update call to intel_huc_is_authenticated in the pxp code to check for GSC authentication Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@xxxxxxxxx> Cc: Alan Previn <alan.previn.teres.alexis@xxxxxxxxx> Cc: John Harrison <John.C.Harrison@xxxxxxxxx> Reviewed-by: Alan Previn <alan.previn.teres.alexis@xxxxxxxxx> #v2 ---drivers/gpu/drm/i915/gt/uc/intel_huc.c | 111 ++++++++++++++++-----drivers/gpu/drm/i915/gt/uc/intel_huc.h | 16 ++- drivers/gpu/drm/i915/gt/uc/intel_huc_fw.c | 4 +- drivers/gpu/drm/i915/i915_reg.h | 3 + drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.c | 2 +- include/uapi/drm/i915_drm.h | 3 +- 6 files changed, 104 insertions(+), 35 deletions(-)diff --git a/drivers/gpu/drm/i915/gt/uc/intel_huc.c b/drivers/gpu/drm/i915/gt/uc/intel_huc.cindex 37c6a8ca5c71..ab5246ae3979 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_huc.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_huc.c @@ -10,6 +10,7 @@ #include "intel_huc.h" #include "intel_huc_print.h" #include "i915_drv.h" +#include "i915_reg.h" #include <linux/device/bus.h> #include <linux/mei_aux.h> @@ -22,15 +23,23 @@ * 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 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. + * triggering its security authentication. This is done differently depending+ * on the platform:+ * - older platforms (from Gen9 to most Gen12s): the load is performed via DMA+ * and the authentication via GuC + * - DG2: load and authentication are both performed via GSC.+ * - MTL and newer platforms: the load is performed via DMA (same as with + * not-DG2 older platforms), while the authentication is done in 2-steps, + * a first auth for clear-media workloads via GuC and a second one for all+ * workloads via GSC.+ * On platforms where the GuC does the authentication, to correctly do so 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.+ * (S3/S4, FLR); on older platforms the HuC must also be reloaded on GuC/GT+ * reset, while on newer ones it will survive that. ** See https://github.com/intel/media-driver for the latest details on HuC* functionality.@@ -106,7 +115,7 @@ static enum hrtimer_restart huc_delayed_load_timer_callback(struct hrtimer *hrti{struct intel_huc *huc = container_of(hrtimer, struct intel_huc, delayed_load.timer);- if (!intel_huc_is_authenticated(huc)) { + if (!intel_huc_is_authenticated(huc, INTEL_HUC_AUTH_BY_GSC)) { if (huc->delayed_load.status == INTEL_HUC_WAITING_ON_GSC) huc_notice(huc, "timed out waiting for MEI GSC\n");else if (huc->delayed_load.status == INTEL_HUC_WAITING_ON_PXP) @@ -124,7 +133,7 @@ static void huc_delayed_load_start(struct intel_huc *huc){ ktime_t delay; - GEM_BUG_ON(intel_huc_is_authenticated(huc));+ GEM_BUG_ON(intel_huc_is_authenticated(huc, INTEL_HUC_AUTH_BY_GSC));/** On resume we don't have to wait for MEI-GSC to be re-probed, but we@@ -284,13 +293,23 @@ void intel_huc_init_early(struct intel_huc *huc) } if (GRAPHICS_VER(i915) >= 11) { - huc->status.reg = GEN11_HUC_KERNEL_LOAD_INFO; - huc->status.mask = HUC_LOAD_SUCCESSFUL; - huc->status.value = HUC_LOAD_SUCCESSFUL;+ huc->status[INTEL_HUC_AUTH_BY_GUC].reg = GEN11_HUC_KERNEL_LOAD_INFO;+ huc->status[INTEL_HUC_AUTH_BY_GUC].mask = HUC_LOAD_SUCCESSFUL;+ huc->status[INTEL_HUC_AUTH_BY_GUC].value = HUC_LOAD_SUCCESSFUL;} else { - huc->status.reg = HUC_STATUS2; - huc->status.mask = HUC_FW_VERIFIED; - huc->status.value = HUC_FW_VERIFIED; + huc->status[INTEL_HUC_AUTH_BY_GUC].reg = HUC_STATUS2; + huc->status[INTEL_HUC_AUTH_BY_GUC].mask = HUC_FW_VERIFIED; + huc->status[INTEL_HUC_AUTH_BY_GUC].value = HUC_FW_VERIFIED; + } + + if (IS_DG2(i915)) {+ huc->status[INTEL_HUC_AUTH_BY_GSC].reg = GEN11_HUC_KERNEL_LOAD_INFO;+ huc->status[INTEL_HUC_AUTH_BY_GSC].mask = HUC_LOAD_SUCCESSFUL;+ huc->status[INTEL_HUC_AUTH_BY_GSC].value = HUC_LOAD_SUCCESSFUL;+ } else {+ huc->status[INTEL_HUC_AUTH_BY_GSC].reg = HECI_FWSTS5(MTL_GSC_HECI1_BASE); + huc->status[INTEL_HUC_AUTH_BY_GSC].mask = HECI_FWSTS5_HUC_AUTH_DONE; + huc->status[INTEL_HUC_AUTH_BY_GSC].value = HECI_FWSTS5_HUC_AUTH_DONE;} } @@ -381,28 +400,38 @@ void intel_huc_suspend(struct intel_huc *huc) delayed_huc_load_complete(huc); } -int intel_huc_wait_for_auth_complete(struct intel_huc *huc) +static const char *auth_mode_string(struct intel_huc *huc, + enum intel_huc_authentication_type type) +{+ bool partial = huc->fw.has_gsc_headers && type == INTEL_HUC_AUTH_BY_GUC;+ + return partial ? "clear media" : "all workloads"; +} + +int intel_huc_wait_for_auth_complete(struct intel_huc *huc, + enum intel_huc_authentication_type type) { struct intel_gt *gt = huc_to_gt(huc); int ret; ret = __intel_wait_for_register(gt->uncore, - huc->status.reg, - huc->status.mask, - huc->status.value, + huc->status[type].reg, + huc->status[type].mask, + huc->status[type].value, 2, 50, NULL); /* mark the load process as complete even if the wait failed */ delayed_huc_load_complete(huc); if (ret) { - huc_err(huc, "firmware not verified %pe\n", ERR_PTR(ret)); + huc_err(huc, "firmware not verified for %s: %pe\n", + auth_mode_string(huc, type), ERR_PTR(ret));intel_uc_fw_change_status(&huc->fw, INTEL_UC_FIRMWARE_LOAD_FAIL);return ret; } intel_uc_fw_change_status(&huc->fw, INTEL_UC_FIRMWARE_RUNNING); - huc_info(huc, "authenticated!\n");+ huc_info(huc, "authenticated for %s!\n", auth_mode_string(huc, type));I can drop it. I just left it because it was already there.
Hmm, it looks like Michal added it as part of the drm_info -> huc_info update. Maybe check with him? But to me an exclamation implies an error or other exceptional circumstance. So I would drop it myself.
John.
return 0; } @@ -442,7 +471,7 @@ int intel_huc_auth(struct intel_huc *huc) } /* Check authentication status, it should be done by now */ - ret = intel_huc_wait_for_auth_complete(huc);+ ret = intel_huc_wait_for_auth_complete(huc, INTEL_HUC_AUTH_BY_GUC);if (ret) goto fail; @@ -453,16 +482,29 @@ int intel_huc_auth(struct intel_huc *huc) return ret; } -bool intel_huc_is_authenticated(struct intel_huc *huc) +bool intel_huc_is_authenticated(struct intel_huc *huc, + enum intel_huc_authentication_type type) { 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); + status = intel_uncore_read(gt->uncore, huc->status[type].reg); - return (status & huc->status.mask) == huc->status.value;+ return (status & huc->status[type].mask) == huc->status[type].value;+} + +static bool huc_is_fully_authenticated(struct intel_huc *huc) +{ + struct intel_uc_fw *huc_fw = &huc->fw; + + if (!huc_fw->has_gsc_headers) + return intel_huc_is_authenticated(huc, INTEL_HUC_AUTH_BY_GUC);+ else if (intel_huc_is_loaded_by_gsc(huc) || HAS_ENGINE(huc_to_gt(huc), GSC0))+ return intel_huc_is_authenticated(huc, INTEL_HUC_AUTH_BY_GSC); + else + return false; } /**@@ -477,7 +519,9 @@ bool intel_huc_is_authenticated(struct intel_huc *huc)*/ int intel_huc_check_status(struct intel_huc *huc) { - switch (__intel_uc_fw_status(&huc->fw)) { + struct intel_uc_fw *huc_fw = &huc->fw; + + switch (__intel_uc_fw_status(huc_fw)) { case INTEL_UC_FIRMWARE_NOT_SUPPORTED: return -ENODEV; case INTEL_UC_FIRMWARE_DISABLED: @@ -494,7 +538,17 @@ int intel_huc_check_status(struct intel_huc *huc) break; } - return intel_huc_is_authenticated(huc); + /*+ * meu binaries loaded by GuC are first partially authenticated by GuCmeu?Same as the other one, will fix. DanieleJohn.+ * and then fully authenticated by GSC + */ + if (huc_is_fully_authenticated(huc)) + return 1; /* full auth */+ else if (huc_fw->has_gsc_headers && !intel_huc_is_loaded_by_gsc(huc) &&+ intel_huc_is_authenticated(huc, INTEL_HUC_AUTH_BY_GUC)) + return 2; /* clear media only */ + else + return 0; } static bool huc_has_delayed_load(struct intel_huc *huc)@@ -508,7 +562,10 @@ void intel_huc_update_auth_status(struct intel_huc *huc)if (!intel_uc_fw_is_loadable(&huc->fw)) return; - if (intel_huc_is_authenticated(huc)) + if (!huc->fw.has_gsc_headers) + return; + + if (huc_is_fully_authenticated(huc)) intel_uc_fw_change_status(&huc->fw, INTEL_UC_FIRMWARE_RUNNING); else if (huc_has_delayed_load(huc))@@ -541,5 +598,5 @@ void intel_huc_load_status(struct intel_huc *huc, struct drm_printer *p)with_intel_runtime_pm(gt->uncore->rpm, wakeref) drm_printf(p, "HuC status: 0x%08x\n", - intel_uncore_read(gt->uncore, huc->status.reg));+ intel_uncore_read(gt->uncore, huc->status[INTEL_HUC_AUTH_BY_GUC].reg));}diff --git a/drivers/gpu/drm/i915/gt/uc/intel_huc.h b/drivers/gpu/drm/i915/gt/uc/intel_huc.hindex 112f0dce4702..3f6aa7c37abc 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_huc.h +++ b/drivers/gpu/drm/i915/gt/uc/intel_huc.h @@ -22,6 +22,12 @@ enum intel_huc_delayed_load_status { INTEL_HUC_DELAYED_LOAD_ERROR, }; +enum intel_huc_authentication_type { + INTEL_HUC_AUTH_BY_GUC = 0, + INTEL_HUC_AUTH_BY_GSC, + INTEL_HUC_AUTH_MAX_MODES +}; + struct intel_huc { /* Generic uC firmware management */ struct intel_uc_fw fw; @@ -31,7 +37,7 @@ struct intel_huc { i915_reg_t reg; u32 mask; u32 value; - } status; + } status[INTEL_HUC_AUTH_MAX_MODES]; struct { struct i915_sw_fence fence; @@ -49,10 +55,12 @@ int intel_huc_init(struct intel_huc *huc); void intel_huc_fini(struct intel_huc *huc); void intel_huc_suspend(struct intel_huc *huc); int intel_huc_auth(struct intel_huc *huc); -int intel_huc_wait_for_auth_complete(struct intel_huc *huc); +int intel_huc_wait_for_auth_complete(struct intel_huc *huc, + enum intel_huc_authentication_type type); +bool intel_huc_is_authenticated(struct intel_huc *huc, + enum intel_huc_authentication_type type); int intel_huc_check_status(struct intel_huc *huc); void intel_huc_update_auth_status(struct intel_huc *huc); -bool intel_huc_is_authenticated(struct intel_huc *huc);void intel_huc_register_gsc_notifier(struct intel_huc *huc, const struct bus_type *bus); void intel_huc_unregister_gsc_notifier(struct intel_huc *huc, const struct bus_type *bus); @@ -81,7 +89,7 @@ static inline bool intel_huc_is_loaded_by_gsc(const struct intel_huc *huc)static inline bool intel_huc_wait_required(struct intel_huc *huc) {return intel_huc_is_used(huc) && intel_huc_is_loaded_by_gsc(huc) &&- !intel_huc_is_authenticated(huc); + !intel_huc_is_authenticated(huc, INTEL_HUC_AUTH_BY_GSC); }void intel_huc_load_status(struct intel_huc *huc, struct drm_printer *p); diff --git a/drivers/gpu/drm/i915/gt/uc/intel_huc_fw.c b/drivers/gpu/drm/i915/gt/uc/intel_huc_fw.cindex 3355dc1e2bc6..d2b4176c81d6 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_huc_fw.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_huc_fw.c@@ -160,7 +160,7 @@ int intel_huc_fw_load_and_auth_via_gsc(struct intel_huc *huc) * component gets re-bound and this function called again. If so, just* mark the HuC as loaded. */ - if (intel_huc_is_authenticated(huc)) { + if (intel_huc_is_authenticated(huc, INTEL_HUC_AUTH_BY_GSC)) {intel_uc_fw_change_status(&huc->fw, INTEL_UC_FIRMWARE_RUNNING);return 0; }@@ -173,7 +173,7 @@ int intel_huc_fw_load_and_auth_via_gsc(struct intel_huc *huc) intel_uc_fw_change_status(&huc->fw, INTEL_UC_FIRMWARE_TRANSFERRED);- return intel_huc_wait_for_auth_complete(huc);+ return intel_huc_wait_for_auth_complete(huc, INTEL_HUC_AUTH_BY_GSC);} /**diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.hindex 0523418129c5..c14433795c91 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -941,6 +941,9 @@ #define HECI_H_GS1(base) _MMIO((base) + 0xc4c) #define HECI_H_GS1_ER_PREP REG_BIT(0) +#define HECI_FWSTS5(base) _MMIO(base + 0xc68) +#define HECI_FWSTS5_HUC_AUTH_DONE (1 << 19) + #define HSW_GTT_CACHE_EN _MMIO(0x4024) #define GTT_CACHE_EN_ALL 0xF0007FFF #define GEN7_WR_WATERMARK _MMIO(0x4028)diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.c b/drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.cindex 8dc41de3f6f7..016bd8fad89d 100644 --- a/drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.c +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.c@@ -196,7 +196,7 @@ bool intel_pxp_gsccs_is_ready_for_sessions(struct intel_pxp *pxp)* gsc-proxy init flow (the last set of dependencies that * are out of order) will suffice. */ - if (intel_huc_is_authenticated(&pxp->ctrl_gt->uc.huc) &&+ if (intel_huc_is_authenticated(&pxp->ctrl_gt->uc.huc, INTEL_HUC_AUTH_BY_GSC) &&intel_gsc_uc_fw_proxy_init_done(&pxp->ctrl_gt->uc.gsc)) return true;diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.hindex f31dfacde601..a1848e806059 100644 --- a/include/uapi/drm/i915_drm.h +++ b/include/uapi/drm/i915_drm.h @@ -674,7 +674,8 @@ typedef struct drm_i915_irq_wait {* If the IOCTL is successful, the returned parameter will be set to one of the* following values: * * 0 if HuC firmware load is not complete, - * * 1 if HuC firmware is authenticated and running. + * * 1 if HuC firmware is loaded and fully authenticated,+ * * 2 if HuC firmware is loaded and authenticated for clear media only*/ #define I915_PARAM_HUC_STATUS 42
