Hi
> -----Original Message-----
> From: Guo, Junfeng <junfeng....@intel.com>
> Sent: Sunday, October 9, 2022 10:15
> To: Ferruh Yigit <ferruh.yi...@amd.com>; Zhang, Qi Z
> <qi.z.zh...@intel.com>; Wu, Jingjing <jingjing...@intel.com>
> Cc: ferruh.yi...@xilinx.com; dev@dpdk.org; Li, Xiaoyun
> <xiaoyun...@intel.com>; awogbem...@google.com; Richardson, Bruce
> <bruce.richard...@intel.com>; Lin, Xueqin <xueqin....@intel.com>
> Subject: RE: [PATCH v4 7/9] net/gve: add support for Rx/Tx
>
>
>
> > -----Original Message-----
> > From: Ferruh Yigit <ferruh.yi...@amd.com>
> > Sent: Thursday, October 6, 2022 22:25
> > To: Guo, Junfeng <junfeng....@intel.com>; Zhang, Qi Z
> > <qi.z.zh...@intel.com>; Wu, Jingjing <jingjing...@intel.com>
> > Cc: ferruh.yi...@xilinx.com; dev@dpdk.org; Li, Xiaoyun
> > <xiaoyun...@intel.com>; awogbem...@google.com; Richardson, Bruce
> > <bruce.richard...@intel.com>; Lin, Xueqin <xueqin....@intel.com>
> > Subject: Re: [PATCH v4 7/9] net/gve: add support for Rx/Tx
> >
> > On 9/27/2022 8:32 AM, Junfeng Guo wrote:
> >
> > >
> > > Add Rx/Tx of GQI_QPL queue format and GQI_RDA queue format.
> > >
> > > Signed-off-by: Xiaoyun Li <xiaoyun...@intel.com>
> > > Signed-off-by: Junfeng Guo <junfeng....@intel.com>
> >
> > <...>
> >
> > > --- a/drivers/net/gve/gve_ethdev.c
> > > +++ b/drivers/net/gve/gve_ethdev.c
> > > @@ -583,6 +583,11 @@ gve_dev_init(struct rte_eth_dev *eth_dev)
> > > if (err)
> > > return err;
> > >
> > > + if (gve_is_gqi(priv)) {
> > > + eth_dev->rx_pkt_burst = gve_rx_burst;
> > > + eth_dev->tx_pkt_burst = gve_tx_burst;
> > > + }
> > > +
> >
> > What do you think to add a log here for 'else' case, to inform user
> > why datapath is not working?
>
> Agreed, make sense!
> Currently only one queue mode (i.e., qpl mode) is supported on the GCP env.
> Will add a log to inform this in the else case. Thanks!
This explanation is not correct. Only QPL mode is supported in GCP now. This is
env limitation but not related to the else code here.
gve_is_gqi() includes two modes GQI_QPL and GQI_RDA. And both of these datapath
is supported in rxtx.
GQI means its queue model is single queue model (txq for tx and rxq for rx).
And there're 2 ways for this queue model QPL and RDA.
QPL needs to copy packets from/to several reserved pages negotiated with
backend. RDA is just like normal device and uses PA in descs.
The datapath not supported is DQO_RDA which uses different hardware so
different queue model (split/double queue model). Tx will use txq and
tx_completion_q and Rx will use rxq and rx_completion_q.
This is not implemented in the datapath for now and will be implemented in the
future.
So if you want to add comment here. Please say "DQO_RDA is not implemented and
will be added in the future". Don't say it's not available in GCP env which is
not the reason.
>
> >
> > <...>
> >
> > > +uint16_t
> > > +gve_rx_burst(void *rx_queue, struct rte_mbuf **rx_pkts, uint16_t
> > nb_pkts)
> > > +{
> > > + volatile struct gve_rx_desc *rxr, *rxd;
> > > + struct gve_rx_queue *rxq = rx_queue;
> > > + uint16_t rx_id = rxq->rx_tail;
> > > + struct rte_mbuf *rxe;
> > > + uint16_t nb_rx, len;
> > > + uint64_t addr;
> > > +
> > > + rxr = rxq->rx_desc_ring;
> > > +
> > > + for (nb_rx = 0; nb_rx < nb_pkts; nb_rx++) {
> > > + rxd = &rxr[rx_id];
> > > + if (GVE_SEQNO(rxd->flags_seq) != rxq->expected_seqno)
> > > + break;
> > > +
> > > + if (rxd->flags_seq & GVE_RXF_ERR)
> > > + continue;
> > > +
> > > + len = rte_be_to_cpu_16(rxd->len) - GVE_RX_PAD;
> > > + rxe = rxq->sw_ring[rx_id];
> > > + rxe->data_off = RTE_PKTMBUF_HEADROOM;
> > > + if (rxq->is_gqi_qpl) {
> > > + addr = (uint64_t)(rxq->qpl->mz->addr) +
> > > + rx_id * PAGE_SIZE
> > + GVE_RX_PAD;
> > > + rte_memcpy((void *)((size_t)rxe->buf_addr +
> > > + rxe-
> > >data_off),
> > > + (void *)(size_t)addr, len);
> >
> > Why a 'memcpy' is needed? Can't it DMA to mbuf data buffer?
When queue model is gpi_qpl (this is negotiated and gotten using adminq with
backend), the device needs to register a block of memory (called page list).
And tx needs to copy the packets to this memory and rx will get packets from
this area.
Backend will be responsible for getting(tx)/giving(rx) packets from this memory
to the device/line (We don't really know how backend does this).
Please refer to
https://www.kernel.org/doc/html/v5.4/networking/device_drivers/google/gve.html.
There's a bit more explanation about this queue format.
>
> Well, only qpl (queue page list) mode supported on the GCP env now.
> So the DMA may not be used in current case.
And yes, it's because GCP doesn't support GQI_RDA for now so GQI_QPL has to be
implemented. But even if GCP env supports RDA in the future, unless they
completely remove QPL support, QPL is still needed.
Because queue format/model is getting from backend through
gve_adminq_describe_device(). You may just get the QPI version. The device
can't really control which queue format to get.
>
> >
> > > + }
> > > + rxe->nb_segs = 1;
> > > + rxe->next = NULL;
> > > + rxe->pkt_len = len;
> > > + rxe->data_len = len;
> > > + rxe->port = rxq->port_id;
> > > + rxe->packet_type = 0;
> > > + rxe->ol_flags = 0;
> > > +
> >
> > As far as I can see 'sw_ring[]' filled using 'rte_pktmbuf_alloc_bulk()'
> > API, which should reset mbuf fields to default values, so some of the
> > assignment above can be redundant.
>
> Yes, some fields are already assigned at 'rte_pktmbuf_reset()'.
> Will remove the redundant ones in the coming version. Thanks!
>
> >
> > > + if (rxd->flags_seq & GVE_RXF_TCP)
> > > + rxe->packet_type |= RTE_PTYPE_L4_TCP;
> > > + if (rxd->flags_seq & GVE_RXF_UDP)
> > > + rxe->packet_type |= RTE_PTYPE_L4_UDP;
> > > + if (rxd->flags_seq & GVE_RXF_IPV4)
> > > + rxe->packet_type |= RTE_PTYPE_L3_IPV4;
> > > + if (rxd->flags_seq & GVE_RXF_IPV6)
> > > + rxe->packet_type |= RTE_PTYPE_L3_IPV6;
> > > +
> >
> > If you are setting packet_type, it is better to implement
> > 'dev_supported_ptypes_get()' dev_ops too, to announce host which
> > packet type parsin supporting. (+ dev_ptypes_set() dev_ops) And later
> > driver can announce "Packet type parsing" feature in .ini file.
>
> Well, on current GCP env, the APIs for supported ptypes get/set have not
> been exposed even in the base code. The only one in the base code is for
> the dqo mode (gve_adminq_get_ptype_map_dqo). But this also cannot be
> used on current GCP env. We can only implement this once they are
> supported and exposed at GCP. Thanks!
You're mixing the concept again. GCP env only supports QPL is not an excuse.
The packet type is supported even in QPL. It's just very limited to L4_TCP/UDP
and L3_IPV4/6. Ptypes_get is possible and it'll be RTE_PTYPE_L3_IPV4/6 and
RTE_PTYPE_L4_UDP/TCP.
For DQO mode you mentioned, it'll be more flexible and have more support. I'm
not sure what's your plan but it can be implemented whenever based on the plan
not GCP env availability. The base code is there. It's just you may not be able
to timely verify and debug it.
Ptype_set is not supported since the hardware doesn't support it (There's no
such adminq).
