Re: [Intel-gfx] [PATCH 4/6] drm/i915/gsc: Do a driver-FLR on unload if GSC was loaded

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

Few nits - most of which are repeats from existing review comments.
I did have 1 feedback. Functionally, code logic is correct.

To speed things up, I'll provide a conditional R-b if you address the feedback below + fix the the BIT3->to-BIT4 uncore-
flags fix. Others are nits in my book: 
(conditional) Reviewed-by: Alan Previn <alan.previn.teres.alexis@intel.com>


On Mon, 2022-11-21 at 15:16 -0800, Ceraolo Spurio, Daniele wrote:
> If the GSC was loaded, the only way to stop it during the driver unload
> flow is to do a driver-FLR.
> The driver-FLR is not the same as PCI config space FLR in that
> it doesn't reset the SGUnit and doesn't modify the PCI config
> space. Thus, it doesn't require a re-enumeration of the PCI BARs.
> However, the driver-FLR does cause a memory wipe of graphics memory
> on all discrete GPU platforms or a wipe limited to stolen memory
> on the integrated GPU platforms.


Alan: [snip]


> > +	/*
> +	 * Once the GSC FW is loaded, the only way to kill it on driver unload
> +	 * is to do a driver FLR. Given this is a very disruptive action, we
> +	 * want to do it as the last action before releasing the access to the
> +	 * MMIO bar, which means we need to do it as part of the primary uncore
> +	 * cleanup.
> +	 */
> +	intel_uncore_set_flr_on_fini(&gt->i915->uncore);

Alan: Nit: Perhaps define what disruptive (i.e. the whole memory wiping impact) - aligns with what Rodrigo commented i
think?

Alan: Nit: Might be important for developers debugging issues to state (in comments) that the security FW has been
provided a dynamically allocated memory which is why it MUST be killed on unload (unlike prior Gen SOCs).

Alan: Feedback: I think intel_uncore_set_flr_on_fini should called before gsc_fw_load() (or after but still called if
loading failed with and error indicating the instruction was already sent such as the timeout error, before the bail).
This would be better to ensure a clean slate is set upon unload even if gsc firmware was attempted to get loaded.

Alan: [snip]


> +	/*
> +	 * Make sure any pending FLR requests have cleared by waiting for the
> +	 * FLR trigger bit to go to zero. Also clear GU_DEBUG's DRIVERFLR_STATUS
> +	 * to make sure it's not still set from a prior attempt (it's a write to
> +	 * clear bit).
> +	 * Note that we should never be in a situation where a previous attempt
> +	 * is still pending (unless the HW is totally dead), but better to be
> +	 * safe in case something unexpected happens
> +	 */
> +	ret = intel_wait_for_register_fw(uncore, GU_CNTL, DRIVERFLR, 0, flr_timeout_ms);
> +	if (ret) {
> +		drm_err(&i915->drm,
> +			"Failed to wait for Driver-FLR bit to clear! %d\n",
> +			ret);
> +		return;
> +	}
> +	intel_uncore_write_fw(uncore, GU_DEBUG, DRIVERFLR_STATUS);
> +
Alan: Nit: with the current definition of MTL registers, nothing is wrong with above code but for the sake of code-
intent-readability, perhaps better to use intel_uncore_rmw_fw on above too.

Alan: [snip]

> @@ -153,6 +153,7 @@ struct intel_uncore {
>  #define UNCORE_HAS_FPGA_DBG_UNCLAIMED	BIT(1)
>  #define UNCORE_HAS_DBG_UNCLAIMED	BIT(2)
>  #define UNCORE_HAS_FIFO			BIT(3)
> +#define UNCORE_NEEDS_FLR_ON_FINI	BIT(3)
>  
Alan: Fix: yeah - this needs to be 4 - u already caught that.