[v4,5/5] dts: add functions for managing VFs to Node

From: Jeremy Spewock <jspewock@iol.unh.edu>

In order for test suites to create virtual functions there has to be
functions in the API that developers can use. This patch adds the
ability to create virtual functions to the Node API so that they are
reachable within test suites.

Bugzilla ID: 1500
Depends-on: patch-144318 ("dts: add binding to different drivers to TG
node")

Signed-off-by: Jeremy Spewock <jspewock@iol.unh.edu>
---
 dts/framework/testbed_model/node.py | 97 ++++++++++++++++++++++++++++-
 1 file changed, 94 insertions(+), 3 deletions(-)
  

I'm wondering whether we should move some of the functionality to the 
Port class, such as creating VFs and related logic. I wanted to move 
update_port and such there, but I ran into problems with imports. Maybe 
if we utilize the if TYPE_CHECKING: guard the imports would work.

Seems like a lot would be simplified if we moved the VFs ports inside 
the Port class.

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

>   class Node(ABC):
> @@ -276,6 +277,96 @@ def _bind_port_to_driver(self, port: Port, for_dpdk: bool = True) -> None:
>               verify=True,
>           )
>   
> +    def create_virtual_functions(
> +        self, num: int, pf_port: Port, dpdk_driver: str | None = None
> +    ) -> list[VirtualFunction]:
> +        """Create virtual functions (VFs) from a given physical function (PF) on the node.
> +
> +        Virtual functions will be created if there are not any currently configured on `pf_port`.
> +        If there are greater than or equal to `num` VFs already configured on `pf_port`, those will
> +        be used instead of creating more. In order to create VFs, the PF must be bound to its
> +        kernel driver. This method will handle binding `pf_port` and any other ports in the test
> +        run that reside on the same device back to their OS drivers if this was not done already.
> +        VFs gathered in this method will be bound to `driver` if one is provided, or the DPDK
> +        driver for `pf_port` and then added to `self.ports`.
> +
> +        Args:
> +            num: The number of VFs to create. Must be greater than 0.
> +            pf_port: The PF to create the VFs on.

We should check that the passed port actually resides on this node.

> +            dpdk_driver: Optional driver to bind the VFs to after they are created. Defaults to the
> +                DPDK driver of `pf_port`.
> +
> +        Raises:
> +            InternalError: If `num` is less than or equal to 0.
> +        """
> +        if num <= 0:
> +            raise InternalError(
> +                "Method for creating virtual functions received a non-positive value."
> +            )
> +        if not dpdk_driver:
> +            dpdk_driver = pf_port.os_driver_for_dpdk
> +        # Get any other port that is on the same device which DTS is aware of
> +        all_device_ports = [
> +            p for p in self.ports if p.pci.split(".")[0] == pf_port.pci.split(".")[0]
> +        ]

Maybe we should create a PciAddress class that would process the address 
and provide useful methods, one of which we'd use here.

> +        # Ports must be bound to the kernel driver in order to create VFs from them
> +        for port in all_device_ports:
> +            self._bind_port_to_driver(port, False)
> +            # Some PMDs require the interface being up in order to make VFs

These are OS drivers, not PMDs.

> +            self.configure_port_state(port)
> +        created_vfs = self.main_session.set_num_virtual_functions(num, pf_port)
> +        # We don't need more then `num` VFs from the list
> +        vf_pcis = self.main_session.get_pci_addr_of_vfs(pf_port)[:num]
> +        devbind_info = self.main_session.send_command(
> +            f"{self.path_to_devbind_script} -s", privileged=True
> +        ).stdout

This looks like a good candidate for TextParser.

> +
> +        ret = []
> +
> +        for pci in vf_pcis:
> +            original_driver = re.search(f"{pci}.*drv=([\\d\\w-]*)", devbind_info)
> +            os_driver = original_driver[1] if original_driver else pf_port.os_driver
> +            vf_config = PortConfig(
> +                self.name, pci, dpdk_driver, os_driver, pf_port.peer.node, pf_port.peer.pci
> +            )
> +            vf_port = VirtualFunction(self.name, vf_config, created_vfs, pf_port)
> +            self.main_session.update_ports([vf_port])

This should be called after the for cycle so we only call it once. We 
can bind all VF ports after (again, preferably with just one call).
  

On 23/09/2024 19:42, jspewock@iol.unh.edu wrote:
> +
> +    def get_vfs_on_port(self, pf_port: Port) -> list[VirtualFunction]:
> +        """Get all virtual functions (VFs) that DTS is aware of on `pf_port`.
> +
> +        Args:
> +            pf_port: The port to search for the VFs on.
> +
> +        Returns:
> +            A list of VFs in the framework that were created/gathered from `pf_port`.
> +        """
> +        return [p for p in self.ports if isinstance(p, VirtualFunction) and p.pf_port == pf_port]

If the change proposed by Juraj to store VFs under the PF goes through, 
this could end up being simpler.

> +
> +    def remove_virtual_functions(self, pf_port: Port) -> None:
> +        """Removes all virtual functions (VFs) created on `pf_port` by DTS.
> +
> +        Finds all the VFs that were created from `pf_port` and either removes them if they were
> +        created by the DTS framework or binds them back to their os_driver if they were preexisting
> +        on the node.
> +
> +        Args:
> +            pf_port: Port to remove the VFs from.
> +        """
> +        vf_ports = self.get_vfs_on_port(pf_port)
> +        if any(vf.created_by_framework for vf in vf_ports):
> +            self.main_session.set_num_virtual_functions(0, pf_port)
> +        else:
> +            self._logger.info("Skipping removing VFs since they were not created by DTS.")

This will cause VFs that were created by us to not be removed. We should 
ensure at least these ones are cleaned up.