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

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

On 24.06.2024 17:01, Gustavo Sousa wrote:
> Quoting Michal Wajdeczko (2024-06-24 08:45:26-03:00)
>> 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 ignore writes to SOFTWARE_FLAGS_SPR33 since it
> 
> Drive-by comment: I might be missing something, but it looks like the
> new code would actually try to do the write as PF instead of ignoring
> it?

true, VF will try to write anyway, as it's not 'unexpected' anymore, but
rather part of the wmb flow,

and yes, this comment might be unclear, likely I should say:

  "We will explicitly _allow bad_ writes to SOFTWARE_FLAGS_SPR33"

as we just want to ignore the WARN but keep the write

> 
> --
> Gustavo Sousa
> 
>> 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>
>> ---
>> 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);
>> +
>> +        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));
>> -        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)
>> +                xe_gt_sriov_vf_write32(gt, reg, val);
>> +        else
>> +                writel(val, (reg.ext ? tile->mmio_ext.regs : tile->mmio.regs) + addr);
>> }
>>
>> u32 xe_mmio_read32(struct xe_gt *gt, struct xe_reg reg)
>> -- 
>> 2.43.0
>>