Re: [PATCH 9/9] drm/xe/vf: Custom HuC initialization if VF

[Michal Wajdeczko <michal.wajdeczko@intel.com>]

On 20.06.2024 01:23, Matthew Brost wrote:
> On Thu, Jun 20, 2024 at 01:14:29AM +0200, Michal Wajdeczko wrote:
>>
>>
>> On 20.06.2024 01:11, Matthew Brost wrote:
>>> On Wed, Jun 19, 2024 at 11:45:57PM +0200, Michal Wajdeczko wrote:
>>>> The HuC firmware is loaded and initialized by the PF driver. Make
>>>> sure VF driver performs only limited data structure initialization.
>>>>
>>>> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
>>>> ---
>>>>  drivers/gpu/drm/xe/xe_huc.c | 4 ++++
>>>>  1 file changed, 4 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/xe/xe_huc.c b/drivers/gpu/drm/xe/xe_huc.c
>>>> index 6238fb354914..c88761fe31c9 100644
>>>> --- a/drivers/gpu/drm/xe/xe_huc.c
>>>> +++ b/drivers/gpu/drm/xe/xe_huc.c
>>>> @@ -21,6 +21,7 @@
>>>>  #include "xe_guc.h"
>>>>  #include "xe_map.h"
>>>>  #include "xe_mmio.h"
>>>> +#include "xe_sriov.h"
>>>>  #include "xe_uc_fw.h"
>>>>  
>>>>  static struct xe_gt *
>>>> @@ -92,6 +93,9 @@ int xe_huc_init(struct xe_huc *huc)
>>>>  	if (!xe_uc_fw_is_enabled(&huc->fw))
>>>>  		return 0;
>>>>  
>>>> +	if (IS_SRIOV_VF(xe))
>>>> +		return 0;
>>>> +
>>>
>>> With this change I assume the main part of xe_huc_auth is never called
>>> on a VF?
>>>
>>> Does xe_uc_fw_is_loadable return false on a VF?
>>
>> yes, as on VF it is marked as PRELOADED, so:
>>
>> static inline bool xe_uc_fw_is_loadable(struct xe_uc_fw *uc_fw)
>> {
>> 	return __xe_uc_fw_status(uc_fw) >= XE_UC_FIRMWARE_LOADABLE &&
>> 		__xe_uc_fw_status(uc_fw) != XE_UC_FIRMWARE_PRELOADED;
>> }
>>
>> returns false
>>
> 
> To be clear, I'd add asserts to parts of functions which should not be
> executed on a VF if it is expected to short circuit on another function.
> 
> e.g.
> 
> xe_huc_auth()
> 	if (!xe_uc_fw_is_loadable)
> 		return
> 
> 	xe_assert(!VF);

hmm, but then we might pollute the whole driver with such asserts, as VF
really can do only limited stuff

besides, we already track any unwanted access to unavailable registers
from VFs in xe_mmio_read32()

	if (!reg.vf && IS_SRIOV_VF(gt_to_xe(gt)))
		val = xe_gt_sriov_vf_read32(gt, reg);

...

xe_gt_WARN(gt, IS_ENABLED(CONFIG_DRM_XE_DEBUG),
	   "VF is trying to read an inaccessible register %#x+%#x\n",
	   reg.addr, addr - reg.addr);

and I'm planning to add similar code (or xe_assert) to xe_mmio_write()

finally, GuC will report errors if VF will try to execute privileged action

so IMO spreading xe_assert(!VF) all over is not ideal

> 
> This patch LGTM to me though. With that:
> Reviewed-by: Matthew Brost <matthew.brost@intel.com>
> 
>>>
>>> Matt
>>>
>>>>  	if (huc->fw.has_gsc_headers) {
>>>>  		ret = huc_alloc_gsc_pkt(huc);
>>>>  		if (ret)
>>>> -- 
>>>> 2.43.0
>>>>