> -----Original Message-----
> From: Long Li <lon...@microsoft.com>
> Sent: Tuesday, September 19, 2023 4:02 AM
> To: Wei Hu <w...@microsoft.com>; dev@dpdk.org
> Cc: sta...@dpdk.org; Ferruh Yigit <ferruh.yi...@amd.com>; Luca Boccassi
> <bl...@debian.org>; Kevin Traynor <ktray...@redhat.com>
> Subject: RE: [PATCH 1/1] net/mana: add 32 bit short doorbell
>
> > Subject: [PATCH 1/1] net/mana: add 32 bit short doorbell
> >
> > Add 32 bit short doorbell support. Ring short doorbell when running in
> > 32 bit applicactions.
> >
> > Cc: sta...@dpdk.org
> >
> > Signed-off-by: Wei Hu <w...@microsoft.com>
> > ---
> > drivers/net/mana/gdma.c | 95
> > +++++++++++++++++++++++++++++++++++++++++
> > drivers/net/mana/mana.h | 25 +++++++++++
> > drivers/net/mana/rx.c | 52 ++++++++++++++++++++++
> > drivers/net/mana/tx.c | 28 ++++++++++++
> > 4 files changed, 200 insertions(+)
> >
> > diff --git a/drivers/net/mana/gdma.c b/drivers/net/mana/gdma.c index
> > 65685fe236..d1da025d1b 100644
> > --- a/drivers/net/mana/gdma.c
> > +++ b/drivers/net/mana/gdma.c
> > @@ -166,6 +166,97 @@ gdma_post_work_request(struct
> mana_gdma_queue
> > *queue,
> > return 0;
> > }
> >
> > +#ifdef RTE_ARCH_32
> > +union gdma_short_doorbell_entry {
> > + uint32_t as_uint32;
> > +
> > + struct {
> > + uint32_t tail_ptr_incr : 16; /* Number of CQEs */
> > + uint32_t id : 12;
> > + uint32_t reserved : 3;
> > + uint32_t arm : 1;
> > + } cq;
> > +
> > + struct {
> > + uint32_t tail_ptr_incr : 16; /* In number of bytes */
> > + uint32_t id : 12;
> > + uint32_t reserved : 4;
> > + } rq;
> > +
> > + struct {
> > + uint32_t tail_ptr_incr : 16; /* In number of bytes */
> > + uint32_t id : 12;
> > + uint32_t reserved : 4;
> > + } sq;
> > +
> > + struct {
> > + uint32_t tail_ptr_incr : 16; /* Number of EQEs */
> > + uint32_t id : 12;
> > + uint32_t reserved : 3;
> > + uint32_t arm : 1;
> > + } eq;
> > +}; /* HW DATA */
> > +
> > +enum {
> > + DOORBELL_SHORT_OFFSET_SQ = 0x10,
> > + DOORBELL_SHORT_OFFSET_RQ = 0x410,
> > + DOORBELL_SHORT_OFFSET_CQ = 0x810,
> > + DOORBELL_SHORT_OFFSET_EQ = 0xFF0,
> > +};
> > +
> > +/*
> > + * Write to hardware doorbell to notify new activity.
> > + */
> > +int
> > +mana_ring_short_doorbell(void *db_page, enum gdma_queue_types
> > queue_type,
> > + uint32_t queue_id, uint32_t tail_incr, uint8_t arm) {
> > + uint8_t *addr = db_page;
> > + union gdma_short_doorbell_entry e = {};
> > +
> > + if ((queue_id & ~GDMA_SHORT_DB_QID_MASK) ||
> > + (tail_incr & ~GDMA_SHORT_DB_INC_MASK)) {
> > + DP_LOG(ERR, "%s: queue_id %u or "
> > + "tail_incr %u overflowed, queue type %d",
> > + __func__, queue_id, tail_incr, queue_type);
> > + return -EINVAL;
> > + }
> > +
> > + switch (queue_type) {
> > + case GDMA_QUEUE_SEND:
> > + e.sq.id = queue_id;
> > + e.sq.tail_ptr_incr = tail_incr;
> > + addr += DOORBELL_SHORT_OFFSET_SQ;
> > + break;
> > +
> > + case GDMA_QUEUE_RECEIVE:
> > + e.rq.id = queue_id;
> > + e.rq.tail_ptr_incr = tail_incr;
> > + addr += DOORBELL_SHORT_OFFSET_RQ;
> > + break;
> > +
> > + case GDMA_QUEUE_COMPLETION:
> > + e.cq.id = queue_id;
> > + e.cq.tail_ptr_incr = tail_incr;
> > + e.cq.arm = arm;
> > + addr += DOORBELL_SHORT_OFFSET_CQ;
> > + break;
> > +
> > + default:
> > + DP_LOG(ERR, "Unsupported queue type %d", queue_type);
> > + return -1;
> > + }
> > +
> > + /* Ensure all writes are done before ringing doorbell */
> > + rte_wmb();
> > +
> > + DP_LOG(DEBUG, "db_page %p addr %p queue_id %u type %u tail %u
> > arm %u",
> > + db_page, addr, queue_id, queue_type, tail_incr, arm);
> > +
> > + rte_write32(e.as_uint32, addr);
> > + return 0;
> > +}
> > +#else
> > union gdma_doorbell_entry {
> > uint64_t as_uint64;
> >
> > @@ -248,6 +339,7 @@ mana_ring_doorbell(void *db_page, enum
> > gdma_queue_types queue_type,
> > rte_write64(e.as_uint64, addr);
> > return 0;
> > }
> > +#endif
> >
> > /*
> > * Poll completion queue for completions.
> > @@ -287,6 +379,9 @@ gdma_poll_completion_queue(struct
> mana_gdma_queue
> > *cq,
> > num_comp++;
> >
> > cq->head++;
> > +#ifdef RTE_ARCH_32
> > + cq->head_incr_to_short_db++;
> > +#endif
> >
> > DP_LOG(DEBUG, "comp new 0x%x old 0x%x cqe 0x%x wq %u
> sq %u head
> > %u",
> > new_owner_bits, old_owner_bits, cqe_owner_bits, diff --
> git
> > a/drivers/net/mana/mana.h b/drivers/net/mana/mana.h index
> > 5801491d75..848d87c096 100644
> > --- a/drivers/net/mana/mana.h
> > +++ b/drivers/net/mana/mana.h
> > @@ -50,6 +50,19 @@ struct mana_shared_data { #define
> MAX_TX_WQE_SIZE
> > 512 #define MAX_RX_WQE_SIZE 256
> >
> > +/* For 32 bit only */
> > +#ifdef RTE_ARCH_32
> > +#define GDMA_SHORT_DB_INC_MASK 0xffff
> > +#define GDMA_SHORT_DB_QID_MASK 0xfff
> > +
> > +#define GDMA_SHORT_DB_MAX_WQE (0x10000 /
> > GDMA_WQE_ALIGNMENT_UNIT_SIZE)
> > +
> > +#define TX_WQE_SHORT_DB_THRESHOLD \
> > + (GDMA_SHORT_DB_MAX_WQE - (2 * MAX_TX_WQE_SIZE)) #define
> > +RX_WQE_SHORT_DB_THRESHOLD \
>
> GDMA_SHORT_DB_MAX_WQE is in BU, MAX_TX_WQE_SIZE is in bytes.
>
> Doing math on two different units...
>
That is good point. I will correct this.
> And why using "2*", using "1*" is good enough?
>
>
Sure. I will use 1 in next revision.
>
>
> > + (GDMA_SHORT_DB_MAX_WQE - (2 * MAX_RX_WQE_SIZE)) #endif
> > +
> > /* Values from the GDMA specification document, WQE format
> > description */ #define INLINE_OOB_SMALL_SIZE_IN_BYTES 8 #define
> > INLINE_OOB_LARGE_SIZE_IN_BYTES 24 @@ -375,6 +388,9 @@ struct
> > mana_gdma_queue {
> > uint32_t id;
> > uint32_t head;
> > uint32_t tail;
> > +#ifdef RTE_ARCH_32
> > + uint32_t head_incr_to_short_db;
> > +#endif
> > };
> >
> > #define MANA_MR_BTREE_PER_QUEUE_N 64
> > @@ -425,6 +441,9 @@ struct mana_rxq {
> > */
> > uint32_t desc_ring_head, desc_ring_tail;
> >
> > + /* For storing wqe increment count btw each short doorbell ring */
> > + uint32_t wqe_cnt_to_short_db;
> > +
> > struct mana_gdma_queue gdma_rq;
> > struct mana_gdma_queue gdma_cq;
> > struct gdma_comp *gdma_comp_buf;
> > @@ -455,8 +474,14 @@ extern int mana_logtype_init;
> >
> > #define PMD_INIT_FUNC_TRACE() PMD_INIT_LOG(DEBUG, " >>")
> >
> > +#ifdef RTE_ARCH_32
> > +int mana_ring_short_doorbell(void *db_page, enum gdma_queue_types
> > queue_type,
> > + uint32_t queue_id, uint32_t tail_incr,
> > + uint8_t arm);
> > +#else
> > int mana_ring_doorbell(void *db_page, enum gdma_queue_types
> > queue_type,
> > uint32_t queue_id, uint32_t tail, uint8_t arm);
> > +#endif
> > int mana_rq_ring_doorbell(struct mana_rxq *rxq);
> >
> > int gdma_post_work_request(struct mana_gdma_queue *queue, diff --git
> > a/drivers/net/mana/rx.c b/drivers/net/mana/rx.c index
> > 14d9085801..303d129e5b 100644
> > --- a/drivers/net/mana/rx.c
> > +++ b/drivers/net/mana/rx.c
> > @@ -39,10 +39,23 @@ mana_rq_ring_doorbell(struct mana_rxq *rxq)
> > /* Hardware Spec specifies that software client should set 0 for
> > * wqe_cnt for Receive Queues.
> > */
> > +#ifdef RTE_ARCH_32
> > + if (rxq->wqe_cnt_to_short_db) {
> > + ret = mana_ring_short_doorbell(db_page,
> > GDMA_QUEUE_RECEIVE,
> > + rxq->gdma_rq.id,
> > + rxq->wqe_cnt_to_short_db *
> > +
> > GDMA_WQE_ALIGNMENT_UNIT_SIZE,
> > + 0);
> > + } else {
>
> Is it possible that rxq->wqe_cnt_to_short_db might be 0 from the caller? The
> caller shouldn't ring the doorbell if there is nothing to ring.
>
This is just a safeguard. I can remove the check.
>
>
>
>
> > + /* No need to ring, just return */
> > + ret = 0;
> > + }
> > +#else
> > ret = mana_ring_doorbell(db_page, GDMA_QUEUE_RECEIVE,
> > rxq->gdma_rq.id,
> > rxq->gdma_rq.head *
> > GDMA_WQE_ALIGNMENT_UNIT_SIZE,
> > 0);
> > +#endif
> >
> > if (ret)
> > DP_LOG(ERR, "failed to ring RX doorbell ret %d", ret); @@ -
> > 97,6 +110,7 @@ mana_alloc_and_post_rx_wqe(struct mana_rxq *rxq)
> > /* update queue for tracking pending packets */
> > desc->pkt = mbuf;
> > desc->wqe_size_in_bu = wqe_size_in_bu;
> > + rxq->wqe_cnt_to_short_db += wqe_size_in_bu;
> > rxq->desc_ring_head = (rxq->desc_ring_head + 1) % rxq-
> > >num_desc;
> > } else {
> > DP_LOG(DEBUG, "failed to post recv ret %d", ret); @@ -
> > 115,12 +129,22 @@ mana_alloc_and_post_rx_wqes(struct mana_rxq *rxq)
> > int ret;
> > uint32_t i;
> >
> > +#ifdef RTE_ARCH_32
> > + rxq->wqe_cnt_to_short_db = 0;
> > +#endif
> > for (i = 0; i < rxq->num_desc; i++) {
> > ret = mana_alloc_and_post_rx_wqe(rxq);
> > if (ret) {
> > DP_LOG(ERR, "failed to post RX ret = %d", ret);
> > return ret;
> > }
> > +
> > +#ifdef RTE_ARCH_32
> > + if (rxq->wqe_cnt_to_short_db >
> > RX_WQE_SHORT_DB_THRESHOLD) {
> > + mana_rq_ring_doorbell(rxq);
> > + rxq->wqe_cnt_to_short_db = 0;
> > + }
> > +#endif
> > }
> >
> > mana_rq_ring_doorbell(rxq);
> > @@ -349,6 +373,9 @@ mana_start_rx_queues(struct rte_eth_dev *dev)
> >
> > /* CQ head starts with count */
> > rxq->gdma_cq.head = rxq->gdma_cq.count;
> > +#ifdef RTE_ARCH_32
> > + rxq->gdma_cq.head_incr_to_short_db = 0; #endif
> >
> > DRV_LOG(INFO, "rxq cq id %u buf %p count %u size %u",
> > rxq->gdma_cq.id, rxq->gdma_cq.buffer, @@ -397,6
> > +424,10 @@ mana_rx_burst(void *dpdk_rxq, struct rte_mbuf **pkts,
> > uint16_t pkts_n)
> > uint32_t i;
> > int polled = 0;
> >
> > +#ifdef RTE_ARCH_32
> > + rxq->wqe_cnt_to_short_db = 0;
> > +#endif
> > +
> > repoll:
> > /* Polling on new completions if we have no backlog */
> > if (rxq->comp_buf_idx == rxq->comp_buf_len) { @@ -505,6 +536,16
> @@
> > mana_rx_burst(void *dpdk_rxq, struct rte_mbuf **pkts, uint16_t
> > pkts_n)
> > wqe_posted++;
> > if (pkt_received == pkts_n)
> > break;
> > +
> > +#ifdef RTE_ARCH_32
> > + /* Ring short doorbell if approaching the wqe increment
> > + * limit.
> > + */
> > + if (rxq->wqe_cnt_to_short_db >
> > RX_WQE_SHORT_DB_THRESHOLD) {
> > + mana_rq_ring_doorbell(rxq);
> > + rxq->wqe_cnt_to_short_db = 0;
> > + }
> > +#endif
> > }
> >
> > rxq->backlog_idx = pkt_idx;
> > @@ -529,6 +570,16 @@ static int
> > mana_arm_cq(struct mana_rxq *rxq, uint8_t arm) {
> > struct mana_priv *priv = rxq->priv;
> > +#ifdef RTE_ARCH_32
> > + uint16_t cqe_incr = (uint16_t)rxq->gdma_cq.head_incr_to_short_db;
>
> How do you make sure head_incr_to_short_db doesn't overflow?
>
I have checked this with hardware team. In my opinion it would be easily
overflown.
The hw team seems suggesting the hw will take care of this.
Thanks,
Wei
>
> Thanks,
>
> Long
