[PATCH net] netdevsim: Fix physical port index

[PATCH net] netdevsim: Fix physical port index

[Frode Nordahl]

At present when a netdevsim device is added, the first physical
port will have an index of 1.  This behavior differ from what any
real hardware driver would do, which would start the index at 0.

When using netdevsim to test the devlink-port interface this
behavior becomes a problem because the provided data is incorrect.

Example:
$ sudo modprobe netdevsim
$ sudo sh -c 'echo "10 1" > /sys/bus/netdevsim/new_device'
$ sudo sh -c 'echo 4 > /sys/class/net/eni10np1/device/sriov_numvfs'
$ sudo devlink dev eswitch set netdevsim/netdevsim10 mode switchdev
$ devlink port show
netdevsim/netdevsim10/0: type eth netdev eni10np1 flavour physical port 1
netdevsim/netdevsim10/128: type eth netdev eni10npf0vf0 flavour pcivf pfnum 0 vfnum 0
netdevsim/netdevsim10/129: type eth netdev eni10npf0vf1 flavour pcivf pfnum 0 vfnum 1
netdevsim/netdevsim10/130: type eth netdev eni10npf0vf2 flavour pcivf pfnum 0 vfnum 2
netdevsim/netdevsim10/131: type eth netdev eni10npf0vf3 flavour pcivf pfnum 0 vfnum 3

With this patch applied you would instead get:
$ sudo modprobe netdevsim
$ sudo sh -c 'echo "10 1" > /sys/bus/netdevsim/new_device'
$ sudo sh -c 'echo 4 > /sys/class/net/eni10np0/device/sriov_numvfs'
$ sudo devlink dev eswitch set netdevsim/netdevsim10 mode switchdev
$ devlink port show
netdevsim/netdevsim10/0: type eth netdev eni10np0 flavour physical port 0
netdevsim/netdevsim10/128: type eth netdev eni10npf0vf0 flavour pcivf pfnum 0 vfnum 0
netdevsim/netdevsim10/129: type eth netdev eni10npf0vf1 flavour pcivf pfnum 0 vfnum 1
netdevsim/netdevsim10/130: type eth netdev eni10npf0vf2 flavour pcivf pfnum 0 vfnum 2
netdevsim/netdevsim10/131: type eth netdev eni10npf0vf3 flavour pcivf pfnum 0 vfnum 3

The above more accurately resembles what a real system would look
like.

Fixes: 8320d1459127 ("netdevsim: implement dev probe/remove skeleton with port initialization")
Signed-off-by: Frode Nordahl <frode.nordahl@canonical.com>
---
 drivers/net/netdevsim/dev.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/netdevsim/dev.c b/drivers/net/netdevsim/dev.c
