[igt-dev] [PATCH i-g-t 1/8] lib/igt_sriov_device: add core SR-IOV helpers
Michal Wajdeczko
michal.wajdeczko at intel.com
Fri Nov 10 19:22:51 UTC 2023
On 09.11.2023 07:55, Laguna, Lukasz wrote:
>
> On 11/6/2023 23:07, Michal Wajdeczko wrote:
>>
>> On 06.11.2023 20:59, Lukasz Laguna wrote:
>>> From: Katarzyna Dec <katarzyna.dec at intel.com>
>>>
>>> Create lib for core SR-IOV helpers that allow to manage SR-IOV devices.
>>>
>>> Signed-off-by: Katarzyna Dec <katarzyna.dec at intel.com>
>>> Reviewed-by: Lukasz Laguna <lukasz.laguna at intel.com>
>>> Signed-off-by: Lukasz Laguna <lukasz.laguna at intel.com>
>>> Reviewed-by: Marcin Bernatowicz <marcin.bernatowicz at linux.intel.com>
>>> ---
>>> lib/igt_sriov_device.c | 174 +++++++++++++++++++++++++++++++++++++++++
>>> lib/igt_sriov_device.h | 20 +++++
>>> lib/meson.build | 1 +
>>> 3 files changed, 195 insertions(+)
>>> create mode 100644 lib/igt_sriov_device.c
>>> create mode 100644 lib/igt_sriov_device.h
>>>
>>> diff --git a/lib/igt_sriov_device.c b/lib/igt_sriov_device.c
>>> new file mode 100644
>>> index 000000000..7d53c2045
>>> --- /dev/null
>>> +++ b/lib/igt_sriov_device.c
>>> @@ -0,0 +1,174 @@
>>> +// SPDX-License-Identifier: MIT
>>> +/*
>>> + * Copyright(c) 2023 Intel Corporation. All rights reserved.
>>> + */
>>> +
>>> +#include "igt_core.h"
>>> +#include "igt_sriov_device.h"
>>> +#include "igt_sysfs.h"
>>> +
>>> +/**
>>> + * igt_sriov_is_pf:
>> nit: short function description missing, not required anymore?
> Most of the IGT helpers don't have it and from what I see it's not added
> for new helpers.
so will ask in a different way: what documentation rules IGT is trying
to follow ? is it kernel style [1] or something else ?
for me it looks like a former, but maybe I'm missing something
[1]
https://www.kernel.org/doc/html/latest/doc-guide/kernel-doc.html#function-documentation
>>
>>> + * @device: device file descriptor
>>> + *
>>> + * Check if device is PF by checking existence of sriov_totalvfs file
>>> + * and non-zero value read from that file.
>> just to clarify/be precise, do you want to check here "device"
>> capability or "driver" capability ?
>>
>> IIRC the PCI subsystem will set these attributes on the "PF device" even
>> if attached "PF driver" does not provide required hook to enable VFs
>> (and this is main purpose of being a "PF", no?)
> Helper is for checking device capability. If device is not PF test will
> skip, if driver doesn't provide required hook to enable VFstest will fail.
so if device is a PF but driver decides to not support VFs and decrease
sriov_totalvfs back to 0, what would this function tell us ?
>>
>>> + *
>>> + * Returns:
>>> + * True if device is PF, false otherwise.
>>> + */
>>> +bool igt_sriov_is_pf(int device)
>>> +{
>>> + int sysfs;
>>> + bool ret;
>>> + uint32_t totalvfs;
>> nit: we shouldn't specify width if not needed, maybe plain unsigned int
>> will just work ? shame that there are no __igt_sysfs_get_uint()
>>
>>> +
>>> + sysfs = igt_sysfs_open(device);
>>> + igt_assert_fd(sysfs);
>>> +
>>> + ret = __igt_sysfs_get_u32(sysfs, "device/sriov_totalvfs",
>>> &totalvfs);
>>> + close(sysfs);
>>> +
>>> + if (!ret)
>>> + return false;
>>> +
>>> + return totalvfs > 0;
>>> +}
>>> +
>>> +static uint32_t __pf_attr_get_u32(int pf, const char *attr)
>>> +{
>>> + int sysfs;
>>> + uint32_t value;
>>> +
>>> + igt_assert(igt_sriov_is_pf(pf));
>>> +
>>> + sysfs = igt_sysfs_open(pf);
>>> + igt_assert_fd(sysfs);
>>> +
>>> + value = igt_sysfs_get_u32(sysfs, attr);
>>> + close(sysfs);
>>> +
>>> + return value;
>>> +}
>>> +
>>> +static bool __pf_attr_set_u32(int pf, const char *attr, uint32_t value)
>>> +{
>>> + int sysfs;
>>> + bool ret;
>>> +
>>> + igt_assert(igt_sriov_is_pf(pf));
>>> +
>>> + sysfs = igt_sysfs_open(pf);
>>> + igt_assert_fd(sysfs);
>>> +
>>> + ret = __igt_sysfs_set_u32(sysfs, attr, value);
>>> + close(sysfs);
>>> +
>>> + return ret;
>>> +}
>>> +
>>> +/**
>>> + * igt_sriov_get_totalvfs:
>>> + * @pf: PF device file descriptor
>>> + *
>>> + * Get maximum number of VFs that can be enabled.
>>> + *
>>> + * Returns:
>>> + * Maximum number of VFs that could be associated with given PF.
>>> + */
>>> +unsigned int igt_sriov_get_total_vfs(int pf)
>>> +{
>>> + return __pf_attr_get_u32(pf, "device/sriov_totalvfs");
>>> +}
>>> +
>>> +/**
>>> + * igt_sriov_get_numvfs:
>>> + * @pf: PF device file descriptor
>>> + *
>>> + * Get number of enabled VFs.
>>> + *
>>> + * Returns:
>>> + * Number of VFs enabled by given PF.
>>> + */
>>> +unsigned int igt_sriov_get_enabled_vfs(int pf)
>>> +{
>>> + return __pf_attr_get_u32(pf, "device/sriov_numvfs");
>>> +}
>>> +
>>> +/**
>>> + * igt_sriov_enable_vfs:
>> Helper for VFs enabling
>>
>>> + * @pf: PF device file descriptor
>>> + * @num_vfs: Number of virtual functions to be enabled
>>> + *
>>> + * Helper for VFs enabling.
>> "This will try to enable VFs by writing @num_vfs to ...
> Done
>>
>>> + *
>>> + * Returns:
>>> + * True on success and false on failure.
>>> + */
>>> +bool igt_sriov_enable_vfs(int pf, unsigned int num_vfs)
>>> +{
>>> + igt_assert(num_vfs > 0);
>>> + igt_debug("Enabling %u VFs\n", num_vfs);
>>> + return __pf_attr_set_u32(pf, "device/sriov_numvfs", num_vfs);
>>> +}
>>> +
>>> +/**
>>> + * igt_sriov_disable_vfs:
>> Helper for VFs disabling
>>
>>> + * @pf: PF device file descriptor
>>> + *
>>> + * Set 0 in sriov_numvfs file to disable VFs.
>> "This will try to disable already enabled VFs by writing 0 to ...
> Done
>>
>>> + *
>>> + * Returns:
>>> + * True on success and false on failure.
>>> + */
>>> +bool igt_sriov_disable_vfs(int pf)
>>> +{
>>> + igt_debug("Disabling %u VFs\n", igt_sriov_get_enabled_vfs(pf));
>> this will trigger unnecessary "read" operation just for debug purposes,
>> while in log you should already see "Enabling N VFs", so we know the N.
> Done
>>
>>> + return __pf_attr_set_u32(pf, "device/sriov_numvfs", 0);
>>> +}
>>> +
>>> +/**
>>> + * igt_sriov_is_driver_autoprobe_enabled:
>>> + * @pf: PF device file descriptor
>>> + *
>>> + * Get current driver autoprobe setting.
>> hmm, this is SRIOV driver autoprobe (or VF driver autoprobe for
>> short/clarity)
> Done
>>
>>> + *
>>> + * Returns:
>>> + * True if autoprobe is enabled, false otherwise.
>>> + */
>>> +bool igt_sriov_is_driver_autoprobe_enabled(int pf)
>>> +{
>>> + return __pf_attr_get_u32(pf, "device/sriov_drivers_autoprobe");
>>> +}
>>> +
>>> +/**
>>> + * igt_sriov_enable_driver_autoprobe:
>>> + * @pf: PF device file descriptor
>>> + *
>>> + * Set driver autoprobe to true.
>>> + *
>>> + * During VFs enabling driver will be bound to VFs.
>> "If successful, then kernel will automatically bind VFs to a compatible
>> driver immediately after they are enabled.
> Done
>>
>>> + *
>>> + * Return:
>>> + * True if setting driver autoprobe succeed, otherwise false.
>>> + */
>>> +bool igt_sriov_enable_driver_autoprobe(int pf)
>>> +{
>>> + return __pf_attr_set_u32(pf, "device/sriov_drivers_autoprobe",
>>> true);
>>> +}
>>> +
>>> +/**
>>> + * igt_sriov_disable_driver_autoprobe:
>>> + * @pf: PF device file descriptor
>>> + *
>>> + * Set driver autoprobe to false.
>>> + *
>>> + * During VFs enabling driver won't be bound to VFs.
>>> + *
>>> + * Return:
>>> + * True if setting driver autoprobe succeed, otherwise false.
>>> + */
>>> +bool igt_sriov_disable_driver_autoprobe(int pf)
>>> +{
>>> + return __pf_attr_set_u32(pf, "device/sriov_drivers_autoprobe",
>>> false);
>>> +}
>>> diff --git a/lib/igt_sriov_device.h b/lib/igt_sriov_device.h
>>> new file mode 100644
>>> index 000000000..f2c5c44fa
>>> --- /dev/null
>>> +++ b/lib/igt_sriov_device.h
>>> @@ -0,0 +1,20 @@
>>> +/* SPDX-License-Identifier: MIT */
>>> +/*
>>> + * Copyright(c) 2023 Intel Corporation. All rights reserved.
>>> + */
>>> +
>>> +#ifndef __IGT_SRIOV_DEVICE_H__
>>> +#define __IGT_SRIOV_DEVICE_H__
>>> +
>>> +#include <stdint.h>
>>> +
>>> +bool igt_sriov_is_pf(int device);
>>> +unsigned int igt_sriov_get_total_vfs(int pf);
>>> +unsigned int igt_sriov_get_enabled_vfs(int pf);
>>> +bool igt_sriov_enable_vfs(int pf, unsigned int num_vfs);
>>> +bool igt_sriov_disable_vfs(int pf);
>>> +bool igt_sriov_is_driver_autoprobe_enabled(int pf);
>>> +bool igt_sriov_enable_driver_autoprobe(int pf);
>>> +bool igt_sriov_disable_driver_autoprobe(int pf);
>> nit: I'm wondering if there is any pattern to have:
>>
>> a) void function that has igt_assert() inside
>> b) bool function that has no igt_assert() inside
>>
>> then for writing scenario you just use a) without need to check
>> and use b) only when doing lower level testing and use dedicated checks
> Maybe we could use __ prefix for b), but I don't see a need for such
> split TBH.
instead of repeating the code like:
+ igt_assert(igt_sriov_disable_driver_autoprobe(pf_fd));
+ igt_assert(!igt_sriov_is_driver_autoprobe_enabled(pf_fd));
+ igt_assert(igt_sriov_enable_vfs(pf_fd, num_vfs));
+ igt_assert_eq(num_vfs, igt_sriov_get_enabled_vfs(pf_fd));
+ igt_assert(igt_sriov_disable_vfs(pf_fd));
you will just write:
+ igt_sriov_disable_driver_autoprobe(pf_fd);
+ igt_sriov_enable_vfs(pf_fd, num_vfs);
+ igt_sriov_disable_vfs(pf_fd);
as asserts with required checks will be already inside
>>
>>> +
>>> +#endif /* __IGT_SRIOV_DEVICE_H__ */
>>> diff --git a/lib/meson.build b/lib/meson.build
>>> index a7bccafc3..083baa68a 100644
>>> --- a/lib/meson.build
>>> +++ b/lib/meson.build
>>> @@ -38,6 +38,7 @@ lib_sources = [
>>> 'igt_primes.c',
>>> 'igt_pci.c',
>>> 'igt_rand.c',
>>> + 'igt_sriov_device.c',
>>> 'igt_stats.c',
>>> 'igt_syncobj.c',
>>> 'igt_sysfs.c',
More information about the igt-dev
mailing list