Re: [PATCH 1/2] drm/xe/vf: Track writes to inaccessible registers from VF

["Piotr Piórkowski" <piotr.piorkowski@intel.com>]

Michal Wajdeczko <michal.wajdeczko@intel.com> wrote on śro [2024-cze-26 10:43:03 +0200]:
> Only limited set of registers is accessible for the VF driver.
> The hardware will silently drop writes to inaccessible registers,
> but to improve our driver lets track all such unexpected writes
> on debug builds.
> 
> We will explicitly allow bad writes to SOFTWARE_FLAGS_SPR33 since
> it is used by the driver just to mimic wmb and we do not have any
> similar unused scratch register accessible from the VF.
> 
> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Gustavo Sousa <gustavo.sousa@intel.com>
> ---
> v2: update commit message (Gustavo)
> ---
>  drivers/gpu/drm/xe/xe_gt_sriov_vf.c | 22 ++++++++++++++++++++++
>  drivers/gpu/drm/xe/xe_gt_sriov_vf.h |  1 +
>  drivers/gpu/drm/xe/xe_mmio.c        |  6 +++++-
>  3 files changed, 28 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_gt_sriov_vf.c b/drivers/gpu/drm/xe/xe_gt_sriov_vf.c
> index 41e46a00c01e..36cefe3161e1 100644
> --- a/drivers/gpu/drm/xe/xe_gt_sriov_vf.c
> +++ b/drivers/gpu/drm/xe/xe_gt_sriov_vf.c
> @@ -892,6 +892,28 @@ u32 xe_gt_sriov_vf_read32(struct xe_gt *gt, struct xe_reg reg)
>  	return rr->value;
>  }
>  
> +/**
> + * xe_gt_sriov_vf_write32 - Track writes to an inaccessible registers.
> + * @gt: the &xe_gt
> + * @reg: the register to write
> + * @val: value to write
> + *
> + * This function is for VF use only.
> + * This function is dedicated for registers that VFs can't write directly.
> + * It will trigger a WARN if running on debug build.
> + */
> +void xe_gt_sriov_vf_write32(struct xe_gt *gt, struct xe_reg reg, u32 val)
> +{
> +	u32 addr = xe_mmio_adjusted_addr(gt, reg.addr);
> +
> +	xe_gt_assert(gt, IS_SRIOV_VF(gt_to_xe(gt)));
> +	xe_gt_assert(gt, !reg.vf);
> +
NIT: I know that the name of this functios is supposed to correspond to xe_gt_sriov_vf_read32,
     but the content does not correspond to it at all. 
     Maybe let add here some command like:

/*
 * For now we do not handle in any special way writes to registers that
 * VF does not have access to, but let's log a warning about the attempt
 * of such use
 */
> +	xe_gt_WARN(gt, IS_ENABLED(CONFIG_DRM_XE_DEBUG),
> +		   "VF is trying to write %#x to an inaccessible register %#x+%#x\n",
> +		   val, reg.addr, addr - reg.addr);

 
> +}
> +
>  /**
>   * xe_gt_sriov_vf_print_config - Print VF self config.
>   * @gt: the &xe_gt
> diff --git a/drivers/gpu/drm/xe/xe_gt_sriov_vf.h b/drivers/gpu/drm/xe/xe_gt_sriov_vf.h
> index 0de7f8cbcfa6..e541ce57bec2 100644
> --- a/drivers/gpu/drm/xe/xe_gt_sriov_vf.h
> +++ b/drivers/gpu/drm/xe/xe_gt_sriov_vf.h
> @@ -22,6 +22,7 @@ u32 xe_gt_sriov_vf_gmdid(struct xe_gt *gt);
>  u16 xe_gt_sriov_vf_guc_ids(struct xe_gt *gt);
>  u64 xe_gt_sriov_vf_lmem(struct xe_gt *gt);
>  u32 xe_gt_sriov_vf_read32(struct xe_gt *gt, struct xe_reg reg);
> +void xe_gt_sriov_vf_write32(struct xe_gt *gt, struct xe_reg reg, u32 val);
>  
>  void xe_gt_sriov_vf_print_config(struct xe_gt *gt, struct drm_printer *p);
>  void xe_gt_sriov_vf_print_runtime(struct xe_gt *gt, struct drm_printer *p);
> diff --git a/drivers/gpu/drm/xe/xe_mmio.c b/drivers/gpu/drm/xe/xe_mmio.c
> index f92faad4b96d..ff72afd79272 100644
> --- a/drivers/gpu/drm/xe/xe_mmio.c
> +++ b/drivers/gpu/drm/xe/xe_mmio.c
> @@ -151,7 +151,11 @@ void xe_mmio_write32(struct xe_gt *gt, struct xe_reg reg, u32 val)
>  	u32 addr = xe_mmio_adjusted_addr(gt, reg.addr);
>  
>  	trace_xe_reg_rw(gt, true, addr, val, sizeof(val));
Should we still trace registries to which VF do not have access ?
Maybe we should also log there that the write is rejected ?

> -	writel(val, (reg.ext ? tile->mmio_ext.regs : tile->mmio.regs) + addr);
> +
> +	if (!reg.vf && IS_SRIOV_VF(gt_to_xe(gt)) && reg.addr != SOFTWARE_FLAGS_SPR33.addr)

It seems to me that it would look better if you handled a case of SOFTWARE_FLAGS_SPR33 in
xe_gt_sriov_vf_write32
But no matter if you leave it here or move it: this case needs some comment in the code
(With a lot of code changes, the commit description is no longer easily accessible)

> +		xe_gt_sriov_vf_write32(gt, reg, val);
> +	else
> +		writel(val, (reg.ext ? tile->mmio_ext.regs : tile->mmio.regs) + addr);

I suspect I know what you are going to say, but what about mmio_ext ?

>  }
>  
>  u32 xe_mmio_read32(struct xe_gt *gt, struct xe_reg reg)
> -- 
> 2.43.0
> 

--