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.ale...@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(>->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.
