Re: [PATCH v4 4/5] dts: add OS abstractions for creating virtual functions

["Juraj Linkeš" <juraj.linkes@pantheon.tech>]

> diff --git a/dts/framework/testbed_model/linux_session.py b/dts/framework/testbed_model/linux_session.py

> @@ -210,3 +214,37 @@ def configure_ipv4_forwarding(self, enable: bool) -> None:
>           """Overrides :meth:`~.os_session.OSSession.configure_ipv4_forwarding`."""
>           state = 1 if enable else 0
>           self.send_command(f"sysctl -w net.ipv4.ip_forward={state}", privileged=True)
> +
> +    def set_num_virtual_functions(self, num: int, pf_port: Port) -> bool:
> +        """Overrides :meth:`~.os_session.OSSession.set_num_virtual_functions`."""
> +        sys_bus_path = f"/sys/bus/pci/devices/{pf_port.pci}/sriov_numvfs".replace(":", "\\:")
> +        curr_num_vfs = int(self.send_command(f"cat {sys_bus_path}").stdout)
> +        if num > 0 and curr_num_vfs >= num:
> +            self._logger.info(
> +                f"{curr_num_vfs} VFs already configured on port {pf_port.identifier.pci} on node "
> +                f"{pf_port.identifier.node}."
> +            )
> +            return False
> +        elif num > 0 and curr_num_vfs > 0:

These two conditions could be written as:
if num > 0:
     if curr_num_vfs >= num:
         return False
     elif curr_num_vfs > 0:
         raise InternalError()

Maybe it's not worth the extra indent, but it's easier to understand, so 
I lean towards doing it this way.


> +    def get_pci_addr_of_vfs(self, pf_port: Port) -> list[str]:
> +        """Overrides :meth:`~.os_session.OSSession.get_pci_addr_of_vfs`."""
> +        sys_bus_path = f"/sys/bus/pci/devices/{pf_port.pci}".replace(":", "\\:")
> +        curr_num_vfs = int(self.send_command(f"cat {sys_bus_path}/sriov_numvfs").stdout)
> +        if curr_num_vfs > 0:
> +            pci_addrs = self.send_command(
> +                'awk -F "PCI_SLOT_NAME=" "/PCI_SLOT_NAME=/ {print \\$2}" '
> +                + f"{sys_bus_path}/virtfn*/uevent",

We could use a TextParser here. Not sure if it's a good fit though.


> diff --git a/dts/framework/testbed_model/os_session.py b/dts/framework/testbed_model/os_session.py

> @@ -395,3 +395,43 @@ def configure_ipv4_forwarding(self, enable: bool) -> None:

> +    @abstractmethod
> +    def set_num_virtual_functions(self, num: int, pf_port: Port) -> bool:
> +        """Update the number of virtual functions (VFs) on a port.
> +
> +        It should be noted that, due to the nature of VFs, if there are already VFs that exist on
> +        the physical function (PF) prior to calling this function, additional ones cannot be added.
> +        The only way to add more VFs is to remove the existing and then set the desired amount. For
> +        this reason, this method will handle creation in the following order:
> +
> +        1. Use existing VFs on the PF if the number of existing VFs is greater than or equal to
> +        `num`
> +        2. Throw an exception noting that VFs cannot be created if the PF has some VFs already set
> +        on it, but the total VFs that it has are less then `num`.
> +        3. Create `num` VFs on the PF if there are none on it already
> +
> +        Args:
> +            num: The number of VFs to set on the port.
> +            pf_port: The port to add the VFs to.
> +
> +        Raises:
> +            InternalError: If `pf_port` has less than `num` VFs configured on it
> +                already.
> +
> +        Returns:
> +            :data:`True` if this method successfully created VFs, :data:`False` if existing VFs
> +            were used instead.
> +        """

The whole docstring talks only about the creation of VFs, but we can 
also remove VFs with this.