Re: [PATCH 03/12] drm/xe/pf: Drop GuC notifications for non-existing VF

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

On 16.08.2024 15:01, Piotr Piórkowski wrote:
> Michal Wajdeczko <michal.wajdeczko@intel.com> wrote on pią [2024-sie-09 18:51:50 +0200]:
>> It is unlikely that GuC will ever send a G2H notification with an
>> invalid VFID and it is currently harmless if that actually happen.
>> But in upcoming patches we will start using that VFID as an index
>> and we must be sure it is a valid to avoid a crash due to a buggy
>> firmware or a currupted G2H message.
>>
>> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
>> ---
>>  drivers/gpu/drm/xe/xe_gt_sriov_pf_control.c | 6 ++++++
>>  1 file changed, 6 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/xe/xe_gt_sriov_pf_control.c b/drivers/gpu/drm/xe/xe_gt_sriov_pf_control.c
>> index d11839d9e7d4..ad447d867e51 100644
>> --- a/drivers/gpu/drm/xe/xe_gt_sriov_pf_control.c
>> +++ b/drivers/gpu/drm/xe/xe_gt_sriov_pf_control.c
>> @@ -8,6 +8,7 @@
>>  #include "xe_device.h"
>>  #include "xe_gt.h"
>>  #include "xe_gt_sriov_pf_control.h"
>> +#include "xe_gt_sriov_pf_helpers.h"
>>  #include "xe_gt_sriov_printk.h"
>>  #include "xe_guc_ct.h"
>>  #include "xe_sriov.h"
>> @@ -212,6 +213,11 @@ static void pf_handle_vf_flr_done(struct xe_gt *gt, u32 vfid)
>>  
>>  static int pf_handle_vf_event(struct xe_gt *gt, u32 vfid, u32 eventid)
>>  {
>> +	xe_gt_sriov_dbg_verbose(gt, "received VF%u event %#x\n", vfid, eventid);
>> +
>> +	if (vfid > xe_gt_sriov_pf_get_totalvfs(gt))
>> +		return -EPROTO;
> 
> I would probably add some log here.
> I'm afraid that during debug the information about the last event is not enough

it should be the CTB layer responsibility to print an error message and
dump this invalid or corrupted or unexpected G2H message, no point in
doing this in every possible G2H message handler; note that 'silent'
return is aligned with the existing code in this file.

for now CTB will just report:

	xe_gt_err(gt, "G2H action 0x%04x failed (%pe)\n",
		  action, ERR_PTR(ret));

but it should be easy to add (what we had in i915) full dump of the
problematic G2H message in case of at least the EPROTO error

> 
> Thanks,
> Piotr
>> +
>>  	switch (eventid) {
>>  	case GUC_PF_NOTIFY_VF_FLR:
>>  		pf_handle_vf_flr(gt, vfid);
>> -- 
>> 2.43.0
>>
>