On 12/1/2022 2:40 PM, Teres Alexis, Alan Previn wrote:
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@xxxxxxxxx>
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?
I'll add it in the FLR function and refer to that one
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.
Ok, I'll move it to before.
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.
This can't be a rmw, this register has a bunch of bits that are write to
clear/take action, so we must write only the FLR bit.
Daniele
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.
