Re: [UNIKRAFT PATCH v2 2/2] lib/uknetdev: Get feature supported on uk_netdev

[Hugo Lefeuvre <hugo.lefeuvre@xxxxxxxxx>]

Hi Sharan,

thanks a lot for this patch. It looks good to me. I only a few minor
comments (inline).

Also, I really think that we should rebase this on top of staging and
merge it right away. It is not fundamentally related to the tap driver,
and having it merged now together with the static IP series would be a
good thing.

regards,
Hugo

On Tue, 2020-06-30 at 12:47 +0200, Sharan Santhanam wrote:
> The uk_netdev_info provides a way to forward information on netdevice
> to the network stack. We extend this with a bitmap of the features
> supported on device. The bit 0,1 of the bitmap indicates if the
> device supports interrupt.
> 
> Signed-off-by: Sharan Santhanam <sharan.santhanam@xxxxxxxxx>
> ---
>  lib/uknetdev/include/uk/netdev_core.h | 9 +++++++++
>  plat/drivers/tap/tap.c                | 1 +
>  plat/drivers/virtio/virtio_net.c      | 1 +
>  3 files changed, 11 insertions(+)
> 
> diff --git a/lib/uknetdev/include/uk/netdev_core.h
> b/lib/uknetdev/include/uk/netdev_core.h
> index 827f1872..954aa8b0 100644
> --- a/lib/uknetdev/include/uk/netdev_core.h
> +++ b/lib/uknetdev/include/uk/netdev_core.h
> @@ -77,6 +77,14 @@ UK_TAILQ_HEAD(uk_netdev_list, struct uk_netdev);
>   */
>  #define UK_NETDEV_HWADDR_LEN 6 /**< Length of Ethernet address. */
>  
> +#define UK_FEATURE_RXQ_INTR_BIT                  0
> +#define UK_FEATURE_RXQ_INTR_AVAILABLE  (1UL <<
> UK_FEATURE_RXQ_INTR_BIT)

I'm missing a comment here. Something like:

/* device supports interrupts for the receive queue */

> +#define UK_FEATURE_TXQ_INTR_BIT                  1
> +#define UK_FEATURE_TXQ_INTR_AVAILABLE  (1UL <<
> UK_FEATURE_TXQ_INTR_BIT)
> 

same here.

/* device supports interrupts for the transmit queue */

> +#define uk_netdev_rxintr_supported(feature)  \
> +     (feature & (UK_FEATURE_RXQ_INTR_AVAILABLE))
> +
>  struct uk_hwaddr {
>       uint8_t addr_bytes[UK_NETDEV_HWADDR_LEN];
>  } __packed;
> @@ -91,6 +99,7 @@ struct uk_netdev_info {
>       uint16_t max_mtu;   /**< Maximum supported MTU size. */
>       uint16_t nb_encap_tx;  /**< Number of bytes required as
> headroom for tx. */
>       uint16_t nb_encap_rx;  /**< Number of bytes required as
> headroom for rx. */
> +     uint32_t features;

same here.

/* bitmap of the features supported on the device */

>  };
>  
>  /**
> diff --git a/plat/drivers/tap/tap.c b/plat/drivers/tap/tap.c
> index 505f0ccb..fb96785c 100644
> --- a/plat/drivers/tap/tap.c
> +++ b/plat/drivers/tap/tap.c
> @@ -519,6 +519,7 @@ static void tap_netdev_info_get(struct uk_netdev
> *dev __unused,
>       dev_info->max_tx_queues = 1;
>       dev_info->nb_encap_tx = 0;
>       dev_info->nb_encap_rx = 0;
> +     dev_info->features = 0;
>  }
>  

Moving this to a separate patch would allow us to merge this patch
right away without waiting for the tap driver (which will take longer
to review).

>  static unsigned int tap_netdev_promisc_get(struct uk_netdev *n)
> diff --git a/plat/drivers/virtio/virtio_net.c
> b/plat/drivers/virtio/virtio_net.c
> index 3025ed3f..0a8312d6 100644
> --- a/plat/drivers/virtio/virtio_net.c
> +++ b/plat/drivers/virtio/virtio_net.c
> @@ -1052,6 +1052,7 @@ static void virtio_net_info_get(struct
> uk_netdev *dev,
>       dev_info->max_tx_queues = vndev->max_vqueue_pairs;
>       dev_info->nb_encap_tx = sizeof(struct
> virtio_net_hdr_padded);
>       dev_info->nb_encap_rx = sizeof(struct
> virtio_net_hdr_padded);
> +     dev_info->features = UK_FEATURE_RXQ_INTR_AVAILABLE;
>  }
>  
>  static int virtio_net_start(struct uk_netdev *n)