Re: [PATCH i-g-t 1/2] lib/igt_sriov_device: Add intel_is_vf_device helper

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

On 3/3/2025 9:20 AM, Laguna, Lukasz wrote:
> 
> On 2/19/2025 12:53, Marcin Bernatowicz wrote:
>> Introduce intel_is_vf_device helper to determine whether a given device
>> is a Virtual Function (VF). The function reads the VF_CAPABILITY_REGISTER
>> and checks if the least significant bit is set to identify VF device.
>>
>> Signed-off-by: Marcin Bernatowicz <marcin.bernatowicz@linux.intel.com>
>> Cc: Adam Miszczak <adam.miszczak@linux.intel.com>
>> Cc: Jakub Kolakowski <jakub1.kolakowski@intel.com>
>> Cc: Lukasz Laguna <lukasz.laguna@intel.com>
>> Cc: Michał Wajdeczko <michal.wajdeczko@intel.com>
>> Cc: Michał Winiarski <michal.winiarski@intel.com>
>> Cc: Piotr Piórkowski <piotr.piorkowski@intel.com>
>> Cc: Satyanarayana K V P <satyanarayana.k.v.p@intel.com>
>> Cc: Tomasz Lis <tomasz.lis@intel.com>
>> ---
>>   lib/igt_sriov_device.c | 26 ++++++++++++++++++++++++++
>>   lib/igt_sriov_device.h |  1 +
>>   2 files changed, 27 insertions(+)
>>
>> diff --git a/lib/igt_sriov_device.c b/lib/igt_sriov_device.c
>> index 2b83cd43c..ffa996d6b 100644
>> --- a/lib/igt_sriov_device.c
>> +++ b/lib/igt_sriov_device.c
>> @@ -14,6 +14,7 @@
>>   #include "igt_device.h"
>>   #include "igt_sriov_device.h"
>>   #include "igt_sysfs.h"
>> +#include "intel_io.h"
>>   #include "xe/xe_query.h"
>>   /**
>> @@ -464,3 +465,28 @@ bool igt_sriov_device_reset(int pf, unsigned int 
>> vf_num)
>>       return ret;
>>   }
>> +
>> +/**
>> + * intel_is_vf_device - Check if device is VF
>> + * @device: device file descriptor
>> + *
>> + * Determines if a device is a Virtual Function (VF)
>> + * by reading VF_CAPABILITY_REGISTER. If the least
>> + * significant bit is set the device is VF.
>> + *
>> + * Return:
>> + * True if device is VF, false otherwise.
>> + */
>> +bool intel_is_vf_device(int fd)
>> +{
>> +#define VF_CAP_REG        0x1901f8
>> +    struct intel_mmio_data mmio_data;
>> +    uint32_t value;
>> +
>> +    intel_register_access_init(&mmio_data, 
>> igt_device_get_pci_device(fd), false);
>> +    value = intel_register_read(&mmio_data, VF_CAP_REG);
>> +    intel_register_access_fini(&mmio_data);
>> +    igt_require((value & ~1) == 0);
> 
> Is it really needed?

This should protect against 0xFFFFFFFF reads.

> 
>> +
>> +    return (value & 1) != 0;
> 
> just return value & 1 ?
> 
>> +}
>> diff --git a/lib/igt_sriov_device.h b/lib/igt_sriov_device.h
>> index 2b6acb6d7..912449086 100644
>> --- a/lib/igt_sriov_device.h
>> +++ b/lib/igt_sriov_device.h
>> @@ -33,6 +33,7 @@ void igt_sriov_unbind_vf_drm_driver(int pf, unsigned 
>> int vf_num);
>>   int igt_sriov_device_sysfs_open(int pf, unsigned int vf_num);
>>   bool igt_sriov_device_reset_exists(int pf, unsigned int vf_num);
>>   bool igt_sriov_device_reset(int pf, unsigned int vf_num);
>> +bool intel_is_vf_device(int device);
>>   /**
>>    * __is_valid_range - Helper to check VF range is valid