Re: [igt-dev] [PATCH i-g-t 1/8] lib/igt_sriov_device: add core SR-IOV helpers

["Laguna, Lukasz" <lukasz.laguna@intel.com>]

On 11/10/2023 20:22, Michal Wajdeczko wrote:
>
> 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@intel.com>
>>>>
>>>> Create lib for core SR-IOV helpers that allow to manage SR-IOV devices.
>>>>
>>>> Signed-off-by: Katarzyna Dec <katarzyna.dec@intel.com>
>>>> Reviewed-by: Lukasz Laguna <lukasz.laguna@intel.com>
>>>> Signed-off-by: Lukasz Laguna <lukasz.laguna@intel.com>
>>>> Reviewed-by: Marcin Bernatowicz <marcin.bernatowicz@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 ?
If we consider such case then I can rename this helper to 
igt_sriov_vfs_supported() and create separate one igt_sriov_is_pf() that 
only checks if sriov_totalvfs exists. What do you think?
>
>>>> + *
>>>> + * 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',