rtnetlink: Add native SR-IOV VF status

[kbowling]

Summary

Add a transport-neutral kernel snapshot for NIC-specific SR-IOV VF
status and an optional iflib provider method.  Providers report only
cached state while their driver lock is held.

Honor RTEXT_FILTER_VF on RTM_GETLINK requests and encode the status as
native typed route Netlink attributes.  Represent VFs, driver
namespaces, and namespace fields as directly repeated nested
attributes.  Presence masks in consumers can distinguish omission from
false or zero.

Drivers may add custom status under stable, versioned namespaces.  The
named, typed representation lets generic transports and consumers carry
or display fields without knowing their driver-specific schemas, while
the driver retains ownership of their names and meanings.

Document the ABI and add parser and RTM_GETLINK coverage.

[kbowling]

sys/net/if_vf_status.h

52

@pouria I'm not entirely thrilled about the C typing here vs the nvlist keys but at least a conversation starter

[glebius]

P.S. Haven't yet reviewed the netlink part.

sys/net/if.c

2346–2348

Let's define some sane value for maximum num_vfs and assert it, rather than allow the function to fail.

2363–2364

Assert, don't allow stupid function use.

2390–2394

Provide some sane maximums and assert them. SIZE_MAX definitely is not a sane one.

2481–2483

Usual contract is that if function fails it doesn't modify the return value.

2490–2491

Better assert that driver either returns error or return 0 and sets statusp.

sys/net/if_vf_status.h

23–39

This also can be a enum.

101–109

If coded as bitfields you will have space to add more bools later without version bump. Also, the structure will shrink.

110

If you want it versioned, you probably want to shift this member to the very top.

[gallatin]

sys/net/if_vf_status.h

22

Huh? I understand "absence distinct from false or zero;", but the rest is really hard to read. If this was authored by AI, can you please re-write this in your own words?

55

I'm a native english speaker, and I like to think I'm fairly well read. However, I stumbled over this comment. Can you please make it more concise and readable? Especially for non-native english speakers. It may be as simple as re-writing in your own words if this was authored by AI

sys/net/iflib.c

4705

Isn't the usual way to make a default method that returns ENOTSUP rather than looking up the method each time and comparing to default? In fact, I think you already have this via null_vf_status(), so why do you need to look it up and compare to default?

[kbowling]

sys/net/if.c

2346–2348

converted to asserts per your other comments. UINT16_MAX based on PCI.

sys/net/if_vf_status.h

110

It is the negotiated PF/VF mailbox API string, not a version of struct if_vf_info. Added a comment to make this clearer.

sys/net/iflib.c

4705

Reworked slightly. The per query KOBJ lookup is gone and PFs use the ordinary default ENOTSUP method. Registration still omits the callback on actual VFs.

[gallatin]

LGTM, but please wait for somebody to review the netlink part, which I did not do.

[glebius]

In D58776#1349436, @gallatin wrote:

LGTM, but please wait for somebody to review the netlink part, which I did not do.

Yesterday I talked to Alexander melifaro@ and he promised to take look at all reviews he is subscribed to that touch netlink this week.

[kbowling]

In D58776#1349473, @glebius wrote:

In D58776#1349436, @gallatin wrote:

LGTM, but please wait for somebody to review the netlink part, which I did not do.

Yesterday I talked to Alexander melifaro@ and he promised to take look at all reviews he is subscribed to that touch netlink this week.

As written this can't go anywhere w/o D58775 and it's worth getting the ifconfig side checked off too in case anything back feeds here.

sys/net/if.c

2241

Before commit or if there is another round of review changes, I'm going to drop these ifr_nv_* methods and the struct change, they aren't needed by anything here anymore nor for the time being with the C typed data.

[melifaro]

LGTM (modulo removing nvlist helpers); left some comments in the diff.

Could you elaborate on the " This allows drivers to add information without expanding a central enumeration." a bit?
If the only concern is central enumeration, then it can be addressed differently - by having per-driver attributes.
Linux does it the following way: inside IFLA_LINKINFO attribute it stores IFLA_INFO_KIND attribute with the driver type and IFLA_INFO_DATA where any customer driver-specific structures can be stored.
I should have implemented something like that but never managed to (there are some bits and pieces in if_vlan.c that can serve as a simple example for the GET requests)

