Re: [PATCH v3 5/7] drm/i915/mtl/huc: auth HuC via GSC

["Teres Alexis, Alan Previn" <alan.previn.teres.alexis@intel.com>]

On Fri, 2023-05-26 at 17:52 -0700, Ceraolo Spurio, Daniele wrote:
> The full authentication via the GSC requires an heci packet submission
> to the GSC FW via the GSC CS. The GSC has new PXP command for this
> (literally called NEW_HUC_AUTH).
> The intel_huc_auth fuction is also updated to handle both authentication
> types.
> 


alan:snip

> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.c b/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.c
> index b26f493f86fa..c659cc01f32f 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.c
> @@ -29,13 +29,32 @@ static void gsc_work(struct work_struct *work)
>  
>  	if (actions & GSC_ACTION_FW_LOAD) {
>  		ret = intel_gsc_uc_fw_upload(gsc);
> -		if (ret == -EEXIST) /* skip proxy if not a new load */
> -			actions &= ~GSC_ACTION_FW_LOAD;
> -		else if (ret)
> +		if (!ret)
> +			/* setup proxy on a new load */
> +			actions |= GSC_ACTION_SW_PROXY;
> +		else if (ret != -EEXIST)
>  			goto out_put;
alan: I realize that this worker doesnt print error values that
come back from intel_gsc_uc_fw_upload - wonder if we should print
it before goto out_put.

> +
> +		/*
> +		 * The HuC auth can be done both before or after the proxy init;
> +		 * if done after, a proxy request will be issued and must be
> +		 * serviced before the authentication can complete.
> +		 * Since this worker also handles proxy requests, we can't
> +		 * perform an action that requires the proxy from within it and
> +		 * then stall waiting for it, because we'd be blocking the
> +		 * service path. Therefore, it is easier for us to load HuC
> +		 * first and do proxy later. The GSC will ack the HuC auth and
> +		 * then send the HuC proxy request as part of the proxy init
> +		 * flow.
> +		 * Note that we can only do the GSC auth if the GuC auth was
> +		 * successful.
> +		 */
> +		if (intel_uc_uses_huc(&gt->uc) &&
> +		    intel_huc_is_authenticated(&gt->uc.huc, INTEL_HUC_AUTH_BY_GUC))
> +			intel_huc_auth(&gt->uc.huc, INTEL_HUC_AUTH_BY_GSC);
>  	}
>  
> -	if (actions & (GSC_ACTION_FW_LOAD | GSC_ACTION_SW_PROXY)) {
> +	if (actions & GSC_ACTION_SW_PROXY) {
>  		if (!intel_gsc_uc_fw_init_done(gsc)) {
>  			gt_err(gt, "Proxy request received with GSC not loaded!\n");
>  			goto out_put;
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc_heci_cmd_submit.c b/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc_heci_cmd_submit.c
> index 579c0f5a1438..0ad090304ca0 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc_heci_cmd_submit.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc_heci_cmd_submit.c
> 
alan:snip

> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_huc.c b/drivers/gpu/drm/i915/gt/uc/intel_huc.c
> index ab5246ae3979..5a4058d39550 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_huc.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_huc.c
> 

alan:snip

> 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 d2b4176c81d6..8e538d639b05 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_huc_fw.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_huc_fw.c
> 
alan:snip


> +int intel_huc_fw_auth_via_gsccs(struct intel_huc *huc)
> +{
> +	struct drm_i915_gem_object *obj;
> +	void *pkt_vaddr;
> +	u64 pkt_offset;
> +	int retry = 5;
> +	int err = 0;
> +
> +	if (!huc->heci_pkt)
> +		return -ENODEV;
> +
> +	obj = huc->heci_pkt->obj;
> +	pkt_offset = i915_ggtt_offset(huc->heci_pkt);
> +
> +	pkt_vaddr = i915_gem_object_pin_map_unlocked(obj,
> +						     i915_coherent_map_type(i915, obj, true));
> +	if (IS_ERR(pkt_vaddr))
> +		return PTR_ERR(pkt_vaddr);
> +
> +	msg_in = pkt_vaddr;
> +	msg_out = pkt_vaddr + SZ_4K;
this 4K*2 (4k for input and 4K for output) seems to be hardcoded in two different locations.
One is here in intel_huc_fw_auth_via_gsccs and the other location in intel_huc_init which was
even in a different file. Perhaps its better to use a #define after to the definition of
PXP43_CMDID_NEW_HUC_AUTH... maybe something like a "#define PXP43_NEW_HUC_AUTH_INOUT_PKT_SIZE (SZ_4K)"
> +
> +	intel_gsc_uc_heci_cmd_emit_mtl_header(&msg_in->header,
> +					      HECI_MEADDRESS_PXP,
> +					      sizeof(*msg_in), 0);
> +
> +	msg_in->huc_in.header.api_version = PXP_APIVER(4, 3);
> +	msg_in->huc_in.header.command_id = PXP43_CMDID_NEW_HUC_AUTH;
> +	msg_in->huc_in.header.status = 0;
> +	msg_in->huc_in.header.buffer_len = sizeof(msg_in->huc_in) -
> +					   sizeof(msg_in->huc_in.header);
> +	msg_in->huc_in.huc_base_address = huc->fw.vma_res.start;
> +	msg_in->huc_in.huc_size = huc->fw.obj->base.size;

alan: is this right?: fw.obj.base.size is the rounded up firmware size that was
allocated from intel_uc_fw_fetch when it calls i915_gem_object_create_shmem_from_data
That latter funcation populates obj with the "rounded up to 4K" size.
So is it okay use the 4k rounded up number for the size of the firmware microcode that needs to be authenticated?
(or, since this is a GSC FW command, does GSC FW parse the mei headers and extract the exact size to authenticate?)


alan:snip

> +
> +out_unpin:
> +	i915_gem_object_unpin_map(obj);
> +	return err;
> +}
>  
alan:snip