Re: [UNIKRAFT PATCH v4 11/12] plat/xen/drivers/net: Add receive operation

[Sharan Santhanam <sharan.santhanam@xxxxxxxxx>]

Hello Costin,

Please find the review comment inline:

Thanks & Regards
Sharan

On 8/13/20 10:53 AM, Costin Lupu wrote:
> Incoming packets are written in pages allocated by the netfront. Such pages are
> allocated and registered to the shared ring after completion of receiving
> operations.
>
> Signed-off-by: Costin Lupu <costin.lupu@xxxxxxxxx>
> Signed-off-by: Razvan Cojocaru <razvan.cojocaru93@xxxxxxxxx>
> ---
>   plat/xen/drivers/net/netfront.c | 100 ++++++++++++++++++++++++++++++++
>   1 file changed, 100 insertions(+)
>
> diff --git a/plat/xen/drivers/net/netfront.c b/plat/xen/drivers/net/netfront.c
> index f2b81329..101e38c8 100644
> --- a/plat/xen/drivers/net/netfront.c
> +++ b/plat/xen/drivers/net/netfront.c
> @@ -221,6 +221,49 @@ static int netfront_rxq_enqueue(struct uk_netdev_rx_queue 
> *rxq,
>         return 0;
>   }
>   
> +static int netfront_rxq_dequeue(struct uk_netdev_rx_queue *rxq,
> +               struct uk_netbuf **netbuf)
> +{
> +       RING_IDX prod, cons;
> +       netif_rx_response_t *rx_rsp;
> +       uint16_t len, id;
> +       struct uk_netbuf *buf = NULL;
> +       int count = 0;
> +
> +       UK_ASSERT(rxq != NULL);
> +       UK_ASSERT(netbuf != NULL);
> +
> +       prod = rxq->ring.sring->rsp_prod;
> +       rmb(); /* Ensure we see queued responses up to 'rp'. */
> +       cons = rxq->ring.rsp_cons;
> +       /* No new descriptor since last dequeue operation */
> +       if (cons == prod)
We should set it here:

*netbuf = NULL;

> +               goto out;
> +
> +       /* get response */
> +       rx_rsp = RING_GET_RESPONSE(&rxq->ring, cons);
> +       UK_ASSERT(rx_rsp->status > NETIF_RSP_NULL);
> +       id = rx_rsp->id;
> +       UK_ASSERT(id < NET_RX_RING_SIZE);
> +
> +       /* remove grant for buffer data */
> +       gnttab_end_access(rxq->gref[id]);
> +
> +       buf = rxq->netbuf[id];
> +       len = (uint16_t) rx_rsp->status;
> +       if (len > ETH_PKT_LEN)
> +               len = ETH_PKT_LEN;
> +       buf->len = len;
> +
> +       *netbuf = buf;
> +
> +       rxq->ring.rsp_cons++;
> +       count = 1;
> +
> +out:
> +       return count;
> +}
> +
>   static int netfront_rx_fillup(struct uk_netdev_rx_queue *rxq, uint16_t 
> nb_desc)
>   {
>         struct uk_netbuf *netbuf[nb_desc];
> @@ -271,6 +314,62 @@ static int netfront_rxq_intr_enable(struct 
> uk_netdev_rx_queue *rxq)
>         return (more > 0);
>   }
>   
> +static int netfront_recv(struct uk_netdev *n,
> +               struct uk_netdev_rx_queue *rxq,
> +               struct uk_netbuf **pkt)
> +{
> +       int rc, status = 0;
> +
> +       UK_ASSERT(n != NULL);
> +       UK_ASSERT(rxq != NULL);
> +       UK_ASSERT(pkt != NULL);
> +
> +       /* Queue interrupts have to be off when calling receive */
> +       UK_ASSERT(!(rxq->intr_enabled & NETFRONT_INTR_EN));
> +
> +       rc = netfront_rxq_dequeue(rxq, pkt);
> +       UK_ASSERT(rc >= 0);
> +
> +       status |= (*pkt) ? UK_NETDEV_STATUS_SUCCESS : 0x0;
> +       status |= netfront_rx_fillup(rxq, rc);
> +
> +       /* Enable interrupt only when user had previously enabled it */
> +       if (rxq->intr_enabled & NETFRONT_INTR_USR_EN_MASK) {
> +               /* Need to enable the interrupt on the last packet */
> +               rc = netfront_rxq_intr_enable(rxq);
> +               if (rc == 1 && !(*pkt)) {
> +                       /**
> +                        * Packet arrive after reading the queue and before
> +                        * enabling the interrupt
> +                        */
> +                       rc = netfront_rxq_dequeue(rxq, pkt);
> +                       UK_ASSERT(rc >= 0);
> +                       status |= UK_NETDEV_STATUS_SUCCESS;
> +
> +                       /*
> +                        * Since we received something, we need to fillup
> +                        * and notify
> +                        */
> +                       status |= netfront_rx_fillup(rxq, rc);
> +
> +                       /* Need to enable the interrupt on the last packet */
> +                       rc = netfront_rxq_intr_enable(rxq);
> +                       status |= (rc == 1) ? UK_NETDEV_STATUS_MORE : 0x0;
> +               } else if (*pkt) {
> +                       /* When we originally got a packet and there is more */
> +                       status |= (rc == 1) ? UK_NETDEV_STATUS_MORE : 0x0;
> +               }
> +       } else if (*pkt) {
> +               /**
> +                * For polling case, we report always there are further
> +                * packets unless the queue is empty.
> +                */
> +               status |= UK_NETDEV_STATUS_MORE;
> +       }
> +
> +       return status;
> +}
> +
>   static struct uk_netdev_tx_queue *netfront_txq_setup(struct uk_netdev *n,
>                 uint16_t queue_id,
>                 uint16_t nb_desc __unused,
> @@ -700,6 +799,7 @@ static int netfront_add_dev(struct xenbus_device *xendev)
>   
>   	/* register netdev */
>         nfdev->netdev.tx_one = netfront_xmit;
> +       nfdev->netdev.rx_one = netfront_recv;
>         nfdev->netdev.ops = &netfront_ops;
>         rc = uk_netdev_drv_register(&nfdev->netdev, drv_allocator, DRIVER_NAME);
>         if (rc < 0) {