cxgbe: Report SR-IOV VF status

[kbowling]

Summary

Retain the PF-accepted MAC and VLAN settings from the per-port t4iov
companion and expose them through the corresponding cxgbe ifnet.

Publish, snapshot, and destroy the cache under the existing adapter
synchronized-operation mechanism so status queries cannot race IOV
configuration or teardown.

Track successful t4iov attachment independently of the active VF count.
Restrict reporting to the port main VI, return an empty status for a
supported but unconfigured PF, and omit status from VF and auxiliary
VIs.

[kbowling]

This is a basic implementation, someone more familiar with the driver may look at the nvlist schema and https://reviews.freebsd.org/D58739 which demonstrates driver extensions.

[jhb]

sys/dev/cxgbe/adapter.h

315

Maybe struct t4_vf_info? I think that's more consistent with some other cxgbe type names. Really cxgbe would probably call this struct vf_info similar to how struct adapter, but the collision in type names is terrible, so using a t4_ prefix is fine.

sys/dev/cxgbe/t4_iov.c

445

Since you need to do a sync op anyway, just do a single one earlier in the function, and it can use INTR_OK like the existing ones. That is:

has_mac = ...;

if (begin_synchronized_op(adap, NULL, SLEEP_OK | INTR_OK, "t4vfadd") != 0)
   return (ENXIO);

if (has_mac) {
    mac = ...;
    bcopy(...);
    rc = -t4_vf_set_mac(...);
    if (rc != 0) {
        device_printf(...);
        goto out;
}

if (nvlist_exists_number(...)) {
  /* Replace bare "returns" with 'rc = EFOO; goto out;' */
}

However, I think there's also a simpler solution which is a smaller patch. Instead do something like this for the t4iov_add_vf() function:

static int
t4iov_add_vf(...)
{
    const void *mac;
    struct t4iov_softc *sc;
    struct adapter *adap;
    struct t4_iov_vf *vf;
    ...

    adap = ...;
    vf = &sc->sc_port->iov_vfs[vfnum];
    memset(vf, 0, sizeof(*vf));

    ...

    if (nvlist_exists_binary(config, "mac-addr")) {
        ....
        if (rc != 0) {
             ...
        }
        memcpy(vf->mac, ma, ETHER_ADDR_LEN);
   }

   if (nvlist_exists_number(config, "vlan")) {
        ...
        if (vlan == VF_VLAN_TRUNK) {
           vlan = DOT1Q_VID_NULL;
        } else {
           vf->access_vlan = true;
           vf->vlan = vlan;
        }
        ...
    }

    vf->configured = true;
    return (0);
}

The PCI bus driver already needs to synchronize IOV setup / teardown on a given PF, and it should also avoid calling IOV_ADD_VF on the same VF concurrently as a result. Drivers don't need additional protection from that. If the query race is what you are worried about, you can expand the scope of the sync_op as in my first suggestion but still do the inline assignments from my second suggestion.

491

This can't happen unless the PCI bus driver screws up, so I don't think it's worth checking.

sys/dev/cxgbe/t4_main.c

3107

I would add a continue here if the VF isn't configured as there is no point in reporting other fields for an unconfigured VF. It also lets you do the assignments inline in t4iov_add_vf() instead of deferring them all to the end.

[kbowling]

Update to committed netlink API, address review feedback

[jhb]

sys/dev/cxgbe/adapter.h

335

I don't think you need this. Anytime this is false, iov_num_vfs will be 0, so the bool is redundant.

sys/dev/cxgbe/t4_main.c

3100

I think you can use M_WAITOK here since the synchronized_op uses SLEEP_OK.

Also, I think you can just let the num_vfs == 0 case fall out in the normal path without special handling:

error = begin_synchronized_op(...);
if (error != 0)
    return (error);
num_vfs = pi->iov_num_vfs;
snapshot = mallocarray(num_vfs, sizeof(*snapshot), M_CXGBE, M_WAITOK);
memcpy(snapshot, pi->iov_vfs, num_vfs * sizeof(*snapshot));
end_synchronized_op(...);

3108

I wonder if in practice this function should just use M_WAITOK and then it can never return NULL which would simplify callers. Presumably if_vfstatus() is only invoked from an ioctl() handler in which case it can safely sleep and M_WAITOK should be fine? In cases where sleeping is ok, M_WAITOK is generally preferable as it results in fewer error cases to handle minimizing complexity.

[kbowling]

Address review feedback

sys/dev/cxgbe/adapter.h

335

We deliberately distinguish “IOV attached, none configured” from “IOV unavailable.” Removing it would also make VF interfaces or kernels without PCI_IOV appear to support empty PF reports.