Date: Mon, 16 Feb 2015 19:50:56 -0600 From: Mike Karels <mike@karels.net> To: "George Neville-Neil" <gnn@neville-neil.com> Cc: "freebsd-net@freebsd.org" <freebsd-net@freebsd.org>, "freebsd-arch@freebsd.org" <freebsd-arch@freebsd.org> Subject: Re: Adding new media types to if_media.h Message-ID: <201502170150.t1H1ouxM020621@mail.karels.net> In-Reply-To: Your message of Mon, 09 Feb 2015 21:08:41 %2B0000. <BC6AB805-B699-438E-9EC6-015C005D6B1D@neville-neil.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On Feb 9, gnn wrote: > On 8 Feb 2015, at 22:41, Mike Karels wrote: > > Sorry to reply to a thread after such a long delay, but I think it is > > unresolved, and needs more discussion. I'd like to elaborate a bit on > > my goals and proposal. I believe Adrian has newer thoughts than have > > been > > circulated here as well. > > > > The last message(s) have gone to freebsd-arch and freebsd-net. If > > someone > > wants to pick one, we could consolidate, but this seems relevant to > > both. > > > > I'm going to top-post to try to summarize and extend the discussion, > > but the > > preceding emails follow for reference. > > > > To recap: the existing if_media interface is running out of steam, at > > least > > in that the "Media variant" field, with 5 bits, is going to be > > insufficient > > to express existing 40 Gb/s variants. The if_media media type is a > > 32-bit > > int with a bunch of sub-fields for type (e.g. Ethernet), > > subtype/variant > > (e.g. 10baseT, 10base5, 1000baseT, etc), flags, and some MII-related > > fields. > > > > I made a proposal to extend the interface in a small way, specifically > > to > > replace the "media word" with a 64-bit int that is mostly the same, > > but > > has a new, larger variant/subtype field. The main reason for this > > proposal > > is to maintain the driver KPI (glimpse showed me 240 inclusions of > > if_media.h > > in the kernel in 8.2). That interface includes an initialization > > using a > > scalar value of fields ORed with each other. It would also be easy to > > preserve a 32-bit user-level API/ABI that can express most of the > > current > > state, with a subtype/variant field value reserved for "other" (there > > is > > already one for "unknown", but that is not quite the same). fwiw, I > > found 45 references to this user-level API in our tree, including both > > base and "ports"-type software, which includes libpcap, snmpd, > > dhclient, > > quagga, xorp, atm, devd, and rtsold, which argues for a > > backward-compatible > > API/ABI as well as a more-complete current interface for ifconfig at > > least. > > > > More generally, I see two problems with the existing if_media > > interface: > > > > 1. It doesn't have enough bits for all the fields, in particular, > > variant/ > > subtype for Ethernet. That is the immediate issue. > > > > 2. The interface is not sufficiently generic; it was designed around > > Ethernet > > including MII, token ring, FDDI, and a few other interface types. > > Some of > > the fields like "instance" are primarily for MII as far as I know, and > > are > > basically unused. It is definitely not sufficient for 802.11, which > > has > > rolled its own interfaces. > > > > To solve the second problem, I think the right approach would be to > > reduce > > this interface to a truly generic one, such as media type (e.g. > > Ethernet), > > generic flags, and perhaps generic status. Then there should be a > > separate > > media-specific interface for each type, such as Ethernet and 802.11. > > To a > > small extent, we already have that. Solving the second, more general > > problem, > > requires a whole new driver KPI that will require surgery to every > > driver, > > which is not an exercise that I would consider. > > > > Using a separate int for each existing field, as proposed, would break > > the > > driver KPI, but would not really make the interface generic. Trying > > to > > make a single interface with the union of all network interface > > requirements > > seems like a bad idea to me (we failed last time; the "we" is BSDi, > > where > > I was the architect when this interface was first designed). (No, I > > didn't > > design this interface.) > > > > Solving the first problem only, I think it is preferable to preserve a > > compatible driver KPI, which means using a scalar value encoding what > > is > > necessary. Although that interface is rather Ethernet-centric, that > > is > > really what it is used for. > > > > An additional, selfish goal is to make it easy to back-port drivers > > using > > the new interface to older versions (which I am quite likely to do). > > Preserving the KPI and general user API will be highly useful there. > > I'd be likely to do a 11-style version of ifconfig personally, but it > > might not be difficult to do in a more general way. > > > > I am willing to do a prototype for -current for evaluation. > > > > Comments, alternatives, ? > I agree with your statements above and I'd like to see the prototype. Well, I developed the prototype as I had planned, using a 64-bit media word, and found that I got about 100 files in GENERIC that didn't compile; they attempted to store "media words" in an int. My kingdom for a typedef. That didn't meet my goal of KPI compatibility, so I went to Plan B. Plan B is to steal an unused bit (RFU) to indicate an "extended" media type. I then used the variant/subtype field to store the extended type. Effectively, the previously unused bit doubles the effective size of the subtype field. Given that the previous 5-bit field lasted us 18 years, I figured that doubling it would last a while. I also changed the SIOGGIFMEDIA ioctl, splitting it for binary compatibility; extended types are all mapped to IFM_OTHER (31) using the old interface, but are visible using the new one. With these changes, I modified one driver (vtnet) to use an extended type, and the rest of GENERIC is happy. The changes to ifconfig are also fairly small. The patch is appended, where email programs will screw it up, or at ftp://ftp.karels.net/outgoing/if_media.patch. The VFAST subtype is a throw-away for testing. This seems like a reasonably pragmatic change to support the new 40 Gb/s media types until someone wants to design an improved but non-backward- compatible interface. I think it meets the goal of suitability for back-porting; it could be MFCed. Mike Index: sys/net/if_media.h =================================================================== --- sys/net/if_media.h (revision 278804) +++ sys/net/if_media.h (working copy) @@ -120,15 +120,29 @@ * 5-7 Media type * 8-15 Type specific options * 16-18 Mode (for multi-mode devices) - * 19 RFU + * 19 "extended" bit for media variant * 20-27 Shared (global) options * 28-31 Instance */ /* + * As we have used all of the original values for the media variant (subtype) + * for Ethernet, extended subtypes have been added, marked with XSUBTYPE, + * which is effectively the "high bit" of the media variant (subtype) field. + * IFM_OTHER (the highest basic type) is reserved to indicate use of an + * extended type when using an old SIOCGIFMEDIA operation. This is true + * for all media types, not just Ethernet. + */ +#define XSUBTYPE 0x80000 /* extended variant high bit */ +#define _X(var) ((var) | XSUBTYPE) /* extended variant */ +#define IFM_OTHER 31 /* Other: some extended type */ +#define OMEDIA(var) (((var) & XSUBTYPE) ? IFM_OTHER : (var)) + +/* * Ethernet */ #define IFM_ETHER 0x00000020 +/* NB: 0,1,2 are auto, manual, none defined below */ #define IFM_10_T 3 /* 10BaseT - RJ45 */ #define IFM_10_2 4 /* 10Base2 - Thinnet */ #define IFM_10_5 5 /* 10Base5 - AUI */ @@ -156,11 +170,17 @@ #define IFM_40G_CR4 27 /* 40GBase-CR4 */ #define IFM_40G_SR4 28 /* 40GBase-SR4 */ #define IFM_40G_LR4 29 /* 40GBase-LR4 */ +#define IFM_AVAIL30 30 /* available */ +/* #define IFM_OTHER 31 Other: some extended type */ +/* note 31 is the max! */ + +/* Extended variants/subtypes */ +#define IFM_VFAST _X(0) /* test "V.fast" */ +/* note _X(31) is the max! */ /* * Please update ieee8023ad_lacp.c:lacp_compose_key() * after adding new Ethernet media types. */ -/* note 31 is the max! */ #define IFM_ETH_MASTER 0x00000100 /* master mode (1000baseT) */ #define IFM_ETH_RXPAUSE 0x00000200 /* receive PAUSE frames */ @@ -170,6 +190,7 @@ * Token ring */ #define IFM_TOKEN 0x00000040 +/* NB: 0,1,2 are auto, manual, none defined below */ #define IFM_TOK_STP4 3 /* Shielded twisted pair 4m - DB9 */ #define IFM_TOK_STP16 4 /* Shielded twisted pair 16m - DB9 */ #define IFM_TOK_UTP4 5 /* Unshielded twisted pair 4m - RJ45 */ @@ -187,6 +208,7 @@ * FDDI */ #define IFM_FDDI 0x00000060 +/* NB: 0,1,2 are auto, manual, none defined below */ #define IFM_FDDI_SMF 3 /* Single-mode fiber */ #define IFM_FDDI_MMF 4 /* Multi-mode fiber */ #define IFM_FDDI_UTP 5 /* CDDI / UTP */ @@ -220,6 +242,7 @@ #define IFM_IEEE80211_OFDM27 23 /* OFDM 27Mbps */ /* NB: not enough bits to express MCS fully */ #define IFM_IEEE80211_MCS 24 /* HT MCS rate */ +/* #define IFM_OTHER 31 Other: some extended type */ #define IFM_IEEE80211_ADHOC 0x00000100 /* Operate in Adhoc mode */ #define IFM_IEEE80211_HOSTAP 0x00000200 /* Operate in Host AP mode */ @@ -241,6 +264,7 @@ * ATM */ #define IFM_ATM 0x000000a0 +/* NB: 0,1,2 are auto, manual, none defined below */ #define IFM_ATM_UNKNOWN 3 #define IFM_ATM_UTP_25 4 #define IFM_ATM_TAXI_100 5 @@ -277,7 +301,7 @@ * Masks */ #define IFM_NMASK 0x000000e0 /* Network type */ -#define IFM_TMASK 0x0000001f /* Media sub-type */ +#define IFM_TMASK 0x0008001f /* Media sub-type */ #define IFM_IMASK 0xf0000000 /* Instance */ #define IFM_ISHIFT 28 /* Instance shift */ #define IFM_OMASK 0x0000ff00 /* Type specific options */ @@ -372,6 +396,7 @@ { IFM_40G_CR4, "40Gbase-CR4" }, \ { IFM_40G_SR4, "40Gbase-SR4" }, \ { IFM_40G_LR4, "40Gbase-LR4" }, \ + { IFM_VFAST, "V.fast" }, \ { 0, NULL }, \ } @@ -603,6 +628,7 @@ { IFM_AUTO, "autoselect" }, \ { IFM_MANUAL, "manual" }, \ { IFM_NONE, "none" }, \ + { IFM_OTHER, "other" }, \ { 0, NULL }, \ } @@ -673,6 +699,7 @@ { IFM_ETHER | IFM_40G_CR4, IF_Gbps(40ULL) }, \ { IFM_ETHER | IFM_40G_SR4, IF_Gbps(40ULL) }, \ { IFM_ETHER | IFM_40G_LR4, IF_Gbps(40ULL) }, \ + { IFM_ETHER | IFM_VFAST, IF_Gbps(40ULL) }, \ \ { IFM_TOKEN | IFM_TOK_STP4, IF_Mbps(4) }, \ { IFM_TOKEN | IFM_TOK_STP16, IF_Mbps(16) }, \ Index: sys/sys/sockio.h =================================================================== --- sys/sys/sockio.h (revision 278810) +++ sys/sys/sockio.h (working copy) @@ -128,5 +128,6 @@ #define SIOCGIFGROUP _IOWR('i', 136, struct ifgroupreq) /* get ifgroups */ #define SIOCDIFGROUP _IOW('i', 137, struct ifgroupreq) /* delete ifgroup */ #define SIOCGIFGMEMB _IOWR('i', 138, struct ifgroupreq) /* get members */ +#define SIOCGIFXMEDIA _IOWR('i', 139, struct ifmediareq) /* get net xmedia */ #endif /* !_SYS_SOCKIO_H_ */ Index: sys/net/if.c =================================================================== --- sys/net/if.c (revision 278749) +++ sys/net/if.c (working copy) @@ -2561,6 +2561,7 @@ case SIOCGIFPSRCADDR: case SIOCGIFPDSTADDR: case SIOCGIFMEDIA: + case SIOCGIFXMEDIA: case SIOCGIFGENERIC: if (ifp->if_ioctl == NULL) return (EOPNOTSUPP); Index: sys/net/if_media.c =================================================================== --- sys/net/if_media.c (revision 278804) +++ sys/net/if_media.c (working copy) @@ -67,7 +67,9 @@ static struct ifmedia_entry *ifmedia_match(struct ifmedia *ifm, int flags, int mask); +#define IFMEDIA_DEBUG #ifdef IFMEDIA_DEBUG +#include <net/if_var.h> int ifmedia_debug = 0; SYSCTL_INT(_debug, OID_AUTO, ifmedia, CTLFLAG_RW, &ifmedia_debug, 0, "if_media debugging msgs"); @@ -271,6 +273,7 @@ * Get list of available media and current media on interface. */ case SIOCGIFMEDIA: + case SIOCGIFXMEDIA: { struct ifmedia_entry *ep; int *kptr, count; @@ -278,8 +281,13 @@ kptr = NULL; /* XXX gcc */ - ifmr->ifm_active = ifmr->ifm_current = ifm->ifm_cur ? - ifm->ifm_cur->ifm_media : IFM_NONE; + if (cmd == SIOCGIFMEDIA) { + ifmr->ifm_active = ifmr->ifm_current = ifm->ifm_cur ? + OMEDIA(ifm->ifm_cur->ifm_media) : IFM_NONE; + } else { + ifmr->ifm_active = ifmr->ifm_current = ifm->ifm_cur ? + ifm->ifm_cur->ifm_media : IFM_NONE; + } ifmr->ifm_mask = ifm->ifm_mask; ifmr->ifm_status = 0; (*ifm->ifm_status)(ifp, ifmr); @@ -317,7 +325,10 @@ ep = LIST_FIRST(&ifm->ifm_list); for (; ep != NULL && count < ifmr->ifm_count; ep = LIST_NEXT(ep, ifm_list), count++) - kptr[count] = ep->ifm_media; + if (cmd == SIOCGIFMEDIA) + kptr[count] = OMEDIA(ep->ifm_media); + else + kptr[count] = ep->ifm_media; if (ep != NULL) error = E2BIG; /* oops! */ @@ -505,7 +516,7 @@ printf("<unknown type>\n"); return; } - printf(desc->ifmt_string); + printf("%s", desc->ifmt_string); /* Any mode. */ for (desc = ttos->modes; desc && desc->ifmt_string != NULL; desc++) Index: sys/dev/virtio/network/if_vtnet.c =================================================================== --- sys/dev/virtio/network/if_vtnet.c (revision 278749) +++ sys/dev/virtio/network/if_vtnet.c (working copy) @@ -938,6 +938,7 @@ ifmedia_init(&sc->vtnet_media, IFM_IMASK, vtnet_ifmedia_upd, vtnet_ifmedia_sts); ifmedia_add(&sc->vtnet_media, VTNET_MEDIATYPE, 0, NULL); + ifmedia_add(&sc->vtnet_media, IFM_ETHER | IFM_VFAST, 0, NULL); ifmedia_set(&sc->vtnet_media, VTNET_MEDIATYPE); /* Read (or generate) the MAC address for the adapter. */ @@ -1103,6 +1104,7 @@ case SIOCSIFMEDIA: case SIOCGIFMEDIA: + case SIOCGIFXMEDIA: error = ifmedia_ioctl(ifp, ifr, &sc->vtnet_media, cmd); break; Index: sbin/ifconfig/ifmedia.c =================================================================== --- sbin/ifconfig/ifmedia.c (revision 278749) +++ sbin/ifconfig/ifmedia.c (working copy) @@ -109,11 +109,17 @@ { struct ifmediareq ifmr; int *media_list, i; + int xmedia = 1; (void) memset(&ifmr, 0, sizeof(ifmr)); (void) strncpy(ifmr.ifm_name, name, sizeof(ifmr.ifm_name)); - if (ioctl(s, SIOCGIFMEDIA, (caddr_t)&ifmr) < 0) { + /* + * Check if interface supports extended media types. + */ + if (ioctl(s, SIOCGIFXMEDIA, (caddr_t)&ifmr) < 0) + xmedia = 0; + if (xmedia == 0 && ioctl(s, SIOCGIFMEDIA, (caddr_t)&ifmr) < 0) { /* * Interface doesn't support SIOC{G,S}IFMEDIA. */ @@ -130,8 +136,13 @@ err(1, "malloc"); ifmr.ifm_ulist = media_list; - if (ioctl(s, SIOCGIFMEDIA, (caddr_t)&ifmr) < 0) - err(1, "SIOCGIFMEDIA"); + if (xmedia) { + if (ioctl(s, SIOCGIFXMEDIA, (caddr_t)&ifmr) < 0) + err(1, "SIOCGIFXMEDIA"); + } else { + if (ioctl(s, SIOCGIFMEDIA, (caddr_t)&ifmr) < 0) + err(1, "SIOCGIFMEDIA"); + } printf("\tmedia: "); print_media_word(ifmr.ifm_current, 1); @@ -194,6 +205,7 @@ { static struct ifmediareq *ifmr = NULL; int *mwords; + int xmedia = 1; if (ifmr == NULL) { ifmr = (struct ifmediareq *)malloc(sizeof(struct ifmediareq)); @@ -213,7 +225,10 @@ * the current media type and the top-level type. */ - if (ioctl(s, SIOCGIFMEDIA, (caddr_t)ifmr) < 0) { + if (ioctl(s, SIOCGIFXMEDIA, (caddr_t)ifmr) < 0) { + xmedia = 0; + } + if (xmedia == 0 && ioctl(s, SIOCGIFMEDIA, (caddr_t)ifmr) < 0) { err(1, "SIOCGIFMEDIA"); } @@ -225,8 +240,13 @@ err(1, "malloc"); ifmr->ifm_ulist = mwords; - if (ioctl(s, SIOCGIFMEDIA, (caddr_t)ifmr) < 0) - err(1, "SIOCGIFMEDIA"); + if (xmedia) { + if (ioctl(s, SIOCGIFXMEDIA, (caddr_t)ifmr) < 0) + err(1, "SIOCGIFXMEDIA"); + } else { + if (ioctl(s, SIOCGIFMEDIA, (caddr_t)ifmr) < 0) + err(1, "SIOCGIFMEDIA"); + } } return ifmr;
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201502170150.t1H1ouxM020621>