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

[Michal Wajdeczko]

On 27.06.2024 11:23, Piotr Piórkowski wrote:
> Michal Wajdeczko <michal.wajdeczko at 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 at intel.com>
>> Cc: Gustavo Sousa <gustavo.sousa at 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
>  */

will update function description and/or add comment like that

>> +	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 ?

in ideal world we shouldn't see any attempts from VF driver to write to
unsupported registers, so for code simplicity I would leave it as is

> Maybe we should also log there that the write is rejected ?

and in less ideal world we will have a full WARN that will have a better
visibility than traces

> 
>> -	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

hmm, maybe the better solution would be to select a different register
that is marked as VF accessible, question is whether it needs to be RW

@Matt - can we pick RO register VF_CAP(0x1901f8) ?

will that be sufficient to mimics wmb?
will that work for PF/native ?
if not, what can VF do to mimics wmb on DGFX ?

> 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 ?

I never seen any platform with has_mmio_ext, so not sure how it will
support VFs, but still, similar to xe_gt_sriov_vf_read32(), the
xe_gt_sriov_vf_write32() takes xe_reg as parameter so any adjustment
needed to handle reg.ext could be done there as needed

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