Re: [PATCH v2 3/3] drm/xe/vf: Don't expose privileged GT debugfs files if VF

["Bernatowicz, Marcin" <marcin.bernatowicz@linux.intel.com>]

On 4/3/2025 4:26 PM, Michal Wajdeczko wrote:
> Some of the debugfs files require access to the registers that are
> not accessible to the VFs. Don't expose those files on VF drivers.
> 
> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Marcin Bernatowicz <marcin.bernatowicz@linux.intel.com>
> Cc: Lucas De Marchi <lucas.demarchi@intel.com>
> ---
> v2: avoid "privileged" word, make it clear it's about VF/PF (Lucas)
>      add hint for developers where to add new files (Lucas)
> ---
>   drivers/gpu/drm/xe/xe_gt_debugfs.c | 30 ++++++++++++++++++++++--------
>   1 file changed, 22 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_gt_debugfs.c b/drivers/gpu/drm/xe/xe_gt_debugfs.c
> index 2d63a69cbfa3..a88076e9cc7d 100644
> --- a/drivers/gpu/drm/xe/xe_gt_debugfs.c
> +++ b/drivers/gpu/drm/xe/xe_gt_debugfs.c
> @@ -299,20 +299,20 @@ static int hwconfig(struct xe_gt *gt, struct drm_printer *p)
>   	return 0;
>   }
>   
> -static const struct drm_info_list debugfs_list[] = {
> -	{"hw_engines", .show = xe_gt_debugfs_simple_show, .data = hw_engines},
> +/*
> + * only for GT debugfs files which can be safely used on the VF as well:
> + * - without access to the GT privileged registers
> + * - without access to the PF specific data
> + */
> +static const struct drm_info_list vf_safe_debugfs_list[] = {
>   	{"force_reset", .show = xe_gt_debugfs_simple_show, .data = force_reset},
>   	{"force_reset_sync", .show = xe_gt_debugfs_simple_show, .data = force_reset_sync},
>   	{"sa_info", .show = xe_gt_debugfs_simple_show, .data = sa_info},
>   	{"topology", .show = xe_gt_debugfs_simple_show, .data = topology},
> -	{"steering", .show = xe_gt_debugfs_simple_show, .data = steering},
>   	{"ggtt", .show = xe_gt_debugfs_simple_show, .data = ggtt},
> -	{"powergate_info", .show = xe_gt_debugfs_simple_show, .data = powergate_info},
>   	{"register-save-restore", .show = xe_gt_debugfs_simple_show, .data = register_save_restore},
>   	{"workarounds", .show = xe_gt_debugfs_simple_show, .data = workarounds},
>   	{"tunings", .show = xe_gt_debugfs_simple_show, .data = tunings},
> -	{"pat", .show = xe_gt_debugfs_simple_show, .data = pat},
> -	{"mocs", .show = xe_gt_debugfs_simple_show, .data = mocs},
>   	{"default_lrc_rcs", .show = xe_gt_debugfs_simple_show, .data = rcs_default_lrc},
>   	{"default_lrc_ccs", .show = xe_gt_debugfs_simple_show, .data = ccs_default_lrc},
>   	{"default_lrc_bcs", .show = xe_gt_debugfs_simple_show, .data = bcs_default_lrc},
> @@ -322,6 +322,15 @@ static const struct drm_info_list debugfs_list[] = {
>   	{"hwconfig", .show = xe_gt_debugfs_simple_show, .data = hwconfig},
>   };
>   
> +/* everything else should be added here */
> +static const struct drm_info_list pf_only_debugfs_list[] = {
> +	{"hw_engines", .show = xe_gt_debugfs_simple_show, .data = hw_engines},
> +	{"mocs", .show = xe_gt_debugfs_simple_show, .data = mocs},
> +	{"pat", .show = xe_gt_debugfs_simple_show, .data = pat},
> +	{"powergate_info", .show = xe_gt_debugfs_simple_show, .data = powergate_info},
> +	{"steering", .show = xe_gt_debugfs_simple_show, .data = steering},
> +};
> +
>   void xe_gt_debugfs_register(struct xe_gt *gt)
>   {
>   	struct xe_device *xe = gt_to_xe(gt);
> @@ -345,10 +354,15 @@ void xe_gt_debugfs_register(struct xe_gt *gt)
>   	 */
>   	root->d_inode->i_private = gt;
>   
> -	drm_debugfs_create_files(debugfs_list,
> -				 ARRAY_SIZE(debugfs_list),
> +	drm_debugfs_create_files(vf_safe_debugfs_list,
> +				 ARRAY_SIZE(vf_safe_debugfs_list),
>   				 root, minor);
>   
> +	if (!IS_SRIOV_VF(xe))
> +		drm_debugfs_create_files(pf_only_debugfs_list,
> +					 ARRAY_SIZE(pf_only_debugfs_list),
> +					 root, minor);
> +
>   	xe_uc_debugfs_register(&gt->uc, root);
>   
>   	if (IS_SRIOV_PF(xe))

LGTM,
I tested this patch together with 
https://patchwork.freedesktop.org/patch/640267/?series=145667&rev=1 (a 
fix for guc_info on VF) using 
igt@intel_sysfs_debugfs@xe-debugfs-read-all-entries,
I did not encounter any issues on the VF.

Some maintenance issue is with xe_debugfs@gt (moved to 
igt@intel_sysfs_debugfs@xe-gt) which explicitly lists some of expected 
debugfs nodes. @Matthew  Do we need to keep xe_debugfs@gt test and 
replicate the explicit file list logic or 
igt@intel_sysfs_debugfs@xe-debugfs-read-all-entries (reads all exposed 
debugfs entries) is sufficient ?

Tested-by: Marcin Bernatowicz <marcin.bernatowicz@linux.intel.com>
Reviewed-by: Marcin Bernatowicz <marcin.bernatowicz@linux.intel.com>