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

[Luca Vizzarro <Luca.Vizzarro@arm.com>]

Usual problem with the commit subject.

On 23/09/2024 19:42, jspewock@iol.unh.edu wrote:
> +    def set_num_virtual_functions(self, num: int, pf_port: Port) -> bool:

I'd call this set_num_of_virtual_functions, set_virtual_functions_num, 
or set_number_of_vfs. I think I prefer the last one.

> +        """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:
> +            self._logger.error(
> +                f"Not enough VFs configured on port {pf_port.identifier.pci} on node "
> +                f"{pf_port.identifier.node}. Need {num} but only {curr_num_vfs} are configured. "
> +                "DTS is unable to modify number of preexisting VFs."
> +            )
> +            raise InternalError("Failed to create VFs on port.")

This is not an InternalError. That exception represents a bug internal 
to DTS, it's not related to the node not satisfying requirements. I am 
guessing this requires a new kind of error.

> +        self.send_command(f"echo {num} > {sys_bus_path}", privileged=True, verify=True)
> +        return True
> +
> +    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",

I like the use of awk. Using a TextWrapper may be overkill when we have 
easier options like this one.


> +    def set_num_virtual_functions(self, num: int, pf_port: Port) -> bool:
> +        """Update the number of virtual functions (VFs) on a port.

Like Juraj mentioned, this docstring should say that it can be used for 
creating and removing VFs.