Re: [PATCH v3 2/5] drm/xe/vf: Fix GuC FW check for VF migration support

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

On 10/16/2025 2:05 PM, Tomasz Lis wrote:
> The check was done before GuC ABI version could be acquired.
> Comparing only to zeros provides very stable results, though
> not the ones expected.

instead of above sentence, better say that this was triggering:

<4> [174.830604] xe 0000:00:02.1: [drm] Assertion `gt->sriov.vf.guc_version.major` failed!
...


> 
> This change dislodged part of the VF migration support check
> and moved it to after GuC handshake.

and describe your changes in imperative mood

[1] https://docs.kernel.org/process/submitting-patches.html#describe-your-changes

> 
> v2: Use xe_sriov_vf_ccs_migration_bb_needed()

you can keep change log under ---

> 
> Tested-by: Matthew Brost <matthew.brost@intel.com>

I guess above was true for # rev1

> Closes: https://gitlab.freedesktop.org/drm/xe/kernel/-/issues/6349
> Fixes: be5590c384f3 ("drm/xe/vf: Enable CCS save/restore only on supported GUC versions")
> Signed-off-by: Tomasz Lis <tomasz.lis@intel.com>
> ---
>  drivers/gpu/drm/xe/xe_gt_sriov_vf.c | 41 +++++++++++++++++++++++++++++
>  drivers/gpu/drm/xe/xe_gt_sriov_vf.h |  1 +
>  drivers/gpu/drm/xe/xe_guc.c         |  2 ++
>  drivers/gpu/drm/xe/xe_sriov_vf.c    | 10 -------
>  4 files changed, 44 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_gt_sriov_vf.c b/drivers/gpu/drm/xe/xe_gt_sriov_vf.c
> index 46518e629ba3..34c68de6e2f3 100644
> --- a/drivers/gpu/drm/xe/xe_gt_sriov_vf.c
> +++ b/drivers/gpu/drm/xe/xe_gt_sriov_vf.c
> @@ -314,6 +314,47 @@ static int guc_action_vf_notify_resfix_done(struct xe_guc *guc)
>  	return ret > 0 ? -EPROTO : ret;
>  }
>  
> +static void vf_disable_migration(struct xe_gt *gt, const char *fmt, ...)
> +{
> +	struct xe_device *xe = gt_to_xe(gt);
> +	struct va_format vaf;
> +	va_list va_args;
> +
> +	xe_gt_assert(gt, IS_SRIOV_VF(xe));
> +
> +	va_start(va_args, fmt);
> +	vaf.fmt = fmt;
> +	vaf.va  = &va_args;
> +	xe_gt_sriov_notice(gt, "migration disabled: %pV\n", &vaf);
> +	va_end(va_args);
> +
> +	xe->sriov.vf.migration.enabled = false;

this looks like a layer violation

and we already have a function that wraps that at the device level

maybe just promote device-level vf_disable_migration(xe,...) from xe_sriov_vf.c
and call it from this gt-level place ?

hmm, but see below [2]

> +}
> +
> +/**
> + * xe_gt_sriov_vf_guc_check_migration_support - Check for disable migration due to GuC.
> + * @gt: the &xe_gt struct instance linked to target GuC
> + *
> + * Performs late disable of VF migration feature in case GuC FW cannot support it.
> + */
> +void xe_gt_sriov_vf_guc_check_migration_support(struct xe_gt *gt)
> +{
> +	struct xe_device *xe = gt_to_xe(gt);
> +
> +	if (!xe_sriov_vf_migration_supported(xe))
> +		return;
> +
> +	if (xe_sriov_vf_ccs_migration_bb_needed(xe)) {
> +		struct xe_uc_fw_version guc_version;
> +
> +		xe_gt_sriov_vf_guc_versions(gt, NULL, &guc_version);
> +		if (MAKE_GUC_VER_STRUCT(guc_version) < MAKE_GUC_VER(1, 23, 0))
> +			return vf_disable_migration(gt,
> +				"CCS migration requires GuC ABI >= 1.23 but only %u.%u found",
> +				guc_version.major, guc_version.minor);

since we split migration checks from one place,
this CCS GuC ABI condition shall be placed in sriov_vf_ccs.c subcomponent

> +	}
> +}
> +
>  /**
>   * vf_notify_resfix_done - Notify GuC about resource fixups apply completed.
>   * @gt: the &xe_gt struct instance linked to target GuC
> diff --git a/drivers/gpu/drm/xe/xe_gt_sriov_vf.h b/drivers/gpu/drm/xe/xe_gt_sriov_vf.h
> index af40276790fa..60a3b9b05b20 100644
> --- a/drivers/gpu/drm/xe/xe_gt_sriov_vf.h
> +++ b/drivers/gpu/drm/xe/xe_gt_sriov_vf.h
> @@ -26,6 +26,7 @@ void xe_gt_sriov_vf_migrated_event_handler(struct xe_gt *gt);
>  int xe_gt_sriov_vf_init_early(struct xe_gt *gt);
>  int xe_gt_sriov_vf_init(struct xe_gt *gt);
>  bool xe_gt_sriov_vf_recovery_pending(struct xe_gt *gt);
> +void xe_gt_sriov_vf_guc_check_migration_support(struct xe_gt *gt);
>  
>  u32 xe_gt_sriov_vf_gmdid(struct xe_gt *gt);
>  u16 xe_gt_sriov_vf_guc_ids(struct xe_gt *gt);
> diff --git a/drivers/gpu/drm/xe/xe_guc.c b/drivers/gpu/drm/xe/xe_guc.c
> index d94490979adc..3c4e64233b3a 100644
> --- a/drivers/gpu/drm/xe/xe_guc.c
> +++ b/drivers/gpu/drm/xe/xe_guc.c
> @@ -713,6 +713,8 @@ static int vf_guc_init_noalloc(struct xe_guc *guc)
>  	if (err)
>  		return err;
>  
> +	xe_gt_sriov_vf_guc_check_migration_support(gt);
> +

[2] so this is now going through these layers:

guc_vf    vf_guc_init_noalloc
gt_vf       xe_gt_sriov_vf_guc_check_migration_support
xe_vf         xe_sriov_vf_migration_supported
xe_vf_ccs     xe_sriov_vf_ccs_migration_bb_needed
gt_vf         xe_gt_sriov_vf_guc_versions
gt_vf         vf_disable_migration
xe_vf           xe->sriov.vf.migration.enabled

so maybe better leave this VF GuC init as-is and just make "late" checks
in xe_device_probe either in xe_sriov_init_late

xe        xe_sriov_init_late
xe_vf       xe_sriov_vf_init_late
xe_vf         xe_sriov_vf_migration_supported
xe_vf_ccs     xe_sriov_vf_ccs_init_late
xe_vf_ccs     xe_sriov_vf_ccs_migration_bb_needed
gt_vf           xe_gt_sriov_vf_guc_versions
xe_vf         vf_disable_migration

or after for_each_gt/xe_gt_init_early loop

xe        xe_device_probe
xe_vf       xe_sriov_vf_check_migration
xe_vf         xe_sriov_vf_migration_supported
xe_vf_ccs     xe_sriov_vf_ccs_init_late
xe_vf_ccs     xe_sriov_vf_ccs_migration_bb_needed
gt_vf           xe_gt_sriov_vf_guc_versions
xe_vf         vf_disable_migration

or just make it as part of the xe_sriov_vf_ccs_init()
since before that point CCS migration is not working either

>  	err = xe_gt_sriov_vf_query_config(gt);
>  	if (err)
>  		return err;
> diff --git a/drivers/gpu/drm/xe/xe_sriov_vf.c b/drivers/gpu/drm/xe/xe_sriov_vf.c
> index 911d5720917b..5fb042c05112 100644
> --- a/drivers/gpu/drm/xe/xe_sriov_vf.c
> +++ b/drivers/gpu/drm/xe/xe_sriov_vf.c
> @@ -163,16 +163,6 @@ static void vf_migration_init_early(struct xe_device *xe)
>  		return vf_disable_migration(xe, "requires gfx version >= 20, but only %u found",
>  					    GRAPHICS_VER(xe));
>  
> -	if (!IS_DGFX(xe)) {
> -		struct xe_uc_fw_version guc_version;
> -
> -		xe_gt_sriov_vf_guc_versions(xe_device_get_gt(xe, 0), NULL, &guc_version);
> -		if (MAKE_GUC_VER_STRUCT(guc_version) < MAKE_GUC_VER(1, 23, 0))
> -			return vf_disable_migration(xe,
> -						    "CCS migration requires GuC ABI >= 1.23 but only %u.%u found",
> -						    guc_version.major, guc_version.minor);
> -	}
> -
>  	xe->sriov.vf.migration.enabled = true;
>  	xe_sriov_dbg(xe, "migration support enabled\n");

this would be non-reliable, as we might still disable migration later on

so we should either remove it completely (assuming its "enabled" until explicitly disabled)
or reverse the logic and use this flag instead:

	xe->sriov.vf.migration.disabled

>  }