Skip site navigation (1)Skip section navigation (2)
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>