if(9): Implement support for nvlist-based set- and get- network interface capabilities.

[kib]

Summary

The existing 31 capabilities supported are now called legacy capabilities while
new capabilities will be added and supported by the use of SIOCGIFCAPNV and
SIOCSIFCAPNV. Refer to the if(9) manual page for a detailed description.

MFC after: 1 week
Sponsored by: NVIDIA Networking

Test Plan

The mlx5en changes will be committed separately, and are just there as an example.

[• hselasky]

Fix locking issue in mlx5en(4).

[• hselasky]

Fix conflict between libnvpair and libnv when building rescue binaries.

[• hselasky]

Fix order of variables.

[• hselasky]

make buildworld && make buildkernel

1. passed

[debdrup]

I only noticed one thing in the mdoc syntax.

share/man/man9/ifnet.9

1356–1357

The syntax here needs work; I think my suggestion will work.

Be sure to check with mandoc and igor.

[• hselasky]

Update ifnet.9 as suggested by @debdrup .

[debdrup]

Manual page looks good, I'll let others review the code.

[kib]

Directly add if_capenable2 and if_capabilities2. Pre-parse them from nvlist, same as if_capenable, to avoid repeated code in the drivers.

I might add some note about SIOCSIFCAPNV_DRIVER socket option in the man page some time later.

[• hselasky]

sys/dev/mlx5/mlx5_en/mlx5_en_main.c

4558–4559

Should add here:

ifp->if_capenable2 = ifp->if_capabilities2;

sys/net/if.h

436

Maybe 64 Kbytes would be good enough. 2MBytes seems large.

[• hselasky]

sys/dev/mlx5/mlx5_en/mlx5_en_main.c

3532

Do we need to update "ifp->if_capenable2" here?

[kib]

sys/dev/mlx5/mlx5_en/mlx5_en_main.c

3532

Yes, we do. But this should be done on the cap-by-cap basis, same as it is done for if_capenable. We need to use e.g. mask2 or reuse mask and do similar manipulations.

This is why I did not touched this aspect. Or, do you mean something different?

sys/net/if.h

436

This value is arbitrary. I do not see a problem with 'too large value' at the moment. For one-off ioctls, it should not matter.

[kib]

Properly handle if_capenable2 for mce(4).

[kib]

ping?

[debdrup]

.Dd also needs to be bumped.

share/man/man9/ifnet.9

1356

Still missing a space here between NULL and the comma.

[kib]

Space in .Dv NULL ,
Bump man page date in advance.

[debdrup]

mdoc looks good to me.

[jhb]

sbin/ifconfig/ifconfig.c

1478–1511

Hmm, this is still not addressed. That is, you still won't show "new" capabilities in the capabilities line. capabilities corresponds to if_capabilities whereas options corresponds to if_capenable and both are extended. I think what you want instead is to do something like:

if (ioctl(s, SIOCGIFCAP, ...) == 0) {
    if ((ifr.ifr_curcap & IFCAP_NV != 0) {
      ...
      ioctl(s, SIOCGIFCAPNV, ...);
      ...
      printf("\toptions");
      cookie = NULL;
      for (first = true; ;; first = false) {
          nvname = nvlist_next(...);
          ...
      }
      if (supmedia) {
          printf("\tcapabilities");
          cookie = NULL;
          for (first = true; first = false) {
              nvame = nvlist_next(...);
              if (nvame == NULL) {
                  ...
              }
              if (type == NV_TYPE_BOOL) {
                  printf("%c%s", first ? ' ' : ',', nvname);
              }
          }
          nvlist_destroy(nvcap);
          free(buf);
      }
    } else {
      if (ifr.ifr_curcap != 0) {
          printb("\toptions", ifr.ifr_curcap, IFCAPBITS);
          putchar('\n');
      }
      if (supmedia && ifr.ifr_reqcap != 0) {
          printb("\tcapabilties", ifr.ifr_reqcap, IFCAPBITS);
          putchar('\n');
      }
}

share/mk/src.libnames.mk

570

s/JNV/NV/

sys/net/if.c

2687

ENOTTY?

2726

Rather than adding a new ioctl, I would just reuse SIOCSIFCAPNV here and the driver contract would be that you are getting the helper struct in the arg in the driver rather than the user pointer. I think we do similar things in some other network ioctls already, for example ADDMULTI and DELMULTI.

[kib]

Remove SIOCSIFCAPNV_DRIVER.
Properly print capabilities from nv list for ifconfig -m.
Fix typo in the libnv variable dir name.

[jhb]

Thanks

[• hselasky]

Changes look good to me. Some minor comments added.

sbin/ifconfig/ifconfig.c

1518

Whitespace changes here could be skipped.

share/man/man9/ifnet.9

31

Fix date before commit?

1341

.Xr ioctl 2 .

1342

Spelling?

Caller must provide the pointer to

Caller must provide a pointer to

1353

can skip the word "then"

sys/dev/mlx5/mlx5_en/mlx5_en_main.c

3437

I think it would be clever to zero-init drv_ioctl_data_d:

struct siocsifcapnv_driver_data *drv_ioctl_data, drv_ioctl_data_d;

struct siocsifcapnv_driver_data *drv_ioctl_data:
struct siocsifcapnv_driver_data drv_ioctl_data_d = {};

sys/net/if.h

486

Upper case "nv" -> "NV" ?

[kib]

sbin/ifconfig/ifconfig.c

1518

How so? Patch moves the block under the nested 'else'.

share/man/man9/ifnet.9

1341

IMO there is no point in cross-reference there. We talk not about syscall, but about some feature implemented as part of the described framework.

sys/dev/mlx5/mlx5_en/mlx5_en_main.c

3437

It is not needed, in the sense that driver code which fills the structure, uses it as well, but this is not a critical path.

sys/net/if.h

486

Why? Other abbreviations, like 'pcp', 'vlan', or 'fib' are not upper-cased.

[kib]

Minor tweaks suggested by Hans:

• man page grammar fixes
• initialize fake ifcapnv parameter structure in mlx5en ioctl handler

[• hselasky]

I'm good with this.

sbin/ifconfig/ifconfig.c

1518

You wrapped a long line, but it is not important.