>-----Original Message----- >From: Michal Wajdeczko [mailto:michal.wajdeczko@xxxxxxxxxxxxxxx] >Sent: Friday, December 9, 2016 4:37 AM >To: Srivatsa, Anusha <anusha.srivatsa@xxxxxxxxx> >Cc: intel-gfx@xxxxxxxxxxxxxxxxxxxxx; Alex Dai <yu.dai@xxxxxxxxx>; Peter Antoine ><peter.antoine@xxxxxxxxx> >Subject: Re: [PATCH 7/8] drm/i915/huc: Support HuC authentication > >On Thu, Dec 08, 2016 at 03:02:18PM -0800, anushasr wrote: >> From: Peter Antoine <peter.antoine@xxxxxxxxx> >> >> The HuC authentication is done by host2guc call. The HuC RSA keys are >> sent to GuC for authentication. >> >> v2: rebased on top of drm-intel-nightly. >> changed name format and upped version 1.7. >> v3: rebased on top of drm-intel-nightly. >> v4: changed wait_for_automic to wait_for >> v5: rebased. >> v7: rebased. >> v8: rebased. >> v9: rebased. Rename intel_huc_auh() to intel_guc_auth_huc() and place >> the prototype in intel_guc.h,correct the comments. >> v10: rebased. >> v11: rebased. >> v12: rebased on top of drm-tip >> v13: rebased. Moved intel_guc_auth_huc from i915_guc_submission.c to >> intel_uc.c.Update dev to dev_priv in intel_guc_auth_huc(). >> Renamed HOST2GUC_ACTION_AUTHENTICATE_HUC TO INTEL_GUC_ACTION_ >> AUTHENTICATE_HUC >> >> Tested-by: Xiang Haihao <haihao.xiang@xxxxxxxxx> >> Signed-off-by: Anusha Srivatsa <anusha.srivatsa@xxxxxxxxx> >> Signed-off-by: Alex Dai <yu.dai@xxxxxxxxx> >> Signed-off-by: Peter Antoine <peter.antoine@xxxxxxxxx> >> --- >> drivers/gpu/drm/i915/intel_guc_fwif.h | 1 + >> drivers/gpu/drm/i915/intel_guc_loader.c | 2 ++ >> drivers/gpu/drm/i915/intel_uc.c | 61 >+++++++++++++++++++++++++++++++++ >> drivers/gpu/drm/i915/intel_uc.h | 1 + >> 4 files changed, 65 insertions(+) >> >> diff --git a/drivers/gpu/drm/i915/intel_guc_fwif.h >> b/drivers/gpu/drm/i915/intel_guc_fwif.h >> index c1e7faf..94a974d 100644 >> --- a/drivers/gpu/drm/i915/intel_guc_fwif.h >> +++ b/drivers/gpu/drm/i915/intel_guc_fwif.h >> @@ -504,6 +504,7 @@ enum intel_guc_action { >> INTEL_GUC_ACTION_EXIT_S_STATE = 0x502, >> INTEL_GUC_ACTION_SLPC_REQUEST = 0x3003, >> INTEL_GUC_ACTION_UK_LOG_ENABLE_LOGGING = 0x0E000, >> + INTEL_GUC_ACTION_AUTHENTICATE_HUC = 0x4000, >> INTEL_GUC_ACTION_LIMIT >> }; >> >> diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c >> b/drivers/gpu/drm/i915/intel_guc_loader.c >> index b971351..89d092b 100644 >> --- a/drivers/gpu/drm/i915/intel_guc_loader.c >> +++ b/drivers/gpu/drm/i915/intel_guc_loader.c >> @@ -529,6 +529,8 @@ int intel_guc_setup(struct drm_i915_private *dev_priv) >> intel_uc_fw_status_repr(guc_fw->fetch_status), >> intel_uc_fw_status_repr(guc_fw->load_status)); >> >> + intel_guc_auth_huc(dev_priv); >> + >> if (i915.enable_guc_submission) { >> if (i915.guc_log_level >= 0) >> gen9_enable_guc_interrupts(dev_priv); >> diff --git a/drivers/gpu/drm/i915/intel_uc.c >> b/drivers/gpu/drm/i915/intel_uc.c index 8ae6795..445b9ad 100644 >> --- a/drivers/gpu/drm/i915/intel_uc.c >> +++ b/drivers/gpu/drm/i915/intel_uc.c >> @@ -138,3 +138,64 @@ int intel_guc_log_control(struct intel_guc *guc, >> u32 control_val) >> >> return intel_guc_send(guc, action, ARRAY_SIZE(action)); } >> + >> +/** >> + * intel_guc_auth_huc() - authenticate ucode >> + * @dev: the drm device > >Mismatched param name. > > >> + * >> + * Triggers a HuC fw authentication request to the GuC via host-2-guc >> + * interface. >> + */ >> +void intel_guc_auth_huc(struct drm_i915_private *dev_priv) > >Can we use *guc as the first param to match other intel_guc functions? > In function intel_guc_init() and intel_guc_load() we are using dev_priv as the first parameter..... >> +{ >> + struct intel_guc *guc = &dev_priv->guc; >> + struct intel_huc *huc = &dev_priv->huc; >> + struct i915_vma *vma; >> + int ret; >> + u32 data[2]; >> + >> + /* Bypass the case where there is no HuC firmware */ >> + if (huc->huc_fw.fetch_status == UC_FIRMWARE_NONE || >> + huc->huc_fw.load_status == UC_FIRMWARE_NONE) >> + return; >> + >> + if (guc->guc_fw.load_status != UC_FIRMWARE_SUCCESS) { >> + DRM_ERROR("HuC: GuC fw wasn't loaded. Can't authenticate"); > >Hmm, this looks like late handling of earlier error. >Note that other functions in this file assume that Guc is working fine. Michal, after intel_uc_init_early() which initializes a mutex lock, intel_guc_auth_huc() is the first function that the control goes to and hence all checking happens here. If anything is wrong a suitable error message is displayed and we return out immediately. If on the other hand things are proper, as expected post error checking, the other functions are called.... Hope this clears your concern. -Anusha > >> + return; >> + } >> + >> + if (huc->huc_fw.load_status != UC_FIRMWARE_SUCCESS) { >> + DRM_ERROR("HuC: fw wasn't loaded. Nothing to authenticate"); >> + return; >> + } >> + >> + vma = i915_gem_object_ggtt_pin(huc->huc_fw.uc_fw_obj, NULL, 0, 0, >0); >> + if (IS_ERR(vma)) { >> + DRM_DEBUG_DRIVER("pin failed %d\n", (int)PTR_ERR(vma)); >> + return; >> + } >> + >> + >> + /* Invalidate GuC TLB to let GuC take the latest updates to GTT. */ >> + I915_WRITE(GEN8_GTCR, GEN8_GTCR_INVALIDATE); >> + >> + /* Specify auth action and where public signature is. */ >> + data[0] = INTEL_GUC_ACTION_AUTHENTICATE_HUC; >> + data[1] = i915_ggtt_offset(vma) + huc->huc_fw.rsa_offset; >> + >> + ret = intel_guc_send(guc, data, ARRAY_SIZE(data)); > >Hmm, maybe this function shall be split into two parts: > >intel_huc_auth() in intel_huc_loader.c that contains most of the logic from >current function, but calls intel_guc_auth_huc() from this file that just sends >action to the guc (similar to other simple functions in this file. > The intention is to make intel_guc_auth_huc() as simple as other functions in the file? Two points on my mind- 1. Wont splitting into intel_huc_auth() and intel_guc_auth_huc() be more confusing than just intel_guc_auth_huc? 2.Even on splitting we will have only the preliminary check - check if guc is loaded, check if huc is loaded in intel_huc_Auth. The actual specification of authentication action, sending it to guc, checking if that is a success or not has to happen in intel_huc_auth_guc(). Am I correct? >> + if (ret) { >> + DRM_ERROR("HuC: GuC did not ack Auth request\n"); >> + goto out; >> + } >> + >> + /* Check authentication status, it should be done by now */ >> + ret = wait_for((I915_READ(HUC_STATUS2) & HUC_FW_VERIFIED) > 0, >50); >> + if (ret) { >> + DRM_ERROR("HuC: Authentication failed\n"); >> + goto out; >> + } >> + >> +out: >> + i915_vma_unpin(vma); >> +} >> diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h >> index ac92946..1db8bc2 100644 >> --- a/drivers/gpu/drm/i915/intel_uc.h >> +++ b/drivers/gpu/drm/i915/intel_uc.h >> @@ -196,6 +196,7 @@ int intel_guc_sample_forcewake(struct intel_guc *guc); >> int intel_guc_log_flush_complete(struct intel_guc *guc); >> int intel_guc_log_flush(struct intel_guc *guc); >> int intel_guc_log_control(struct intel_guc *guc, u32 control_val); >> +void intel_guc_auth_huc(struct drm_i915_private *dev_priv); >> >> /* intel_guc_loader.c */ >> extern void intel_guc_init(struct drm_i915_private *dev_priv); >> -- >> 2.7.4 >> Cheers, Anusha >> _______________________________________________ >> Intel-gfx mailing list >> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx >> https://lists.freedesktop.org/mailman/listinfo/intel-gfx _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