index 54345c096a16..8045852cf0fe 100644
--- a/drivers/net/netdevsim/dev.c
+++ b/drivers/net/netdevsim/dev.c
@@ -1371,7 +1371,7 @@ static int __nsim_dev_port_add(struct nsim_dev *nsim_dev, enum nsim_dev_port_typ
 	devlink_port = &nsim_dev_port->devlink_port;
 	if (nsim_dev_port_is_pf(nsim_dev_port)) {
 		attrs.flavour = DEVLINK_PORT_FLAVOUR_PHYSICAL;
-		attrs.phys.port_number = port_index + 1;
+		attrs.phys.port_number = port_index;
 	} else {
 		attrs.flavour = DEVLINK_PORT_FLAVOUR_PCI_VF;
 		attrs.pci_vf.pf = 0;
-- 
2.32.0

Re: [PATCH net] netdevsim: Fix physical port index

[Jakub Kicinski]

On Wed, 24 Nov 2021 09:11:06 +0100 Frode Nordahl wrote:
> At present when a netdevsim device is added, the first physical
> port will have an index of 1.  This behavior differ from what any
> real hardware driver would do, which would start the index at 0.
> 
> When using netdevsim to test the devlink-port interface this
> behavior becomes a problem because the provided data is incorrect.
> 
> Example:
> $ sudo modprobe netdevsim
> $ sudo sh -c 'echo "10 1" > /sys/bus/netdevsim/new_device'
> $ sudo sh -c 'echo 4 > /sys/class/net/eni10np1/device/sriov_numvfs'
> $ sudo devlink dev eswitch set netdevsim/netdevsim10 mode switchdev
> $ devlink port show
> netdevsim/netdevsim10/0: type eth netdev eni10np1 flavour physical port 1
> netdevsim/netdevsim10/128: type eth netdev eni10npf0vf0 flavour pcivf pfnum 0 vfnum 0
> netdevsim/netdevsim10/129: type eth netdev eni10npf0vf1 flavour pcivf pfnum 0 vfnum 1
> netdevsim/netdevsim10/130: type eth netdev eni10npf0vf2 flavour pcivf pfnum 0 vfnum 2
> netdevsim/netdevsim10/131: type eth netdev eni10npf0vf3 flavour pcivf pfnum 0 vfnum 3
> 
> With this patch applied you would instead get:
> $ sudo modprobe netdevsim
> $ sudo sh -c 'echo "10 1" > /sys/bus/netdevsim/new_device'
> $ sudo sh -c 'echo 4 > /sys/class/net/eni10np0/device/sriov_numvfs'
> $ sudo devlink dev eswitch set netdevsim/netdevsim10 mode switchdev
> $ devlink port show
> netdevsim/netdevsim10/0: type eth netdev eni10np0 flavour physical port 0
> netdevsim/netdevsim10/128: type eth netdev eni10npf0vf0 flavour pcivf pfnum 0 vfnum 0
> netdevsim/netdevsim10/129: type eth netdev eni10npf0vf1 flavour pcivf pfnum 0 vfnum 1
> netdevsim/netdevsim10/130: type eth netdev eni10npf0vf2 flavour pcivf pfnum 0 vfnum 2
> netdevsim/netdevsim10/131: type eth netdev eni10npf0vf3 flavour pcivf pfnum 0 vfnum 3
> 
> The above more accurately resembles what a real system would look
> like.
> 
> Fixes: 8320d1459127 ("netdevsim: implement dev probe/remove skeleton with port initialization")
> Signed-off-by: Frode Nordahl <frode.nordahl@canonical.com>

Why do you care about the port ID starting at 0? It's not guaranteed.
The device can use any encoding scheme to assign IDs, user space should
make no assumptions here.

Please use get_maintainers to CC all the relevant people.

Re: [PATCH net] netdevsim: Fix physical port index

[Frode Nordahl]

On Wed, Nov 24, 2021 at 3:20 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Wed, 24 Nov 2021 09:11:06 +0100 Frode Nordahl wrote:
> > At present when a netdevsim device is added, the first physical
> > port will have an index of 1.  This behavior differ from what any
> > real hardware driver would do, which would start the index at 0.
> >
> > When using netdevsim to test the devlink-port interface this
> > behavior becomes a problem because the provided data is incorrect.
> >
> > Example:
> > $ sudo modprobe netdevsim
> > $ sudo sh -c 'echo "10 1" > /sys/bus/netdevsim/new_device'
> > $ sudo sh -c 'echo 4 > /sys/class/net/eni10np1/device/sriov_numvfs'
> > $ sudo devlink dev eswitch set netdevsim/netdevsim10 mode switchdev
> > $ devlink port show
> > netdevsim/netdevsim10/0: type eth netdev eni10np1 flavour physical port 1
> > netdevsim/netdevsim10/128: type eth netdev eni10npf0vf0 flavour pcivf pfnum 0 vfnum 0
> > netdevsim/netdevsim10/129: type eth netdev eni10npf0vf1 flavour pcivf pfnum 0 vfnum 1
> > netdevsim/netdevsim10/130: type eth netdev eni10npf0vf2 flavour pcivf pfnum 0 vfnum 2
> > netdevsim/netdevsim10/131: type eth netdev eni10npf0vf3 flavour pcivf pfnum 0 vfnum 3
> >
> > With this patch applied you would instead get:
> > $ sudo modprobe netdevsim
> > $ sudo sh -c 'echo "10 1" > /sys/bus/netdevsim/new_device'
> > $ sudo sh -c 'echo 4 > /sys/class/net/eni10np0/device/sriov_numvfs'
> > $ sudo devlink dev eswitch set netdevsim/netdevsim10 mode switchdev
> > $ devlink port show
> > netdevsim/netdevsim10/0: type eth netdev eni10np0 flavour physical port 0
> > netdevsim/netdevsim10/128: type eth netdev eni10npf0vf0 flavour pcivf pfnum 0 vfnum 0
> > netdevsim/netdevsim10/129: type eth netdev eni10npf0vf1 flavour pcivf pfnum 0 vfnum 1
> > netdevsim/netdevsim10/130: type eth netdev eni10npf0vf2 flavour pcivf pfnum 0 vfnum 2
> > netdevsim/netdevsim10/131: type eth netdev eni10npf0vf3 flavour pcivf pfnum 0 vfnum 3
> >
> > The above more accurately resembles what a real system would look
> > like.
> >
> > Fixes: 8320d1459127 ("netdevsim: implement dev probe/remove skeleton with port initialization")
> > Signed-off-by: Frode Nordahl <frode.nordahl@canonical.com>
>
> Why do you care about the port ID starting at 0? It's not guaranteed.
> The device can use any encoding scheme to assign IDs, user space should
> make no assumptions here.

I don't care too much about the ID itself starting at 0 per se, but I
would expect the ID's provided through devlink-port to match between
the value specified for DEVLINK_ATTR_PORT_PCI_PF_NUMBER on the
simulated PCI_VF flavoured ports, the value specified for
DEVLINK_ATTR_PORT_NUMBER on the simulated physical ports and the value
specified for DEVLINK_ATTR_PORT_PCI_PF_NUMBER  on the simulated PCI_PF
flavoured ports.

For a user space application running on a host with a regular
devlink-enabled NIC (let's say a ConnectX-5), it can figure out the
relationship between the ports with the regular sysfs API.

However, for a user space application running on the Arm cores of a
devlink-enabled SmartNIC with control plane CPUs (let's say a
BlueField2), the relationship between the representor ports is not
exposed in the regular sysfs API. So this is where the devlink-port
interface becomes important. From a PHYSICAL representor I need to
find which PF representors are associated, from there I need to find
VF representors associated, and the other way round.

> Please use get_maintainers to CC all the relevant people.

Thank you for pointing that out. Apologies for skipping that step,
added the missing ones now.

-- 
Frode Nordahl

Re: [PATCH net] netdevsim: Fix physical port index

[Jakub Kicinski]

On Wed, 24 Nov 2021 17:24:16 +0100 Frode Nordahl wrote:
> I don't care too much about the ID itself starting at 0 per se, but I
> would expect the ID's provided through devlink-port to match between
> the value specified for DEVLINK_ATTR_PORT_PCI_PF_NUMBER on the
> simulated PCI_VF flavoured ports, the value specified for
> DEVLINK_ATTR_PORT_NUMBER on the simulated physical ports and the value
> specified for DEVLINK_ATTR_PORT_PCI_PF_NUMBER  on the simulated PCI_PF
> flavoured ports.
> 
> For a user space application running on a host with a regular
> devlink-enabled NIC (let's say a ConnectX-5), it can figure out the
> relationship between the ports with the regular sysfs API.
> 
> However, for a user space application running on the Arm cores of a
> devlink-enabled SmartNIC with control plane CPUs (let's say a
> BlueField2), the relationship between the representor ports is not
> exposed in the regular sysfs API. So this is where the devlink-port
> interface becomes important. From a PHYSICAL representor I need to
> find which PF representors are associated, from there I need to find
> VF representors associated, and the other way round.

I see, thanks for this explanation.

There is no fundamental association between physical port and PF in
NICs in switchdev mode. Neither does the bare metal host have any
business knowing anything about physical ports (including the number 
of them).

Obviously that's the theory, vendors like to abuse the APIs and cause
all sort of.. fun.

Re: [PATCH net] netdevsim: Fix physical port index

[Frode Nordahl]

(Apologies for any duplicates, my mailer somehow introduced HTML parts
which the ML does not like, resending to fix that).

On Wed, Nov 24, 2021 at 6:40 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Wed, 24 Nov 2021 17:24:16 +0100 Frode Nordahl wrote:
> > I don't care too much about the ID itself starting at 0 per se, but I
> > would expect the ID's provided through devlink-port to match between
> > the value specified for DEVLINK_ATTR_PORT_PCI_PF_NUMBER on the
> > simulated PCI_VF flavoured ports, the value specified for
> > DEVLINK_ATTR_PORT_NUMBER on the simulated physical ports and the value
> > specified for DEVLINK_ATTR_PORT_PCI_PF_NUMBER  on the simulated PCI_PF
> > flavoured ports.
> >
> > For a user space application running on a host with a regular
> > devlink-enabled NIC (let's say a ConnectX-5), it can figure out the
> > relationship between the ports with the regular sysfs API.
> >
> > However, for a user space application running on the Arm cores of a
> > devlink-enabled SmartNIC with control plane CPUs (let's say a
> > BlueField2), the relationship between the representor ports is not
> > exposed in the regular sysfs API. So this is where the devlink-port
> > interface becomes important. From a PHYSICAL representor I need to
> > find which PF representors are associated, from there I need to find
> > VF representors associated, and the other way round.
>
> I see, thanks for this explanation.
>
> There is no fundamental association between physical port and PF in
> NICs in switchdev mode. Neither does the bare metal host have any
> business knowing anything about physical ports (including the number
> of them).

For systems where the NICs are connected to multiple CPUs I have not
seen information about the physical ports exposed to the bare metal
host.

I assume there is an association between the PF and VF ports from devlink POV?

> Obviously that's the theory, vendors like to abuse the APIs and cause
> all sort of.. fun.

The need to associate PF with physical comes from this kind of thing.

In recent kernels the bare metal host PF MAC is exposed through
devlink to the SmartNIC CPU PF representor. However for SmartNIC CPUs
running older kernels this is not available, and some vendors have
provided an interim sysfs interface relative to the physical port
netdev.

So to deal with this the fallback support would need to guess the
association, and we'll just have to hope for recent kernels getting
into the wild sooner.

-- 
Frode Nordahl

Re: [PATCH net] netdevsim: Fix physical port index

[Jakub Kicinski]

On Wed, 24 Nov 2021 19:25:10 +0100 Frode Nordahl wrote:
> I assume there is an association between the PF and VF ports from devlink POV?

Yes, the VFs have PF id which should match the PF id from which they
were spawned.