Re: [PATCH 2/2] drm/xe/vf: Catch all unexpected register reads

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

On 11.03.2025 15:23, Ghimiray, Himal Prasad wrote:
> 
> 
> On 11-03-2025 19:27, Michal Wajdeczko wrote:
>> While we can only mimic read32 for a few GT registers for which
>> the PF shared the values, we shouldn't avoid calling helper code
>> if we try to access non-GT register, as then we miss to trigger
>> a debug warning. For cases where sriov_vf_gt was not set, just
>> use primary_gt instead.
> 
> Under what scenario sriov_vf_gt can be not set for VF ?
> Isn't this initialized for each gt during probe for VF ?

this is true for the reg reads that use gt->mmio, but it's not the case
for reg read which use tile->mmio, and we had this case in driver-flr:

static void __xe_driver_flr(struct xe_device *xe)
{
...	struct xe_mmio *mmio = xe_root_tile_mmio(xe);

where this unexpected reg access was unnoticed, now it will show as:

 [ ] xe 0000:00:02.1: [drm] GT0: VF is trying to read an inaccessible
register 0x10100c+0x0
 [ ] Call Trace:
 ...
 [ ]  xe_mmio_read32+0x179/0x2b0 [xe]
 [ ]  ? release_nodes+0x48/0x120
 [ ]  xe_driver_flr_fini+0x47/0xa0 [xe]
 [ ]  devm_action_release+0x12/0x30
 [ ]  release_nodes+0x3a/0x120
 [ ]  devres_release_all+0x97/0xe0
 [ ]  device_unbind_cleanup+0x12/0x80
 [ ]  device_release_driver_internal+0x23a/0x280
 [ ]  device_release_driver+0x12/0x20

> 
>>
>> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
>> Cc: Matt Roper <matthew.d.roper@intel.com>
>> ---
>>   drivers/gpu/drm/xe/xe_mmio.c | 10 ++++++----
>>   1 file changed, 6 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/xe/xe_mmio.c b/drivers/gpu/drm/xe/xe_mmio.c
>> index 70a36e777546..13e06a956ceb 100644
>> --- a/drivers/gpu/drm/xe/xe_mmio.c
>> +++ b/drivers/gpu/drm/xe/xe_mmio.c
>> @@ -204,8 +204,9 @@ void xe_mmio_write32(struct xe_mmio *mmio, struct
>> xe_reg reg, u32 val)
>>         trace_xe_reg_rw(mmio, true, addr, val, sizeof(val));
>>   -    if (!reg.vf && mmio->sriov_vf_gt)
>> -        xe_gt_sriov_vf_write32(mmio->sriov_vf_gt, reg, val);
>> +    if (!reg.vf && IS_SRIOV_VF(mmio->tile->xe))
>> +        xe_gt_sriov_vf_write32(mmio->sriov_vf_gt ?:
>> +                       mmio->tile->primary_gt, reg, val);
>>       else
>>           writel(val, mmio->regs + addr);
>>   }
>> @@ -218,8 +219,9 @@ u32 xe_mmio_read32(struct xe_mmio *mmio, struct
>> xe_reg reg)
>>       /* Wa_15015404425 */
>>       mmio_flush_pending_writes(mmio);
>>   -    if (!reg.vf && mmio->sriov_vf_gt)
>> -        val = xe_gt_sriov_vf_read32(mmio->sriov_vf_gt, reg);
>> +    if (!reg.vf && IS_SRIOV_VF(mmio->tile->xe))
>> +        val = xe_gt_sriov_vf_read32(mmio->sriov_vf_gt ?:
>> +                        mmio->tile->primary_gt, reg);
>>       else
>>           val = readl(mmio->regs + addr);
>>   
>