sys/net/if_vf_status.h

52

Wouldn't we need to map nvlist keys back to C structures anyway?

sys/netlink/netlink_snl_route_parsers.h

178

We seem to have IFLAF_VF_<XX>, IFVF_F_<XX> and SNL_VF_F_<XX>.
Is there are reason all 3 exists? Can't we reuse IFLAF_VF_<XX> as a bit identifier?

[kbowling]

In D58776#1350103, @melifaro wrote:

LGTM (modulo removing nvlist helpers); left some comments in the diff.

Could you elaborate on the " This allows drivers to add information without expanding a central enumeration." a bit?
If the only concern is central enumeration, then it can be addressed differently - by having per-driver attributes.
Linux does it the following way: inside IFLA_LINKINFO attribute it stores IFLA_INFO_KIND attribute with the driver type and IFLA_INFO_DATA where any customer driver-specific structures can be stored.
I should have implemented something like that but never managed to (there are some bits and pieces in if_vlan.c that can serve as a simple example for the GET requests)

The problem: contemporary NICs may have programmable data paths that can do just about anything, so the extension path lets the driver define additions. See D58739 for a sample usage. Nailing down some convention here is important given that a driver may be some proprietary thing etc.

[kbowling]

Removed now unneeded ifr_nv_* churn, use IFLAF_VF_* numbers directly (and in subordinate reviews), add some clarity around extensions.

[melifaro]

Thank you for addressing the feedback!
Q - is there a reason we do the "caching" inside if.c layer and not add a netlink hook to the driver (similar to ifhwioctl) to call, filling in those custom requests and driver-specific data in its own namespace attribute?
That would reduce the barrier on adding new driver-specific properties (no requirements on the kernel version).

sys/net/if.c

2461

I can understand (to some extent) the use case for passing string/integer values.
Update the driver, don't touch kernel or ifconfig bits - and allow to read/write the simple sysctl-like tunables that are driver specific.

What is the use case for passing binary data?

[kbowling]

In D58776#1350510, @melifaro wrote:

Thank you for addressing the feedback!
Q - is there a reason we do the "caching" inside if.c layer and not add a netlink hook to the driver (similar to ifhwioctl) to call, filling in those custom requests and driver-specific data in its own namespace attribute?
That would reduce the barrier on adding new driver-specific properties (no requirements on the kernel version).

I'm not sure I understand the caching question, if.c does not retain a cache. The driver constructs a per request snapshot under its own lock, rtnetlink serializes it afte the lock is released and then frees it (and caching might be overstated, it is the design goal but my shoddy mlx5en draft doesn't do this yet because I don't have deep familiarity with that driver or HW). Named fields under a versioned driver namespace already allow new bool, number, or string properties without changes to if.c, rtnetlink, or ifconfig. Only promotion into the common cross driver schema requires a kernel change.

sys/net/if.c

2461

You're probably right as it stands. For the current use as a read only interface, nil.

I can dream up some uses like feeding in an option ROM or chain of trust for a cloud service provider, although I'm not sure how that would lay in between iovctl and this right now. So writable configuration would be some separate project I don't currently have plans for. Should I remove this?

[pouria]

Thank you!

sys/net/if_vf_status.h

1

101–109

AFAICU, this:

	bool configured:1;
	bool initialized:1;
        ...

is not same as:

	bool configured:1,
        ...
	     initialized:1;

[kbowling]

sys/net/if_vf_status.h

1

I will drop '-' on commit or if there is a more substantial followup review round.

101–109

It is functionally the same if you check the compiler output since it doesn't cross an allocation boundary. Explicitly listing the type seems to be the more common FreeBSD convention, see struct cpu_info or struct g_part_table etc, but there are a couple counter examples. I don't care either way.

[kbowling]

I plan to commit this, D58777, D58778 if no more followups are received in the next 48 hours.