Page MenuHomeFreeBSD

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

Authored by kib on Oct 18 2021, 3:18 PM.

Diff Detail

rG FreeBSD src repository
Automatic diff as part of commit; lint not applicable.
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

Address comment by @jhb .

Keep existing capabilities as legacy capabilities. Consider moving the TLSTX bits out of there, because it is new and few nics support it currently.

Updated mlx5en driver to support TLS RX caps.

Fix locking issue in mlx5en(4).

Fix conflict between libnvpair and libnv when building rescue binaries.

make buildworld && make buildkernel

  1. passed
hselasky added a reviewer: network.

I only noticed one thing in the mdoc syntax.

1357–1358 ↗(On Diff #103831)

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

Be sure to check with mandoc and igor.

hselasky marked an inline comment as done.

Update ifnet.9 as suggested by @debdrup .

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

This revision is now accepted and ready to land.Mar 15 2022, 7:06 PM
kib edited reviewers, added: hselasky; removed: 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.

This revision now requires review to proceed.Apr 15 2022, 2:28 PM
4558 ↗(On Diff #105039)

Should add here:

ifp->if_capenable2 = ifp->if_capabilities2;
436 ↗(On Diff #105039)

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

3532 ↗(On Diff #105039)

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

kib marked 4 inline comments as done.Apr 16 2022, 1:14 AM
kib added inline comments.
3532 ↗(On Diff #105039)

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?

436 ↗(On Diff #105039)

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 marked 2 inline comments as done.

Properly handle if_capenable2 for mce(4).

This revision is now accepted and ready to land.Apr 16 2022, 7:08 AM

.Dd also needs to be bumped.

1356 ↗(On Diff #105072)

Still missing a space here between NULL and the comma.

kib marked an inline comment as done.

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

This revision now requires review to proceed.May 7 2022, 11:23 PM

mdoc looks good to me.

This revision is now accepted and ready to land.May 8 2022, 4:05 PM
1478 ↗(On Diff #100332)

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, ...);
      cookie = NULL;
      for (first = true; ;; first = false) {
          nvname = nvlist_next(...);
      if (supmedia) {
          cookie = NULL;
          for (first = true; first = false) {
              nvame = nvlist_next(...);
              if (nvame == NULL) {
              if (type == NV_TYPE_BOOL) {
                  printf("%c%s", first ? ' ' : ',', nvname);
    } else {
      if (ifr.ifr_curcap != 0) {
          printb("\toptions", ifr.ifr_curcap, IFCAPBITS);
      if (supmedia && ifr.ifr_reqcap != 0) {
          printb("\tcapabilties", ifr.ifr_reqcap, IFCAPBITS);
570 ↗(On Diff #105775)


2705 ↗(On Diff #105775)


2744 ↗(On Diff #105775)

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 marked 4 inline comments as done.May 18 2022, 10:17 PM

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

This revision now requires review to proceed.May 18 2022, 11:52 PM
This revision is now accepted and ready to land.May 20 2022, 5:32 PM

Changes look good to me. Some minor comments added.

1528 ↗(On Diff #106151)

Whitespace changes here could be skipped.

31 ↗(On Diff #106151)

Fix date before commit?

1341 ↗(On Diff #106151)

.Xr ioctl 2 .

1342 ↗(On Diff #106151)


Caller must provide the pointer to
Caller must provide a pointer to
1353 ↗(On Diff #106151)

can skip the word "then"

3437 ↗(On Diff #106151)

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 = {};
486 ↗(On Diff #106151)

Upper case "nv" -> "NV" ?

kib marked 7 inline comments as done.May 23 2022, 9:58 PM
kib added inline comments.
1528 ↗(On Diff #106151)

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

1341 ↗(On Diff #106151)

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.

3437 ↗(On Diff #106151)

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.

486 ↗(On Diff #106151)

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

kib marked 4 inline comments as done.

Minor tweaks suggested by Hans:

  • man page grammar fixes
  • initialize fake ifcapnv parameter structure in mlx5en ioctl handler
This revision now requires review to proceed.May 23 2022, 10:09 PM

I'm good with this.

1528 ↗(On Diff #106151)

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

This revision is now accepted and ready to land.May 24 2022, 6:03 AM