Re: [UNIKRAFT PATCH] lib/uknetdev: Provide IEEE 802.1q, 802.1ad, Jumbo frame definitions

[Sharan Santhanam <sharan.santhanam@xxxxxxxxx>]

Hello Simon,

This patch seems fine to me.

Reviewed-by: Sharan Santhanam <sharan.santhanam@xxxxxxxxx>

Thanks & Regards

Sharan

On 8/20/20 4:45 PM, Simon Kuenzer wrote:
> This commit includes support for IEEE 802.1q and IEEE 802.1ad tagged
> frames to the recently introduced Ethernet header and frame definitions.
> Due to these standards, an Ethernet frame (without FCS/CRC) can be 1522 B
> (non-jumbo) and 9022 B (jumbo).
> Together with this commit, the definitions are name spaced with `UK_`
> prefix. This is done to avoid possible macro name clashes when porting
> network stacks or packet processing frameworks to libuknetdev.
>
> Signed-off-by: Simon Kuenzer <simon.kuenzer@xxxxxxxxx>
> ---
>   lib/uknetdev/include/uk/netdev_core.h | 56 ++++++++++++++++++++++-----
>   plat/drivers/virtio/virtio_net.c      |  8 ++--
>   2 files changed, 51 insertions(+), 13 deletions(-)
>
> diff --git a/lib/uknetdev/include/uk/netdev_core.h 
> b/lib/uknetdev/include/uk/netdev_core.h
> index 1e8b4e72..b9a994e3 100644
> --- a/lib/uknetdev/include/uk/netdev_core.h
> +++ b/lib/uknetdev/include/uk/netdev_core.h
> @@ -73,16 +73,58 @@ struct uk_netdev;
>   UK_TAILQ_HEAD(uk_netdev_list, struct uk_netdev);
>   
>   /**
> - * Ethernet macros
> + * Ethernet size macros
>    */
> -#define ETH_HDR_LEN           14
> -#define ETH_PKT_PAYLOAD_LEN   1500
> -#define ETH_PKT_LEN           (ETH_HDR_LEN + ETH_PKT_PAYLOAD_LEN)
> +/* Header fields */
> +#define UK_ETH_ADDR_LEN                           6
> +#define UK_ETH_TYPE_LEN                           2
> +#define UK_ETH_8021Q_LEN               (UK_ETH_TYPE_LEN + 2)
> +
> +/* Ethernet header */
> +/* Untagged */
> +#define UK_ETH_HDR_UNTAGGED_LEN                ((2 * UK_ETH_ADDR_LEN) + \
> +                                        UK_ETH_TYPE_LEN)
> +/* Single VLAN tag (IEEE 802.1q) */
> +#define UK_ETH_HDR_8021Q_LEN           (UK_ETH_HDR_UNTAGGED_LEN + \
> +                                        UK_ETH_8021Q_LEN)
> +/* Double VLAN tag (IEEE 802.1q) */
> +#define UK_ETH_HDR_8021AD_LEN          (UK_ETH_HDR_UNTAGGED_LEN + \
> +                                        (2 * UK_ETH_8021Q_LEN))
> +
> +/* Payload */
> +#define UK_ETH_PAYLOAD_MAXLEN          1500
> +#define UK_ETH_JPAYLOAD_MAXLEN         9000 /**< Jumbo frame. */
> +
> +/* Frame sizes */
> +#define UK_ETH_FRAME_MINLEN              60
> +
> +
> +#define UK_ETH_FRAME_UNTAGGED_MAXLEN   (UK_ETH_HDR_UNTAGGED_LEN +      \
> +                                        UK_ETH_PAYLOAD_MAXLEN)
> +#define UK_ETH_FRAME_8021Q_MAXLEN      (UK_ETH_HDR_8021_LEN + \
> +                                        UK_ETH_PAYLOAD_MAXLEN)
> +#define UK_ETH_FRAME_8021AD_MAXLEN     (UK_ETH_HDR_8021ADLEN + \
> +                                        UK_ETH_PAYLOAD_MAXLEN)
> +#define UK_ETH_FRAME_MAXLEN            (UK_ETH_FRAME_8021AD_MAXLEN)
> +
> +
> +#define UK_ETH_JFRAME_UNTAGGED_MAXLEN  (UK_ETH_HDR_UNTAGGED_LEN +      \
> +                                        UK_ETH_JPAYLOAD_MAXLEN)
> +#define UK_ETH_JFRAME_8021Q_MAXLEN     (UK_ETH_HDR_8021_LEN + \
> +                                        UK_ETH_JPAYLOAD_MAXLEN)
> +#define UK_ETH_JFRAME_8021AD_MAXLEN    (UK_ETH_HDR_8021ADLEN + \
> +                                        UK_ETH_JPAYLOAD_MAXLEN)
> +#define UK_ETH_JFRAME_MAXLEN           (UK_ETH_JFRAME_8021AD_MAXLEN)
>   
>   /**
>    * A structure used for Ethernet hardware addresses
>    */
> -#define UK_NETDEV_HWADDR_LEN 6 /**< Length of Ethernet address. */
> +#define UK_NETDEV_HWADDR_LEN           (UK_ETH_ADDR_LEN)
> +
> +struct uk_hwaddr {
> +       uint8_t addr_bytes[UK_NETDEV_HWADDR_LEN];
> +} __packed;
> +
>   
>   /**
>    * The netdevice support rx/tx interrupt.
> @@ -95,10 +137,6 @@ UK_TAILQ_HEAD(uk_netdev_list, struct uk_netdev);
>   #define uk_netdev_rxintr_supported(feature)   \
>         (feature & (UK_FEATURE_RXQ_INTR_AVAILABLE))
>   
> -struct uk_hwaddr {
> -       uint8_t addr_bytes[UK_NETDEV_HWADDR_LEN];
> -} __packed;
> -
>   /**
>    * A structure used to describe network device capabilities.
>    */
> diff --git a/plat/drivers/virtio/virtio_net.c b/plat/drivers/virtio/virtio_net.c
> index b0663e60..6f5d2422 100644
> --- a/plat/drivers/virtio/virtio_net.c
> +++ b/plat/drivers/virtio/virtio_net.c
> @@ -47,8 +47,8 @@
>    *               12 bytes in length in modern mode.
>    */
>   #define VIRTIO_HDR_LEN          12
> -#define VIRTIO_PKT_BUFFER_LEN ((ETH_PKT_PAYLOAD_LEN)   \
> -                              + (ETH_HDR_LEN)          \
> +#define VIRTIO_PKT_BUFFER_LEN ((UK_ETH_PAYLOAD_MAXLEN) \
> +                              + (UK_ETH_HDR_UNTAGGED_LEN) \
>                                + (VIRTIO_HDR_LEN))
>   
>   #define DRIVER_NAME           "virtio-net"
> @@ -511,7 +511,7 @@ static int virtio_netdev_rxq_dequeue(struct 
> uk_netdev_rx_queue *rxq,
>                 *netbuf = NULL;
>                 return rxq->nb_desc;
>         }
> -       if (unlikely((len < VIRTIO_HDR_LEN + ETH_HDR_LEN)
> +       if (unlikely((len < VIRTIO_HDR_LEN + UK_ETH_HDR_UNTAGGED_LEN)
>                      || (len > VIRTIO_PKT_BUFFER_LEN))) {
>                 uk_pr_err("Received invalid packet size: %"__PRIu32"\n", len);
>                 return -EINVAL;
> @@ -1137,7 +1137,7 @@ static int virtio_net_add_dev(struct virtio_dev *vdev)
>         }
>         vndev->uid = rc;
>         rc = 0;
> -       vndev->max_mtu = ETH_PKT_PAYLOAD_LEN;
> +       vndev->max_mtu = UK_ETH_PAYLOAD_MAXLEN;
>         vndev->mtu = vndev->max_mtu;
>         vndev->promisc = 0;
>         virtio_netdev_feature_set(vndev);