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

[Ceraolo Spurio, Daniele]

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 at 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?

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.
>