Date: Tue, 17 Feb 2015 09:26:33 -0800 From: Adrian Chadd <adrian@freebsd.org> To: mike@karels.net 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: <CAJ-VmomXdvW4YsneqiRAqO0EV19uOp=OyOLH7rAqMh3EPp=uGg@mail.gmail.com> In-Reply-To: <201502170150.t1H1ouxM020621@mail.karels.net> References: <BC6AB805-B699-438E-9EC6-015C005D6B1D@neville-neil.com> <201502170150.t1H1ouxM020621@mail.karels.net>
next in thread | previous in thread | raw e-mail | index | archive | help
Looks good to me. Thanks for doing this! -a On 16 February 2015 at 17:50, Mike Karels <mike@karels.net> wrote: > 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; > _______________________________________________ > freebsd-net@freebsd.org mailing list > http://lists.freebsd.org/mailman/listinfo/freebsd-net > To unsubscribe, send any mail to "freebsd-net-unsubscribe@freebsd.org"
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAJ-VmomXdvW4YsneqiRAqO0EV19uOp=OyOLH7rAqMh3EPp=uGg>